Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

[Josh Triplett <josh@joshtriplett.org>]

On Mon, Jan 04, 2010 at 06:19:19PM -0800, Paul E. McKenney wrote:
> On Mon, Jan 04, 2010 at 06:03:08PM -0800, Josh Triplett wrote:
> > On Mon, Jan 04, 2010 at 04:04:01PM -0800, Paul E. McKenney wrote:
> > > From: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the
> > > task structure, which happens to be a signed int.  So this patch adds a
> > > check for this counter being negative at the end of __rcu_read_unlock().
> > > This check is under CONFIG_PROVE_LOCKING, so can be thought of as being
> > > part of lockdep.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >  kernel/rcutree_plugin.h |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > > index f11ebd4..e77cdf3 100644
> > > --- a/kernel/rcutree_plugin.h
> > > +++ b/kernel/rcutree_plugin.h
> > > @@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
> > >  	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
> > >  	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
> > >  		rcu_read_unlock_special(t);
> > > +#ifdef CONFIG_PROVE_LOCKING
> > > +	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
> > > +#endif /* #ifdef CONFIG_PROVE_LOCKING */
> > >  }
> > >  EXPORT_SYMBOL_GPL(__rcu_read_unlock);
> > 
> > Given that you *already* need to access t->rcu_read_lock_nesting here,
> > why not just do the test all the time?  Ideally you could access
> > t->rcu_read_lock_nesting once, decrement it, and test for both 0 and
> > negative.
> 
> Because I was paranoid about the extra branch.  Perhaps needlessly
> paranoid, but this is rcu_read_unlock() we are talking about here.  ;-)
> 
> You seem to be suggesting making the first test be "<=", then
> sorting things out later, but given that both the equals-zero and the
> greater-than-zero cases are quite common, I couldn't figure out how to
> avoid the extra test and branch in the common case.  Hence the #ifdef.

No, I think you could simply read the predecremented value into a local
variable, test it once with == 0, then have the WARN_ON_ONCE, and hope
that the compiler figures out it can just test the register once and
then do multiple jumps on the same flags.

You could try it and see what code it generates.

- Josh Triplett