Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()

[David Woodhouse <dwmw2@infradead.org>]

[-- 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 --]