[WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

Hi Peter,

I ran my tests on a series of patches on top of 5.9-rc4, and hit the
following splat:

 ------------[ cut here ]------------
 WARNING: CPU: 0 PID: 2557 at kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20
 Modules linked in: [..]
 CPU: 0 PID: 2557 Comm: ftracetest Tainted: G        W         5.9.0-rc4-test+ #499
 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
 RIP: 0010:rcu_irq_enter+0x15/0x20
 Code: 00 00 00 eb b6 0f 0b eb 81 66 66 2e 0f 1f 84 00 00 00 00 00 90 8b 05 86 6e c4 00 85 c0 74 0d 65 8b 05 db 71 4a 76 85 c0 74 02 <0f> 0b e9 34 ff ff ff 0f 1f 40 00 53 48 c7 c3 80 cc 02 00 e8 63 09
 RSP: 0018:ffff9372786538a0 EFLAGS: 00010002
 RAX: 0000000000000001 RBX: 0000000000000086 RCX: ffff937278654000
 RDX: 000000000001ec80 RSI: ffffffff890721f1 RDI: ffffffff8a677f20
 RBP: ffffffff890721f1 R08: 0000000000000000 R09: ffffffff8b58b430
 R10: ffff937278653a60 R11: 0000000000000001 R12: ffffffff890721f1
 R13: 0000000000000000 R14: ffff937278653920 R15: ffff9372cee128c0
 FS:  00007fde773f2740(0000) GS:ffff9372daa00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000564b58579d68 CR3: 000000007eebe001 CR4: 00000000001706f0
 Call Trace:
  rcu_irq_enter_irqson+0x21/0x40
  trace_preempt_off+0x6e/0xd0
  ? __unwind_start+0x18c/0x1e0
  unwind_next_frame+0x41/0x560
  ? noop_count+0x10/0x10
  __unwind_start+0x153/0x1e0
  ? profile_setup.cold+0xa1/0xa1
  arch_stack_walk+0x76/0x100
  ? __unwind_start+0x18c/0x1e0
  stack_trace_save+0x4b/0x70
  save_trace+0x42/0x350
  __lock_acquire+0x1858/0x2460
  lock_acquire+0xdc/0x3b0
  ? __sched_setscheduler+0x4d4/0x970
  cpuset_read_lock+0x26/0xc0
  ? __sched_setscheduler+0x4d4/0x970
  __sched_setscheduler+0x4d4/0x970
  ? trace_benchmark_reg+0x50/0x50
  _sched_setscheduler+0x68/0xa0
  __kthread_create_on_node+0x145/0x1c0
  ? perf_trace_benchmark_event+0x170/0x170
  kthread_create_on_node+0x51/0x70
  trace_benchmark_reg+0x28/0x50
  tracepoint_probe_register_prio+0x12f/0x310
  ? __mutex_unlock_slowpath+0x45/0x2a0
  __ftrace_event_enable_disable+0x75/0x240
  __ftrace_set_clr_event_nolock+0xef/0x130
  __ftrace_set_clr_event+0x39/0x60
  ftrace_set_clr_event+0x4a/0xa0
  ftrace_event_write+0xda/0x110
  vfs_write+0xca/0x210
  ksys_write+0x70/0xf0
  do_syscall_64+0x33/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 RIP: 0033:0x7fde774e7487
 Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
 RSP: 002b:00007ffeb8ec9d38 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fde774e7487
 RDX: 0000000000000004 RSI: 0000564b584eb690 RDI: 0000000000000001
 RBP: 0000564b584eb690 R08: 000000000000000a R09: 0000000000000003
 R10: 0000564b58544510 R11: 0000000000000246 R12: 0000000000000004
 R13: 00007fde775b8500 R14: 0000000000000004 R15: 00007fde775b8700
 irq event stamp: 108343
 hardirqs last  enabled at (108343): [<ffffffff89b6efbc>] exc_nmi+0xbc/0x160
 hardirqs last disabled at (108342): [<ffffffff89b6ef9d>] exc_nmi+0x9d/0x160
 softirqs last  enabled at (107622): [<ffffffff89e003b4>] __do_softirq+0x3b4/0x501
 softirqs last disabled at (107615): [<ffffffff89c01072>] asm_call_on_stack+0x12/0x20


What looks to have happened was:

  cpuset_read_lock()
     lockdep called
       save stack trace
          preempt_disable()
             trace_preempt_disable();
                rcu_irq_enter_irqson();
                   local_irq_save() (ignored by lockdep due to recursion set)
                      rcu_irq_enter();
                         lockdep_assert_irqs_disabled() (no, because it was ignored by recursion being set)

                          BOOM!


Thoughts?

Note, the warning goes away with the below patch.

-- Steve

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 6a584b3e5c74..3e5bc1dd71c6 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -550,7 +550,8 @@ do {									\
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
+           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
 } while (0)
 
 #define lockdep_assert_in_irq()						\

[WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

Hi Peter,

I ran my tests on a series of patches on top of 5.9-rc4, and hit the
following splat:

 ------------[ cut here ]------------
 WARNING: CPU: 0 PID: 2557 at kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20
 Modules linked in: [..]
 CPU: 0 PID: 2557 Comm: ftracetest Tainted: G        W         5.9.0-rc4-test+ #499
 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
 RIP: 0010:rcu_irq_enter+0x15/0x20
 Code: 00 00 00 eb b6 0f 0b eb 81 66 66 2e 0f 1f 84 00 00 00 00 00 90 8b 05 86 6e c4 00 85 c0 74 0d 65 8b 05 db 71 4a 76 85 c0 74 02 <0f> 0b e9 34 ff ff ff 0f 1f 40 00 53 48 c7 c3 80 cc 02 00 e8 63 09
 RSP: 0018:ffff9372786538a0 EFLAGS: 00010002
 RAX: 0000000000000001 RBX: 0000000000000086 RCX: ffff937278654000
 RDX: 000000000001ec80 RSI: ffffffff890721f1 RDI: ffffffff8a677f20
 RBP: ffffffff890721f1 R08: 0000000000000000 R09: ffffffff8b58b430
 R10: ffff937278653a60 R11: 0000000000000001 R12: ffffffff890721f1
 R13: 0000000000000000 R14: ffff937278653920 R15: ffff9372cee128c0
 FS:  00007fde773f2740(0000) GS:ffff9372daa00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000564b58579d68 CR3: 000000007eebe001 CR4: 00000000001706f0
 Call Trace:
  rcu_irq_enter_irqson+0x21/0x40
  trace_preempt_off+0x6e/0xd0
  ? __unwind_start+0x18c/0x1e0
  unwind_next_frame+0x41/0x560
  ? noop_count+0x10/0x10
  __unwind_start+0x153/0x1e0
  ? profile_setup.cold+0xa1/0xa1
  arch_stack_walk+0x76/0x100
  ? __unwind_start+0x18c/0x1e0
  stack_trace_save+0x4b/0x70
  save_trace+0x42/0x350
  __lock_acquire+0x1858/0x2460
  lock_acquire+0xdc/0x3b0
  ? __sched_setscheduler+0x4d4/0x970
  cpuset_read_lock+0x26/0xc0
  ? __sched_setscheduler+0x4d4/0x970
  __sched_setscheduler+0x4d4/0x970
  ? trace_benchmark_reg+0x50/0x50
  _sched_setscheduler+0x68/0xa0
  __kthread_create_on_node+0x145/0x1c0
  ? perf_trace_benchmark_event+0x170/0x170
  kthread_create_on_node+0x51/0x70
  trace_benchmark_reg+0x28/0x50
  tracepoint_probe_register_prio+0x12f/0x310
  ? __mutex_unlock_slowpath+0x45/0x2a0
  __ftrace_event_enable_disable+0x75/0x240
  __ftrace_set_clr_event_nolock+0xef/0x130
  __ftrace_set_clr_event+0x39/0x60
  ftrace_set_clr_event+0x4a/0xa0
  ftrace_event_write+0xda/0x110
  vfs_write+0xca/0x210
  ksys_write+0x70/0xf0
  do_syscall_64+0x33/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 RIP: 0033:0x7fde774e7487
 Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
 RSP: 002b:00007ffeb8ec9d38 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fde774e7487
 RDX: 0000000000000004 RSI: 0000564b584eb690 RDI: 0000000000000001
 RBP: 0000564b584eb690 R08: 000000000000000a R09: 0000000000000003
 R10: 0000564b58544510 R11: 0000000000000246 R12: 0000000000000004
 R13: 00007fde775b8500 R14: 0000000000000004 R15: 00007fde775b8700
 irq event stamp: 108343
 hardirqs last  enabled at (108343): [<ffffffff89b6efbc>] exc_nmi+0xbc/0x160
 hardirqs last disabled at (108342): [<ffffffff89b6ef9d>] exc_nmi+0x9d/0x160
 softirqs last  enabled at (107622): [<ffffffff89e003b4>] __do_softirq+0x3b4/0x501
 softirqs last disabled at (107615): [<ffffffff89c01072>] asm_call_on_stack+0x12/0x20


What looks to have happened was:

  cpuset_read_lock()
     lockdep called
       save stack trace
          preempt_disable()
             trace_preempt_disable();
                rcu_irq_enter_irqson();
                   local_irq_save() (ignored by lockdep due to recursion set)
                      rcu_irq_enter();
                         lockdep_assert_irqs_disabled() (no, because it was ignored by recursion being set)

                          BOOM!


Thoughts?

Note, the warning goes away with the below patch.

-- Steve

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 6a584b3e5c74..3e5bc1dd71c6 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -550,7 +550,8 @@ do {									\
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
+           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
 } while (0)
 
 #define lockdep_assert_in_irq()						\

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

Ping.

-- Steve


On Thu, 17 Sep 2020 13:18:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Peter,
> 
> I ran my tests on a series of patches on top of 5.9-rc4, and hit the
> following splat:
> 
>  ------------[ cut here ]------------
>  WARNING: CPU: 0 PID: 2557 at kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20
>  Modules linked in: [..]
>  CPU: 0 PID: 2557 Comm: ftracetest Tainted: G        W         5.9.0-rc4-test+ #499
>  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
>  RIP: 0010:rcu_irq_enter+0x15/0x20
>  Code: 00 00 00 eb b6 0f 0b eb 81 66 66 2e 0f 1f 84 00 00 00 00 00 90 8b 05 86 6e c4 00 85 c0 74 0d 65 8b 05 db 71 4a 76 85 c0 74 02 <0f> 0b e9 34 ff ff ff 0f 1f 40 00 53 48 c7 c3 80 cc 02 00 e8 63 09
>  RSP: 0018:ffff9372786538a0 EFLAGS: 00010002
>  RAX: 0000000000000001 RBX: 0000000000000086 RCX: ffff937278654000
>  RDX: 000000000001ec80 RSI: ffffffff890721f1 RDI: ffffffff8a677f20
>  RBP: ffffffff890721f1 R08: 0000000000000000 R09: ffffffff8b58b430
>  R10: ffff937278653a60 R11: 0000000000000001 R12: ffffffff890721f1
>  R13: 0000000000000000 R14: ffff937278653920 R15: ffff9372cee128c0
>  FS:  00007fde773f2740(0000) GS:ffff9372daa00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000564b58579d68 CR3: 000000007eebe001 CR4: 00000000001706f0
>  Call Trace:
>   rcu_irq_enter_irqson+0x21/0x40
>   trace_preempt_off+0x6e/0xd0
>   ? __unwind_start+0x18c/0x1e0
>   unwind_next_frame+0x41/0x560
>   ? noop_count+0x10/0x10
>   __unwind_start+0x153/0x1e0
>   ? profile_setup.cold+0xa1/0xa1
>   arch_stack_walk+0x76/0x100
>   ? __unwind_start+0x18c/0x1e0
>   stack_trace_save+0x4b/0x70
>   save_trace+0x42/0x350
>   __lock_acquire+0x1858/0x2460
>   lock_acquire+0xdc/0x3b0
>   ? __sched_setscheduler+0x4d4/0x970
>   cpuset_read_lock+0x26/0xc0
>   ? __sched_setscheduler+0x4d4/0x970
>   __sched_setscheduler+0x4d4/0x970
>   ? trace_benchmark_reg+0x50/0x50
>   _sched_setscheduler+0x68/0xa0
>   __kthread_create_on_node+0x145/0x1c0
>   ? perf_trace_benchmark_event+0x170/0x170
>   kthread_create_on_node+0x51/0x70
>   trace_benchmark_reg+0x28/0x50
>   tracepoint_probe_register_prio+0x12f/0x310
>   ? __mutex_unlock_slowpath+0x45/0x2a0
>   __ftrace_event_enable_disable+0x75/0x240
>   __ftrace_set_clr_event_nolock+0xef/0x130
>   __ftrace_set_clr_event+0x39/0x60
>   ftrace_set_clr_event+0x4a/0xa0
>   ftrace_event_write+0xda/0x110
>   vfs_write+0xca/0x210
>   ksys_write+0x70/0xf0
>   do_syscall_64+0x33/0x40
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>  RIP: 0033:0x7fde774e7487
>  Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
>  RSP: 002b:00007ffeb8ec9d38 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>  RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fde774e7487
>  RDX: 0000000000000004 RSI: 0000564b584eb690 RDI: 0000000000000001
>  RBP: 0000564b584eb690 R08: 000000000000000a R09: 0000000000000003
>  R10: 0000564b58544510 R11: 0000000000000246 R12: 0000000000000004
>  R13: 00007fde775b8500 R14: 0000000000000004 R15: 00007fde775b8700
>  irq event stamp: 108343
>  hardirqs last  enabled at (108343): [<ffffffff89b6efbc>] exc_nmi+0xbc/0x160
>  hardirqs last disabled at (108342): [<ffffffff89b6ef9d>] exc_nmi+0x9d/0x160
>  softirqs last  enabled at (107622): [<ffffffff89e003b4>] __do_softirq+0x3b4/0x501
>  softirqs last disabled at (107615): [<ffffffff89c01072>] asm_call_on_stack+0x12/0x20
> 
> 
> What looks to have happened was:
> 
>   cpuset_read_lock()
>      lockdep called
>        save stack trace
>           preempt_disable()
>              trace_preempt_disable();
>                 rcu_irq_enter_irqson();
>                    local_irq_save() (ignored by lockdep due to recursion set)
>                       rcu_irq_enter();
>                          lockdep_assert_irqs_disabled() (no, because it was ignored by recursion being set)
> 
>                           BOOM!
> 
> 
> Thoughts?
> 
> Note, the warning goes away with the below patch.
> 
> -- Steve
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..3e5bc1dd71c6 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -550,7 +550,8 @@ do {									\
>  
>  #define lockdep_assert_irqs_disabled()					\
>  do {									\
> -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
>  } while (0)
>  
>  #define lockdep_assert_in_irq()						\

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Peter Zijlstra]

On Thu, Sep 17, 2020 at 01:16:46PM -0400, Steven Rostedt wrote:
> Hi Peter,
> 
> I ran my tests on a series of patches on top of 5.9-rc4, and hit the
> following splat:
> 
>  ------------[ cut here ]------------
>  WARNING: CPU: 0 PID: 2557 at kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20
>  RIP: 0010:rcu_irq_enter+0x15/0x20

>  Call Trace:
>   rcu_irq_enter_irqson+0x21/0x40
>   trace_preempt_off+0x6e/0xd0
>   unwind_next_frame+0x41/0x560
>   __unwind_start+0x153/0x1e0
>   arch_stack_walk+0x76/0x100
>   stack_trace_save+0x4b/0x70
>   save_trace+0x42/0x350
>   __lock_acquire+0x1858/0x2460
>   lock_acquire+0xdc/0x3b0
>   cpuset_read_lock+0x26/0xc0

> What looks to have happened was:
> 
>   cpuset_read_lock()
>      lockdep called
>        save stack trace
>           preempt_disable()
>              trace_preempt_disable();
>                 rcu_irq_enter_irqson();
>                    local_irq_save() (ignored by lockdep due to recursion set)
>                       rcu_irq_enter();
>                          lockdep_assert_irqs_disabled() (no, because it was ignored by recursion being set)
> 
>                           BOOM!
> 
> 
> Thoughts?

> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..3e5bc1dd71c6 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -550,7 +550,8 @@ do {									\
>  
>  #define lockdep_assert_irqs_disabled()					\
>  do {									\
> -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
>  } while (0)

Blergh, IIRC there's header hell that way. The sane fix is killing off
that trace_*_rcuidle() disease.

But I think this will also cure it.

---
diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 6a339ce328e0..4f90293d170b 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -432,7 +432,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		return false;
 
 	/* Don't let modules unload while we're reading their ORC data. */
-	preempt_disable();
+	preempt_disable_notrace();
 
 	/* End-of-stack check for user tasks: */
 	if (state->regs && user_mode(state->regs))
@@ -612,14 +612,14 @@ bool unwind_next_frame(struct unwind_state *state)
 		goto err;
 	}
 
-	preempt_enable();
+	preempt_enable_notrace();
 	return true;
 
 err:
 	state->error = true;
 
 the_end:
-	preempt_enable();
+	preempt_enable_notrace();
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
 }

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

On Wed, 30 Sep 2020 20:13:23 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

>  diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> > index 6a584b3e5c74..3e5bc1dd71c6 100644
> > --- a/include/linux/lockdep.h
> > +++ b/include/linux/lockdep.h
> > @@ -550,7 +550,8 @@ do {									\
> >  
> >  #define lockdep_assert_irqs_disabled()					\
> >  do {									\
> > -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> > +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> > +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
> >  } while (0)  
> 
> Blergh, IIRC there's header hell that way. The sane fix is killing off
> that trace_*_rcuidle() disease.

Really?

I could run this through all my other tests to see if that is the case.
That is, to see if it stumbles across header hell.

> 
> But I think this will also cure it.
> 
> ---
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 6a339ce328e0..4f90293d170b 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -432,7 +432,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  		return false;
>  
>  	/* Don't let modules unload while we're reading their ORC data. */
> -	preempt_disable();
> +	preempt_disable_notrace();
>  
>  	/* End-of-stack check for user tasks: */
>  	if (state->regs && user_mode(state->regs))
> @@ -612,14 +612,14 @@ bool unwind_next_frame(struct unwind_state *state)
>  		goto err;
>  	}
>  
> -	preempt_enable();
> +	preempt_enable_notrace();
>  	return true;
>  
>  err:
>  	state->error = true;
>  
>  the_end:
> -	preempt_enable();
> +	preempt_enable_notrace();
>  	state->stack_info.type = STACK_TYPE_UNKNOWN;
>  	return false;
>  }

I think you are going to play whack-a-mole with this approach. This will
happen anytime preempt_disable is being traced within lockdep internal code.

I just hit this:

register_lock_class
  assign_lock_key
    __is_module_percpu_address
      preempt_disable
         trace_preempt_disable
            rcu_irq_enter_irqson
              [..]


Same thing, different path.

-- Steve

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Peter Zijlstra]

On Wed, Sep 30, 2020 at 03:10:26PM -0400, Steven Rostedt wrote:
> On Wed, 30 Sep 2020 20:13:23 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> >  diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> > > index 6a584b3e5c74..3e5bc1dd71c6 100644
> > > --- a/include/linux/lockdep.h
> > > +++ b/include/linux/lockdep.h
> > > @@ -550,7 +550,8 @@ do {									\
> > >  
> > >  #define lockdep_assert_irqs_disabled()					\
> > >  do {									\
> > > -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> > > +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> > > +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
> > >  } while (0)  
> > 
> > Blergh, IIRC there's header hell that way. The sane fix is killing off
> > that trace_*_rcuidle() disease.
> 
> Really?
> 
> I could run this through all my other tests to see if that is the case.
> That is, to see if it stumbles across header hell.

I went through a lot of pain to make that per-cpu to avoid using
current. But that might've been driven by
lockdep_assert_preemption_disabled(), which is used in seqlock.h which
in turn is included all over the place.

That said, there's at least two things we can do:

 - make lockdep_recursion per-cpu too, IIRC we only ever set that when
   we have IRQs disabled anyway.

OR

 - inspired by the above, as can save/clear - restore hardirqs_enabled
   when we frob lockdep_recursion.

Admittedly, the second is somewhat gross :-)

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

On Wed, 30 Sep 2020 21:22:42 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Sep 30, 2020 at 03:10:26PM -0400, Steven Rostedt wrote:
> > On Wed, 30 Sep 2020 20:13:23 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> > >  diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h  
> > > > index 6a584b3e5c74..3e5bc1dd71c6 100644
> > > > --- a/include/linux/lockdep.h
> > > > +++ b/include/linux/lockdep.h
> > > > @@ -550,7 +550,8 @@ do {									\
> > > >  
> > > >  #define lockdep_assert_irqs_disabled()					\
> > > >  do {									\
> > > > -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> > > > +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> > > > +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
> > > >  } while (0)    
> > > 
> > > Blergh, IIRC there's header hell that way. The sane fix is killing off
> > > that trace_*_rcuidle() disease.  
> > 
> > Really?
> > 
> > I could run this through all my other tests to see if that is the case.
> > That is, to see if it stumbles across header hell.  
> 
> I went through a lot of pain to make that per-cpu to avoid using
> current. But that might've been driven by
> lockdep_assert_preemption_disabled(), which is used in seqlock.h which
> in turn is included all over the place.
> 
> That said, there's at least two things we can do:
> 
>  - make lockdep_recursion per-cpu too, IIRC we only ever set that when
>    we have IRQs disabled anyway.
> 
> OR
> 
>  - inspired by the above, as can save/clear - restore hardirqs_enabled
>    when we frob lockdep_recursion.
> 
> Admittedly, the second is somewhat gross :-)

I think making lockdep_recursion percpu sounds like the best approach.

-- Steve

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Peter Zijlstra]

On Wed, Sep 30, 2020 at 03:52:22PM -0400, Steven Rostedt wrote:
> On Wed, 30 Sep 2020 21:22:42 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Sep 30, 2020 at 03:10:26PM -0400, Steven Rostedt wrote:
> > > On Wed, 30 Sep 2020 20:13:23 +0200
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > >   
> > > >  diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h  
> > > > > index 6a584b3e5c74..3e5bc1dd71c6 100644
> > > > > --- a/include/linux/lockdep.h
> > > > > +++ b/include/linux/lockdep.h
> > > > > @@ -550,7 +550,8 @@ do {									\
> > > > >  
> > > > >  #define lockdep_assert_irqs_disabled()					\
> > > > >  do {									\
> > > > > -	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
> > > > > +	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled) &&	\
> > > > > +           likely(!(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)));\
> > > > >  } while (0)    
> > > > 
> > > > Blergh, IIRC there's header hell that way. The sane fix is killing off
> > > > that trace_*_rcuidle() disease.  
> > > 
> > > Really?
> > > 
> > > I could run this through all my other tests to see if that is the case.
> > > That is, to see if it stumbles across header hell.  
> > 
> > I went through a lot of pain to make that per-cpu to avoid using
> > current. But that might've been driven by
> > lockdep_assert_preemption_disabled(), which is used in seqlock.h which
> > in turn is included all over the place.
> > 
> > That said, there's at least two things we can do:
> > 
> >  - make lockdep_recursion per-cpu too, IIRC we only ever set that when
> >    we have IRQs disabled anyway.
> > 
> > OR
> > 
> >  - inspired by the above, as can save/clear - restore hardirqs_enabled
> >    when we frob lockdep_recursion.
> > 
> > Admittedly, the second is somewhat gross :-)
> 
> I think making lockdep_recursion percpu sounds like the best approach.
> 
> -- Steve

Bugger, we use current->lockdep_recursion also for lockdep_off(); I
always forget about that thing.

Anyway, something like so then.

---
 include/linux/lockdep.h  |  13 +++---
 kernel/locking/lockdep.c | 101 ++++++++++++++++++++++++++++-------------------
 2 files changed, 68 insertions(+), 46 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 57d642d378c7..1cc982536640 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -559,6 +559,7 @@ do {									\
 
 DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
+DECLARE_PER_CPU(unsigned int, lockdep_recursion);
 
 /*
  * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
@@ -568,25 +569,27 @@ DECLARE_PER_CPU(int, hardirq_context);
  * read the value from our previous CPU.
  */
 
+#define __lockdep_enabled	(debug_locks && !raw_cpu_read(lockdep_recursion))
+
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled));	\
+	WARN_ON_ONCE(__lockdep_enabled && raw_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context));	\
+	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirq_context)); \
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
 do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
-		     debug_locks			&&		\
+		     __lockdep_enabled			&&		\
 		     (preempt_count() != 0		||		\
 		      !raw_cpu_read(hardirqs_enabled)));		\
 } while (0)
@@ -594,7 +597,7 @@ do {									\
 #define lockdep_assert_preemption_disabled()				\
 do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
-		     debug_locks			&&		\
+		     __lockdep_enabled			&&		\
 		     (preempt_count() == 0		&&		\
 		      raw_cpu_read(hardirqs_enabled)));			\
 } while (0)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 454355c033d2..47942557b4fb 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -76,6 +76,22 @@ module_param(lock_stat, int, 0644);
 #define lock_stat 0
 #endif
 
+DEFINE_PER_CPU(unsigned int, lockdep_recursion);
+
+static inline bool lockdep_enabled(void)
+{
+	if (!debug_locks)
+		return false;
+
+	if (__this_cpu_read(lockdep_recursion))
+		return false;
+
+	if (current->lockdep_recursion)
+		return false;
+
+	return true;
+}
+
 /*
  * lockdep_lock: protects the lockdep graph, the hashes and the
  *               class/list/hash allocators.
@@ -93,7 +109,7 @@ static inline void lockdep_lock(void)
 
 	arch_spin_lock(&__lock);
 	__owner = current;
-	current->lockdep_recursion++;
+	__this_cpu_inc(lockdep_recursion);
 }
 
 static inline void lockdep_unlock(void)
@@ -101,7 +117,7 @@ static inline void lockdep_unlock(void)
 	if (debug_locks && DEBUG_LOCKS_WARN_ON(__owner != current))
 		return;
 
-	current->lockdep_recursion--;
+	__this_cpu_dec(lockdep_recursion);
 	__owner = NULL;
 	arch_spin_unlock(&__lock);
 }
@@ -408,10 +424,15 @@ void lockdep_init_task(struct task_struct *task)
 	task->lockdep_recursion = 0;
 }
 
+static __always_inline void lockdep_recursion_inc(void)
+{
+	__this_cpu_inc(lockdep_recursion);
+}
+
 static __always_inline void lockdep_recursion_finish(void)
 {
-	if (WARN_ON_ONCE((--current->lockdep_recursion) & LOCKDEP_RECURSION_MASK))
-		current->lockdep_recursion = 0;
+	if (WARN_ON_ONCE(__this_cpu_dec_return(lockdep_recursion)))
+		__this_cpu_write(lockdep_recursion, 0);
 }
 
 void lockdep_set_selftest_task(struct task_struct *task)
@@ -4012,7 +4033,7 @@ void lockdep_hardirqs_on_prepare(unsigned long ip)
 	if (unlikely(in_nmi()))
 		return;
 
-	if (unlikely(current->lockdep_recursion & LOCKDEP_RECURSION_MASK))
+	if (unlikely(__this_cpu_read(lockdep_recursion)))
 		return;
 
 	if (unlikely(lockdep_hardirqs_enabled())) {
@@ -4048,7 +4069,7 @@ void lockdep_hardirqs_on_prepare(unsigned long ip)
 
 	current->hardirq_chain_key = current->curr_chain_key;
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__trace_hardirqs_on_caller();
 	lockdep_recursion_finish();
 }
@@ -4081,7 +4102,7 @@ void noinstr lockdep_hardirqs_on(unsigned long ip)
 		goto skip_checks;
 	}
 
-	if (unlikely(current->lockdep_recursion & LOCKDEP_RECURSION_MASK))
+	if (unlikely(__this_cpu_read(lockdep_recursion)))
 		return;
 
 	if (lockdep_hardirqs_enabled()) {
@@ -4134,7 +4155,7 @@ void noinstr lockdep_hardirqs_off(unsigned long ip)
 	if (in_nmi()) {
 		if (!IS_ENABLED(CONFIG_TRACE_IRQFLAGS_NMI))
 			return;
-	} else if (current->lockdep_recursion & LOCKDEP_RECURSION_MASK)
+	} else if (__this_cpu_read(lockdep_recursion))
 		return;
 
 	/*
@@ -4167,7 +4188,7 @@ void lockdep_softirqs_on(unsigned long ip)
 {
 	struct irqtrace_events *trace = &current->irqtrace;
 
-	if (unlikely(!debug_locks || current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	/*
@@ -4182,7 +4203,7 @@ void lockdep_softirqs_on(unsigned long ip)
 		return;
 	}
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	/*
 	 * We'll do an OFF -> ON transition:
 	 */
@@ -4205,7 +4226,7 @@ void lockdep_softirqs_on(unsigned long ip)
  */
 void lockdep_softirqs_off(unsigned long ip)
 {
-	if (unlikely(!debug_locks || current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	/*
@@ -4590,11 +4611,11 @@ void lockdep_init_map_waits(struct lockdep_map *lock, const char *name,
 	if (subclass) {
 		unsigned long flags;
 
-		if (DEBUG_LOCKS_WARN_ON(current->lockdep_recursion))
+		if (DEBUG_LOCKS_WARN_ON(!lockdep_enabled()))
 			return;
 
 		raw_local_irq_save(flags);
-		current->lockdep_recursion++;
+		lockdep_recursion_inc();
 		register_lock_class(lock, subclass, 1);
 		lockdep_recursion_finish();
 		raw_local_irq_restore(flags);
@@ -5277,11 +5298,11 @@ void lock_set_class(struct lockdep_map *lock, const char *name,
 {
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	check_flags(flags);
 	if (__lock_set_class(lock, name, key, subclass, ip))
 		check_chain_key(current);
@@ -5294,11 +5315,11 @@ void lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	check_flags(flags);
 	if (__lock_downgrade(lock, ip))
 		check_chain_key(current);
@@ -5336,7 +5357,7 @@ static void verify_lock_unused(struct lockdep_map *lock, struct held_lock *hlock
 
 static bool lockdep_nmi(void)
 {
-	if (current->lockdep_recursion & LOCKDEP_RECURSION_MASK)
+	if (__this_cpu_read(lockdep_recursion))
 		return false;
 
 	if (!in_nmi())
@@ -5371,7 +5392,11 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 
 	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
 
-	if (unlikely(current->lockdep_recursion)) {
+	if (!debug_locks)
+		return;
+
+	raw_local_irq_save(flags);
+	if (unlikely(!lockdep_enabled())) {
 		/* XXX allow trylock from NMI ?!? */
 		if (lockdep_nmi() && !trylock) {
 			struct held_lock hlock;
@@ -5388,13 +5413,13 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 
 			verify_lock_unused(lock, &hlock, subclass);
 		}
+		raw_local_irq_restore(flags);
 		return;
 	}
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__lock_acquire(lock, subclass, trylock, read, check,
 		       irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
 	lockdep_recursion_finish();
@@ -5408,13 +5433,13 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_release(lock, ip);
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	if (__lock_release(lock, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
@@ -5427,13 +5452,13 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)
 	unsigned long flags;
 	int ret = 0;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return 1; /* avoid false negative lockdep_assert_held() */
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	ret = __lock_is_held(lock, read);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5448,13 +5473,13 @@ struct pin_cookie lock_pin_lock(struct lockdep_map *lock)
 	struct pin_cookie cookie = NIL_COOKIE;
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return cookie;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	cookie = __lock_pin_lock(lock);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5467,13 +5492,13 @@ void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 {
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__lock_repin_lock(lock, cookie);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5484,13 +5509,13 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 {
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
 
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__lock_unpin_lock(lock, cookie);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5620,15 +5645,12 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_acquired(lock, ip);
 
-	if (unlikely(!lock_stat || !debug_locks))
-		return;
-
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lock_stat || !lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__lock_contended(lock, ip);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);
@@ -5641,15 +5663,12 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_contended(lock, ip);
 
-	if (unlikely(!lock_stat || !debug_locks))
-		return;
-
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(!lock_stat || !lockdep_enabled()))
 		return;
 
 	raw_local_irq_save(flags);
 	check_flags(flags);
-	current->lockdep_recursion++;
+	lockdep_recursion_inc();
 	__lock_acquired(lock, ip);
 	lockdep_recursion_finish();
 	raw_local_irq_restore(flags);

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Steven Rostedt]

On Wed, 30 Sep 2020 20:13:23 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Blergh, IIRC there's header hell that way. The sane fix is killing off
> that trace_*_rcuidle() disease.
> 
> But I think this will also cure it.

I guess you still don't build modules ;-). I had to add a
EXPORT_SYMBOL(lockdep_recursion) to get it to build, and then move the
checks within the irq disabling to get rid of the using cpu pointers within
preemptable code warnings

But it appears to solve the problem.

-- Steve

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 0e100c9784a5..70610f217b4e 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -77,6 +77,7 @@ module_param(lock_stat, int, 0644);
 #endif
 
 DEFINE_PER_CPU(unsigned int, lockdep_recursion);
+EXPORT_SYMBOL(lockdep_recursion);
 
 static inline bool lockdep_enabled(void)
 {
@@ -4241,13 +4242,13 @@ void lockdep_init_map_waits(struct lockdep_map *lock, const char *name,
 	if (subclass) {
 		unsigned long flags;
 
-		if (DEBUG_LOCKS_WARN_ON(!lockdep_enabled()))
-			return;
-
 		raw_local_irq_save(flags);
+		if (DEBUG_LOCKS_WARN_ON(!lockdep_enabled()))
+			goto out;
 		lockdep_recursion_inc();
 		register_lock_class(lock, subclass, 1);
 		lockdep_recursion_finish();
+out:
 		raw_local_irq_restore(flags);
 	}
 }
@@ -4928,15 +4929,15 @@ void lock_set_class(struct lockdep_map *lock, const char *name,
 {
 	unsigned long flags;
 
-	if (unlikely(!lockdep_enabled()))
-		return;
-
 	raw_local_irq_save(flags);
+	if (unlikely(!lockdep_enabled()))
+		goto out;
 	lockdep_recursion_inc();
 	check_flags(flags);
 	if (__lock_set_class(lock, name, key, subclass, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_set_class);
@@ -4945,15 +4946,15 @@ void lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
-	if (unlikely(!lockdep_enabled()))
-		return;
-
 	raw_local_irq_save(flags);
+	if (unlikely(!lockdep_enabled()))
+		goto out;
 	lockdep_recursion_inc();
 	check_flags(flags);
 	if (__lock_downgrade(lock, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_downgrade);
@@ -5041,16 +5042,18 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_release(lock, ip);
 
+	raw_local_irq_save(flags);
+
 	if (unlikely(!lockdep_enabled()))
-		return;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 
 	lockdep_recursion_inc();
 	if (__lock_release(lock, ip))
 		check_chain_key(current);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_release);
@@ -5060,15 +5063,17 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)
 	unsigned long flags;
 	int ret = 0;
 
-	if (unlikely(!lockdep_enabled()))
-		return 1; /* avoid false negative lockdep_assert_held() */
-
 	raw_local_irq_save(flags);
+	if (unlikely(!lockdep_enabled())) {
+		ret = 1; /* avoid false negative lockdep_assert_held() */
+		goto out;
+	}
 	check_flags(flags);
 
 	lockdep_recursion_inc();
 	ret = __lock_is_held(lock, read);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 
 	return ret;
@@ -5081,15 +5086,16 @@ struct pin_cookie lock_pin_lock(struct lockdep_map *lock)
 	struct pin_cookie cookie = NIL_COOKIE;
 	unsigned long flags;
 
+	raw_local_irq_save(flags);
 	if (unlikely(!lockdep_enabled()))
-		return cookie;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 
 	lockdep_recursion_inc();
 	cookie = __lock_pin_lock(lock);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 
 	return cookie;
@@ -5100,15 +5106,16 @@ void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 {
 	unsigned long flags;
 
+	raw_local_irq_save(flags);
 	if (unlikely(!lockdep_enabled()))
-		return;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 
 	lockdep_recursion_inc();
 	__lock_repin_lock(lock, cookie);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_repin_lock);
@@ -5117,15 +5124,16 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 {
 	unsigned long flags;
 
+	raw_local_irq_save(flags);
 	if (unlikely(!lockdep_enabled()))
-		return;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 
 	lockdep_recursion_inc();
 	__lock_unpin_lock(lock, cookie);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_unpin_lock);
@@ -5253,14 +5261,15 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_acquired(lock, ip);
 
+	raw_local_irq_save(flags);
 	if (unlikely(!lock_stat || !lockdep_enabled()))
-		return;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 	lockdep_recursion_inc();
 	__lock_contended(lock, ip);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_contended);
@@ -5271,14 +5280,15 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_contended(lock, ip);
 
+	raw_local_irq_save(flags);
 	if (unlikely(!lock_stat || !lockdep_enabled()))
-		return;
+		goto out;
 
-	raw_local_irq_save(flags);
 	check_flags(flags);
 	lockdep_recursion_inc();
 	__lock_acquired(lock, ip);
 	lockdep_recursion_finish();
+out:
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lock_acquired);

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Peter Zijlstra]

On Fri, Oct 02, 2020 at 01:56:44PM -0400, Steven Rostedt wrote:
> On Wed, 30 Sep 2020 20:13:23 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Blergh, IIRC there's header hell that way. The sane fix is killing off
> > that trace_*_rcuidle() disease.
> > 
> > But I think this will also cure it.
> 
> I guess you still don't build modules ;-). I had to add a
> EXPORT_SYMBOL(lockdep_recursion) to get it to build, and then move the

Correct, my regular configs are all without modules.

> checks within the irq disabling to get rid of the using cpu pointers within
> preemptable code warnings

Ah, I think I lost a s/__this_cpu_read/raw_cpu_read/ somewhere. The
thing is, if we're preemptible/migratable it will be 0 on both CPUs and
it doesn't matter which 0 we read. If it is !0, IRQs will be disabled
and we can't get migrated.

Anyway, let me go write a Changelog to go with it.

Re: [WARNING] kernel/rcu/tree.c:1058 rcu_irq_enter+0x15/0x20

[Peter Zijlstra]

On Fri, Oct 02, 2020 at 08:13:13PM +0200, Peter Zijlstra wrote:
> > checks within the irq disabling to get rid of the using cpu pointers within
> > preemptable code warnings
> 
> Ah, I think I lost a s/__this_cpu_read/raw_cpu_read/ somewhere. The
> thing is, if we're preemptible/migratable it will be 0 on both CPUs and
> it doesn't matter which 0 we read. If it is !0, IRQs will be disabled
> and we can't get migrated.

Aargh, this isn't in fact correct, and that means I have to revert:

  fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")

The trouble is that on a bunch of architectures we can read the other
CPUs variable from the new CPU, long after the old CPU has continued
executing things.

So this really needs to be this_cpu_read(). Luckily Sven has already
fixed s390, but let me go audit all the other archs.

[PATCH] lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"

[Peter Zijlstra]

On Mon, Oct 05, 2020 at 09:52:06AM +0200, Peter Zijlstra wrote:
> On Fri, Oct 02, 2020 at 08:13:13PM +0200, Peter Zijlstra wrote:
> > > checks within the irq disabling to get rid of the using cpu pointers within
> > > preemptable code warnings
> > 
> > Ah, I think I lost a s/__this_cpu_read/raw_cpu_read/ somewhere. The
> > thing is, if we're preemptible/migratable it will be 0 on both CPUs and
> > it doesn't matter which 0 we read. If it is !0, IRQs will be disabled
> > and we can't get migrated.
> 
> Aargh, this isn't in fact correct, and that means I have to revert:
> 
>   fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")
> 
> The trouble is that on a bunch of architectures we can read the other
> CPUs variable from the new CPU, long after the old CPU has continued
> executing things.
> 
> So this really needs to be this_cpu_read(). Luckily Sven has already
> fixed s390, but let me go audit all the other archs.

---
Subject: lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"
From: Peter Zijlstra <peterz@infradead.org>
Date: Mon Oct 5 09:56:57 CEST 2020

The thinking in commit:

  fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")

is flawed. While it is true that when we're migratable both CPUs will
have a 0 value, it doesn't hold that when we do get migrated in the
middle of a raw_cpu_op(), the old CPU will still have 0 by the time we
get around to reading it on the new CPU.

Luckily, the reason for that commit (s390 using preempt_disable()
instead of preempt_disable_notrace() in their percpu code), has since
been fixed by commit:

  1196f12a2c96 ("s390: don't trace preemption in percpu macros")

An audit of arch/*/include/asm/percpu*.h shows there are no other
architectures affected by this particular issue.

Fixes: fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/lockdep.h |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -537,19 +537,19 @@ do {									\
 #define lock_map_release(l)			lock_release(l, _THIS_IP_)
 
 #ifdef CONFIG_PROVE_LOCKING
-# define might_lock(lock) 						\
+# define might_lock(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 0, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_read(lock) 						\
+# define might_lock_read(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 1, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_nested(lock, subclass) 				\
+# define might_lock_nested(lock, subclass)				\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, subclass, 0, 1, 1, NULL,		\
@@ -561,29 +561,21 @@ DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 DECLARE_PER_CPU(unsigned int, lockdep_recursion);
 
-/*
- * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
- * per-cpu variables. This is required because this_cpu_read() will potentially
- * call into preempt/irq-disable and that obviously isn't right. This is also
- * correct because when IRQs are enabled, it doesn't matter if we accidentally
- * read the value from our previous CPU.
- */
-
-#define __lockdep_enabled	(debug_locks && !raw_cpu_read(lockdep_recursion))
+#define __lockdep_enabled	(debug_locks && !this_cpu_read(lockdep_recursion))
 
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirq_context)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirq_context)); \
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
@@ -591,7 +583,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() != 0		||		\
-		      !raw_cpu_read(hardirqs_enabled)));		\
+		      !this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #define lockdep_assert_preemption_disabled()				\
@@ -599,7 +591,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() == 0		&&		\
-		      raw_cpu_read(hardirqs_enabled)));			\
+		      this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #else

[tip: locking/core] lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     820d8a6c966343059ba58b9a82f570f27bf147d6
Gitweb:        https://git.kernel.org/tip/820d8a6c966343059ba58b9a82f570f27bf147d6
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Mon, 05 Oct 2020 09:56:57 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Oct 2020 18:14:17 +02:00

lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"

The thinking in commit:

  fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")

is flawed. While it is true that when we're migratable both CPUs will
have a 0 value, it doesn't hold that when we do get migrated in the
middle of a raw_cpu_op(), the old CPU will still have 0 by the time we
get around to reading it on the new CPU.

Luckily, the reason for that commit (s390 using preempt_disable()
instead of preempt_disable_notrace() in their percpu code), has since
been fixed by commit:

  1196f12a2c96 ("s390: don't trace preemption in percpu macros")

An audit of arch/*/include/asm/percpu*.h shows there are no other
architectures affected by this particular issue.

Fixes: fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20201005095958.GJ2651@hirez.programming.kicks-ass.net
---
 include/linux/lockdep.h | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1cc9825..f559487 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -537,19 +537,19 @@ do {									\
 #define lock_map_release(l)			lock_release(l, _THIS_IP_)
 
 #ifdef CONFIG_PROVE_LOCKING
-# define might_lock(lock) 						\
+# define might_lock(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 0, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_read(lock) 						\
+# define might_lock_read(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 1, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_nested(lock, subclass) 				\
+# define might_lock_nested(lock, subclass)				\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, subclass, 0, 1, 1, NULL,		\
@@ -561,29 +561,21 @@ DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 DECLARE_PER_CPU(unsigned int, lockdep_recursion);
 
-/*
- * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
- * per-cpu variables. This is required because this_cpu_read() will potentially
- * call into preempt/irq-disable and that obviously isn't right. This is also
- * correct because when IRQs are enabled, it doesn't matter if we accidentally
- * read the value from our previous CPU.
- */
-
-#define __lockdep_enabled	(debug_locks && !raw_cpu_read(lockdep_recursion))
+#define __lockdep_enabled	(debug_locks && !this_cpu_read(lockdep_recursion))
 
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirq_context)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirq_context)); \
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
@@ -591,7 +583,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() != 0		||		\
-		      !raw_cpu_read(hardirqs_enabled)));		\
+		      !this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #define lockdep_assert_preemption_disabled()				\
@@ -599,7 +591,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() == 0		&&		\
-		      raw_cpu_read(hardirqs_enabled)));			\
+		      this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #else

[tip: locking/core] lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     baffd723e44dc3d7f84f0b8f1fe1ece00ddd2710
Gitweb:        https://git.kernel.org/tip/baffd723e44dc3d7f84f0b8f1fe1ece00ddd2710
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Mon, 05 Oct 2020 09:56:57 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 09 Oct 2020 08:54:00 +02:00

lockdep: Revert "lockdep: Use raw_cpu_*() for per-cpu variables"

The thinking in commit:

  fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")

is flawed. While it is true that when we're migratable both CPUs will
have a 0 value, it doesn't hold that when we do get migrated in the
middle of a raw_cpu_op(), the old CPU will still have 0 by the time we
get around to reading it on the new CPU.

Luckily, the reason for that commit (s390 using preempt_disable()
instead of preempt_disable_notrace() in their percpu code), has since
been fixed by commit:

  1196f12a2c96 ("s390: don't trace preemption in percpu macros")

An audit of arch/*/include/asm/percpu*.h shows there are no other
architectures affected by this particular issue.

Fixes: fddf9055a60d ("lockdep: Use raw_cpu_*() for per-cpu variables")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lkml.kernel.org/r/20201005095958.GJ2651@hirez.programming.kicks-ass.net
---
 include/linux/lockdep.h | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index b1227be..1130f27 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -512,19 +512,19 @@ static inline void print_irqtrace_events(struct task_struct *curr)
 #define lock_map_release(l)			lock_release(l, _THIS_IP_)
 
 #ifdef CONFIG_PROVE_LOCKING
-# define might_lock(lock) 						\
+# define might_lock(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 0, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_read(lock) 						\
+# define might_lock_read(lock)						\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, 0, 0, 1, 1, NULL, _THIS_IP_);	\
 	lock_release(&(lock)->dep_map, _THIS_IP_);			\
 } while (0)
-# define might_lock_nested(lock, subclass) 				\
+# define might_lock_nested(lock, subclass)				\
 do {									\
 	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
 	lock_acquire(&(lock)->dep_map, subclass, 0, 1, 1, NULL,		\
@@ -536,29 +536,21 @@ DECLARE_PER_CPU(int, hardirqs_enabled);
 DECLARE_PER_CPU(int, hardirq_context);
 DECLARE_PER_CPU(unsigned int, lockdep_recursion);
 
-/*
- * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
- * per-cpu variables. This is required because this_cpu_read() will potentially
- * call into preempt/irq-disable and that obviously isn't right. This is also
- * correct because when IRQs are enabled, it doesn't matter if we accidentally
- * read the value from our previous CPU.
- */
-
-#define __lockdep_enabled	(debug_locks && !raw_cpu_read(lockdep_recursion))
+#define __lockdep_enabled	(debug_locks && !this_cpu_read(lockdep_recursion))
 
 #define lockdep_assert_irqs_enabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_irqs_disabled()					\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && raw_cpu_read(hardirqs_enabled)); \
+	WARN_ON_ONCE(__lockdep_enabled && this_cpu_read(hardirqs_enabled)); \
 } while (0)
 
 #define lockdep_assert_in_irq()						\
 do {									\
-	WARN_ON_ONCE(__lockdep_enabled && !raw_cpu_read(hardirq_context)); \
+	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirq_context)); \
 } while (0)
 
 #define lockdep_assert_preemption_enabled()				\
@@ -566,7 +558,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() != 0		||		\
-		      !raw_cpu_read(hardirqs_enabled)));		\
+		      !this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #define lockdep_assert_preemption_disabled()				\
@@ -574,7 +566,7 @@ do {									\
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
 		     __lockdep_enabled			&&		\
 		     (preempt_count() == 0		&&		\
-		      raw_cpu_read(hardirqs_enabled)));			\
+		      this_cpu_read(hardirqs_enabled)));		\
 } while (0)
 
 #else