Messages in this thread |  | | Date | Sat, 12 Mar 2022 09:44:57 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH v3 2/3] sched/cpuacct: optimize away RCU read lock |
| |
On Sat, Mar 12, 2022 at 01:15:33PM +0100, Peter Zijlstra wrote: > 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.
My point exactly!!!
> 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)
Without in any way agreeing with that assessment of difficulty, especially in the general case... It is -way- easier just to tell RCU what your design rules are for the code in question.
> I don't see a problem. No bugs hidden.
C'mon, Peter!
There really was a bug hidden. That someone intended to add some calls to rcu_read_lock() and rcu_read_unlock() in the proper places. Their failure to add them really was a bug.
That bug was hidden by: (1) There being a preempt_disable() or whatever that by coincidence happened to be covering the part of the code containing the rcu_dereference() and (2) Your proposed change that would make rcu_dereference() unable to detect that bug.
And that bug can be quite bad. Given your proposed change, RCU cannot detect this bug:
/* Preemption is enabled. */ /* There should be an rcu_read_lock() here. */ preempt_disable(); p = rcu_dereference(gp); do_something_with(p); preempt_enable(); /* Without the rcu_read_lock(), *p is history. */ do_something_else_with(p); /* There should be an rcu_read_unlock() here. */
> > > 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 ?
This use case might well be entirely correct, but it is most certainly not the common case.
Therefore, my answer to this requested chance in rcu_dereference() semantics is "no".
Thanx, Paul
|  |