[PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()

David Woodhouse posted 1 patch 1 year, 1 month ago
include/linux/irqflags.h |  6 ++++++
kernel/cpu.c             |  1 +
kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
3 files changed, 31 insertions(+)
[PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by David Woodhouse 1 year, 1 month ago
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


Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by David Woodhouse 2 months ago
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)
>  {

Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by Boqun Feng 2 months ago
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)
> >  {
> 
Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by David Woodhouse 2 months ago
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.
Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by Boqun Feng 2 months ago
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
Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by David Woodhouse 2 months ago
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'? :)

Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by Boqun Feng 2 months ago
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
Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
Posted by David Woodhouse 2 months ago
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.