Messages in this thread | | | Date | Sun, 14 Feb 2010 09:04:09 -0800 | From | "Paul E. McKenney" <> | Subject | Re: rcu_dereference() without protection in select_task_rq_fair() |
| |
On Sun, Feb 14, 2010 at 11:12:38AM +0100, Peter Zijlstra wrote: > On Thu, 2010-02-11 at 08:52 -0800, Paul E. McKenney wrote: > > Hello, Peter, > > > > My lockdep-ified RCU complains about the for_each_domain() in > > select_task_rq_fair(), see below for the lockdep complaint. I added > > rcu_dereference_check() annotations as follows: > > > > #define for_each_domain_rd(p) \ > > rcu_dereference_check((p), \ > > rcu_read_lock_sched_held() || \ > > lockdep_is_held(&sched_domains_mutex)) > > > > #define for_each_domain(cpu, __sd) \ > > for (__sd = for_each_domain_rd(cpu_rq(cpu)->sd); __sd; __sd = __sd->parent) > > > > In other words, I believe (perhaps incorrectly) that for_each_domain() > > can be called either within an RCU-sched read-side critical section or > > with sched_domains_mutex held. Lockdep claims that no locks of any > > kind, RCU or otherwise, were held. I considered the possibility that > > this was an initialization-time thing, but the code traverses CPU > > structures rather than task structures. > > > > One other possibility is that this is safe due to the fact that we are > > booting up, before the second CPU has come online. Are you relying on > > this? > > > > For reference, here is the definition of rcu_read_lock_sched_held(): > > > > static inline int rcu_read_lock_sched_held(void) > > { > > int lockdep_opinion = 0; > > > > if (debug_locks) > > lockdep_opinion = lock_is_held(&rcu_sched_lock_map); > > return lockdep_opinion || preempt_count() != 0; > > } > > > > Help? > > We use synchronize_sched() and preempt_disable() for the sched domain > stuff. The comment above for_each_domain(): > > /* > * The domain tree (rq->sd) is protected by RCU's quiescent state transition. > * See detach_destroy_domains: synchronize_sched for details. > * > * The domain tree of any CPU may only be accessed from within > * preempt-disabled sections. > */ > #define for_each_domain(cpu, __sd) \ > for (__sd = rcu_dereference(cpu_rq(cpu)->sd); __sd; __sd = __sd->parent) > > explains this usage, also see detach_destroy_domains(). > > So one thing you can do is add (preempt_count() & ~PREEMPT_ACTIVE) to > the rcu_read_lock_sched_held() function to catch all those cases that > rely on preemption without having to add annotations to everything that > disables preemption.
OK, but doesn't the "preempt_count() != 0" that is in the current version of rcu_read_lock_sched_held() already cover this check?
In other words, I believe that I have located a usage of for_each_domain() that violates the rule that it may only be called within preempt-disabled sections.
Thanx, Paul
| |