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

Commit Message

Lecopzer Chen March 24, 2022, 2:14 p.m. UTC

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 <[e-mail]>
Signed-off-by: Pingfan Liu <[e-mail]>
Signed-off-by: Lecopzer Chen <[e-mail]>
Suggested-by: Petr Mladek <[e-mail]>
---
 include/linux/nmi.h |  3 ++
 kernel/watchdog.c   | 69 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 70 insertions(+), 2 deletions(-)

Comments

Petr Mladek April 4, 2022, 2:41 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Pingfan Liu <[e-mail]>
> Signed-off-by: Lecopzer Chen <[e-mail]>
> Suggested-by: Petr Mladek <[e-mail]>
> ---
>  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;


> +}
> +
> +/*
> + * 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.

> +	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 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;
}

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;
}

Best Regards,
Petr

Lecopzer Chen April 5, 2022, 1:35 p.m. UTC | #2

> 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 <[e-mail]>
> > Signed-off-by: Pingfan Liu <[e-mail]>
> > Signed-off-by: Lecopzer Chen <[e-mail]>
> > Suggested-by: Petr Mladek <[e-mail]>
> > ---
> >  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

Petr Mladek April 5, 2022, 3:19 p.m. UTC | #3

On Tue 2022-04-05 21:35:03, Lecopzer Chen wrote:
> > 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
> > > 
> > > --- a/kernel/watchdog.c
> > > +++ b/kernel/watchdog.c
> > > +}
> > > +
> > > +/*
> > > + * 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;

I see. It is exactly the reason why I suggest to remove the
optimization and keep the code simple.

> 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;

Grrr, this is so complicated and confusing. It might be because of
badly selected variable names or comments. But I think that it is
simply a bad approach.

OK, you suggest two variables. If I get it correctly:

    + The variable "delayed_init_allowed"
     tries to prevent the race in lockup_detector_check().

     It will make sure that the work could not be queued after
     flush_work() finishes.

     Is this obvious from the comment?
     Is this obvious from the variable name?

     I am sorry. But it is not obvious to me. I understand it only
     because I see it together in this mail. It will be pretty
     hard to get it from the code when I see it one year later.


   + The variable "allow_lockup_detector_init_retry" has an unclear
     meaning. It might mean:

	+ watchdog_nmi_probe() ended with -EBUSY in
	  lockup_detector_init() and we can try the delayed init.

	+ but it also means that watchdog_nmi_probe() succeeded in
	  lockup_detector_delay_init() and there is no need to
	  try the delayed init any longer.

       Is this obvious from the variable name?
       Is it explained anywhere?
       Is it easy to understand?

       No, from my POV. It is really bad idea to have a single
       variable with so many meanings.


And this is my problem with this approach. There was one variable with
unclear meanting. And you are trying to fix it by two variables
with unclear meaning.

> 	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;
> }

No, please keep it simple. Just have one variable that will say
whether we are allowed to queue the work:

  + It will be allowed when watchdog_nmi_probe() ended
    with -EBUSY in lockup_detector_init()

  + It will not longer be allowed when watchdog_nmi_probe()
    succeeded or when lockup_detector_check() flushes
    the pending works.


Best Regards,
Petr

Lecopzer Chen April 7, 2022, 4:21 p.m. UTC | #4

> On Tue 2022-04-05 21:35:03, Lecopzer Chen wrote:
> > > 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
> > > > 
> > > > --- a/kernel/watchdog.c
> > > > +++ b/kernel/watchdog.c
> > > > +}
> > > > +
> > > > +/*
> > > > + * 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;
> 
> I see. It is exactly the reason why I suggest to remove the
> optimization and keep the code simple.
> 
> > 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;
> 
> Grrr, this is so complicated and confusing. It might be because of
> badly selected variable names or comments. But I think that it is
> simply a bad approach.
> 
> OK, you suggest two variables. If I get it correctly:
> 
>     + The variable "delayed_init_allowed"
>      tries to prevent the race in lockup_detector_check().
> 
>      It will make sure that the work could not be queued after
>      flush_work() finishes.
> 
>      Is this obvious from the comment?
>      Is this obvious from the variable name?
> 
>      I am sorry. But it is not obvious to me. I understand it only
>      because I see it together in this mail. It will be pretty
>      hard to get it from the code when I see it one year later.
> 
> 
>    + The variable "allow_lockup_detector_init_retry" has an unclear
>      meaning. It might mean:
> 
> 	+ watchdog_nmi_probe() ended with -EBUSY in
> 	  lockup_detector_init() and we can try the delayed init.
> 
> 	+ but it also means that watchdog_nmi_probe() succeeded in
> 	  lockup_detector_delay_init() and there is no need to
> 	  try the delayed init any longer.
> 
>        Is this obvious from the variable name?
>        Is it explained anywhere?
>        Is it easy to understand?
> 
>        No, from my POV. It is really bad idea to have a single
>        variable with so many meanings.
> 
> 
> And this is my problem with this approach. There was one variable with
> unclear meanting. And you are trying to fix it by two variables
> with unclear meaning.
> 

I really apreciate for your reply, many thanks for it.

For my point of view, the naming for "delayed_init_allowed" is the
whole system state now is able to(allowed) do delayed init.
The "allow_lockup_detector_init_retry" is that delayed init is ready
to retry register NMI watchdog.

Thus the meaning of delayed_init_"allowed" is, we are allowed to
do delayed init including
    1. initialization of delayed init
    2. the retry of delayed init

I'm sorry for that I didn't express the meaning clearly.
I'll have definition in detail in the later version patch not only
a brief comment and I hope you can review much easier.
This only explain the thought how I made decision for the naming.

So let me back to the discussion below.



> > 	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;
> > }
> 
> No, please keep it simple. Just have one variable that will say
> whether we are allowed to queue the work:
> 
>   + It will be allowed when watchdog_nmi_probe() ended
>     with -EBUSY in lockup_detector_init()
> 
>   + It will not longer be allowed when watchdog_nmi_probe()
>     succeeded or when lockup_detector_check() flushes
>     the pending works.
> 

Okay, let me think about it. I'll try to find a better solution that
only uses one variable.
And it's strongly about how users use it in 5th patch, I'll give further
reply in 5th patch


thanks
BRs,
Lecopzer

Patch

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();
+	allow_lockup_detector_init_retry = false;
+}
+
+/*
+ * 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);
+
+	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");
+	return 0;
+}
+late_initcall_sync(lockup_detector_check);
+
 void __init lockup_detector_init(void)
 {
+	int ret;
+
 	if (tick_nohz_full_enabled())
 		pr_info("Disabling watchdog on nohz_full cores by default\n");
 
 	cpumask_copy(&watchdog_cpumask,
 		     housekeeping_cpumask(HK_FLAG_TIMER));
 
-	if (!watchdog_nmi_probe())
+	ret = watchdog_nmi_probe();
+	if (!ret)
 		nmi_watchdog_available = true;
+	else if (ret == -EBUSY)
+		allow_lockup_detector_init_retry = true;
+
 	lockup_detector_setup();
 	watchdog_sysctl_init();
 }