rcu warnings cause stack overflow

rcu warnings cause stack overflow

[Heiko Carstens]

Hi Frederic,

your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
Actually this found a bug on s390 (thanks!) but it probably didn't work
as expected.
On architectures which implement WARN_ON_ONCE with an exception this
additional warning will lead to a stack overflow (if it triggers):

[   55.746956] Kernel stack overflow.
[   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
scan]
[   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
[   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
[   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
[   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
[   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
[   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
[   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
[   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
[   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
[   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
[   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
[   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
[   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
[   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
[   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
[   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
[   55.747276] Call Trace:
[   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
[   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
[   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
[   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
[   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
[   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
[   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158

[...lots more of the same...] 

[   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
[   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
[   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
[   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
[   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
[   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
[   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
[   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
[   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
[   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
[   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
[   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
[   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
[   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
[   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
[   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
[   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
[   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
[   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
[   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
[   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
[   55.748588]  [<0000000000100020>] _stext+0x20/0x80
[   55.748603] 2 locks held by swapper/0/0:
[   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
[   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
[   55.748663] Last Breaking-Event-Address:
[   55.748667]  [<0000000000000000>] 0x0

This simply happens because WARN_ON_ONCE causes an exception, the excpetion
handler wants to call a notifier call chain (notify_die), which again uses
rcu_read_lock(), which again causes an exception and so on...
Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
the flag that the warning already happened. Seems to be quite some effort to
change this behaviour.

Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
will find illegal uses. But it won't work for lockdep off configs...
So we probably want something better than the patch below.

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 81c04f4..6da8ca4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -239,13 +239,11 @@ static inline int rcu_is_cpu_idle(void)
 
 static inline void rcu_lock_acquire(struct lockdep_map *map)
 {
-	WARN_ON_ONCE(rcu_is_cpu_idle());
 	lock_acquire(map, 0, 0, 2, 1, NULL, _THIS_IP_);
 }
 
 static inline void rcu_lock_release(struct lockdep_map *map)
 {
-	WARN_ON_ONCE(rcu_is_cpu_idle());
 	lock_release(map, 1, _THIS_IP_);
 }
 

Re: rcu warnings cause stack overflow

[Frederic Weisbecker]

On Wed, Feb 01, 2012 at 11:06:52AM +0100, Heiko Carstens wrote:
> Hi Frederic,
> 
> your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
> quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
> Actually this found a bug on s390 (thanks!) but it probably didn't work
> as expected.
> On architectures which implement WARN_ON_ONCE with an exception this
> additional warning will lead to a stack overflow (if it triggers):
> 
> [   55.746956] Kernel stack overflow.
> [   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
> scan]
> [   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
> [   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
> [   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
> [   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
> [   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
> [   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
> [   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
> [   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
> [   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
> [   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
> [   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
> [   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
> [   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
> [   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
> [   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
> [   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
> [   55.747276] Call Trace:
> [   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
> [   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> [   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> [   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> [   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
> [   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
> [   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> 
> [...lots more of the same...] 
> 
> [   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> [   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> [   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> [   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
> [   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
> [   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> [   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> [   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> [   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> [   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
> [   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
> [   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> [   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
> [   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
> [   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
> [   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
> [   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
> [   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
> [   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
> [   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
> [   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
> [   55.748588]  [<0000000000100020>] _stext+0x20/0x80
> [   55.748603] 2 locks held by swapper/0/0:
> [   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
> [   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
> [   55.748663] Last Breaking-Event-Address:
> [   55.748667]  [<0000000000000000>] 0x0
> 
> This simply happens because WARN_ON_ONCE causes an exception, the excpetion
> handler wants to call a notifier call chain (notify_die), which again uses
> rcu_read_lock(), which again causes an exception and so on...
> Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
> the flag that the warning already happened. Seems to be quite some effort to
> change this behaviour.
> 
> Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> will find illegal uses. But it won't work for lockdep off configs...
> So we probably want something better than the patch below.

Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
in s390? Is it to have a whole new stack for the warning path in order
to avoid stack overflow from the place that called the WARN_ON() ?

Anyway perhaps we need a recursion protection on WARN_ON_ONCE(), such
as:

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 84458b0..f76635f 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -137,9 +137,13 @@ extern void warn_slowpath_null(const char *file, const int line);
 	static bool __warned;					\
 	int __ret_warn_once = !!(condition);			\
 								\
-	if (unlikely(__ret_warn_once))				\
-		if (WARN_ON(!__warned)) 			\
+	if (unlikely(__ret_warn_once)) {			\
+		if (!__warned) {				\
 			__warned = true;			\
+			barrier();				\
+			WARN_ON(1);				\
+		}						\
+	}							\
 	unlikely(__ret_warn_once);				\
 })
 

Re: rcu warnings cause stack overflow

[Paul E. McKenney]

On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> On Wed, Feb 01, 2012 at 11:06:52AM +0100, Heiko Carstens wrote:
> > Hi Frederic,
> > 
> > your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
> > quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
> > Actually this found a bug on s390 (thanks!) but it probably didn't work
> > as expected.
> > On architectures which implement WARN_ON_ONCE with an exception this
> > additional warning will lead to a stack overflow (if it triggers):
> > 
> > [   55.746956] Kernel stack overflow.
> > [   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
> > scan]
> > [   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
> > [   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
> > [   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
> > [   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
> > [   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
> > [   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
> > [   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
> > [   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
> > [   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
> > [   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
> > [   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
> > [   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
> > [   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
> > [   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
> > [   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
> > [   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
> > [   55.747276] Call Trace:
> > [   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
> > [   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > [   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > [   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > [   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
> > [   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
> > [   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > 
> > [...lots more of the same...] 
> > 
> > [   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > [   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > [   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > [   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
> > [   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
> > [   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > [   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > [   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > [   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > [   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
> > [   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
> > [   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > [   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
> > [   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
> > [   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
> > [   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
> > [   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
> > [   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
> > [   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
> > [   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
> > [   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
> > [   55.748588]  [<0000000000100020>] _stext+0x20/0x80
> > [   55.748603] 2 locks held by swapper/0/0:
> > [   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
> > [   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
> > [   55.748663] Last Breaking-Event-Address:
> > [   55.748667]  [<0000000000000000>] 0x0
> > 
> > This simply happens because WARN_ON_ONCE causes an exception, the excpetion
> > handler wants to call a notifier call chain (notify_die), which again uses
> > rcu_read_lock(), which again causes an exception and so on...
> > Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
> > the flag that the warning already happened. Seems to be quite some effort to
> > change this behaviour.
> > 
> > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > will find illegal uses. But it won't work for lockdep off configs...
> > So we probably want something better than the patch below.
> 
> Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> in s390? Is it to have a whole new stack for the warning path in order
> to avoid stack overflow from the place that called the WARN_ON() ?
> 
> Anyway perhaps we need a recursion protection on WARN_ON_ONCE(), such
> as:

This makes sense to me, but I am also taking Heiko's patch
removing the WARN_ON()s in favor of the lockdep checks now in
rcu_read_lock_held().  After all, the rcu_lock_acquire() WARN_ON_ONCE()
only appears if PROVE_LOCKING=y, so the added requirement is not a
big deal.  Also, CONFIG_PROVE_RCU is in the testing requirements in
Documentation/SubmitChecklist.

							Thanx, Paul

> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 84458b0..f76635f 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -137,9 +137,13 @@ extern void warn_slowpath_null(const char *file, const int line);
>  	static bool __warned;					\
>  	int __ret_warn_once = !!(condition);			\
>  								\
> -	if (unlikely(__ret_warn_once))				\
> -		if (WARN_ON(!__warned)) 			\
> +	if (unlikely(__ret_warn_once)) {			\
> +		if (!__warned) {				\
>  			__warned = true;			\
> +			barrier();				\
> +			WARN_ON(1);				\
> +		}						\
> +	}							\
>  	unlikely(__ret_warn_once);				\
>  })
> 
> 

Re: rcu warnings cause stack overflow

[Frederic Weisbecker]

On Wed, Feb 01, 2012 at 09:18:56AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > On Wed, Feb 01, 2012 at 11:06:52AM +0100, Heiko Carstens wrote:
> > > Hi Frederic,
> > > 
> > > your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
> > > quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
> > > Actually this found a bug on s390 (thanks!) but it probably didn't work
> > > as expected.
> > > On architectures which implement WARN_ON_ONCE with an exception this
> > > additional warning will lead to a stack overflow (if it triggers):
> > > 
> > > [   55.746956] Kernel stack overflow.
> > > [   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
> > > scan]
> > > [   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
> > > [   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
> > > [   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
> > > [   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
> > > [   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
> > > [   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
> > > [   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
> > > [   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
> > > [   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
> > > [   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
> > > [   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
> > > [   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
> > > [   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
> > > [   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
> > > [   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
> > > [   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
> > > [   55.747276] Call Trace:
> > > [   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
> > > [   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > [   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > [   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > [   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
> > > [   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > [   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > 
> > > [...lots more of the same...] 
> > > 
> > > [   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > [   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > [   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > [   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
> > > [   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > [   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > [   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > [   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > [   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > [   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
> > > [   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > [   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > [   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
> > > [   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
> > > [   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
> > > [   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
> > > [   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
> > > [   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
> > > [   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
> > > [   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
> > > [   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
> > > [   55.748588]  [<0000000000100020>] _stext+0x20/0x80
> > > [   55.748603] 2 locks held by swapper/0/0:
> > > [   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
> > > [   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
> > > [   55.748663] Last Breaking-Event-Address:
> > > [   55.748667]  [<0000000000000000>] 0x0
> > > 
> > > This simply happens because WARN_ON_ONCE causes an exception, the excpetion
> > > handler wants to call a notifier call chain (notify_die), which again uses
> > > rcu_read_lock(), which again causes an exception and so on...
> > > Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
> > > the flag that the warning already happened. Seems to be quite some effort to
> > > change this behaviour.
> > > 
> > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > will find illegal uses. But it won't work for lockdep off configs...
> > > So we probably want something better than the patch below.
> > 
> > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > in s390? Is it to have a whole new stack for the warning path in order
> > to avoid stack overflow from the place that called the WARN_ON() ?
> > 
> > Anyway perhaps we need a recursion protection on WARN_ON_ONCE(), such
> > as:
> 
> This makes sense to me, but I am also taking Heiko's patch
> removing the WARN_ON()s in favor of the lockdep checks now in
> rcu_read_lock_held().  After all, the rcu_lock_acquire() WARN_ON_ONCE()
> only appears if PROVE_LOCKING=y, so the added requirement is not a
> big deal.  Also, CONFIG_PROVE_RCU is in the testing requirements in
> Documentation/SubmitChecklist.
> 
> 							Thanx, Paul

I think if we do this, we lose the debugging coverage on places that
make use of rcu_read_lock{_sched,_bh}() without using rcu_dereference{_sched,_bh}().

It seems walking the task list with list_for_each_entry_rcu() under rcu_read_lock() is
one such example because rcu lists are using rcu_dereference_raw().

Also I realize a problem with my below patch. The exception will call rcu_read_lock()
from another callsite so we are still going to recurse. Only of one level but we will
recurse. This could mess up the warning output, although it prevents from the
stack overflow. If we still go that direction, we probably need a more global, or per
cpu, recursion protection of these RCU warnings.

> 
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index 84458b0..f76635f 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -137,9 +137,13 @@ extern void warn_slowpath_null(const char *file, const int line);
> >  	static bool __warned;					\
> >  	int __ret_warn_once = !!(condition);			\
> >  								\
> > -	if (unlikely(__ret_warn_once))				\
> > -		if (WARN_ON(!__warned)) 			\
> > +	if (unlikely(__ret_warn_once)) {			\
> > +		if (!__warned) {				\
> >  			__warned = true;			\
> > +			barrier();				\
> > +			WARN_ON(1);				\
> > +		}						\
> > +	}							\
> >  	unlikely(__ret_warn_once);				\
> >  })
> > 
> > 
> 

Re: rcu warnings cause stack overflow

[Paul E. McKenney]

On Wed, Feb 01, 2012 at 07:08:15PM +0100, Frederic Weisbecker wrote:
> On Wed, Feb 01, 2012 at 09:18:56AM -0800, Paul E. McKenney wrote:
> > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > On Wed, Feb 01, 2012 at 11:06:52AM +0100, Heiko Carstens wrote:
> > > > Hi Frederic,
> > > > 
> > > > your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
> > > > quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
> > > > Actually this found a bug on s390 (thanks!) but it probably didn't work
> > > > as expected.
> > > > On architectures which implement WARN_ON_ONCE with an exception this
> > > > additional warning will lead to a stack overflow (if it triggers):
> > > > 
> > > > [   55.746956] Kernel stack overflow.
> > > > [   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
> > > > scan]
> > > > [   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
> > > > [   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
> > > > [   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
> > > > [   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
> > > > [   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
> > > > [   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
> > > > [   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
> > > > [   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
> > > > [   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
> > > > [   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
> > > > [   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
> > > > [   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
> > > > [   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
> > > > [   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
> > > > [   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
> > > > [   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
> > > > [   55.747276] Call Trace:
> > > > [   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
> > > > [   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > [   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > [   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > [   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > [   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > [   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > 
> > > > [...lots more of the same...] 
> > > > 
> > > > [   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > [   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > [   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > [   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > [   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > [   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > [   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > [   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > [   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > [   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > [   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > [   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > [   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
> > > > [   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
> > > > [   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
> > > > [   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
> > > > [   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
> > > > [   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
> > > > [   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
> > > > [   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
> > > > [   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
> > > > [   55.748588]  [<0000000000100020>] _stext+0x20/0x80
> > > > [   55.748603] 2 locks held by swapper/0/0:
> > > > [   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
> > > > [   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
> > > > [   55.748663] Last Breaking-Event-Address:
> > > > [   55.748667]  [<0000000000000000>] 0x0
> > > > 
> > > > This simply happens because WARN_ON_ONCE causes an exception, the excpetion
> > > > handler wants to call a notifier call chain (notify_die), which again uses
> > > > rcu_read_lock(), which again causes an exception and so on...
> > > > Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
> > > > the flag that the warning already happened. Seems to be quite some effort to
> > > > change this behaviour.
> > > > 
> > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > So we probably want something better than the patch below.
> > > 
> > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > in s390? Is it to have a whole new stack for the warning path in order
> > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > 
> > > Anyway perhaps we need a recursion protection on WARN_ON_ONCE(), such
> > > as:
> > 
> > This makes sense to me, but I am also taking Heiko's patch
> > removing the WARN_ON()s in favor of the lockdep checks now in
> > rcu_read_lock_held().  After all, the rcu_lock_acquire() WARN_ON_ONCE()
> > only appears if PROVE_LOCKING=y, so the added requirement is not a
> > big deal.  Also, CONFIG_PROVE_RCU is in the testing requirements in
> > Documentation/SubmitChecklist.
> > 
> > 							Thanx, Paul
> 
> I think if we do this, we lose the debugging coverage on places that
> make use of rcu_read_lock{_sched,_bh}() without using rcu_dereference{_sched,_bh}().
> 
> It seems walking the task list with list_for_each_entry_rcu() under rcu_read_lock() is
> one such example because rcu lists are using rcu_dereference_raw().

Good point.  I will convert these to rcu_lockdep_assert()s.

> Also I realize a problem with my below patch. The exception will call rcu_read_lock()
> from another callsite so we are still going to recurse. Only of one level but we will
> recurse. This could mess up the warning output, although it prevents from the
> stack overflow. If we still go that direction, we probably need a more global, or per
> cpu, recursion protection of these RCU warnings.

Hmmm...  Should this be global or specific to S390?

							Thanx, Paul

> > > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > > index 84458b0..f76635f 100644
> > > --- a/include/asm-generic/bug.h
> > > +++ b/include/asm-generic/bug.h
> > > @@ -137,9 +137,13 @@ extern void warn_slowpath_null(const char *file, const int line);
> > >  	static bool __warned;					\
> > >  	int __ret_warn_once = !!(condition);			\
> > >  								\
> > > -	if (unlikely(__ret_warn_once))				\
> > > -		if (WARN_ON(!__warned)) 			\
> > > +	if (unlikely(__ret_warn_once)) {			\
> > > +		if (!__warned) {				\
> > >  			__warned = true;			\
> > > +			barrier();				\
> > > +			WARN_ON(1);				\
> > > +		}						\
> > > +	}							\
> > >  	unlikely(__ret_warn_once);				\
> > >  })
> > > 
> > > 
> > 
> 

Re: rcu warnings cause stack overflow

[Frederic Weisbecker]

On Wed, Feb 01, 2012 at 10:22:09AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 01, 2012 at 07:08:15PM +0100, Frederic Weisbecker wrote:
> > On Wed, Feb 01, 2012 at 09:18:56AM -0800, Paul E. McKenney wrote:
> > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > On Wed, Feb 01, 2012 at 11:06:52AM +0100, Heiko Carstens wrote:
> > > > > Hi Frederic,
> > > > > 
> > > > > your patch 00f49e5729 "rcu: Warn when rcu_read_lock() is used in extended
> > > > > quiescent state" adds a WARN_ON_ONCE to rcu_lock_acquire().
> > > > > Actually this found a bug on s390 (thanks!) but it probably didn't work
> > > > > as expected.
> > > > > On architectures which implement WARN_ON_ONCE with an exception this
> > > > > additional warning will lead to a stack overflow (if it triggers):
> > > > > 
> > > > > [   55.746956] Kernel stack overflow.
> > > > > [   55.746966] Modules linked in: qeth_l3 binfmt_misc dm_multipath scsi_dh dm_mod qeth vmur ccwgroup [last unloaded: scsi_wait_
> > > > > scan]
> > > > > [   55.746999] CPU: 0 Not tainted 3.3.0-rc1-00167-gf8275f9 #90
> > > > > [   55.747005] Process swapper/0 (pid: 0, task: 0000000000911100, ksp: 0000000000907d50)
> > > > > [   55.747013] Krnl PSW : 0404000180000000 00000000005d5728 (illegal_op+0x1c/0x134)
> > > > > [   55.747034]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3
> > > > > [   55.747043] Krnl GPRS: 0000000000000001 00000000005d570c 00000000009040e8 0000000000000002
> > > > > [   55.747054]            00000000005d83dc ffffffffffffffff 0000000000000000 0400000000907cc8
> > > > > [   55.747064]            0404100180000000 00000000005d8478 0000000000000008 00000000009040e8
> > > > > [   55.747074]            0000000000904000 00000000005dc550 0000000000904048 0000000000904048
> > > > > [   55.747096] Krnl Code: 00000000005d571c: b90400ef            lgr     %r14,%r15
> > > > > [   55.747118]            00000000005d5720: b90400b2            lgr     %r11,%r2
> > > > > [   55.747194]           #00000000005d5724: a7840001            brc     8,5d5726
> > > > > [   55.747205]           >00000000005d5728: a7fbff18            aghi    %r15,-232
> > > > > [   55.747216]            00000000005d572c: e3e0f0980024        stg     %r14,152(%r15)
> > > > > [   55.747228]            00000000005d5732: e31020100004        lg      %r1,16(%r2)
> > > > > [   55.747242]            00000000005d5738: 58c020a0            l       %r12,160(%r2)
> > > > > [   55.747257]            00000000005d573c: 91012009            tm      9(%r2),1
> > > > > [   55.747276] Call Trace:
> > > > > [   55.747282] ([<00000000005d60b4>] pgm_check_handler+0x154/0x158)
> > > > > [   55.747296]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > > [   55.747309] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > > [   55.747322]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > > [   55.747335]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > > [   55.747349]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > > [   55.747364]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > > 
> > > > > [...lots more of the same...] 
> > > > > 
> > > > > [   55.747379]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > > [   55.747425] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > > [   55.747432]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > > [   55.747440]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > > [   55.747448]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > > [   55.747457]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > > [   55.747797]  [<00000000005d8478>] __atomic_notifier_call_chain+0xd8/0xfc
> > > > > [   55.747806] ([<00000000005d83dc>] __atomic_notifier_call_chain+0x3c/0xfc)
> > > > > [   55.747816]  [<00000000005d84c6>] atomic_notifier_call_chain+0x2a/0x3c
> > > > > [   55.747826]  [<00000000005d852a>] notify_die+0x52/0x60
> > > > > [   55.748456]  [<00000000005d57da>] illegal_op+0xce/0x134
> > > > > [   55.748463]  [<00000000005d60b4>] pgm_check_handler+0x154/0x158
> > > > > [   55.748472]  [<000000000017afa0>] select_task_rq_fair+0x1478/0x14b4
> > > > > [   55.748483] ([<0000000000179bb8>] select_task_rq_fair+0x90/0x14b4)
> > > > > [   55.748493]  [<0000000000170702>] try_to_wake_up+0x136/0x47c
> > > > > [   55.748506]  [<000000000015b446>] autoremove_wake_function+0x26/0x58
> > > > > [   55.748518]  [<000000000016693a>] __wake_up_common+0x76/0xb4
> > > > > [   55.748530]  [<000000000016aed0>] __wake_up+0x4c/0x60
> > > > > [   55.748541]  [<0000000000109ee0>] s390_handle_mcck+0x194/0x1f8
> > > > > [   55.748557]  [<000000000010486a>] cpu_idle+0x192/0x1c0
> > > > > [   55.748570]  [<0000000000977916>] start_kernel+0x402/0x410
> > > > > [   55.748588]  [<0000000000100020>] _stext+0x20/0x80
> > > > > [   55.748603] 2 locks held by swapper/0/0:
> > > > > [   55.748612]  #0:  (crw_handler_wait_q.lock){......}, at: [<000000000016aeb6>] __wake_up+0x32/0x60
> > > > > [   55.748648]  #1:  (&p->pi_lock){-.-.-.}, at: [<000000000017060c>] try_to_wake_up+0x40/0x47c
> > > > > [   55.748663] Last Breaking-Event-Address:
> > > > > [   55.748667]  [<0000000000000000>] 0x0
> > > > > 
> > > > > This simply happens because WARN_ON_ONCE causes an exception, the excpetion
> > > > > handler wants to call a notifier call chain (notify_die), which again uses
> > > > > rcu_read_lock(), which again causes an exception and so on...
> > > > > Unfortunately WARN_ON_ONCE first causes an exception and only afterwards sets
> > > > > the flag that the warning already happened. Seems to be quite some effort to
> > > > > change this behaviour.
> > > > > 
> > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > So we probably want something better than the patch below.
> > > > 
> > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > > 
> > > > Anyway perhaps we need a recursion protection on WARN_ON_ONCE(), such
> > > > as:
> > > 
> > > This makes sense to me, but I am also taking Heiko's patch
> > > removing the WARN_ON()s in favor of the lockdep checks now in
> > > rcu_read_lock_held().  After all, the rcu_lock_acquire() WARN_ON_ONCE()
> > > only appears if PROVE_LOCKING=y, so the added requirement is not a
> > > big deal.  Also, CONFIG_PROVE_RCU is in the testing requirements in
> > > Documentation/SubmitChecklist.
> > > 
> > > 							Thanx, Paul
> > 
> > I think if we do this, we lose the debugging coverage on places that
> > make use of rcu_read_lock{_sched,_bh}() without using rcu_dereference{_sched,_bh}().
> > 
> > It seems walking the task list with list_for_each_entry_rcu() under rcu_read_lock() is
> > one such example because rcu lists are using rcu_dereference_raw().
> 
> Good point.  I will convert these to rcu_lockdep_assert()s.

But rcu_dereference_raw() can be used outside rcu_read_lock() as well.

> 
> > Also I realize a problem with my below patch. The exception will call rcu_read_lock()
> > from another callsite so we are still going to recurse. Only of one level but we will
> > recurse. This could mess up the warning output, although it prevents from the
> > stack overflow. If we still go that direction, we probably need a more global, or per
> > cpu, recursion protection of these RCU warnings.
> 
> Hmmm...  Should this be global or specific to S390?

I guess specific to S390. But it may be a good idea to protect against
WARN() recursions in general. One would expect such function to be quite
robust.

Re: rcu warnings cause stack overflow

[Heiko Carstens]

On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > will find illegal uses. But it won't work for lockdep off configs...
> > So we probably want something better than the patch below.
> 
> Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> in s390? Is it to have a whole new stack for the warning path in order
> to avoid stack overflow from the place that called the WARN_ON() ?

The reason was to reduce the code footprint of the WARN_ON() and also
be able to print the register contents at the time the warning happened.

All architectures which define __WARN_TAINT implement warnings with
exceptions. Currently that are parisc, powerpc, s390 and sh.

Re: rcu warnings cause stack overflow

[Frederic Weisbecker]

On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > will find illegal uses. But it won't work for lockdep off configs...
> > > So we probably want something better than the patch below.
> > 
> > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > in s390? Is it to have a whole new stack for the warning path in order
> > to avoid stack overflow from the place that called the WARN_ON() ?
> 
> The reason was to reduce the code footprint of the WARN_ON() and also
> be able to print the register contents at the time the warning happened.

Ah ok, makes sense.

> 
> All architectures which define __WARN_TAINT implement warnings with
> exceptions. Currently that are parisc, powerpc, s390 and sh.
> 

Re: rcu warnings cause stack overflow

[Paul E. McKenney]

On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > So we probably want something better than the patch below.
> > > 
> > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > in s390? Is it to have a whole new stack for the warning path in order
> > > to avoid stack overflow from the place that called the WARN_ON() ?
> > 
> > The reason was to reduce the code footprint of the WARN_ON() and also
> > be able to print the register contents at the time the warning happened.
> 
> Ah ok, makes sense.

So Frederic should push his anti-recursion patch, then?

							Thanx, Paul

> > All architectures which define __WARN_TAINT implement warnings with
> > exceptions. Currently that are parisc, powerpc, s390 and sh.
> > 
> 

Re: rcu warnings cause stack overflow

[Heiko Carstens]

On Thu, Feb 02, 2012 at 11:11:16AM -0800, Paul E. McKenney wrote:
> On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> > On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > So we probably want something better than the patch below.
> > > > 
> > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > 
> > > The reason was to reduce the code footprint of the WARN_ON() and also
> > > be able to print the register contents at the time the warning happened.
> > 
> > Ah ok, makes sense.
> 
> So Frederic should push his anti-recursion patch, then?

Yes, please.

Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>

It still generates recursive warnings because the WARNON_ONCE is inlined and
every different usage will generate an exception, but it didn't produce a
stack overflow anymore.
To avoid the recursive warning the patch below would help. Not sure if it's
worth it...

Subject: [PATCH] rcu: move rcu_is_cpu_idle() check warning into C file

From: Heiko Carstens <heiko.carstens@de.ibm.com>

rcu_read_lock() and rcu_read_unlock() generate a warning if a cpu is in
extended quiescant state. Since these functions are inlined this can cause
a lot of warnings if in the processing of the WARN_ON_ONCE() there is
another usage of e.g. rcu_read_lock(). To make sure we only get one
warning (and avoid possible stack overflows) uninline the check.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 include/linux/rcupdate.h |    9 +++++++--
 kernel/rcupdate.c        |    6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 81c04f4..9fe7be5 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -230,22 +230,27 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
 
 #ifdef CONFIG_PROVE_RCU
 extern int rcu_is_cpu_idle(void);
+extern void rcu_warn_if_is_cpu_idle(void);
 #else /* !CONFIG_PROVE_RCU */
 static inline int rcu_is_cpu_idle(void)
 {
 	return 0;
 }
+
+static inline void rcu_warn_if_is_cpu_idle(void)
+{
+}
 #endif /* else !CONFIG_PROVE_RCU */
 
 static inline void rcu_lock_acquire(struct lockdep_map *map)
 {
-	WARN_ON_ONCE(rcu_is_cpu_idle());
+	rcu_warn_if_is_cpu_idle();
 	lock_acquire(map, 0, 0, 2, 1, NULL, _THIS_IP_);
 }
 
 static inline void rcu_lock_release(struct lockdep_map *map)
 {
-	WARN_ON_ONCE(rcu_is_cpu_idle());
+	rcu_warn_if_is_cpu_idle();
 	lock_release(map, 1, _THIS_IP_);
 }
 
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 2bc4e13..5deca18 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -141,6 +141,12 @@ int rcu_my_thread_group_empty(void)
 	return thread_group_empty(current);
 }
 EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
+
+void rcu_warn_if_is_cpu_idle(void)
+{
+	WARN_ON_ONCE(rcu_is_cpu_idle());
+}
+EXPORT_SYMBOL_GPL(rcu_warn_if_is_cpu_idle);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
 #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD

Re: rcu warnings cause stack overflow

[Paul E. McKenney]

On Fri, Feb 03, 2012 at 10:32:14AM +0100, Heiko Carstens wrote:
> On Thu, Feb 02, 2012 at 11:11:16AM -0800, Paul E. McKenney wrote:
> > On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> > > On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > > So we probably want something better than the patch below.
> > > > > 
> > > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > > 
> > > > The reason was to reduce the code footprint of the WARN_ON() and also
> > > > be able to print the register contents at the time the warning happened.
> > > 
> > > Ah ok, makes sense.
> > 
> > So Frederic should push his anti-recursion patch, then?
> 
> Yes, please.
> 
> Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> It still generates recursive warnings because the WARNON_ONCE is inlined and
> every different usage will generate an exception, but it didn't produce a
> stack overflow anymore.
> To avoid the recursive warning the patch below would help. Not sure if it's
> worth it...
> 
> Subject: [PATCH] rcu: move rcu_is_cpu_idle() check warning into C file
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> rcu_read_lock() and rcu_read_unlock() generate a warning if a cpu is in
> extended quiescant state. Since these functions are inlined this can cause
> a lot of warnings if in the processing of the WARN_ON_ONCE() there is
> another usage of e.g. rcu_read_lock(). To make sure we only get one
> warning (and avoid possible stack overflows) uninline the check.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  include/linux/rcupdate.h |    9 +++++++--
>  kernel/rcupdate.c        |    6 ++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 81c04f4..9fe7be5 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -230,22 +230,27 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
> 
>  #ifdef CONFIG_PROVE_RCU
>  extern int rcu_is_cpu_idle(void);
> +extern void rcu_warn_if_is_cpu_idle(void);
>  #else /* !CONFIG_PROVE_RCU */
>  static inline int rcu_is_cpu_idle(void)
>  {
>  	return 0;
>  }
> +
> +static inline void rcu_warn_if_is_cpu_idle(void)
> +{
> +}
>  #endif /* else !CONFIG_PROVE_RCU */
> 
>  static inline void rcu_lock_acquire(struct lockdep_map *map)
>  {
> -	WARN_ON_ONCE(rcu_is_cpu_idle());
> +	rcu_warn_if_is_cpu_idle();

Thank you for the patch, but this WARN_ON_ONCE() has now been removed
in favor of lockdep-RCU checks elsewhere.  This has the advantage of
leveraging lockdep's splat-once and anti-recursion facilities.

So I believe that current -rcu covers this.  (And yes, I do need to
push my most recent changes out.)

							Thanx, Paul

>  	lock_acquire(map, 0, 0, 2, 1, NULL, _THIS_IP_);
>  }
> 
>  static inline void rcu_lock_release(struct lockdep_map *map)
>  {
> -	WARN_ON_ONCE(rcu_is_cpu_idle());
> +	rcu_warn_if_is_cpu_idle();
>  	lock_release(map, 1, _THIS_IP_);
>  }
> 
> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> index 2bc4e13..5deca18 100644
> --- a/kernel/rcupdate.c
> +++ b/kernel/rcupdate.c
> @@ -141,6 +141,12 @@ int rcu_my_thread_group_empty(void)
>  	return thread_group_empty(current);
>  }
>  EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
> +
> +void rcu_warn_if_is_cpu_idle(void)
> +{
> +	WARN_ON_ONCE(rcu_is_cpu_idle());
> +}
> +EXPORT_SYMBOL_GPL(rcu_warn_if_is_cpu_idle);
>  #endif /* #ifdef CONFIG_PROVE_RCU */
> 
>  #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD

Re: rcu warnings cause stack overflow

[Frederic Weisbecker]

On Fri, Feb 03, 2012 at 10:33:35AM -0800, Paul E. McKenney wrote:
> On Fri, Feb 03, 2012 at 10:32:14AM +0100, Heiko Carstens wrote:
> > On Thu, Feb 02, 2012 at 11:11:16AM -0800, Paul E. McKenney wrote:
> > > On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> > > > On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > > > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > > > So we probably want something better than the patch below.
> > > > > > 
> > > > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > > > 
> > > > > The reason was to reduce the code footprint of the WARN_ON() and also
> > > > > be able to print the register contents at the time the warning happened.
> > > > 
> > > > Ah ok, makes sense.
> > > 
> > > So Frederic should push his anti-recursion patch, then?
> > 
> > Yes, please.
> > 
> > Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > It still generates recursive warnings because the WARNON_ONCE is inlined and
> > every different usage will generate an exception, but it didn't produce a
> > stack overflow anymore.
> > To avoid the recursive warning the patch below would help. Not sure if it's
> > worth it...
> > 
> > Subject: [PATCH] rcu: move rcu_is_cpu_idle() check warning into C file
> > 
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > rcu_read_lock() and rcu_read_unlock() generate a warning if a cpu is in
> > extended quiescant state. Since these functions are inlined this can cause
> > a lot of warnings if in the processing of the WARN_ON_ONCE() there is
> > another usage of e.g. rcu_read_lock(). To make sure we only get one
> > warning (and avoid possible stack overflows) uninline the check.
> > 
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > ---
> >  include/linux/rcupdate.h |    9 +++++++--
> >  kernel/rcupdate.c        |    6 ++++++
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 81c04f4..9fe7be5 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -230,22 +230,27 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
> > 
> >  #ifdef CONFIG_PROVE_RCU
> >  extern int rcu_is_cpu_idle(void);
> > +extern void rcu_warn_if_is_cpu_idle(void);
> >  #else /* !CONFIG_PROVE_RCU */
> >  static inline int rcu_is_cpu_idle(void)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline void rcu_warn_if_is_cpu_idle(void)
> > +{
> > +}
> >  #endif /* else !CONFIG_PROVE_RCU */
> > 
> >  static inline void rcu_lock_acquire(struct lockdep_map *map)
> >  {
> > -	WARN_ON_ONCE(rcu_is_cpu_idle());
> > +	rcu_warn_if_is_cpu_idle();
> 
> Thank you for the patch, but this WARN_ON_ONCE() has now been removed
> in favor of lockdep-RCU checks elsewhere.  This has the advantage of
> leveraging lockdep's splat-once and anti-recursion facilities.
> 
> So I believe that current -rcu covers this.  (And yes, I do need to
> push my most recent changes out.)

This still uncovers cases where we call rcu_read_lock() without matching
rcu_dereference(). Amongst this we have rcu_dereference_raw(), conditional
rcu_dereference() and may be cases where we simply have no rcu_dereference*
but we use rcu_read_lock() alone for some reason...

Re: rcu warnings cause stack overflow

[Paul E. McKenney]

On Sat, Feb 04, 2012 at 02:13:35PM +0100, Frederic Weisbecker wrote:
> On Fri, Feb 03, 2012 at 10:33:35AM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 03, 2012 at 10:32:14AM +0100, Heiko Carstens wrote:
> > > On Thu, Feb 02, 2012 at 11:11:16AM -0800, Paul E. McKenney wrote:
> > > > On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> > > > > On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > > > > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > > > > So we probably want something better than the patch below.
> > > > > > > 
> > > > > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > > > > 
> > > > > > The reason was to reduce the code footprint of the WARN_ON() and also
> > > > > > be able to print the register contents at the time the warning happened.
> > > > > 
> > > > > Ah ok, makes sense.
> > > > 
> > > > So Frederic should push his anti-recursion patch, then?
> > > 
> > > Yes, please.
> > > 
> > > Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > 
> > > It still generates recursive warnings because the WARNON_ONCE is inlined and
> > > every different usage will generate an exception, but it didn't produce a
> > > stack overflow anymore.
> > > To avoid the recursive warning the patch below would help. Not sure if it's
> > > worth it...
> > > 
> > > Subject: [PATCH] rcu: move rcu_is_cpu_idle() check warning into C file
> > > 
> > > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > 
> > > rcu_read_lock() and rcu_read_unlock() generate a warning if a cpu is in
> > > extended quiescant state. Since these functions are inlined this can cause
> > > a lot of warnings if in the processing of the WARN_ON_ONCE() there is
> > > another usage of e.g. rcu_read_lock(). To make sure we only get one
> > > warning (and avoid possible stack overflows) uninline the check.
> > > 
> > > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > ---
> > >  include/linux/rcupdate.h |    9 +++++++--
> > >  kernel/rcupdate.c        |    6 ++++++
> > >  2 files changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 81c04f4..9fe7be5 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -230,22 +230,27 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
> > > 
> > >  #ifdef CONFIG_PROVE_RCU
> > >  extern int rcu_is_cpu_idle(void);
> > > +extern void rcu_warn_if_is_cpu_idle(void);
> > >  #else /* !CONFIG_PROVE_RCU */
> > >  static inline int rcu_is_cpu_idle(void)
> > >  {
> > >  	return 0;
> > >  }
> > > +
> > > +static inline void rcu_warn_if_is_cpu_idle(void)
> > > +{
> > > +}
> > >  #endif /* else !CONFIG_PROVE_RCU */
> > > 
> > >  static inline void rcu_lock_acquire(struct lockdep_map *map)
> > >  {
> > > -	WARN_ON_ONCE(rcu_is_cpu_idle());
> > > +	rcu_warn_if_is_cpu_idle();
> > 
> > Thank you for the patch, but this WARN_ON_ONCE() has now been removed
> > in favor of lockdep-RCU checks elsewhere.  This has the advantage of
> > leveraging lockdep's splat-once and anti-recursion facilities.
> > 
> > So I believe that current -rcu covers this.  (And yes, I do need to
> > push my most recent changes out.)
> 
> This still uncovers cases where we call rcu_read_lock() without matching
> rcu_dereference(). Amongst this we have rcu_dereference_raw(), conditional
> rcu_dereference() and may be cases where we simply have no rcu_dereference*
> but we use rcu_read_lock() alone for some reason...

Ah!  I moved the !rcu_is_cpu_idle() check to the read-side primitives,
for example:

	static inline void rcu_read_lock(void)
	{
		__rcu_read_lock();
		__acquire(RCU);
		rcu_lock_acquire(&rcu_lock_map);
		rcu_lockdep_assert(!rcu_is_cpu_idle(),
				   "rcu_read_lock() used illegally while idle");
	}

The reason I did this is that there is a good chance that it will be
necessary to allow SRCU readers on CPUs that the other flavors of RCU
believe to be idle.  One reason for this is that the KVM guys want your
tickless idle to apply to KVM guest processes, but they need the guest
to be in an SRCU read-side critical section during that same time.

Now, this would break given current synchronize_srcu(), but Peter
Zijlstra's recent SRCU patches might get us to a point where it would work.

Then of course there will be the challenge of making it scale, but one
thing at a time.  ;-)

							Thanx, Paul