Re: [RFC/INCOMPLETE 01/13] context_tracking: Add context_tracking_assert_state

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, Jun 30, 2015 at 01:04:14PM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > > Yeah, and inverting the condition. Assuming the condition was assert()-style 
> > > inverted to begin with! :-)
> > 
> > It appears to have been.  ;-)
> > 
> > Please see below for an untested patch.  I made this be one big patch, but could 
> > have one patch add RCU_LOCKDEP_WARN(), a series convert uses from 
> > rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove 
> > rcu_lockdep_assert().  Let me know!
> 
> One big patch is perfect I think - it's a rename and a relatively mechanic 
> inversion of conditions, no point in splitting it up any more IMHO. (But it's your 
> call really.)
> 
> So I had a quick look at this patch, and IMHO the RCU_LOCKDEP_WARN() lines read a 
> lot more 'naturally', because the new, inverted conditions now highlight buggy 
> scenarios - which has the same logic parity as the kernel's historic 
> WARN_ON()/BUG_ON() patterns:
> 
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thank you, added!

> This one looked a bit weird:
> 
> > index a0a0dd03c73a..47268fb1d27b 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -589,8 +589,8 @@ EXPORT_SYMBOL_GPL(call_rcu_tasks);
> >  void synchronize_rcu_tasks(void)
> >  {
> >  	/* Complain if the scheduler has not started.  */
> > -	rcu_lockdep_assert(!rcu_scheduler_active,
> > -			   "synchronize_rcu_tasks called too soon");
> > +	RCU_LOCKDEP_WARN(rcu_scheduler_active,
> > +			 "synchronize_rcu_tasks called too soon");
> >  
> 
> So I'd assume that a flag called 'rcu_scheduler_active' would be 1 if the RCU 
> scheduler is active.
> 
> So why do we warn on it being active? Shouldn't the condition be:
> 
> 	RCU_LOCKDEP_WARN(!rcu_scheduler_active,
> 			 "synchronize_rcu_tasks called too soon");
> 
> I.e. we warn when the RCU scheduler is not yet active and we called 
> synchronize_rcu_tasks() too soon?
> 
> So either the original assert() was wrong, or I'm missing something obvious?

You are missing nothing!  But I really do test this stuff...

Ah, I see...  I need the following patch in order to enable lockdep-RCU
on one of my RCU-tasks rcutorture scenarios...  :-/

Good catch!  There were at least three bugs hiding behind that one!  ;-)

							Thanx, Paul

------------------------------------------------------------------------

commit dc883f1668c83f9525a13ee1b3cd45e9e85a0fe5
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Tue Jun 30 09:14:01 2015 -0700

    rcutorture: Enable lockdep-RCU on TASKS01
    
    Currently none of the RCU-tasks scenarios enables lockdep-RCU, which
    causes bugs to be missed.  This commit therefore enables lockdep-RCU
    on TASKS01.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01 b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
index 2cc0e60eba6e..bafe94cbd739 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
@@ -5,6 +5,6 @@ CONFIG_PREEMPT_NONE=n
 CONFIG_PREEMPT_VOLUNTARY=n
 CONFIG_PREEMPT=y
 CONFIG_DEBUG_LOCK_ALLOC=y
-CONFIG_PROVE_LOCKING=n
-#CHECK#CONFIG_PROVE_RCU=n
+CONFIG_PROVE_LOCKING=y
+#CHECK#CONFIG_PROVE_RCU=y
 CONFIG_RCU_EXPERT=y