Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

Frederic Weisbecker
2011-06-10T00:50:48Z

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.

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);