[PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Frederic Weisbecker]

This time I have no current practical cases to fix. Those I fixed
in previous versions were actually using rcu_dereference_raw(), which
is legal in extended qs.

Frederic Weisbecker (3):
  rcu: Detect illegal rcu dereference in extended quiescent state
  rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
  rcu: Warn when rcu_read_lock() is used in extended quiescent state

 include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
 kernel/lockdep.c         |    4 +++
 kernel/rcupdate.c        |    4 +++
 kernel/rcutiny.c         |   13 +++++++++
 kernel/rcutree.c         |   14 +++++++++
 5 files changed, 93 insertions(+), 10 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] rcu: Detect illegal rcu dereference in extended quiescent state

[Frederic Weisbecker]

Report that none of the rcu read lock map are held while we are in
an RCU extended quiescent state. This helps detecting any use of
rcu dereference when we are in dynticks idle mode.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 include/linux/rcupdate.h |   16 ++++++++++++++++
 kernel/rcupdate.c        |    4 ++++
 kernel/rcutiny.c         |   13 +++++++++++++
 kernel/rcutree.c         |   14 ++++++++++++++
 4 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 8be0433..713eb6c 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -179,6 +179,14 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
 }
 #endif	/* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
 
+
+#if defined(CONFIG_PROVE_RCU) && defined(CONFIG_NO_HZ)
+extern bool rcu_check_extended_qs(void);
+#else
+static inline bool rcu_check_extended_qs(void) { return false; }
+#endif
+
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
 extern struct lockdep_map rcu_lock_map;
@@ -215,6 +223,10 @@ static inline int rcu_read_lock_held(void)
 {
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
+
+	if (rcu_check_extended_qs())
+		return 0;
+
 	return lock_is_held(&rcu_lock_map);
 }
 
@@ -246,6 +258,10 @@ static inline int rcu_read_lock_sched_held(void)
 
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
+
+	if (rcu_check_extended_qs())
+		return 0;
+
 	if (debug_locks)
 		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
 	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 7784bd2..d883253 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -88,6 +88,10 @@ int rcu_read_lock_bh_held(void)
 {
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
+
+	if (rcu_check_extended_qs())
+		return 0;
+
 	return in_softirq() || irqs_disabled();
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index 7bbac7d..95581f1 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -77,6 +77,19 @@ void rcu_exit_nohz(void)
 	rcu_dynticks_nesting++;
 }
 
+
+#ifdef CONFIG_PROVE_RCU
+
+bool rcu_check_extended_qs(void)
+{
+	if (!rcu_dynticks_nesting)
+		return true;
+
+	return false;
+}
+
+#endif
+
 #endif /* #ifdef CONFIG_NO_HZ */
 
 /*
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 7e59ffb..c397a86 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -438,6 +438,20 @@ void rcu_irq_exit(void)
 	rcu_enter_nohz();
 }
 
+#ifdef CONFIG_PROVE_RCU
+
+bool rcu_check_extended_qs(void)
+{
+	struct rcu_dynticks *rdtp = &__get_cpu_var(rcu_dynticks);
+
+	if (atomic_read(&rdtp->dynticks) & 0x1)
+		return false;
+
+	return true;
+}
+
+#endif /* CONFIG_PROVE_RCU */
+
 #ifdef CONFIG_SMP
 
 /*
-- 
1.7.5.4

[PATCH 2/3] rcu: Inform the user about dynticks idle mode on PROVE_RCU warning

[Frederic Weisbecker]

Inform the user if we are in extended quiescent state when
an illegal use of rcu is detected.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/lockdep.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index df2ad37..e77de90 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -4008,6 +4008,10 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 	printk("%s:%d %s!\n", file, line, s);
 	printk("\nother info that might help us debug this:\n\n");
 	printk("\nrcu_scheduler_active = %d, debug_locks = %d\n", rcu_scheduler_active, debug_locks);
+
+	if (rcu_check_extended_qs())
+		printk("rcu is in extended quiescent state!\n");
+
 	lockdep_print_held_locks(curr);
 	printk("\nstack backtrace:\n");
 	dump_stack();
-- 
1.7.5.4

[PATCH 3/3] rcu: Warn when rcu_read_lock() is used in extended quiescent state

[Frederic Weisbecker]

We are currently able to detect uses of rcu_dereference_check()
inside extended quiescent states. But rcu_read_lock() and friends
can be used without rcu_dereference(), making their illegal
uses inside extended quiescent invisible sometimes.

Thus, check we don't call these functions from dyntick idle mode.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 include/linux/rcupdate.h |   52 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 713eb6c..073f5f9 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -189,21 +189,53 @@ static inline bool rcu_check_extended_qs(void) { return false; }
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
+static inline void rcu_lock_acquire(struct lockdep_map *map)
+{
+	WARN_ON_ONCE(rcu_check_extended_qs());
+	lock_acquire(map, 0, 0, 2, 1, NULL, _THIS_IP_);
+}
+
+static inline void rcu_lock_release(struct lockdep_map *map)
+{
+	WARN_ON_ONCE(rcu_check_extended_qs());
+	lock_release(map, 1, _THIS_IP_);
+}
+
 extern struct lockdep_map rcu_lock_map;
-# define rcu_read_acquire() \
-		lock_acquire(&rcu_lock_map, 0, 0, 2, 1, NULL, _THIS_IP_)
-# define rcu_read_release()	lock_release(&rcu_lock_map, 1, _THIS_IP_)
+
+static inline void rcu_read_acquire(void)
+{
+	rcu_lock_acquire(&rcu_lock_map);
+}
+
+static inline void rcu_read_release(void)
+{
+	rcu_lock_release(&rcu_lock_map);
+}
 
 extern struct lockdep_map rcu_bh_lock_map;
-# define rcu_read_acquire_bh() \
-		lock_acquire(&rcu_bh_lock_map, 0, 0, 2, 1, NULL, _THIS_IP_)
-# define rcu_read_release_bh()	lock_release(&rcu_bh_lock_map, 1, _THIS_IP_)
+
+static inline void rcu_read_acquire_bh(void)
+{
+	rcu_lock_acquire(&rcu_bh_lock_map);
+}
+
+static inline void rcu_read_release_bh(void)
+{
+	rcu_lock_release(&rcu_bh_lock_map);
+}
 
 extern struct lockdep_map rcu_sched_lock_map;
-# define rcu_read_acquire_sched() \
-		lock_acquire(&rcu_sched_lock_map, 0, 0, 2, 1, NULL, _THIS_IP_)
-# define rcu_read_release_sched() \
-		lock_release(&rcu_sched_lock_map, 1, _THIS_IP_)
+
+static inline void rcu_read_acquire_sched(void)
+{
+	rcu_lock_acquire(&rcu_sched_lock_map);
+}
+
+static inline void rcu_read_release_sched(void)
+{
+	rcu_lock_release(&rcu_sched_lock_map);
+}
 
 extern int debug_lockdep_rcu_enabled(void);
 
-- 
1.7.5.4

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Paul E. McKenney]

On Fri, Jun 24, 2011 at 01:12:37AM +0200, Frederic Weisbecker wrote:
> This time I have no current practical cases to fix. Those I fixed
> in previous versions were actually using rcu_dereference_raw(), which
> is legal in extended qs.
> 
> Frederic Weisbecker (3):
>   rcu: Detect illegal rcu dereference in extended quiescent state
>   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
>   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> 
>  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
>  kernel/lockdep.c         |    4 +++
>  kernel/rcupdate.c        |    4 +++
>  kernel/rcutiny.c         |   13 +++++++++
>  kernel/rcutree.c         |   14 +++++++++
>  5 files changed, 93 insertions(+), 10 deletions(-)

Queued, thank you, Frederic!

I have also applied your approach to SRCU, and I applied the following
to simplify the code a bit -- please let me know if there are any
problems with this approach.

							Thanx, Paul

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

rcu: Remove one layer of abstraction from PROVE_RCU checking

Simplify things a bit by substituting the definitions of the single-line
rcu_read_acquire(), rcu_read_release(), rcu_read_acquire_bh(),
rcu_read_release_bh(), rcu_read_acquire_sched(), and
rcu_read_release_sched() functions at their call points.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 16717c3..7b0cdf4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -264,41 +264,8 @@ static inline void rcu_lock_release(struct lockdep_map *map)
 }
 
 extern struct lockdep_map rcu_lock_map;
-
-static inline void rcu_read_acquire(void)
-{
-	rcu_lock_acquire(&rcu_lock_map);
-}
-
-static inline void rcu_read_release(void)
-{
-	rcu_lock_release(&rcu_lock_map);
-}
-
 extern struct lockdep_map rcu_bh_lock_map;
-
-static inline void rcu_read_acquire_bh(void)
-{
-	rcu_lock_acquire(&rcu_bh_lock_map);
-}
-
-static inline void rcu_read_release_bh(void)
-{
-	rcu_lock_release(&rcu_bh_lock_map);
-}
-
 extern struct lockdep_map rcu_sched_lock_map;
-
-static inline void rcu_read_acquire_sched(void)
-{
-	rcu_lock_acquire(&rcu_sched_lock_map);
-}
-
-static inline void rcu_read_release_sched(void)
-{
-	rcu_lock_release(&rcu_sched_lock_map);
-}
-
 extern int debug_lockdep_rcu_enabled(void);
 
 /**
@@ -369,12 +336,8 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_read_acquire()		do { } while (0)
-# define rcu_read_release()		do { } while (0)
-# define rcu_read_acquire_bh()		do { } while (0)
-# define rcu_read_release_bh()		do { } while (0)
-# define rcu_read_acquire_sched()	do { } while (0)
-# define rcu_read_release_sched()	do { } while (0)
+# define rcu_lock_acquire(a)		do { } while (0)
+# define rcu_lock_release(a)		do { } while (0)
 
 static inline int rcu_read_lock_held(void)
 {
@@ -717,7 +680,7 @@ static inline void rcu_read_lock(void)
 {
 	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_read_acquire();
+	rcu_lock_acquire(&rcu_lock_map);
 }
 
 /*
@@ -737,7 +700,7 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_read_release();
+	rcu_lock_release(&rcu_lock_map);
 	__release(RCU);
 	__rcu_read_unlock();
 }
@@ -758,7 +721,7 @@ static inline void rcu_read_lock_bh(void)
 {
 	__rcu_read_lock_bh();
 	__acquire(RCU_BH);
-	rcu_read_acquire_bh();
+	rcu_lock_acquire(&rcu_bh_lock_map);
 }
 
 /*
@@ -768,7 +731,7 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_read_release_bh();
+	rcu_lock_release(&rcu_bh_lock_map);
 	__release(RCU_BH);
 	__rcu_read_unlock_bh();
 }
@@ -785,7 +748,7 @@ static inline void rcu_read_lock_sched(void)
 {
 	preempt_disable();
 	__acquire(RCU_SCHED);
-	rcu_read_acquire_sched();
+	rcu_lock_acquire(&rcu_sched_lock_map);
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
@@ -802,7 +765,7 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_read_release_sched();
+	rcu_lock_release(&rcu_sched_lock_map);
 	__release(RCU_SCHED);
 	preempt_enable();
 }

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Frederic Weisbecker]

On Thu, Jun 23, 2011 at 08:53:11PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 24, 2011 at 01:12:37AM +0200, Frederic Weisbecker wrote:
> > This time I have no current practical cases to fix. Those I fixed
> > in previous versions were actually using rcu_dereference_raw(), which
> > is legal in extended qs.
> > 
> > Frederic Weisbecker (3):
> >   rcu: Detect illegal rcu dereference in extended quiescent state
> >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > 
> >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> >  kernel/lockdep.c         |    4 +++
> >  kernel/rcupdate.c        |    4 +++
> >  kernel/rcutiny.c         |   13 +++++++++
> >  kernel/rcutree.c         |   14 +++++++++
> >  5 files changed, 93 insertions(+), 10 deletions(-)
> 
> Queued, thank you, Frederic!
> 
> I have also applied your approach to SRCU, and I applied the following
> to simplify the code a bit -- please let me know if there are any
> problems with this approach.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> rcu: Remove one layer of abstraction from PROVE_RCU checking
> 
> Simplify things a bit by substituting the definitions of the single-line
> rcu_read_acquire(), rcu_read_release(), rcu_read_acquire_bh(),
> rcu_read_release_bh(), rcu_read_acquire_sched(), and
> rcu_read_release_sched() functions at their call points.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Yeah looks good. Thanks!

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Paul E. McKenney]

On Fri, Jun 24, 2011 at 01:20:49PM +0200, Frederic Weisbecker wrote:
> On Thu, Jun 23, 2011 at 08:53:11PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 24, 2011 at 01:12:37AM +0200, Frederic Weisbecker wrote:
> > > This time I have no current practical cases to fix. Those I fixed
> > > in previous versions were actually using rcu_dereference_raw(), which
> > > is legal in extended qs.
> > > 
> > > Frederic Weisbecker (3):
> > >   rcu: Detect illegal rcu dereference in extended quiescent state
> > >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> > >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > > 
> > >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> > >  kernel/lockdep.c         |    4 +++
> > >  kernel/rcupdate.c        |    4 +++
> > >  kernel/rcutiny.c         |   13 +++++++++
> > >  kernel/rcutree.c         |   14 +++++++++
> > >  5 files changed, 93 insertions(+), 10 deletions(-)
> > 
> > Queued, thank you, Frederic!
> > 
> > I have also applied your approach to SRCU, and I applied the following
> > to simplify the code a bit -- please let me know if there are any
> > problems with this approach.
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > rcu: Remove one layer of abstraction from PROVE_RCU checking
> > 
> > Simplify things a bit by substituting the definitions of the single-line
> > rcu_read_acquire(), rcu_read_release(), rcu_read_acquire_bh(),
> > rcu_read_release_bh(), rcu_read_acquire_sched(), and
> > rcu_read_release_sched() functions at their call points.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Yeah looks good. Thanks!

And I thought that you might be amused by the following.  Hmmm...  I wonder
how I am going to use event tracing for the portions of RCU that execute
while in dyntick-idle mode...

But first...  It turns out that rcu_check_extended_qs() is sometimes
called with preemption enabled (for example, in CONFIG_TREE_PREEMPT_RCU),
which causes smp_processor_id() to complain.  One way to fix this would be
to write rcu_check_extended_qs() as follows:

	bool rcu_check_extended_qs(void)
	{
		struct rcu_dynticks *rdtp;

		preempt_disable();
		rdtp = &__get_cpu_var(rcu_dynticks);
		if (atomic_read(&rdtp->dynticks) & 0x1) {
			preempt_enable();
			return false;
		}
		preempt_enable();
		return true;
	}
	EXPORT_SYMBOL_GPL(rcu_check_extended_qs);

Does the above make sense, or is there a higher-level bug that should be
addressed in a different way?

See below for the splat due to tracing while in dyntick-idle mode.
Might this explain some otherwise mysterious crashes when tracing is
enabled?

							Thanx, Paul

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

[    0.449600] ===============================
[    0.449605] [ INFO: suspicious RCU usage. ]
[    0.449610] -------------------------------
[    0.449616] /usr/local/autobench/var/tmp/build/arch/powerpc/include/asm/trace.h:122 suspicious rcu_dereference_check() usage!
[    0.449626] 
[    0.449627] other info that might help us debug this:
[    0.449628] 
[    0.449636] 
[    0.449637] rcu_scheduler_active = 1, debug_locks = 0
[    0.449644] rcu is in extended quiescent state!
[    0.449650] no locks held by kworker/0:0/0.
[    0.449655] 
[    0.449656] stack backtrace:
[    0.449662] Call Trace:
[    0.449671] [c0000000e66d7b20] [c00000000001352c] .show_stack+0x70/0x184 (unreliable)
[    0.449684] [c0000000e66d7bd0] [c0000000000b1ef0] .lockdep_rcu_suspicious+0xe8/0x110
[    0.449697] [c0000000e66d7c70] [c000000000044fc0] .__trace_hcall_exit+0x1e4/0x218
[    0.449709] [c0000000e66d7d20] [c000000000045c40] .plpar_hcall_norets+0xb4/0xd0
[    0.449720] [c0000000e66d7d90] [c000000000047cd4] .pseries_dedicated_idle_sleep+0x1b0/0x22c
[    0.449731] [c0000000e66d7e40] [c000000000016004] .cpu_idle+0x144/0x22c
[    0.449743] [c0000000e66d7ed0] [c0000000006572cc] .start_secondary+0x378/0x384
[    0.449754] [c0000000e66d7f90] [c000000000009268] .start_secondary_prolog+0x10/0x14

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Frederic Weisbecker]

On Sat, Jun 25, 2011 at 06:13:15PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 24, 2011 at 01:20:49PM +0200, Frederic Weisbecker wrote:
> > On Thu, Jun 23, 2011 at 08:53:11PM -0700, Paul E. McKenney wrote:
> > > On Fri, Jun 24, 2011 at 01:12:37AM +0200, Frederic Weisbecker wrote:
> > > > This time I have no current practical cases to fix. Those I fixed
> > > > in previous versions were actually using rcu_dereference_raw(), which
> > > > is legal in extended qs.
> > > > 
> > > > Frederic Weisbecker (3):
> > > >   rcu: Detect illegal rcu dereference in extended quiescent state
> > > >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> > > >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > > > 
> > > >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> > > >  kernel/lockdep.c         |    4 +++
> > > >  kernel/rcupdate.c        |    4 +++
> > > >  kernel/rcutiny.c         |   13 +++++++++
> > > >  kernel/rcutree.c         |   14 +++++++++
> > > >  5 files changed, 93 insertions(+), 10 deletions(-)
> > > 
> > > Queued, thank you, Frederic!
> > > 
> > > I have also applied your approach to SRCU, and I applied the following
> > > to simplify the code a bit -- please let me know if there are any
> > > problems with this approach.
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > rcu: Remove one layer of abstraction from PROVE_RCU checking
> > > 
> > > Simplify things a bit by substituting the definitions of the single-line
> > > rcu_read_acquire(), rcu_read_release(), rcu_read_acquire_bh(),
> > > rcu_read_release_bh(), rcu_read_acquire_sched(), and
> > > rcu_read_release_sched() functions at their call points.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > Yeah looks good. Thanks!
> 
> And I thought that you might be amused by the following.  Hmmm...  I wonder
> how I am going to use event tracing for the portions of RCU that execute
> while in dyntick-idle mode...
> 
> But first...  It turns out that rcu_check_extended_qs() is sometimes
> called with preemption enabled (for example, in CONFIG_TREE_PREEMPT_RCU),
> which causes smp_processor_id() to complain.  One way to fix this would be
> to write rcu_check_extended_qs() as follows:
> 
> 	bool rcu_check_extended_qs(void)
> 	{
> 		struct rcu_dynticks *rdtp;
> 
> 		preempt_disable();
> 		rdtp = &__get_cpu_var(rcu_dynticks);
> 		if (atomic_read(&rdtp->dynticks) & 0x1) {
> 			preempt_enable();
> 			return false;
> 		}
> 		preempt_enable();
> 		return true;
> 	}
> 	EXPORT_SYMBOL_GPL(rcu_check_extended_qs);
> 
> Does the above make sense, or is there a higher-level bug that should be
> addressed in a different way?

Ah right. In fact rcu_read_lock_heald() shouldn't expect to have preemption
disabled, at least not in PREEMPT_RCU.

So yeah, looks good.

> 
> See below for the splat due to tracing while in dyntick-idle mode.
> Might this explain some otherwise mysterious crashes when tracing is
> enabled?

May be.

So this is using a tracepoint in dynticks idle mode. There are various
ways to solve this:

- move the tracepoint call out of that place, in an rcu safe place
- call rcu_exit_nohz() / rcu_enter_nohz() there. But we need to know if the
  tracepoint if activated before that, or this will impact the tracing off case too.
- split out the rcu extended qs from tick stop logic (https://patchwork.kernel.org/patch/850542/)
  That looks like a big change just to fix such a bug but anyway it is going to be needed for the nohz
  cpuset patches I'm working on. Once that's split, rcu_enter_nohz() can be called later after
  the tick has been stopped, like right before we hlt the cpu.

> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> [    0.449600] ===============================
> [    0.449605] [ INFO: suspicious RCU usage. ]
> [    0.449610] -------------------------------
> [    0.449616] /usr/local/autobench/var/tmp/build/arch/powerpc/include/asm/trace.h:122 suspicious rcu_dereference_check() usage!
> [    0.449626] 
> [    0.449627] other info that might help us debug this:
> [    0.449628] 
> [    0.449636] 
> [    0.449637] rcu_scheduler_active = 1, debug_locks = 0
> [    0.449644] rcu is in extended quiescent state!
> [    0.449650] no locks held by kworker/0:0/0.
> [    0.449655] 
> [    0.449656] stack backtrace:
> [    0.449662] Call Trace:
> [    0.449671] [c0000000e66d7b20] [c00000000001352c] .show_stack+0x70/0x184 (unreliable)
> [    0.449684] [c0000000e66d7bd0] [c0000000000b1ef0] .lockdep_rcu_suspicious+0xe8/0x110
> [    0.449697] [c0000000e66d7c70] [c000000000044fc0] .__trace_hcall_exit+0x1e4/0x218
> [    0.449709] [c0000000e66d7d20] [c000000000045c40] .plpar_hcall_norets+0xb4/0xd0
> [    0.449720] [c0000000e66d7d90] [c000000000047cd4] .pseries_dedicated_idle_sleep+0x1b0/0x22c
> [    0.449731] [c0000000e66d7e40] [c000000000016004] .cpu_idle+0x144/0x22c
> [    0.449743] [c0000000e66d7ed0] [c0000000006572cc] .start_secondary+0x378/0x384
> [    0.449754] [c0000000e66d7f90] [c000000000009268] .start_secondary_prolog+0x10/0x14
> 

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Paul E. McKenney]

On Sun, Jun 26, 2011 at 03:55:03AM +0200, Frederic Weisbecker wrote:
> On Sat, Jun 25, 2011 at 06:13:15PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 24, 2011 at 01:20:49PM +0200, Frederic Weisbecker wrote:
> > > On Thu, Jun 23, 2011 at 08:53:11PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jun 24, 2011 at 01:12:37AM +0200, Frederic Weisbecker wrote:
> > > > > This time I have no current practical cases to fix. Those I fixed
> > > > > in previous versions were actually using rcu_dereference_raw(), which
> > > > > is legal in extended qs.
> > > > > 
> > > > > Frederic Weisbecker (3):
> > > > >   rcu: Detect illegal rcu dereference in extended quiescent state
> > > > >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> > > > >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > > > > 
> > > > >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> > > > >  kernel/lockdep.c         |    4 +++
> > > > >  kernel/rcupdate.c        |    4 +++
> > > > >  kernel/rcutiny.c         |   13 +++++++++
> > > > >  kernel/rcutree.c         |   14 +++++++++
> > > > >  5 files changed, 93 insertions(+), 10 deletions(-)
> > > > 
> > > > Queued, thank you, Frederic!
> > > > 
> > > > I have also applied your approach to SRCU, and I applied the following
> > > > to simplify the code a bit -- please let me know if there are any
> > > > problems with this approach.
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > ------------------------------------------------------------------------
> > > > 
> > > > rcu: Remove one layer of abstraction from PROVE_RCU checking
> > > > 
> > > > Simplify things a bit by substituting the definitions of the single-line
> > > > rcu_read_acquire(), rcu_read_release(), rcu_read_acquire_bh(),
> > > > rcu_read_release_bh(), rcu_read_acquire_sched(), and
> > > > rcu_read_release_sched() functions at their call points.
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > Yeah looks good. Thanks!
> > 
> > And I thought that you might be amused by the following.  Hmmm...  I wonder
> > how I am going to use event tracing for the portions of RCU that execute
> > while in dyntick-idle mode...
> > 
> > But first...  It turns out that rcu_check_extended_qs() is sometimes
> > called with preemption enabled (for example, in CONFIG_TREE_PREEMPT_RCU),
> > which causes smp_processor_id() to complain.  One way to fix this would be
> > to write rcu_check_extended_qs() as follows:
> > 
> > 	bool rcu_check_extended_qs(void)
> > 	{
> > 		struct rcu_dynticks *rdtp;
> > 
> > 		preempt_disable();
> > 		rdtp = &__get_cpu_var(rcu_dynticks);
> > 		if (atomic_read(&rdtp->dynticks) & 0x1) {
> > 			preempt_enable();
> > 			return false;
> > 		}
> > 		preempt_enable();
> > 		return true;
> > 	}
> > 	EXPORT_SYMBOL_GPL(rcu_check_extended_qs);
> > 
> > Does the above make sense, or is there a higher-level bug that should be
> > addressed in a different way?
> 
> Ah right. In fact rcu_read_lock_heald() shouldn't expect to have preemption
> disabled, at least not in PREEMPT_RCU.
> 
> So yeah, looks good.

OK, I am folding that into your original patch, then.

> > See below for the splat due to tracing while in dyntick-idle mode.
> > Might this explain some otherwise mysterious crashes when tracing is
> > enabled?
> 
> May be.
> 
> So this is using a tracepoint in dynticks idle mode. There are various
> ways to solve this:
> 
> - move the tracepoint call out of that place, in an rcu safe place
> - call rcu_exit_nohz() / rcu_enter_nohz() there. But we need to know if the
>   tracepoint if activated before that, or this will impact the tracing off case too.
> - split out the rcu extended qs from tick stop logic (https://patchwork.kernel.org/patch/850542/)
>   That looks like a big change just to fix such a bug but anyway it is going to be needed for the nohz
>   cpuset patches I'm working on. Once that's split, rcu_enter_nohz() can be called later after
>   the tick has been stopped, like right before we hlt the cpu.

This last sounds to me like the best approach.  And if I see some
mysterious crashes, I will try commenting out that trace point.
Though any mysterious crashes that I see are more likely due to my
messing something up.  ;-)


							Thanx, Paul

> > ------------------------------------------------------------------------
> > 
> > [    0.449600] ===============================
> > [    0.449605] [ INFO: suspicious RCU usage. ]
> > [    0.449610] -------------------------------
> > [    0.449616] /usr/local/autobench/var/tmp/build/arch/powerpc/include/asm/trace.h:122 suspicious rcu_dereference_check() usage!
> > [    0.449626] 
> > [    0.449627] other info that might help us debug this:
> > [    0.449628] 
> > [    0.449636] 
> > [    0.449637] rcu_scheduler_active = 1, debug_locks = 0
> > [    0.449644] rcu is in extended quiescent state!
> > [    0.449650] no locks held by kworker/0:0/0.
> > [    0.449655] 
> > [    0.449656] stack backtrace:
> > [    0.449662] Call Trace:
> > [    0.449671] [c0000000e66d7b20] [c00000000001352c] .show_stack+0x70/0x184 (unreliable)
> > [    0.449684] [c0000000e66d7bd0] [c0000000000b1ef0] .lockdep_rcu_suspicious+0xe8/0x110
> > [    0.449697] [c0000000e66d7c70] [c000000000044fc0] .__trace_hcall_exit+0x1e4/0x218
> > [    0.449709] [c0000000e66d7d20] [c000000000045c40] .plpar_hcall_norets+0xb4/0xd0
> > [    0.449720] [c0000000e66d7d90] [c000000000047cd4] .pseries_dedicated_idle_sleep+0x1b0/0x22c
> > [    0.449731] [c0000000e66d7e40] [c000000000016004] .cpu_idle+0x144/0x22c
> > [    0.449743] [c0000000e66d7ed0] [c0000000006572cc] .start_secondary+0x378/0x384
> > [    0.449754] [c0000000e66d7f90] [c000000000009268] .start_secondary_prolog+0x10/0x14
> > 

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Peter Zijlstra]

On Fri, 2011-06-24 at 01:12 +0200, Frederic Weisbecker wrote:
> This time I have no current practical cases to fix. Those I fixed
> in previous versions were actually using rcu_dereference_raw(), which
> is legal in extended qs.
> 
> Frederic Weisbecker (3):
>   rcu: Detect illegal rcu dereference in extended quiescent state
>   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
>   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> 
>  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
>  kernel/lockdep.c         |    4 +++
>  kernel/rcupdate.c        |    4 +++
>  kernel/rcutiny.c         |   13 +++++++++
>  kernel/rcutree.c         |   14 +++++++++
>  5 files changed, 93 insertions(+), 10 deletions(-)

Right, so the only comment I have is that it might have been nice to
explain what the heck an extended qs is :-) I've since figured it out,
but it did require waking up my brain.

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Frederic Weisbecker]

On Fri, Jun 24, 2011 at 11:18:45AM +0200, Peter Zijlstra wrote:
> On Fri, 2011-06-24 at 01:12 +0200, Frederic Weisbecker wrote:
> > This time I have no current practical cases to fix. Those I fixed
> > in previous versions were actually using rcu_dereference_raw(), which
> > is legal in extended qs.
> > 
> > Frederic Weisbecker (3):
> >   rcu: Detect illegal rcu dereference in extended quiescent state
> >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > 
> >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> >  kernel/lockdep.c         |    4 +++
> >  kernel/rcupdate.c        |    4 +++
> >  kernel/rcutiny.c         |   13 +++++++++
> >  kernel/rcutree.c         |   14 +++++++++
> >  5 files changed, 93 insertions(+), 10 deletions(-)
> 
> Right, so the only comment I have is that it might have been nice to
> explain what the heck an extended qs is :-) I've since figured it out,
> but it did require waking up my brain.

Hehe, right :)

As I have scratched my head for some times with rcu dynticks for the nohz cpuset
things, I guess that in the meantime my unconscious mind developed the idea that rcu extended
quiescent states were a worldwide topic that every people talk about in dinner with their family,
that these even became the core stories of some lullabies. I can still hear its chorus, that makes
one switching to idle peacefully...

Re: [PATCH 0/3 v3] rcu: Detect rcu uses under extended quiescent state

[Paul E. McKenney]

On Fri, Jun 24, 2011 at 11:18:45AM +0200, Peter Zijlstra wrote:
> On Fri, 2011-06-24 at 01:12 +0200, Frederic Weisbecker wrote:
> > This time I have no current practical cases to fix. Those I fixed
> > in previous versions were actually using rcu_dereference_raw(), which
> > is legal in extended qs.
> > 
> > Frederic Weisbecker (3):
> >   rcu: Detect illegal rcu dereference in extended quiescent state
> >   rcu: Inform the user about dynticks idle mode on PROVE_RCU warning
> >   rcu: Warn when rcu_read_lock() is used in extended quiescent state
> > 
> >  include/linux/rcupdate.h |   68 +++++++++++++++++++++++++++++++++++++++-------
> >  kernel/lockdep.c         |    4 +++
> >  kernel/rcupdate.c        |    4 +++
> >  kernel/rcutiny.c         |   13 +++++++++
> >  kernel/rcutree.c         |   14 +++++++++
> >  5 files changed, 93 insertions(+), 10 deletions(-)
> 
> Right, so the only comment I have is that it might have been nice to
> explain what the heck an extended qs is :-) I've since figured it out,
> but it did require waking up my brain.

Good point -- I updated the commit logs to (hopefully) make this
more clear.

							Thanx, Paul