Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Thomas Gleixner
2017-10-03T10:56:57Z

On Tue, 3 Oct 2017, Nicholas Piggin wrote:
> On Tue, 3 Oct 2017 09:04:03 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Tue, 3 Oct 2017, Thomas Gleixner wrote:
> > > On Tue, 3 Oct 2017, Michael Ellerman wrote:
> > > > Hi Thomas,
> > > > Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc
> > > > because we're calling it multiple times for the boot CPU.
> > > >
> > > > The first call is via:
> > > >
> > > > start_wd_on_cpu+0x80/0x2f0
> > > > watchdog_nmi_reconfigure+0x124/0x170
> > > > softlockup_reconfigure_threads+0x110/0x130
> > > > lockup_detector_init+0xbc/0xe0
> > > > kernel_init_freeable+0x18c/0x37c
> > > > kernel_init+0x2c/0x160
> > > > ret_from_kernel_thread+0x5c/0xbc
> > > >
> > > > And then again via the CPU hotplug registration:
> > > >
> > > > start_wd_on_cpu+0x80/0x2f0
> > > > cpuhp_invoke_callback+0x194/0x620
> > > > cpuhp_thread_fun+0x7c/0x1b0
> > > > smpboot_thread_fn+0x290/0x2a0
> > > > kthread+0x168/0x1b0
> > > > ret_from_kernel_thread+0x5c/0xbc
> > > >
> > > >
> > > > The first call is new because previously watchdog_nmi_reconfigure()
> > > > wasn't called from softlockup_reconfigure_threads().
> > >
> > > Hmm, don't you have the same problem with CPU hotplug or do you just get
> > > lucky because the hotplug callback in your code is ordered vs. the
> > > softlockup thread hotplug callback in a way that this does not hit?
>
> I had the idea that it watchdog_nmi_reconfigure() being only called
> with get_online_cpus held would prevent hotplug callbacks running.
>
> >
> > Which leads me to the question why you need the hotplug state at all if the
> > softlockup detector is enabled. Wouldn't it make more sense to only
> > register the state if softlockup detector is turned off in Kconfig and
> > actually move it to the core code?
>
> I don't understand what you mean exactly, but it was done to avoid
> relying on the softlockup detector at all, because it wasn't needed
> for anything else (unlike the perf lockup detector).

If the softlockup detector is enabled along with your hardlockup detector
then the current code in mainline invokes watchdog_nmi_enable(cpu), which
is a weak function and as I just noticed not implemented by powerpc. So
it's a non issue because it's not implemented.

Thanks,

tglx