lkml.org 
[lkml]   [2010]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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


\
 
 \ /
  Last update: 2010-02-14 18:07    [from the cache]
©2003-2011 Jasper Spaans