Messages in this thread | | | Date | Tue, 30 Jun 2015 13:04:14 +0200 | From | Ingo Molnar <> | Subject | 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
| |