> On Sat 2022-02-12 18:43:48, Lecopzer Chen wrote:
> > From: Pingfan Liu <kernelfans@xxxxxxxxx>
> >
> > from: Pingfan Liu <kernelfans@xxxxxxxxx>
> >
> > When lockup_detector_init()->watchdog_nmi_probe(), PMU may be not ready
> > yet. E.g. on arm64, PMU is not ready until
> > device_initcall(armv8_pmu_driver_init). And it is deeply integrated
> > with the driver model and cpuhp. Hence it is hard to push this
> > initialization before smp_init().
> >
> > But it is easy to take an opposite approach by enabling watchdog_hld to
> > get the capability of PMU async.
> >
> > The async model is achieved by expanding watchdog_nmi_probe() with
> > -EBUSY, and a re-initializing work_struct which waits on a wait_queue_head.
> >
> > Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> > Co-developed-by: Lecopzer Chen <lecopzer.chen@xxxxxxxxxxxx>
> > Signed-off-by: Lecopzer Chen <lecopzer.chen@xxxxxxxxxxxx>
> > ---
> > kernel/watchdog.c | 56 +++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index b71d434cf648..fa8490cfeef8 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -839,16 +843,64 @@ static void __init watchdog_sysctl_init(void)
> > #define watchdog_sysctl_init() do { } while (0)
> > #endif /* CONFIG_SYSCTL */
> >
> > +static void lockup_detector_delay_init(struct work_struct *work);
> > +enum hld_detector_state detector_delay_init_state __initdata;
>
> I would call this "lockup_detector_init_state" to use the same
> naming scheme everywhere.
>
> > +
> > +struct wait_queue_head hld_detector_wait __initdata =
> > + __WAIT_QUEUE_HEAD_INITIALIZER(hld_detector_wait);
> > +
> > +static struct work_struct detector_work __initdata =
>
> I would call this "lockup_detector_work" to use the same naming scheme
> everywhere.
For the naming part, I'll revise both of them in next patch.
>
> > + __WORK_INITIALIZER(detector_work, lockup_detector_delay_init);
> > +
> > +static void __init lockup_detector_delay_init(struct work_struct *work)
> > +{
> > + int ret;
> > +
> > + wait_event(hld_detector_wait,
> > + detector_delay_init_state == DELAY_INIT_READY);
>
> DELAY_INIT_READY is defined in the 5th patch.
>
> There are many other build errors because this patch uses something
> that is defined in the 5th patch.
Thanks for pointing this out, the I'll fix 4th and 5th patches to correct the order.
>
> > + ret = watchdog_nmi_probe();
> > + if (!ret) {
> > + nmi_watchdog_available = true;
> > + lockup_detector_setup();
> > + } else {
> > + WARN_ON(ret == -EBUSY);
>
> Why WARN_ON(), please?
>
> Note that it might cause panic() when "panic_on_warn" command line
> parameter is used.
>
> Also the backtrace will not help much. The context is well known.
> This code is called from a workqueue worker.
The motivation to WARN should be:
lockup_detector_init
-> watchdog_nmi_probe return -EBUSY
-> lockup_detector_delay_init checks (detector_delay_init_state == DELAY_INIT_READY)
-> watchdog_nmi_probe checks
+ if (detector_delay_init_state != DELAY_INIT_READY)
+ return -EBUSY;
Since we first check detector_delay_init_state equals to DELAY_INIT_READY
and goes into watchdog_nmi_probe() and checks detector_delay_init_state again
becasue now we move from common part to arch part code.
In this condition, there shouldn't have any racing to detector_delay_init_state.
If it does happend an unknown racing, then shows a warning to it.
I think it make sense to remove WARN now becasue it looks verbosely...
However, I would rather change the following printk to
"Delayed init for lockup detector failed."
Is this fine with you?
>
> > + pr_info("Perf NMI watchdog permanently disabled\n");
> > + }
> > +}
> > +
> > +/* Ensure the check is called after the initialization of PMU driver */
> > +static int __init lockup_detector_check(void)
> > +{
> > + if (detector_delay_init_state < DELAY_INIT_WAIT)
> > + return 0;
> > +
> > + if (WARN_ON(detector_delay_init_state == DELAY_INIT_WAIT)) {
>
> Again. Is WARN_ON() needed?
>
> Also the condition looks wrong. IMHO, this is the expected state.
>
This does expected DELAY_INIT_READY here, which means,
every one who comes here to be checked should be READY and WARN if you're
still in WAIT state, and which means the previous lockup_detector_delay_init()
failed.
IMO, either keeping or removing WARN is fine with me.
I think I'll remove WARN and add
pr_info("Delayed init checking for lockup detector failed, retry for once.");
inside the `if (detector_delay_init_state == DELAY_INIT_WAIT)`
Or would you have any other suggestion? thanks.
> > + detector_delay_init_state = DELAY_INIT_READY;
> > + wake_up(&hld_detector_wait);
> > + }
> > + flush_work(&detector_work);
> > + return 0;
> > +}
> > +late_initcall_sync(lockup_detector_check);
>
> Otherwise, it make sense.
>
> Best Regards,
> Petr
>
> PS: I am not going to review the last patch because I am no familiar
> with arm. I reviewed just the changes in the generic watchdog
> code.
Thanks again for your review.
BRs,
Lecopzer
