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

From: Paul E. McKenney
Date: Tue Jun 30 2015 - 12:16:42 EST


On Tue, Jun 30, 2015 at 01:04:14PM +0200, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>

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

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