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    [W:0.024 / U:0.472 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site