[PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[Thomas Gleixner]

The rework of the watchdog core to use cpu_stop_work broke the watchdog
cpumask on CPU hotplug.

The watchdog_enable/disable() functions are now called unconditionally from
the hotplug callback, i.e. even on CPUs which are not in the watchdog
cpumask.

Only invoke them when the plugged CPU is in the watchdog cpumask.

Fixes: 9cf57731b63e ("watchdog/softlockup: Replace "watchdog/%u" threads with cpu_stop_work")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 kernel/watchdog.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -547,13 +547,15 @@ static void softlockup_start_all(void)
 
 int lockup_detector_online_cpu(unsigned int cpu)
 {
-	watchdog_enable(cpu);
+	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
+		watchdog_enable(cpu);
 	return 0;
 }
 
 int lockup_detector_offline_cpu(unsigned int cpu)
 {
-	watchdog_disable(cpu);
+	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
+		watchdog_disable(cpu);
 	return 0;
 }
 

Re: [PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[Oleg Nesterov]

On 03/26, Thomas Gleixner wrote:
>
> The rework of the watchdog core to use cpu_stop_work broke the watchdog
> cpumask on CPU hotplug.
>
> The watchdog_enable/disable() functions are now called unconditionally from
> the hotplug callback, i.e. even on CPUs which are not in the watchdog
> cpumask.
>
> Only invoke them when the plugged CPU is in the watchdog cpumask.
>
> Fixes: 9cf57731b63e ("watchdog/softlockup: Replace "watchdog/%u" threads with cpu_stop_work")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  kernel/watchdog.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -547,13 +547,15 @@ static void softlockup_start_all(void)
>
>  int lockup_detector_online_cpu(unsigned int cpu)
>  {
> -	watchdog_enable(cpu);
> +	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
> +		watchdog_enable(cpu);
>  	return 0;
>  }
>
>  int lockup_detector_offline_cpu(unsigned int cpu)
>  {
> -	watchdog_disable(cpu);
> +	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
> +		watchdog_disable(cpu);
>  	return 0;
>  }

IIUC without this fix an NMI watchdog can too be enabled at boot time even
if the initial watchdog_cpumask = housekeeping_cpumask(HK_FLAG_TIMER) doesn't
include the plugged CPU.

And after that writing 0 to /proc/sys/kernel/nmi_watchdog clears
NMI_WATCHDOG_ENABLED but this can't disable NMI watchdog's outside of
watchdog_allowed_mask.

So may be this can explain the problem reported by Maxime ?
See https://lore.kernel.org/lkml/b99c5a25-a5fe-18dd-2f1d-bdd6834f03e5@redhat.com/

Oleg.

Re: [PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[Thomas Gleixner]

On Wed, 27 Mar 2019, Oleg Nesterov wrote:
> On 03/26, Thomas Gleixner wrote:
> >
> > The rework of the watchdog core to use cpu_stop_work broke the watchdog
> > cpumask on CPU hotplug.
> >
> > The watchdog_enable/disable() functions are now called unconditionally from
> > the hotplug callback, i.e. even on CPUs which are not in the watchdog
> > cpumask.
> >
> > Only invoke them when the plugged CPU is in the watchdog cpumask.
> 
> IIUC without this fix an NMI watchdog can too be enabled at boot time even
> if the initial watchdog_cpumask = housekeeping_cpumask(HK_FLAG_TIMER) doesn't
> include the plugged CPU.

Yes.

> And after that writing 0 to /proc/sys/kernel/nmi_watchdog clears
> NMI_WATCHDOG_ENABLED but this can't disable NMI watchdog's outside of
> watchdog_allowed_mask.

Correct

> So may be this can explain the problem reported by Maxime ?
> See https://lore.kernel.org/lkml/b99c5a25-a5fe-18dd-2f1d-bdd6834f03e5@redhat.com/

That looks so.

Thanks,

	tglx

Re: [PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[Maxime Coquelin]

On 3/27/19 8:10 PM, Thomas Gleixner wrote:
> On Wed, 27 Mar 2019, Oleg Nesterov wrote:
>> On 03/26, Thomas Gleixner wrote:
>>>
>>> The rework of the watchdog core to use cpu_stop_work broke the watchdog
>>> cpumask on CPU hotplug.
>>>
>>> The watchdog_enable/disable() functions are now called unconditionally from
>>> the hotplug callback, i.e. even on CPUs which are not in the watchdog
>>> cpumask.
>>>
>>> Only invoke them when the plugged CPU is in the watchdog cpumask.
>>
>> IIUC without this fix an NMI watchdog can too be enabled at boot time even
>> if the initial watchdog_cpumask = housekeeping_cpumask(HK_FLAG_TIMER) doesn't
>> include the plugged CPU.
> 
> Yes.
> 
>> And after that writing 0 to /proc/sys/kernel/nmi_watchdog clears
>> NMI_WATCHDOG_ENABLED but this can't disable NMI watchdog's outside of
>> watchdog_allowed_mask.
> 
> Correct
> 
>> So may be this can explain the problem reported by Maxime ?
>> See https://lore.kernel.org/lkml/b99c5a25-a5fe-18dd-2f1d-bdd6834f03e5@redhat.com/
> 
> That looks so.

I had a trial with your patch, and I can confirm it fixes my issue:

Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

> Thanks,
> 
> 	tglx
> 

[tip:core/urgent] watchdog: Respect watchdog cpumask on CPU hotplug

[tip-bot for Thomas Gleixner]

Commit-ID:  7dd47617114921fdd8c095509e5e7b4373cc44a1
Gitweb:     https://git.kernel.org/tip/7dd47617114921fdd8c095509e5e7b4373cc44a1
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 26 Mar 2019 22:51:02 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 28 Mar 2019 13:32:01 +0100

watchdog: Respect watchdog cpumask on CPU hotplug

The rework of the watchdog core to use cpu_stop_work broke the watchdog
cpumask on CPU hotplug.

The watchdog_enable/disable() functions are now called unconditionally from
the hotplug callback, i.e. even on CPUs which are not in the watchdog
cpumask. As a consequence the watchdog can become unstoppable.

Only invoke them when the plugged CPU is in the watchdog cpumask.

Fixes: 9cf57731b63e ("watchdog/softlockup: Replace "watchdog/%u" threads with cpu_stop_work")
Reported-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1903262245490.1789@nanos.tec.linutronix.de

---
 kernel/watchdog.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 403c9bd90413..6a5787233113 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -554,13 +554,15 @@ static void softlockup_start_all(void)
 
 int lockup_detector_online_cpu(unsigned int cpu)
 {
-	watchdog_enable(cpu);
+	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
+		watchdog_enable(cpu);
 	return 0;
 }
 
 int lockup_detector_offline_cpu(unsigned int cpu)
 {
-	watchdog_disable(cpu);
+	if (cpumask_test_cpu(cpu, &watchdog_allowed_mask))
+		watchdog_disable(cpu);
 	return 0;
 }