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

[Boqun Feng <boqun.feng@gmail.com>]

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