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

[Ingo Molnar <mingo@kernel.org>]

* 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>

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?

Thanks,

	Ingo