Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1174445 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC/INCOMPLETE 01/13] context_tracking: Add  context_tracking_assert_state Ingo Molnar <mingo@kernel.org> - 2015-06-30 13:10 +0200

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

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [standalone]

Back to top | Article view | linux.kernel

csiph-web