Re: [PATCH v3 4/5] kernel/watchdog: Adapt the watchdog_hld interface for async model

[Lecopzer Chen <lecopzer.chen@mediatek.com>]

> On Thu 2022-03-24 22:14:04, Lecopzer Chen wrote:
> > 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 and try to initialize
> > the watchdog once again later.
> > The delayed probe is called using workqueues. It need to allocate
> > memory and must be proceed in a normal context.
> > The delayed probe is queued only when the early one returns -EBUSY.
> > It is the return code returned when PMU is not ready yet.
> > 
> > Provide an API - retry_lockup_detector_init() for anyone who needs
> > to delayed init lockup detector.
> > 
> > The original assumption is: nobody should use delayed probe after
> > lockup_detector_check() which has __init attribute.
> > That is, anyone uses this API must call between lockup_detector_init()
> > and lockup_detector_check(), and the caller must have __init attribute
> > 
> > Co-developed-by: Pingfan Liu <kernelfans@gmail.com>
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/linux/nmi.h |  3 ++
> >  kernel/watchdog.c   | 69 +++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 70 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> > index b7bcd63c36b4..1d84c9a8b460 100644
> > --- a/include/linux/nmi.h
> > +++ b/include/linux/nmi.h
> > @@ -118,6 +118,9 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }
> >  
> >  void watchdog_nmi_stop(void);
> >  void watchdog_nmi_start(void);
> > +
> > +extern bool allow_lockup_detector_init_retry;
> > +void retry_lockup_detector_init(void);
> >  int watchdog_nmi_probe(void);
> >  void watchdog_nmi_enable(unsigned int cpu);
> >  void watchdog_nmi_disable(unsigned int cpu);
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index b71d434cf648..308ba29f8f0f 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -103,7 +103,13 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
> >  	hardlockup_detector_perf_disable();
> >  }
> >  
> > -/* Return 0, if a NMI watchdog is available. Error code otherwise */
> > +/*
> > + * Arch specific API.
> > + *
> > + * Return 0 when NMI watchdog is available, negative value otherwise.
> > + * The error code -EBUSY is special. It means that a deferred probe
> > + * might succeed later.
> > + */
> >  int __weak __init watchdog_nmi_probe(void)
> >  {
> >  	return hardlockup_detector_perf_init();
> > @@ -839,16 +845,75 @@ 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);
> > +bool allow_lockup_detector_init_retry __initdata;
> > +
> > +static struct work_struct detector_work __initdata =
> > +		__WORK_INITIALIZER(detector_work, lockup_detector_delay_init);
> > +
> > +static void __init lockup_detector_delay_init(struct work_struct *work)
> > +{
> > +	int ret;
> > +
> > +	ret = watchdog_nmi_probe();
> > +	if (ret) {
> > +		pr_info("Delayed init of the lockup detector failed: %d\n", ret);
> > +		pr_info("Perf NMI watchdog permanently disabled\n");
> > +		return;
> > +	}
> > +
> > +	nmi_watchdog_available = true;
> > +	lockup_detector_setup();
> 
> The name of the variable "allow_lockup_detector_init_retry" is
> slightly confusing in this context. I suggest to add a comment:
> 
> 	/* Retry is not needed any longer. */
> > +	allow_lockup_detector_init_retry = false;
> 

Got it, I'll add it, thanks.


> 
> > +}
> > +
> > +/*
> > + * retry_lockup_detector_init - retry init lockup detector if possible.
> > + *
> > + * Only take effect when allow_lockup_detector_init_retry is true, which
> > + * means it must call between lockup_detector_init() and lockup_detector_check().
> > + * Be aware that caller must have __init attribute, relative functions
> > + * will be freed after kernel initialization.
> > + */
> > +void __init retry_lockup_detector_init(void)
> > +{
> > +	if (!allow_lockup_detector_init_retry)
> > +		return;
> > +
> > +	queue_work_on(__smp_processor_id(), system_wq, &detector_work);
> > +}
> > +
> > +/* Ensure the check is called after the initialization of driver */
> > +static int __init lockup_detector_check(void)
> > +{
> > +	/* Make sure no work is pending. */
> > +	flush_work(&detector_work);
> 
> This is racy. We should first disable
> "allow_lockup_detector_init_retry" to make sure
> that retry_lockup_detector_init() will not queue
> the work any longer.

But disable before flush_work will make the 
    lockup_detector_delay_init() ->
    watchdog_nmi_probe ->
    +	if (!allow_lockup_detector_init_retry)
    +		return -EBUSY;

Plese check the code I provide below.


> 
> > +	if (!allow_lockup_detector_init_retry)
> > +		return 0;
> > +
> > +	allow_lockup_detector_init_retry = false;
> > +	pr_info("Delayed init checking failed, please check your driver.\n");
> 
> This prints that the init failed without checking the state
> of the watchdog. I guess that it works but it is far from
> obvious and any further change might break it.
> 
> Is the message really needed?
> Does it help?
> What exact driver needs checking?
> 
> IMHO, it just makes the code more complicated and
> it is not worth it.
> 

I think you're right, the message was needed in the patch v2 because we
did another retry in lockup_detector_check().
But now we only do "checking" and the failed message in
lockup_detector_delay_init should be enough.


> I suggest to keep it simple:
> 
> /*
>  * Ensure the check is called after the initialization of driver
>  * and before removing init code.
>  */
> static int __init lockup_detector_check(void)
> {
> 	allow_lockup_detector_init_retry = false;
> 	flush_work(&detector_work);
> 
> 	return 0;
> }
> 


Combine with the first racy problem, let me limit retry_lockup_detector_init
can be called only once.

how about:
...
static bool __init delayed_init_allowed = true;
...
/*
 * retry_lockup_detector_init - retry init lockup detector if possible.
 *
 * Only take effect when allow_lockup_detector_init_retry is true, which
 * means it must call between lockup_detector_init() and lockup_detector_check().
 * Be aware that caller must have __init attribute, relative functions
 * will be freed after kernel initialization.
 */
void __init retry_lockup_detector_init(void)
{
	if (!allow_lockup_detector_init_retry || !delayed_init_allowed)
		return;

	/* 
	 * we shouldn't queue any delayed init work twice to avoid
	 * any unwanted racy.
	 */
	delayed_init_allowed = false;
	queue_work_on(__smp_processor_id(), system_wq, &detector_work);
}


/*
 * Ensure the check is called after the initialization of driver
 * and before removing init code.
 */
static int __init lockup_detector_check(void)
{
	delayed_init_allowed = false;
	flush_work(&detector_work);
	allow_lockup_detector_init_retry = false;

	return 0;
}



> or if you really want that message then I would do:
> 
> /*
>  * Ensure the check is called after the initialization of driver
>  * and before removing init code.
>  */
> static int __init lockup_detector_check(void)
> {
> 	bool delayed_init_allowed = allow_lockup_detector_init_retry;
> 
> 	allow_lockup_detector_init_retry = false;
> 	flush_work(&detector_work);
> 
> 	if (delayed_init_allowed && !nmi_watchdog_available)
> 		pr_info("Delayed init failed. Please, check your driver.\n");
> 
> 	return 0;
> }
> 


thanks
BRs,
Lecopzer