Messages in this thread |  | | Date | Sat, 12 Mar 2022 13:15:33 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v3 2/3] sched/cpuacct: optimize away RCU read lock |
| |
On Thu, Mar 10, 2022 at 07:01:52AM -0800, Paul E. McKenney wrote:
> > > > > ./include/linux/cgroup.h:481 suspicious rcu_dereference_check() usage! > > > > > > > > Arguably, with the flavours folded again, rcu_dereference_check() ought > > > > to default include rcu_read_lock_sched_held() or its equivalent I > > > > suppose. > > > > > > > > Paul? > > > > > > That would reduce the number of warnings, but it also would hide bugs. > > > > > > So, are you sure you really want this? > > > > I don't understand... Since the flavours got merged regular RCU has it's > > quescent state held off by preempt_disable. So how can relying on that > > cause bugs? > > Somene forgets an rcu_read_lock() and there happens to be something > like a preempt_disable() that by coincidence covers that particular > rcu_dereference(). The kernel therefore doesn't complain. That someone > goes on to other things, maybe even posthumously. Then some time later > the preempt_disable() goes away, for good and sufficient reasons. > > Good luck figuring out where to put the needed rcu_read_lock() and > rcu_read_unlock().
Well, that's software engineering for you. Also in that case the warning will work as expected. Then figuring out how to fix it is not the problem of the warning -- that worked as advertised.
(also, I don't think it'll be too hard, you just gotta figure out which object is rcu protected -- the warning gives you this, where the lookup happens -- again the warning helps, and how long it's used for, all relatively well definted things)
I don't see a problem. No bugs hidden.
> > And if we can rely on that, then surely rcu_dereferenced_check() ought > > to play by the same rules, otherwise we get silly warnings like these at > > hand. > > > > Specifically, we removed the rcu_read_lock() here because this has > > rq->lock held, which is a raw_spinlock_t which very much implies preempt > > disable, on top of that, it's also an IRQ-safe lock and thus IRQs will > > be disabled. > > > > There is no possible way for RCU to make progress. > > Then let's have that particular rcu_dereference_check() explicitly state > what it needs, which seems to be either rcu_read_lock() on the one hand. > Right now, that could be just this: > > p = rcu_dereference_check(gp, rcu_read_lock_sched_held()); > > Or am I missing something here?
That will work; I just don't agree with it. Per the rules of RCU it is entirely correct to mix rcu_read_lock() and preempt_disable() (or anything that implies the same). So I strongly feel that rcu_dereference() should not warn about obviously correct code. Why would we need to special case this ?
|  |