linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lockdep: add lockdep_cleanup_dead_cpu()
@ 2023-10-28 11:14 David Woodhouse
  2023-10-28 14:13 ` Thomas Gleixner
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: David Woodhouse @ 2023-10-28 11:14 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 3815 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

Add a function to check that an offlone CPU left the tracing infrastructure
in a sane state. The acpi_idle_play_dead() function was recently observed
calling safe_halt() instead of raw_safe_halt(), which had the side-effect
of setting the hardirqs_enabled flag for the offline CPU. On x86 this
triggered lockdep warnings when the CPU came back online, but too early
for the exception to be handled correctly, leading to a triple-fault.

Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
print the events leading up to it, and correct it so that the CPU can
come online again correctly.

[   61.556652] smpboot: CPU 1 is now offline
[   61.556769] CPU 1 left hardirqs enabled!
[   61.556915] irq event stamp: 128149
[   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
[   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
[   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
[   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---

This would have saved us a lot of debugging in the last week.

 include/linux/irqflags.h |  4 ++++
 kernel/cpu.c             |  1 +
 kernel/locking/lockdep.c | 21 +++++++++++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 2b665c32f5fe..547ed55fc7ff 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -24,12 +24,16 @@
   extern void lockdep_hardirqs_on_prepare(void);
   extern void lockdep_hardirqs_on(unsigned long ip);
   extern void lockdep_hardirqs_off(unsigned long ip);
+  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
+				       struct task_struct *idle);
 #else
   static inline void lockdep_softirqs_on(unsigned long ip) { }
   static inline void lockdep_softirqs_off(unsigned long ip) { }
   static inline void lockdep_hardirqs_on_prepare(void) { }
   static inline void lockdep_hardirqs_on(unsigned long ip) { }
   static inline void lockdep_hardirqs_off(unsigned long ip) { }
+  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
+					      struct task_struct *idle) {}
 #endif
 
 #ifdef CONFIG_TRACE_IRQFLAGS
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6de7c6bb74ee..225f5bc3708f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1371,6 +1371,7 @@ static int takedown_cpu(unsigned int cpu)
 
 	cpuhp_bp_sync_dead(cpu);
 
+	lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
 	tick_cleanup_dead_cpu(cpu);
 	rcutree_migrate_callbacks(cpu);
 	return 0;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e85b5ad3e206..a643b29da34d 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4538,6 +4538,27 @@ void lockdep_softirqs_off(unsigned long ip)
 		debug_atomic_inc(redundant_softirqs_off);
 }
 
+/**
+ * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
+ *
+ * Invoked after the CPU is dead. Ensures that the tracing infrastructure
+ * is left in a suitable state for the CPU to be subsequently brought
+ * online again.
+ */
+void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
+{
+	if (unlikely(!debug_locks))
+		return;
+
+	if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
+		pr_warn("CPU %u left hardirqs enabled!", cpu);
+		if (idle)
+			print_irqtrace_events(idle);
+		/* Clean it up for when the CPU comes online again. */
+		per_cpu(hardirqs_enabled, cpu) = 0;
+	}
+}
+
 static int
 mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 {
-- 
2.41.0



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 11:14 [PATCH] lockdep: add lockdep_cleanup_dead_cpu() David Woodhouse
@ 2023-10-28 14:13 ` Thomas Gleixner
  2023-10-28 17:14 ` kernel test robot
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2023-10-28 14:13 UTC (permalink / raw)
  To: David Woodhouse, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

On Sat, Oct 28 2023 at 12:14, David Woodhouse wrote:

> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Add a function to check that an offlone CPU left the tracing infrastructure
> in a sane state. The acpi_idle_play_dead() function was recently observed
> calling safe_halt() instead of raw_safe_halt(), which had the side-effect
> of setting the hardirqs_enabled flag for the offline CPU. On x86 this
> triggered lockdep warnings when the CPU came back online, but too early
> for the exception to be handled correctly, leading to a triple-fault.
>
> Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
> print the events leading up to it, and correct it so that the CPU can
> come online again correctly.
>
> [   61.556652] smpboot: CPU 1 is now offline
> [   61.556769] CPU 1 left hardirqs enabled!
> [   61.556915] irq event stamp: 128149
> [   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
> [   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
> [   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
> [   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 11:14 [PATCH] lockdep: add lockdep_cleanup_dead_cpu() David Woodhouse
  2023-10-28 14:13 ` Thomas Gleixner
@ 2023-10-28 17:14 ` kernel test robot
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
  2023-10-30 11:17 ` [PATCH] " Peter Zijlstra
  3 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2023-10-28 17:14 UTC (permalink / raw)
  To: David Woodhouse, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Waiman Long, Boqun Feng, linux-kernel
  Cc: oe-kbuild-all, Juergen Gross

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/smp/core]
[also build test WARNING on tip/locking/core linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/lockdep-add-lockdep_cleanup_dead_cpu/20231028-191538
base:   tip/smp/core
patch link:    https://lore.kernel.org/r/e5ba02138c31da60daf91ce505ac3860d022332b.camel%40infradead.org
patch subject: [PATCH] lockdep: add lockdep_cleanup_dead_cpu()
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20231029/202310290041.L5ndwcQ9-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231029/202310290041.L5ndwcQ9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/locking/lockdep.c:4557: warning: Function parameter or member 'cpu' not described in 'lockdep_cleanup_dead_cpu'
>> kernel/locking/lockdep.c:4557: warning: Function parameter or member 'idle' not described in 'lockdep_cleanup_dead_cpu'


vim +4557 kernel/locking/lockdep.c

  4548	
  4549	/**
  4550	 * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
  4551	 *
  4552	 * Invoked after the CPU is dead. Ensures that the tracing infrastructure
  4553	 * is left in a suitable state for the CPU to be subsequently brought
  4554	 * online again.
  4555	 */
  4556	void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
> 4557	{
  4558		if (unlikely(!debug_locks))
  4559			return;
  4560	
  4561		if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
  4562			pr_warn("CPU %u left hardirqs enabled!", cpu);
  4563			if (idle)
  4564				print_irqtrace_events(idle);
  4565			/* Clean it up for when the CPU comes online again. */
  4566			per_cpu(hardirqs_enabled, cpu) = 0;
  4567		}
  4568	}
  4569	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 11:14 [PATCH] lockdep: add lockdep_cleanup_dead_cpu() David Woodhouse
  2023-10-28 14:13 ` Thomas Gleixner
  2023-10-28 17:14 ` kernel test robot
@ 2023-10-28 19:24 ` David Woodhouse
  2023-10-29 10:05   ` kernel test robot
                     ` (3 more replies)
  2023-10-30 11:17 ` [PATCH] " Peter Zijlstra
  3 siblings, 4 replies; 17+ messages in thread
From: David Woodhouse @ 2023-10-28 19:24 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 4253 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

Add a function to check that an offline CPU left the tracing infrastructure
in a sane state. The acpi_idle_play_dead() function was recently observed¹
calling safe_halt() instead of raw_safe_halt(), which had the side-effect
of setting the hardirqs_enabled flag for the offline CPU. On x86 this
triggered lockdep warnings when the CPU came back online, but too early
for the exception to be handled correctly, leading to a triple-fault.

Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
print the events leading up to it, and correct it so that the CPU can
come online again correctly.

[   61.556652] smpboot: CPU 1 is now offline
[   61.556769] CPU 1 left hardirqs enabled!
[   61.556915] irq event stamp: 128149
[   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
[   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
[   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
[   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100

¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
v2: Fix spelling. 'Offlone' wasn't quite what I meant to type.
    Add reference to ACPI patch.
    Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp)
    Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/


 include/linux/irqflags.h |  4 ++++
 kernel/cpu.c             |  1 +
 kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 2b665c32f5fe..547ed55fc7ff 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -24,12 +24,16 @@
   extern void lockdep_hardirqs_on_prepare(void);
   extern void lockdep_hardirqs_on(unsigned long ip);
   extern void lockdep_hardirqs_off(unsigned long ip);
+  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
+				       struct task_struct *idle);
 #else
   static inline void lockdep_softirqs_on(unsigned long ip) { }
   static inline void lockdep_softirqs_off(unsigned long ip) { }
   static inline void lockdep_hardirqs_on_prepare(void) { }
   static inline void lockdep_hardirqs_on(unsigned long ip) { }
   static inline void lockdep_hardirqs_off(unsigned long ip) { }
+  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
+					      struct task_struct *idle) {}
 #endif
 
 #ifdef CONFIG_TRACE_IRQFLAGS
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6de7c6bb74ee..225f5bc3708f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1371,6 +1371,7 @@ static int takedown_cpu(unsigned int cpu)
 
 	cpuhp_bp_sync_dead(cpu);
 
+	lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
 	tick_cleanup_dead_cpu(cpu);
 	rcutree_migrate_callbacks(cpu);
 	return 0;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e85b5ad3e206..62bfda8991b8 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4538,6 +4538,30 @@ void lockdep_softirqs_off(unsigned long ip)
 		debug_atomic_inc(redundant_softirqs_off);
 }
 
+/**
+ * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
+ *
+ * @cpu: index of offlined CPU
+ * @idle: task pointer for offlined CPU's idle thread
+ *
+ * Invoked after the CPU is dead. Ensures that the tracing infrastructure
+ * is left in a suitable state for the CPU to be subsequently brought
+ * online again.
+ */
+void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
+{
+	if (unlikely(!debug_locks))
+		return;
+
+	if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
+		pr_warn("CPU %u left hardirqs enabled!", cpu);
+		if (idle)
+			print_irqtrace_events(idle);
+		/* Clean it up for when the CPU comes online again. */
+		per_cpu(hardirqs_enabled, cpu) = 0;
+	}
+}
+
 static int
 mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 {
-- 
2.41.0



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
@ 2023-10-29 10:05   ` kernel test robot
  2023-10-29 17:33   ` Thomas Gleixner
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2023-10-29 10:05 UTC (permalink / raw)
  To: David Woodhouse, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Waiman Long, Boqun Feng, linux-kernel
  Cc: llvm, oe-kbuild-all, Juergen Gross

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/smp/core]
[also build test WARNING on tip/locking/core linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/lockdep-add-lockdep_cleanup_dead_cpu/20231029-032722
base:   tip/smp/core
patch link:    https://lore.kernel.org/r/635fa006e8f3816b4a36b964d6281f0d8efa789b.camel%40infradead.org
patch subject: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
config: um-allnoconfig (https://download.01.org/0day-ci/archive/20231029/202310291748.ld6JE1m3-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231029/202310291748.ld6JE1m3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310291748.ld6JE1m3-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from init/init_task.c:2:
   In file included from include/linux/init_task.h:5:
   In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^
   In file included from init/init_task.c:2:
   In file included from include/linux/init_task.h:9:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from init/init_task.c:2:
   In file included from include/linux/init_task.h:9:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from init/init_task.c:2:
   In file included from include/linux/init_task.h:9:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     692 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     700 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     708 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     717 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     726 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     735 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   13 warnings generated.
--
   In file included from arch/um/kernel/reboot.c:6:
   In file included from include/linux/sched/signal.h:5:
   In file included from include/linux/rculist.h:11:
   In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^
   arch/um/kernel/reboot.c:45:6: warning: no previous prototype for function 'machine_restart' [-Wmissing-prototypes]
      45 | void machine_restart(char * __unused)
         |      ^
   arch/um/kernel/reboot.c:45:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      45 | void machine_restart(char * __unused)
         | ^
         | static 
   arch/um/kernel/reboot.c:51:6: warning: no previous prototype for function 'machine_power_off' [-Wmissing-prototypes]
      51 | void machine_power_off(void)
         |      ^
   arch/um/kernel/reboot.c:51:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      51 | void machine_power_off(void)
         | ^
         | static 
   arch/um/kernel/reboot.c:57:6: warning: no previous prototype for function 'machine_halt' [-Wmissing-prototypes]
      57 | void machine_halt(void)
         |      ^
   arch/um/kernel/reboot.c:57:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      57 | void machine_halt(void)
         | ^
         | static 
   4 warnings generated.
--
   In file included from arch/um/kernel/early_printk.c:7:
   In file included from include/linux/console.h:19:
   In file included from include/linux/rculist.h:11:
   In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^
   1 warning generated.
--
   In file included from kernel/irq_work.c:12:
   In file included from include/linux/irq_work.h:6:
   In file included from include/linux/rcuwait.h:5:
   In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^
   In file included from kernel/irq_work.c:14:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from kernel/irq_work.c:14:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from kernel/irq_work.c:14:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     692 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     700 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     708 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     717 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     726 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     735 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   kernel/irq_work.c:72:13: warning: no previous prototype for function 'arch_irq_work_raise' [-Wmissing-prototypes]
      72 | void __weak arch_irq_work_raise(void)
         |             ^
   kernel/irq_work.c:72:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      72 | void __weak arch_irq_work_raise(void)
         | ^
         | static 
   14 warnings generated.
--
   In file included from lib/maple_tree.c:54:
   In file included from include/linux/maple_tree.h:12:
   In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^
   lib/maple_tree.c:331:21: warning: unused function 'mte_set_full' [-Wunused-function]
     331 | static inline void *mte_set_full(const struct maple_enode *node)
         |                     ^
   lib/maple_tree.c:336:21: warning: unused function 'mte_clear_full' [-Wunused-function]
     336 | static inline void *mte_clear_full(const struct maple_enode *node)
         |                     ^
   lib/maple_tree.c:341:20: warning: unused function 'mte_has_null' [-Wunused-function]
     341 | static inline bool mte_has_null(const struct maple_enode *node)
         |                    ^
   lib/maple_tree.c:672:29: warning: unused function 'mas_pivot' [-Wunused-function]
     672 | static inline unsigned long mas_pivot(struct ma_state *mas, unsigned char piv)
         |                             ^
   lib/maple_tree.c:4319:20: warning: stack frame size (1064) exceeds limit (1024) in 'mas_wr_modify' [-Wframe-larger-than]
    4319 | static inline void mas_wr_modify(struct ma_wr_state *wr_mas)
         |                    ^
   6 warnings generated.


vim +36 include/linux/irqflags.h

    19	
    20	/* Currently lockdep_softirqs_on/off is used only by lockdep */
    21	#ifdef CONFIG_PROVE_LOCKING
    22	  extern void lockdep_softirqs_on(unsigned long ip);
    23	  extern void lockdep_softirqs_off(unsigned long ip);
    24	  extern void lockdep_hardirqs_on_prepare(void);
    25	  extern void lockdep_hardirqs_on(unsigned long ip);
    26	  extern void lockdep_hardirqs_off(unsigned long ip);
    27	  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
    28					       struct task_struct *idle);
    29	#else
    30	  static inline void lockdep_softirqs_on(unsigned long ip) { }
    31	  static inline void lockdep_softirqs_off(unsigned long ip) { }
    32	  static inline void lockdep_hardirqs_on_prepare(void) { }
    33	  static inline void lockdep_hardirqs_on(unsigned long ip) { }
    34	  static inline void lockdep_hardirqs_off(unsigned long ip) { }
    35	  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
  > 36						      struct task_struct *idle) {}
    37	#endif
    38	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
  2023-10-29 10:05   ` kernel test robot
@ 2023-10-29 17:33   ` Thomas Gleixner
  2023-10-29 17:47     ` David Woodhouse
  2023-10-30  8:45   ` [PATCH v3] " David Woodhouse
  2023-10-30 16:37   ` [PATCH v2] " kernel test robot
  3 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2023-10-29 17:33 UTC (permalink / raw)
  To: David Woodhouse, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

On Sat, Oct 28 2023 at 20:24, David Woodhouse wrote:
> @@ -24,12 +24,16 @@
>    extern void lockdep_hardirqs_on_prepare(void);
>    extern void lockdep_hardirqs_on(unsigned long ip);
>    extern void lockdep_hardirqs_off(unsigned long ip);
> +  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
> +				       struct task_struct *idle);

Lacks a forward declaration of 'struct task_struct'


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-29 17:33   ` Thomas Gleixner
@ 2023-10-29 17:47     ` David Woodhouse
  0 siblings, 0 replies; 17+ messages in thread
From: David Woodhouse @ 2023-10-29 17:47 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On Sun, 2023-10-29 at 18:33 +0100, Thomas Gleixner wrote:
> On Sat, Oct 28 2023 at 20:24, David Woodhouse wrote:
> > @@ -24,12 +24,16 @@
> >    extern void lockdep_hardirqs_on_prepare(void);
> >    extern void lockdep_hardirqs_on(unsigned long ip);
> >    extern void lockdep_hardirqs_off(unsigned long ip);
> > +  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
> > +                                      struct task_struct *idle);
> 
> Lacks a forward declaration of 'struct task_struct'
> 

Apparently so; I thought that was fairly much ubiquitous. Was debating
spamming you with a v3 in the space of as many days, or perhaps
revisiting my decision to *pass* the idle task out of kernel/cpu.c.
 
We could always shift the declaration of idle_thread_get() out to
linux/smpboot.h and let the lockdep code call it directly. You already
reviewed my patch to do that, although it was dropped in the end.

https://lore.kernel.org/lkml/20230321194008.785922-2-usama.arif@bytedance.com/

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
  2023-10-29 10:05   ` kernel test robot
  2023-10-29 17:33   ` Thomas Gleixner
@ 2023-10-30  8:45   ` David Woodhouse
  2024-09-24 14:20     ` David Woodhouse
  2023-10-30 16:37   ` [PATCH v2] " kernel test robot
  3 siblings, 1 reply; 17+ messages in thread
From: David Woodhouse @ 2023-10-30  8:45 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 4561 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

Add a function to check that an offline CPU left the tracing infrastructure
in a sane state. The acpi_idle_play_dead() function was recently observed¹
calling safe_halt() instead of raw_safe_halt(), which had the side-effect
of setting the hardirqs_enabled flag for the offline CPU. On x86 this
triggered lockdep warnings when the CPU came back online, but too early
for the exception to be handled correctly, leading to a triple-fault.

Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
print the events leading up to it, and correct it so that the CPU can
come online again correctly.

[   61.556652] smpboot: CPU 1 is now offline
[   61.556769] CPU 1 left hardirqs enabled!
[   61.556915] irq event stamp: 128149
[   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
[   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
[   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
[   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100

¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

v3: Add forward declaration of struct task_struct.

v2: Fix spelling. 'Offlone' wasn't quite what I meant to type.
    Add reference to ACPI patch.
    Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp)
    Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/

 include/linux/irqflags.h |  6 ++++++
 kernel/cpu.c             |  1 +
 kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 2b665c32f5fe..9b44f8b042a0 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -17,6 +17,8 @@
 #include <asm/irqflags.h>
 #include <asm/percpu.h>
 
+struct task_struct;
+
 /* Currently lockdep_softirqs_on/off is used only by lockdep */
 #ifdef CONFIG_PROVE_LOCKING
   extern void lockdep_softirqs_on(unsigned long ip);
@@ -24,12 +26,16 @@
   extern void lockdep_hardirqs_on_prepare(void);
   extern void lockdep_hardirqs_on(unsigned long ip);
   extern void lockdep_hardirqs_off(unsigned long ip);
+  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
+				       struct task_struct *idle);
 #else
   static inline void lockdep_softirqs_on(unsigned long ip) { }
   static inline void lockdep_softirqs_off(unsigned long ip) { }
   static inline void lockdep_hardirqs_on_prepare(void) { }
   static inline void lockdep_hardirqs_on(unsigned long ip) { }
   static inline void lockdep_hardirqs_off(unsigned long ip) { }
+  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
+					      struct task_struct *idle) {}
 #endif
 
 #ifdef CONFIG_TRACE_IRQFLAGS
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6de7c6bb74ee..225f5bc3708f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1371,6 +1371,7 @@ static int takedown_cpu(unsigned int cpu)
 
 	cpuhp_bp_sync_dead(cpu);
 
+	lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
 	tick_cleanup_dead_cpu(cpu);
 	rcutree_migrate_callbacks(cpu);
 	return 0;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e85b5ad3e206..62bfda8991b8 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4538,6 +4538,30 @@ void lockdep_softirqs_off(unsigned long ip)
 		debug_atomic_inc(redundant_softirqs_off);
 }
 
+/**
+ * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
+ *
+ * @cpu: index of offlined CPU
+ * @idle: task pointer for offlined CPU's idle thread
+ *
+ * Invoked after the CPU is dead. Ensures that the tracing infrastructure
+ * is left in a suitable state for the CPU to be subsequently brought
+ * online again.
+ */
+void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
+{
+	if (unlikely(!debug_locks))
+		return;
+
+	if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
+		pr_warn("CPU %u left hardirqs enabled!", cpu);
+		if (idle)
+			print_irqtrace_events(idle);
+		/* Clean it up for when the CPU comes online again. */
+		per_cpu(hardirqs_enabled, cpu) = 0;
+	}
+}
+
 static int
 mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 {
-- 
2.41.0



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 11:14 [PATCH] lockdep: add lockdep_cleanup_dead_cpu() David Woodhouse
                   ` (2 preceding siblings ...)
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
@ 2023-10-30 11:17 ` Peter Zijlstra
  3 siblings, 0 replies; 17+ messages in thread
From: Peter Zijlstra @ 2023-10-30 11:17 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Thomas Gleixner, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, linux-kernel, Juergen Gross

On Sat, Oct 28, 2023 at 12:14:02PM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Add a function to check that an offlone CPU left the tracing infrastructure
> in a sane state. The acpi_idle_play_dead() function was recently observed
> calling safe_halt() instead of raw_safe_halt(), which had the side-effect

Right, so all that is because the offline path needs noinstr, but nobody
got around to doing that :/

Very rought attempt below, will scream bloody murder on build...

vmlinux.o: warning: objtool: idle_task_exit+0x45: call to switch_mm() leaves .noinstr.text section
vmlinux.o: warning: objtool: acpi_idle_play_dead+0x40: call to trace_hardirqs_on() leaves .noinstr.text section
vmlinux.o: warning: objtool: cpuidle_play_dead+0x10: call to cpuidle_get_cpu_driver() leaves .noinstr.text section

The acpi_idle_play_dead() thing is your original issue. No quick ideas
on what to do with switch_mm(), ideally we put that before we kill RCU
dead.

---
 arch/x86/include/asm/cpuid.h  |  4 ++--
 arch/x86/include/asm/smp.h    |  4 +++-
 arch/x86/kernel/process.c     |  4 ++--
 arch/x86/kernel/smpboot.c     | 12 ++++++------
 arch/x86/kernel/tboot.c       |  9 +++++----
 drivers/acpi/processor_idle.c |  2 +-
 drivers/cpuidle/cpuidle.c     |  5 +++--
 kernel/cpu.c                  | 15 +++++++++++----
 kernel/rcu/tree.c             | 15 ++++++++++-----
 kernel/sched/core.c           |  4 ++--
 kernel/sched/idle.c           |  2 +-
 11 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
index 9bee3e7bf973..8417d741ce59 100644
--- a/arch/x86/include/asm/cpuid.h
+++ b/arch/x86/include/asm/cpuid.h
@@ -27,8 +27,8 @@ static inline int have_cpuid_p(void)
 	return 1;
 }
 #endif
-static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
-				unsigned int *ecx, unsigned int *edx)
+static __always_inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
+					 unsigned int *ecx, unsigned int *edx)
 {
 	/* ecx is often an input as well as an output. */
 	asm volatile("cpuid"
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 4fab2ed454f3..41e839afba77 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -85,9 +85,11 @@ static inline void __cpu_die(unsigned int cpu)
 		smp_ops.cpu_die(cpu);
 }
 
-static inline void __noreturn play_dead(void)
+static __always_inline void __noreturn play_dead(void)
 {
+	instrumentation_begin(); // noinstr doesn't do indirect
 	smp_ops.play_dead();
+	instrumentation_end();
 	BUG();
 }
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b6f4e8399fca..c26d8f2af7bb 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -754,7 +754,7 @@ static bool x86_idle_set(void)
 }
 
 #ifndef CONFIG_SMP
-static inline void __noreturn play_dead(void)
+static inline void __noreturn noinstr play_dead(void)
 {
 	BUG();
 }
@@ -766,7 +766,7 @@ void arch_cpu_idle_enter(void)
 	local_touch_nmi();
 }
 
-void __noreturn arch_cpu_idle_dead(void)
+noinstr void __noreturn arch_cpu_idle_dead(void)
 {
 	play_dead();
 }
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 222d62e84cb3..74a926ddfac5 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1474,20 +1474,20 @@ int native_cpu_disable(void)
 	return 0;
 }
 
-void play_dead_common(void)
+noinstr void play_dead_common(void)
 {
 	idle_task_exit();
 
 	cpuhp_ap_report_dead();
 
-	local_irq_disable();
+	lockdep_assert_irqs_disabled();
 }
 
 /*
  * We need to flush the caches before going to sleep, lest we have
  * dirty data in our caches when we come back up.
  */
-static inline void mwait_play_dead(void)
+static __always_inline void mwait_play_dead(void)
 {
 	struct mwait_cpu_dead *md = this_cpu_ptr(&mwait_cpu_dead);
 	unsigned int eax, ebx, ecx, edx;
@@ -1597,7 +1597,7 @@ void smp_kick_mwait_play_dead(void)
 	}
 }
 
-void __noreturn hlt_play_dead(void)
+void __noreturn noinstr hlt_play_dead(void)
 {
 	if (__this_cpu_read(cpu_info.x86) >= 4)
 		wbinvd();
@@ -1610,7 +1610,7 @@ void __noreturn hlt_play_dead(void)
  * native_play_dead() is essentially a __noreturn function, but it can't
  * be marked as such as the compiler may complain about it.
  */
-void native_play_dead(void)
+noinstr void native_play_dead(void)
 {
 	if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS))
 		__update_spec_ctrl(0);
@@ -1629,7 +1629,7 @@ int native_cpu_disable(void)
 	return -ENOSYS;
 }
 
-void native_play_dead(void)
+noinstr void native_play_dead(void)
 {
 	BUG();
 }
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 4c1bcb6053fc..a8fb9a5a4e02 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -105,7 +105,7 @@ static struct mm_struct tboot_mm = {
 	.mmlist         = LIST_HEAD_INIT(init_mm.mmlist),
 };
 
-static inline void switch_to_tboot_pt(void)
+static __always_inline void switch_to_tboot_pt(void)
 {
 	write_cr3(virt_to_phys(tboot_pg_dir));
 }
@@ -193,7 +193,7 @@ static void add_mac_region(phys_addr_t start, unsigned long size)
 	}
 }
 
-static int tboot_setup_sleep(void)
+static noinstr int tboot_setup_sleep(void)
 {
 	int i;
 
@@ -224,7 +224,7 @@ static int tboot_setup_sleep(void)
 
 #endif
 
-void tboot_shutdown(u32 shutdown_type)
+noinstr void tboot_shutdown(u32 shutdown_type)
 {
 	void (*shutdown)(void);
 
@@ -240,9 +240,10 @@ void tboot_shutdown(u32 shutdown_type)
 		return;
 
 	/* if this is S3 then set regions to MAC */
-	if (shutdown_type == TB_SHUTDOWN_S3)
+	if (shutdown_type == TB_SHUTDOWN_S3) {
 		if (tboot_setup_sleep())
 			return;
+	}
 
 	tboot->shutdown_type = shutdown_type;
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 3a34a8c425fe..ad13fe635204 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -583,7 +583,7 @@ static void __cpuidle acpi_idle_do_entry(struct acpi_processor_cx *cx)
  * @dev: the target CPU
  * @index: the index of suggested state
  */
-static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
+static noinstr int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 {
 	struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
 
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 737a026ef58a..67e66646912e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -60,7 +60,7 @@ bool cpuidle_not_available(struct cpuidle_driver *drv,
  *
  * Returns in case of an error or no driver
  */
-int cpuidle_play_dead(void)
+noinstr int cpuidle_play_dead(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
@@ -70,9 +70,10 @@ int cpuidle_play_dead(void)
 		return -ENODEV;
 
 	/* Find lowest-power state that supports long-term idle */
-	for (i = drv->state_count - 1; i >= 0; i--)
+	for (i = drv->state_count - 1; i >= 0; i--) {
 		if (drv->states[i].enter_dead)
 			return drv->states[i].enter_dead(dev, i);
+	}
 
 	return -ENODEV;
 }
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 1a189da3bdac..5ca00bac6650 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -297,7 +297,7 @@ enum cpuhp_sync_state {
  * No synchronization point. Just update of the synchronization state, but implies
  * a full barrier so that the AP changes are visible before the control CPU proceeds.
  */
-static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state)
+static __always_inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state)
 {
 	atomic_t *st = this_cpu_ptr(&cpuhp_state.ap_sync_state);
 
@@ -347,7 +347,7 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) { }
  *
  * No synchronization point. Just update of the synchronization state.
  */
-void cpuhp_ap_report_dead(void)
+noinstr void cpuhp_ap_report_dead(void)
 {
 	cpuhp_ap_update_sync_state(SYNC_STATE_DEAD);
 }
@@ -1391,19 +1391,26 @@ static void cpuhp_complete_idle_dead(void *arg)
 	complete_ap_thread(st, false);
 }
 
-void cpuhp_report_idle_dead(void)
+noinstr void cpuhp_report_idle_dead(void)
 {
-	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
+	struct cpuhp_cpu_state *st;
 
+	instrumentation_begin();
+	st = this_cpu_ptr(&cpuhp_state);
 	BUG_ON(st->state != CPUHP_AP_OFFLINE);
 	rcu_report_dead(smp_processor_id());
+	// instrumentation_end(); // RCU is dead here
+
 	st->state = CPUHP_AP_IDLE_DEAD;
 	/*
 	 * We cannot call complete after rcu_report_dead() so we delegate it
 	 * to an online cpu.
+	 *
+	 * XXX strictly speaking this should be noinstr, there's no RCU anymore
 	 */
 	smp_call_function_single(cpumask_first(cpu_online_mask),
 				 cpuhp_complete_idle_dead, st, 0);
+	instrumentation_end();
 }
 
 static int cpuhp_down_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index cb1caefa8bd0..6c577a6d3581 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4474,13 +4474,17 @@ void rcu_cpu_starting(unsigned int cpu)
  * from the outgoing CPU rather than from the cpuhp_step mechanism.
  * This is because this function must be invoked at a precise location.
  */
-void rcu_report_dead(unsigned int cpu)
+noinstr void rcu_report_dead(unsigned int cpu)
 {
-	unsigned long flags, seq_flags;
+	unsigned long flags;
 	unsigned long mask;
 	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
 	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
 
+	lockdep_assert_irqs_disabled();
+
+	instrumentation_begin();
+
 	// Do any dangling deferred wakeups.
 	do_nocb_deferred_wakeup(rdp);
 
@@ -4488,7 +4492,7 @@ void rcu_report_dead(unsigned int cpu)
 
 	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
 	mask = rdp->grpmask;
-	local_irq_save(seq_flags);
+
 	arch_spin_lock(&rcu_state.ofl_lock);
 	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
 	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
@@ -4500,9 +4504,10 @@ void rcu_report_dead(unsigned int cpu)
 		raw_spin_lock_irqsave_rcu_node(rnp, flags);
 	}
 	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
-	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
+	raw_spin_unlock_irqrestore_rcu_node(rnp, flags); // XXX RCU already dead ?!
 	arch_spin_unlock(&rcu_state.ofl_lock);
-	local_irq_restore(seq_flags);
+
+	instrumentation_end();
 
 	rdp->cpu_started = false;
 }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 81885748871d..4447e75e9275 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9371,7 +9371,7 @@ void sched_setnuma(struct task_struct *p, int nid)
  * Ensure that the idle task is using init_mm right before its CPU goes
  * offline.
  */
-void idle_task_exit(void)
+noinstr void idle_task_exit(void)
 {
 	struct mm_struct *mm = current->active_mm;
 
@@ -9379,7 +9379,7 @@ void idle_task_exit(void)
 	BUG_ON(current != this_rq()->idle);
 
 	if (mm != &init_mm) {
-		switch_mm(mm, &init_mm, current);
+		switch_mm(mm, &init_mm, current); // XXX bugger ! properly broken
 		finish_arch_post_lock_switch();
 	}
 
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 565f8374ddbb..bf39ee9e5334 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -75,7 +75,7 @@ static noinline int __cpuidle cpu_idle_poll(void)
 void __weak arch_cpu_idle_prepare(void) { }
 void __weak arch_cpu_idle_enter(void) { }
 void __weak arch_cpu_idle_exit(void) { }
-void __weak __noreturn arch_cpu_idle_dead(void) { while (1); }
+void __weak __noreturn noinstr arch_cpu_idle_dead(void) { while (1); }
 void __weak arch_cpu_idle(void)
 {
 	cpu_idle_force_poll = 1;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
                     ` (2 preceding siblings ...)
  2023-10-30  8:45   ` [PATCH v3] " David Woodhouse
@ 2023-10-30 16:37   ` kernel test robot
  3 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2023-10-30 16:37 UTC (permalink / raw)
  To: David Woodhouse, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Waiman Long, Boqun Feng, linux-kernel
  Cc: oe-kbuild-all, Juergen Gross

Hi David,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/smp/core]
[also build test ERROR on tip/locking/core linus/master v6.6 next-20231030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/lockdep-add-lockdep_cleanup_dead_cpu/20231029-032722
base:   tip/smp/core
patch link:    https://lore.kernel.org/r/635fa006e8f3816b4a36b964d6281f0d8efa789b.camel%40infradead.org
patch subject: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
config: sparc64-defconfig (https://download.01.org/0day-ci/archive/20231031/202310310038.MkdWejfv-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231031/202310310038.MkdWejfv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310310038.MkdWejfv-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from arch/sparc/kernel/unaligned_64.c:12:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/thread_info.h:27,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/sparc/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from arch/sparc/kernel/adi_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/adi_64.c:124:21: error: no previous prototype for 'find_tag_store' [-Werror=missing-prototypes]
     124 | tag_storage_desc_t *find_tag_store(struct mm_struct *mm,
         |                     ^~~~~~~~~~~~~~
   arch/sparc/kernel/adi_64.c:156:21: error: no previous prototype for 'alloc_tag_store' [-Werror=missing-prototypes]
     156 | tag_storage_desc_t *alloc_tag_store(struct mm_struct *mm,
         |                     ^~~~~~~~~~~~~~~
   arch/sparc/kernel/adi_64.c:299:6: error: no previous prototype for 'del_tag_store' [-Werror=missing-prototypes]
     299 | void del_tag_store(tag_storage_desc_t *tag_desc, struct mm_struct *mm)
         |      ^~~~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from arch/sparc/kernel/pcr.c:6:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/pcr.c:47:6: error: no previous prototype for 'arch_irq_work_raise' [-Werror=missing-prototypes]
      47 | void arch_irq_work_raise(void)
         |      ^~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/linux/mm_types_task.h:13,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from include/linux/moduleloader.h:6,
                    from arch/sparc/kernel/module.c:8:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/module.c: In function 'module_frob_arch_sections':
   arch/sparc/kernel/module.c:62:15: error: variable 'strtab' set but not used [-Werror=unused-but-set-variable]
      62 |         char *strtab;
         |               ^~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from arch/sparc/kernel/pci_sun4v.c:7:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/pci_sun4v.c:258:15: error: no previous prototype for 'dma_4v_iotsb_bind' [-Werror=missing-prototypes]
     258 | unsigned long dma_4v_iotsb_bind(unsigned long devhandle,
         |               ^~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from arch/sparc/kernel/uprobes.c:12:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/uprobes.c:237:17: error: no previous prototype for 'uprobe_trap' [-Werror=missing-prototypes]
     237 | asmlinkage void uprobe_trap(struct pt_regs *regs,
         |                 ^~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from include/linux/sched/mm.h:5,
                    from arch/sparc/kernel/traps_64.c:13:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/traps_64.c:252:6: error: no previous prototype for 'is_no_fault_exception' [-Werror=missing-prototypes]
     252 | bool is_no_fault_exception(struct pt_regs *regs)
         |      ^~~~~~~~~~~~~~~~~~~~~
   arch/sparc/kernel/traps_64.c:2034:6: error: no previous prototype for 'do_mcd_err' [-Werror=missing-prototypes]
    2034 | void do_mcd_err(struct pt_regs *regs, struct sun4v_error_entry ent)
         |      ^~~~~~~~~~
   arch/sparc/kernel/traps_64.c:2152:6: error: no previous prototype for 'sun4v_nonresum_error_user_handled' [-Werror=missing-prototypes]
    2152 | bool sun4v_nonresum_error_user_handled(struct pt_regs *regs,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/sparc/kernel/traps_64.c:2839:13: error: no previous prototype for 'trap_init' [-Werror=missing-prototypes]
    2839 | void __init trap_init(void)
         |             ^~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/thread_info.h:27,
                    from arch/sparc/include/asm/current.h:15,
                    from include/linux/sched.h:12,
                    from arch/sparc/kernel/setup_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/setup_64.c:615:13: error: no previous prototype for 'alloc_irqstack_bootmem' [-Werror=missing-prototypes]
     615 | void __init alloc_irqstack_bootmem(void)
         |             ^~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/thread_info.h:27,
                    from arch/sparc/include/asm/current.h:15,
                    from include/linux/sched.h:12,
                    from arch/sparc/kernel/time_64.c:14:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/kernel/time_64.c:880:20: error: no previous prototype for 'sched_clock' [-Werror=missing-prototypes]
     880 | unsigned long long sched_clock(void)
         |                    ^~~~~~~~~~~
   cc1: all warnings being treated as errors
--
   In file included from include/asm-generic/cmpxchg-local.h:6,
                    from arch/sparc/include/asm/cmpxchg_64.h:111,
                    from arch/sparc/include/asm/cmpxchg.h:5,
                    from arch/sparc/include/asm/atomic_64.h:12,
                    from arch/sparc/include/asm/atomic.h:5,
                    from include/linux/atomic.h:7,
                    from include/asm-generic/bitops/lock.h:5,
                    from arch/sparc/include/asm/bitops_64.h:52,
                    from arch/sparc/include/asm/bitops.h:5,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:22,
                    from arch/sparc/mm/init_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      36 |                                               struct task_struct *idle) {}
         |                                                      ^~~~~~~~~~~
   arch/sparc/mm/init_64.c: In function 'arch_hugetlb_valid_size':
   arch/sparc/mm/init_64.c:355:24: error: variable 'hv_pgsz_idx' set but not used [-Werror=unused-but-set-variable]
     355 |         unsigned short hv_pgsz_idx;
         |                        ^~~~~~~~~~~
   arch/sparc/mm/init_64.c: At top level:
   arch/sparc/mm/init_64.c:2630:6: error: no previous prototype for 'vmemmap_free' [-Werror=missing-prototypes]
    2630 | void vmemmap_free(unsigned long start, unsigned long end,
         |      ^~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +36 include/linux/irqflags.h

    19	
    20	/* Currently lockdep_softirqs_on/off is used only by lockdep */
    21	#ifdef CONFIG_PROVE_LOCKING
    22	  extern void lockdep_softirqs_on(unsigned long ip);
    23	  extern void lockdep_softirqs_off(unsigned long ip);
    24	  extern void lockdep_hardirqs_on_prepare(void);
    25	  extern void lockdep_hardirqs_on(unsigned long ip);
    26	  extern void lockdep_hardirqs_off(unsigned long ip);
    27	  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
    28					       struct task_struct *idle);
    29	#else
    30	  static inline void lockdep_softirqs_on(unsigned long ip) { }
    31	  static inline void lockdep_softirqs_off(unsigned long ip) { }
    32	  static inline void lockdep_hardirqs_on_prepare(void) { }
    33	  static inline void lockdep_hardirqs_on(unsigned long ip) { }
    34	  static inline void lockdep_hardirqs_off(unsigned long ip) { }
    35	  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
  > 36						      struct task_struct *idle) {}
    37	#endif
    38	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2023-10-30  8:45   ` [PATCH v3] " David Woodhouse
@ 2024-09-24 14:20     ` David Woodhouse
  2024-09-26 12:09       ` Boqun Feng
  0 siblings, 1 reply; 17+ messages in thread
From: David Woodhouse @ 2024-09-24 14:20 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Boqun Feng, linux-kernel
  Cc: Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

On Mon, 2023-10-30 at 08:45 +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Add a function to check that an offline CPU left the tracing infrastructure
> in a sane state. The acpi_idle_play_dead() function was recently observed¹
> calling safe_halt() instead of raw_safe_halt(), which had the side-effect
> of setting the hardirqs_enabled flag for the offline CPU. On x86 this
> triggered lockdep warnings when the CPU came back online, but too early
> for the exception to be handled correctly, leading to a triple-fault.
> 
> Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
> print the events leading up to it, and correct it so that the CPU can
> come online again correctly.
> 
> [   61.556652] smpboot: CPU 1 is now offline
> [   61.556769] CPU 1 left hardirqs enabled!
> [   61.556915] irq event stamp: 128149
> [   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
> [   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
> [   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
> [   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
> 
> ¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> 

Ping? Found this lying around in a branch today...


> v3: Add forward declaration of struct task_struct.
> 
> v2: Fix spelling. 'Offlone' wasn't quite what I meant to type.
>     Add reference to ACPI patch.
>     Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp)
>     Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/
> 
>  include/linux/irqflags.h |  6 ++++++
>  kernel/cpu.c             |  1 +
>  kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 2b665c32f5fe..9b44f8b042a0 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -17,6 +17,8 @@
>  #include <asm/irqflags.h>
>  #include <asm/percpu.h>
>  
> +struct task_struct;
> +
>  /* Currently lockdep_softirqs_on/off is used only by lockdep */
>  #ifdef CONFIG_PROVE_LOCKING
>    extern void lockdep_softirqs_on(unsigned long ip);
> @@ -24,12 +26,16 @@
>    extern void lockdep_hardirqs_on_prepare(void);
>    extern void lockdep_hardirqs_on(unsigned long ip);
>    extern void lockdep_hardirqs_off(unsigned long ip);
> +  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
> +                                      struct task_struct *idle);
>  #else
>    static inline void lockdep_softirqs_on(unsigned long ip) { }
>    static inline void lockdep_softirqs_off(unsigned long ip) { }
>    static inline void lockdep_hardirqs_on_prepare(void) { }
>    static inline void lockdep_hardirqs_on(unsigned long ip) { }
>    static inline void lockdep_hardirqs_off(unsigned long ip) { }
> +  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
> +                                             struct task_struct *idle) {}
>  #endif
>  
>  #ifdef CONFIG_TRACE_IRQFLAGS
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6de7c6bb74ee..225f5bc3708f 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1371,6 +1371,7 @@ static int takedown_cpu(unsigned int cpu)
>  
>         cpuhp_bp_sync_dead(cpu);
>  
> +       lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
>         tick_cleanup_dead_cpu(cpu);
>         rcutree_migrate_callbacks(cpu);
>         return 0;
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index e85b5ad3e206..62bfda8991b8 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4538,6 +4538,30 @@ void lockdep_softirqs_off(unsigned long ip)
>                 debug_atomic_inc(redundant_softirqs_off);
>  }
>  
> +/**
> + * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
> + *
> + * @cpu: index of offlined CPU
> + * @idle: task pointer for offlined CPU's idle thread
> + *
> + * Invoked after the CPU is dead. Ensures that the tracing infrastructure
> + * is left in a suitable state for the CPU to be subsequently brought
> + * online again.
> + */
> +void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
> +{
> +       if (unlikely(!debug_locks))
> +               return;
> +
> +       if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
> +               pr_warn("CPU %u left hardirqs enabled!", cpu);
> +               if (idle)
> +                       print_irqtrace_events(idle);
> +               /* Clean it up for when the CPU comes online again. */
> +               per_cpu(hardirqs_enabled, cpu) = 0;
> +       }
> +}
> +
>  static int
>  mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
>  {


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-24 14:20     ` David Woodhouse
@ 2024-09-26 12:09       ` Boqun Feng
  2024-09-26 12:16         ` David Woodhouse
  0 siblings, 1 reply; 17+ messages in thread
From: Boqun Feng @ 2024-09-26 12:09 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

On Tue, Sep 24, 2024 at 03:20:05PM +0100, David Woodhouse wrote:
> On Mon, 2023-10-30 at 08:45 +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Add a function to check that an offline CPU left the tracing infrastructure
> > in a sane state. The acpi_idle_play_dead() function was recently observed¹
> > calling safe_halt() instead of raw_safe_halt(), which had the side-effect
> > of setting the hardirqs_enabled flag for the offline CPU. On x86 this
> > triggered lockdep warnings when the CPU came back online, but too early
> > for the exception to be handled correctly, leading to a triple-fault.
> > 
> > Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
> > print the events leading up to it, and correct it so that the CPU can
> > come online again correctly.
> > 
> > [   61.556652] smpboot: CPU 1 is now offline
> > [   61.556769] CPU 1 left hardirqs enabled!
> > [   61.556915] irq event stamp: 128149
> > [   61.556965] hardirqs last  enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
> > [   61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
> > [   61.557117] softirqs last  enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
> > [   61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
> > 
> > ¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> > 
> 
> Ping? Found this lying around in a branch today...
> 

I think this is already fixed by:

	9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()")

, no?

Regards,
Boqun

> 
> > v3: Add forward declaration of struct task_struct.
> > 
> > v2: Fix spelling. 'Offlone' wasn't quite what I meant to type.
> >     Add reference to ACPI patch.
> >     Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp)
> >     Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/
> > 
> >  include/linux/irqflags.h |  6 ++++++
> >  kernel/cpu.c             |  1 +
> >  kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> > index 2b665c32f5fe..9b44f8b042a0 100644
> > --- a/include/linux/irqflags.h
> > +++ b/include/linux/irqflags.h
> > @@ -17,6 +17,8 @@
> >  #include <asm/irqflags.h>
> >  #include <asm/percpu.h>
> >  
> > +struct task_struct;
> > +
> >  /* Currently lockdep_softirqs_on/off is used only by lockdep */
> >  #ifdef CONFIG_PROVE_LOCKING
> >    extern void lockdep_softirqs_on(unsigned long ip);
> > @@ -24,12 +26,16 @@
> >    extern void lockdep_hardirqs_on_prepare(void);
> >    extern void lockdep_hardirqs_on(unsigned long ip);
> >    extern void lockdep_hardirqs_off(unsigned long ip);
> > +  extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
> > +                                      struct task_struct *idle);
> >  #else
> >    static inline void lockdep_softirqs_on(unsigned long ip) { }
> >    static inline void lockdep_softirqs_off(unsigned long ip) { }
> >    static inline void lockdep_hardirqs_on_prepare(void) { }
> >    static inline void lockdep_hardirqs_on(unsigned long ip) { }
> >    static inline void lockdep_hardirqs_off(unsigned long ip) { }
> > +  static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
> > +                                             struct task_struct *idle) {}
> >  #endif
> >  
> >  #ifdef CONFIG_TRACE_IRQFLAGS
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index 6de7c6bb74ee..225f5bc3708f 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1371,6 +1371,7 @@ static int takedown_cpu(unsigned int cpu)
> >  
> >         cpuhp_bp_sync_dead(cpu);
> >  
> > +       lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
> >         tick_cleanup_dead_cpu(cpu);
> >         rcutree_migrate_callbacks(cpu);
> >         return 0;
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index e85b5ad3e206..62bfda8991b8 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4538,6 +4538,30 @@ void lockdep_softirqs_off(unsigned long ip)
> >                 debug_atomic_inc(redundant_softirqs_off);
> >  }
> >  
> > +/**
> > + * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
> > + *
> > + * @cpu: index of offlined CPU
> > + * @idle: task pointer for offlined CPU's idle thread
> > + *
> > + * Invoked after the CPU is dead. Ensures that the tracing infrastructure
> > + * is left in a suitable state for the CPU to be subsequently brought
> > + * online again.
> > + */
> > +void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
> > +{
> > +       if (unlikely(!debug_locks))
> > +               return;
> > +
> > +       if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
> > +               pr_warn("CPU %u left hardirqs enabled!", cpu);
> > +               if (idle)
> > +                       print_irqtrace_events(idle);
> > +               /* Clean it up for when the CPU comes online again. */
> > +               per_cpu(hardirqs_enabled, cpu) = 0;
> > +       }
> > +}
> > +
> >  static int
> >  mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
> >  {
> 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-26 12:09       ` Boqun Feng
@ 2024-09-26 12:16         ` David Woodhouse
  2024-09-26 12:34           ` Boqun Feng
  0 siblings, 1 reply; 17+ messages in thread
From: David Woodhouse @ 2024-09-26 12:16 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote:
> 
> 
> I think this is already fixed by:
> 
>         9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()")
> 
> , no?

That patch fixed the bug.

*This* patch fixes the fact that lockdep didn't *tell* us about the bug.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-26 12:16         ` David Woodhouse
@ 2024-09-26 12:34           ` Boqun Feng
  2024-09-26 14:34             ` David Woodhouse
  0 siblings, 1 reply; 17+ messages in thread
From: Boqun Feng @ 2024-09-26 12:34 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote:
> On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote:
> > 
> > 
> > I think this is already fixed by:
> > 
> >         9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()")
> > 
> > , no?
> 
> That patch fixed the bug.
> 
> *This* patch fixes the fact that lockdep didn't *tell* us about the bug.

But I thought along with the above commit, Peter also made it possible
that objtool can detect leaving noinstr section in the offline path? Do
you have a case where you can alter hardirqs_enabled flag in offline
path but don't hit the objtool warning?

Anyway, the commit log needs a rework.

Regards,
Boqun

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-26 12:34           ` Boqun Feng
@ 2024-09-26 14:34             ` David Woodhouse
  2024-09-26 15:13               ` Boqun Feng
  0 siblings, 1 reply; 17+ messages in thread
From: David Woodhouse @ 2024-09-26 14:34 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

On Thu, 2024-09-26 at 05:34 -0700, Boqun Feng wrote:
> On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote:
> > On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote:
> > > 
> > > 
> > > I think this is already fixed by:
> > > 
> > >         9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()")
> > > 
> > > , no?
> > 
> > That patch fixed the bug.
> > 
> > *This* patch fixes the fact that lockdep didn't *tell* us about the bug.
> 
> But I thought along with the above commit, Peter also made it possible
> that objtool can detect leaving noinstr section in the offline path? Do
> you have a case where you can alter hardirqs_enabled flag in offline
> path but don't hit the objtool warning?

I do not recall such. Peter?

IIRC the bug fixed by commit 9bb69ba4c177 only showed up under real
Xen, as QEMU doesn't expose processor C-states. So I reintroduced the
equivalent bug by doing this instead:

--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1390,7 +1390,7 @@ void __noreturn hlt_play_dead(void)
                wbinvd();
 
        while (1)
-               native_halt();
+               safe_halt();
 }
 

Without this patch, I get a triple-fault on bringing the CPU back
online as before. With it, as intended, I get a warning, but success:

[root@localhost ~]# echo 0 > /sys/devices/system/cpu/cpu1/online 
[   42.090839] smpboot: CPU 1 is now offline
[   42.091989] CPU 1 left hardirqs enabled!
[   42.091997] irq event stamp: 144559
[   42.094155] hardirqs last  enabled at (144559): [<ffffffff89098b2e>] hlt_play_dead+0x1e/0x30
[   42.096196] hardirqs last disabled at (144558): [<ffffffff891800ee>] do_idle+0xae/0x260
[   42.098062] softirqs last  enabled at (144530): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0
[   42.100056] softirqs last disabled at (144519): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0
[root@localhost ~]# echo 1 > /sys/devices/system/cpu/cpu1/online 
[   47.480889] installing Xen timer for CPU 1
[   47.485308] smpboot: Booting Node 0 Processor 1 APIC 0x1
[   47.491569] cpu 1 spinlock event irq 35


So I think the patch is still applicable.

> Anyway, the commit log needs a rework.

Sure. Other than to refer to commit 9bb69ba4c177 instead of the mailing
list message, is there anything else that needs changing? I suppose I
should drop the word 'recently' from '...was recently observed'? :)


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-26 14:34             ` David Woodhouse
@ 2024-09-26 15:13               ` Boqun Feng
  2024-09-26 15:38                 ` David Woodhouse
  0 siblings, 1 reply; 17+ messages in thread
From: Boqun Feng @ 2024-09-26 15:13 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

On Thu, Sep 26, 2024 at 03:34:57PM +0100, David Woodhouse wrote:
> On Thu, 2024-09-26 at 05:34 -0700, Boqun Feng wrote:
> > On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote:
> > > On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote:
> > > > 
> > > > 
> > > > I think this is already fixed by:
> > > > 
> > > >         9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()")
> > > > 
> > > > , no?
> > > 
> > > That patch fixed the bug.
> > > 
> > > *This* patch fixes the fact that lockdep didn't *tell* us about the bug.
> > 
> > But I thought along with the above commit, Peter also made it possible
> > that objtool can detect leaving noinstr section in the offline path? Do
> > you have a case where you can alter hardirqs_enabled flag in offline
> > path but don't hit the objtool warning?
> 
> I do not recall such. Peter?
> 

Oh I mis-read Peter's response here:

	https://lore.kernel.org/lkml/20231027191435.GF26550@noisy.programming.kicks-ass.net/

, so seems the noinstr annotating for CPU offline path is still a WIP.

> IIRC the bug fixed by commit 9bb69ba4c177 only showed up under real
> Xen, as QEMU doesn't expose processor C-states. So I reintroduced the
> equivalent bug by doing this instead:
> 
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1390,7 +1390,7 @@ void __noreturn hlt_play_dead(void)
>                 wbinvd();
>  
>         while (1)
> -               native_halt();
> +               safe_halt();
>  }
>  
> 
> Without this patch, I get a triple-fault on bringing the CPU back
> online as before. With it, as intended, I get a warning, but success:
> 
> [root@localhost ~]# echo 0 > /sys/devices/system/cpu/cpu1/online 
> [   42.090839] smpboot: CPU 1 is now offline
> [   42.091989] CPU 1 left hardirqs enabled!
> [   42.091997] irq event stamp: 144559
> [   42.094155] hardirqs last  enabled at (144559): [<ffffffff89098b2e>] hlt_play_dead+0x1e/0x30
> [   42.096196] hardirqs last disabled at (144558): [<ffffffff891800ee>] do_idle+0xae/0x260
> [   42.098062] softirqs last  enabled at (144530): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0
> [   42.100056] softirqs last disabled at (144519): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0
> [root@localhost ~]# echo 1 > /sys/devices/system/cpu/cpu1/online 
> [   47.480889] installing Xen timer for CPU 1
> [   47.485308] smpboot: Booting Node 0 Processor 1 APIC 0x1
> [   47.491569] cpu 1 spinlock event irq 35
> 
> 
> So I think the patch is still applicable.
> 

Yeah, it was just that I thought we have static checking for the issue
(via objtool), and I think that's slightly better, because it covers
more problems.

> > Anyway, the commit log needs a rework.
> 
> Sure. Other than to refer to commit 9bb69ba4c177 instead of the mailing
> list message, is there anything else that needs changing? I suppose I
> should drop the word 'recently' from '...was recently observed'? :)
> 

Given that Peter did send a POC for static checking:

	https://lore.kernel.org/lkml/20231030111724.GA12604@noisy.programming.kicks-ass.net/

Maybe you could explain why this is needed even though static checking
is technically possible? Thanks!

Regards,
Boqun

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
  2024-09-26 15:13               ` Boqun Feng
@ 2024-09-26 15:38                 ` David Woodhouse
  0 siblings, 0 replies; 17+ messages in thread
From: David Woodhouse @ 2024-09-26 15:38 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, linux-kernel, Juergen Gross

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, 2024-09-26 at 08:13 -0700, Boqun Feng wrote:
> 
> Given that Peter did send a POC for static checking:
> 
>         https://lore.kernel.org/lkml/20231030111724.GA12604@noisy.programming.kicks-ass.net/
> 
> Maybe you could explain why this is needed even though static checking
> is technically possible? Thanks!

If Peter wants to finish that approach and get it merged, that's fine
by me.



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2024-09-26 15:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-28 11:14 [PATCH] lockdep: add lockdep_cleanup_dead_cpu() David Woodhouse
2023-10-28 14:13 ` Thomas Gleixner
2023-10-28 17:14 ` kernel test robot
2023-10-28 19:24 ` [PATCH v2] " David Woodhouse
2023-10-29 10:05   ` kernel test robot
2023-10-29 17:33   ` Thomas Gleixner
2023-10-29 17:47     ` David Woodhouse
2023-10-30  8:45   ` [PATCH v3] " David Woodhouse
2024-09-24 14:20     ` David Woodhouse
2024-09-26 12:09       ` Boqun Feng
2024-09-26 12:16         ` David Woodhouse
2024-09-26 12:34           ` Boqun Feng
2024-09-26 14:34             ` David Woodhouse
2024-09-26 15:13               ` Boqun Feng
2024-09-26 15:38                 ` David Woodhouse
2023-10-30 16:37   ` [PATCH v2] " kernel test robot
2023-10-30 11:17 ` [PATCH] " Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).