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.
> + __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.
> + 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.
> + 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.
> + 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.
