Messages in this thread | | | Date | Fri, 17 Jun 2011 16:19:03 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states |
| |
On Fri, Jun 10, 2011 at 02:50:43AM +0200, Frederic Weisbecker wrote: > On Thu, Jun 09, 2011 at 05:23:50PM -0700, Paul E. McKenney wrote: > > On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote: > > > Detect uses of rcu that are not supposed to happen when we > > > are in an extended quiescent state. > > > > > > This can happen for example if we use rcu between the time we > > > stop the tick and the time we restart it. Or inside an irq that > > > didn't use rcu_irq_enter,exit() or other possible kind of rcu API > > > misuse. > > > > > > v2: Rebase against latest rcu changes, handle tiny RCU as well > > > > Good idea on checking for RCU read-side critical sections happening > > in dyntick-idle periods! > > > > But wouldn't it be better to put the checks in rcu_read_lock() and > > friends? The problem I see with putting them in rcu_dereference_check() > > is that someone can legitimately do something like the following > > while in dyntick-idle mode: > > > > spin_lock(&mylock); > > /* do a bunch of stuff */ > > p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock)); > > > > The logic below would complain about this usage, despite the fact > > that it is perfectly safe because the update-side lock is held. > > > > Make sense, or am I missing something? > > > > Thanx, Paul > > I'm an idiot. I put my check in rcu_dereference_check() on purpose because > it's always called from places that check one of the rcu locks are held, > but I forgot that's also used for custom conditions with the _check() > things. > > That said, putting the check in rcu_read_lock() and alike would only work > with rcu_read_lock() itself. Few users of rcu_read_lock_sched() actually > call it explicitely but rely on irq disabled or preempt disabled. And I can't put the > checks there as it's fine to disabled irqs in dyntick idle. > > What about the below? (untested yet) > > And I would print the state of dynticks-idle mode in the final lockdep warning.
Printing the dynticks-idle mode would be quite good!
However, it is possible to have an RCU read-side critical section that does not have an rcu_dereference() or an rcu_read_lock_held(). So I do believe that we really do need rcu_read_lock() and friends to do this checking.
That might seem to leave open the possibility of a stray rcu_dereference() being executed from dyntick-idle mode, but the existing PROVE_RCU checking will catch that, right?
So I believe that the simplest approach with the best coverage is to put the checks into RCU's read-side critical-section-entry primitives.
Make sense, or am I confused?
Thanx, Paul
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 6cad1f3..b9e68ae 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -278,7 +278,7 @@ static inline int rcu_read_lock_held(void) > { > if (!debug_lockdep_rcu_enabled()) > return 1; > - return lock_is_held(&rcu_lock_map); > + return lock_is_held(&rcu_lock_map) && !rcu_check_extended_qs(); > } > > /* > @@ -310,13 +310,13 @@ static inline int rcu_read_lock_sched_held(void) > if (!debug_lockdep_rcu_enabled()) > return 1; > if (debug_locks) > - lockdep_opinion = lock_is_held(&rcu_sched_lock_map); > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map) && !rcu_check_extended_qs(); > return lockdep_opinion || preempt_count() != 0 || irqs_disabled(); > } > #else /* #ifdef CONFIG_PREEMPT */ > static inline int rcu_read_lock_sched_held(void) > { > - return 1; > + return !rcu_check_extended_qs(); > } > #endif /* #else #ifdef CONFIG_PREEMPT */ > > diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c > index a088c90..20d6e7228 100644 > --- a/kernel/rcupdate.c > +++ b/kernel/rcupdate.c > @@ -88,7 +88,7 @@ int rcu_read_lock_bh_held(void) > { > if (!debug_lockdep_rcu_enabled()) > return 1; > - return in_softirq() || irqs_disabled(); > + return !rcu_check_extended_qs() && (in_softirq() || irqs_disabled()); > } > EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); >
| |