[PATCH 0/2] Introduce update_arch_nmi_watchdog for arch specific handlers

[PATCH 0/2] Introduce update_arch_nmi_watchdog for arch specific handlers

[Babu Moger]

During our testing we noticed that nmi watchdogs in sparc could not be disabled or
enabled dynamically using sysctl/proc interface. Sparc uses its own arch specific
nmi watchdogs. There is a sysctl and proc interface(proc/sys/kernel/nmi_watchdog)
to enable/disable nmi watchdogs. However, that is not working for sparc. There
is no interface to feed this parameter to arch specific nmi watchdogs.

These patches extend the same sysctl/proc interface to enable or disable
these arch specific nmi watchdogs dynamically. Introduced new function
update_arch_nmi_watchdog which can be implemented in arch specific handlers.
If you think there is a better way to do this. Please advice.

Tested on sparc. Compile tested on x86.

Babu Moger (2):
  watchdog: Introduce update_arch_nmi_watchdog
  sparc: Implement update_arch_nmi_watchdog

 arch/sparc/kernel/nmi.c |   26 ++++++++++++++++++++++++++
 include/linux/nmi.h     |    1 +
 kernel/watchdog.c       |   16 +++++++++++++---
 3 files changed, 40 insertions(+), 3 deletions(-)

[PATCH 1/2] watchdog: Introduce update_arch_nmi_watchdog

[Babu Moger]

Currently we do not have a way to enable/disable arch specific
watchdog handlers if it was implemented by any of the architectures.

This patch introduces new function update_arch_nmi_watchdog
which can be used to enable/disable architecture specific NMI
watchdog handlers. Also exposes watchdog_enabled variable outside
so that arch specific nmi watchdogs can use it to implement
enalbe/disable behavour.

Signed-off-by: Babu Moger <babu.moger@oracle.com>
---
 include/linux/nmi.h |    1 +
 kernel/watchdog.c   |   16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index 4630eea..01b4830 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -66,6 +66,7 @@ static inline bool trigger_allbutself_cpu_backtrace(void)
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 u64 hw_nmi_get_sample_period(int watchdog_thresh);
+extern unsigned long watchdog_enabled;
 extern int nmi_watchdog_enabled;
 extern int soft_watchdog_enabled;
 extern int watchdog_user_enabled;
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9acb29f..1ac2814 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -46,16 +46,21 @@
 
 static DEFINE_MUTEX(watchdog_proc_mutex);
 
-#ifdef CONFIG_HARDLOCKUP_DETECTOR
-static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
+#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
+unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
 #else
-static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
+unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
 #endif
 int __read_mostly nmi_watchdog_enabled;
 int __read_mostly soft_watchdog_enabled;
 int __read_mostly watchdog_user_enabled;
 int __read_mostly watchdog_thresh = 10;
 
+/*
+ * Implemented by arch specific handlers if it defines CONFIG_HAVE_NMI_WATCHDOG
+ */
+void __weak update_arch_nmi_watchdog(void) {}
+
 #ifdef CONFIG_SMP
 int __read_mostly sysctl_softlockup_all_cpu_backtrace;
 int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
@@ -842,6 +847,11 @@ static int proc_watchdog_update(void)
 	int err = 0;
 
 	/*
+	 * Enable/Disable arch specific nmi watchdogs if there is one
+	 */
+	update_arch_nmi_watchdog();
+
+	/*
 	 * Watchdog threads won't be started if they are already active.
 	 * The 'watchdog_running' variable in watchdog_*_all_cpus() takes
 	 * care of this. If those threads are already active, the sample
-- 
1.7.1

Re: [PATCH 1/2] watchdog: Introduce update_arch_nmi_watchdog

[Sam Ravnborg]

On Thu, Oct 06, 2016 at 03:16:42PM -0700, Babu Moger wrote:
> Currently we do not have a way to enable/disable arch specific
> watchdog handlers if it was implemented by any of the architectures.
> 
> This patch introduces new function update_arch_nmi_watchdog
> which can be used to enable/disable architecture specific NMI
> watchdog handlers. Also exposes watchdog_enabled variable outside
> so that arch specific nmi watchdogs can use it to implement
> enalbe/disable behavour.
> 
> Signed-off-by: Babu Moger <babu.moger@oracle.com>
> ---
>  include/linux/nmi.h |    1 +
>  kernel/watchdog.c   |   16 +++++++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 4630eea..01b4830 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -66,6 +66,7 @@ static inline bool trigger_allbutself_cpu_backtrace(void)
>  
>  #ifdef CONFIG_LOCKUP_DETECTOR
>  u64 hw_nmi_get_sample_period(int watchdog_thresh);
> +extern unsigned long watchdog_enabled;

The extern is within an #ifdef, but the definition later is
valid alway.
So extern definition should be outside the #ifdef to match the
actual implementation.

To manipulate / read watchdog_enabled two constants are used: 
NMI_WATCHDOG_ENABLED, SOFT_WATCHDOG_ENABLED

They should be visible too, so uses do not fall into the trap
and uses constants (like in patch 2).

	Sam

Re: [PATCH 1/2] watchdog: Introduce update_arch_nmi_watchdog

[Babu Moger]

On 10/6/2016 11:34 PM, Sam Ravnborg wrote:
> On Thu, Oct 06, 2016 at 03:16:42PM -0700, Babu Moger wrote:
>> Currently we do not have a way to enable/disable arch specific
>> watchdog handlers if it was implemented by any of the architectures.
>>
>> This patch introduces new function update_arch_nmi_watchdog
>> which can be used to enable/disable architecture specific NMI
>> watchdog handlers. Also exposes watchdog_enabled variable outside
>> so that arch specific nmi watchdogs can use it to implement
>> enalbe/disable behavour.
>>
>> Signed-off-by: Babu Moger <babu.moger@oracle.com>
>> ---
>>   include/linux/nmi.h |    1 +
>>   kernel/watchdog.c   |   16 +++++++++++++---
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
>> index 4630eea..01b4830 100644
>> --- a/include/linux/nmi.h
>> +++ b/include/linux/nmi.h
>> @@ -66,6 +66,7 @@ static inline bool trigger_allbutself_cpu_backtrace(void)
>>   
>>   #ifdef CONFIG_LOCKUP_DETECTOR
>>   u64 hw_nmi_get_sample_period(int watchdog_thresh);
>> +extern unsigned long watchdog_enabled;
> The extern is within an #ifdef, but the definition later is
> valid alway.
> So extern definition should be outside the #ifdef to match the
> actual implementation.

      Ok. Sure.

>
> To manipulate / read watchdog_enabled two constants are used:
> NMI_WATCHDOG_ENABLED, SOFT_WATCHDOG_ENABLED

     Sure. I will bring these definitions to nmi.h from watchdog.c

> They should be visible too, so uses do not fall into the trap
> and uses constants (like in patch 2).

     Will re-post v2 version with these changes. Thanks for the comments.

>
> 	Sam

[PATCH 2/2] sparc: Implement update_arch_nmi_watchdog

[Babu Moger]

Implement function update_arch_nmi_watchdog to enable/disable
nmi watchdog. Sparc uses arch specific nmi watchdog handler.
Currently, we do not have a way to enable/disable nmi watchdog
dynamically. With these patches we can enable or disable arch
specinf nmi watchdogs using proc or sysctl interface.

Example commands.
To enable: echo 1 >  /proc/sys/kernel/nmi_watchdog
To disable: echo 0 >  /proc/sys/kernel/nmi_watchdog

It can also achieved using the sysctl parameter kernel.nmi_watchdog

Signed-off-by: Babu Moger <babu.moger@oracle.com>
---
 arch/sparc/kernel/nmi.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/kernel/nmi.c b/arch/sparc/kernel/nmi.c
index a9973bb..27c4e18 100644
--- a/arch/sparc/kernel/nmi.c
+++ b/arch/sparc/kernel/nmi.c
@@ -153,6 +153,8 @@ static void report_broken_nmi(int cpu, int *prev_nmi_count)
 
 void stop_nmi_watchdog(void *unused)
 {
+	if (!__this_cpu_read(wd_enabled))
+		return;
 	pcr_ops->write_pcr(0, pcr_ops->pcr_nmi_disable);
 	__this_cpu_write(wd_enabled, 0);
 	atomic_dec(&nmi_active);
@@ -207,6 +209,8 @@ error:
 
 void start_nmi_watchdog(void *unused)
 {
+	if (__this_cpu_read(wd_enabled))
+		return;
 	__this_cpu_write(wd_enabled, 1);
 	atomic_inc(&nmi_active);
 
@@ -270,3 +274,25 @@ static int __init setup_nmi_watchdog(char *str)
 	return 0;
 }
 __setup("nmi_watchdog=", setup_nmi_watchdog);
+
+#ifdef CONFIG_LOCKUP_DETECTOR
+void update_arch_nmi_watchdog(void)
+{
+	if (atomic_read(&nmi_active) < 0) {
+		printk(KERN_WARNING
+		       "NMI watchdog cannot be enabled or disabled\n");
+		return;
+	}
+
+	/*
+	 * Check for bit 0. Bit 0 is dedicated for hard lockup detector or
+	 * arch specific nmi and bit 1 for the soft lockup detector. We
+	 * are interested only in bit 0 here.
+	 */
+	if (watchdog_enabled & 1)
+		on_each_cpu(start_nmi_watchdog, NULL, 1);
+	else
+		on_each_cpu(stop_nmi_watchdog, NULL, 1);
+
+}
+#endif
-- 
1.7.1

Re: [PATCH 0/2] Introduce update_arch_nmi_watchdog for arch specific handlers

[Don Zickus]

On Thu, Oct 06, 2016 at 03:16:41PM -0700, Babu Moger wrote:
> During our testing we noticed that nmi watchdogs in sparc could not be disabled or
> enabled dynamically using sysctl/proc interface. Sparc uses its own arch specific
> nmi watchdogs. There is a sysctl and proc interface(proc/sys/kernel/nmi_watchdog)
> to enable/disable nmi watchdogs. However, that is not working for sparc. There
> is no interface to feed this parameter to arch specific nmi watchdogs.
> 
> These patches extend the same sysctl/proc interface to enable or disable
> these arch specific nmi watchdogs dynamically. Introduced new function
> update_arch_nmi_watchdog which can be implemented in arch specific handlers.
> If you think there is a better way to do this. Please advice.
> 
> Tested on sparc. Compile tested on x86.

Hi Babu,

Thanks for the patch.  Yeah, I don't test sparc at all (lack of hardware).
Sorry about that.

We did spend quite a bit of time trying to get various soft/hard lockup
logic going for the /proc stuff and I am wondering if your patches are to
simple and expose some of the races we tried to fix.

Therefore I am wondering if we could re-use some of our logic for your case.

The perf stuff is really the x86 equivalent of arch_watchdog_enable.  I am
wondering if we break that out as a __weak default function and then have
sparc override it with its own enable/disable functions.  Something along
the lines below (compiled on x86 but untested)?

Cheers,
Don


diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9acb29f..55cd2d3 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
  */
 static unsigned long cpu0_err;
 
-static int watchdog_nmi_enable(unsigned int cpu)
+int __weak arch_watchdog_nmi_enable(unsigned int cpu)
 {
 	struct perf_event_attr *wd_attr;
 	struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
-	/* nothing to do if the hard lockup detector is disabled */
-	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
-		goto out;
-
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
 		goto out_save;
 	}
 
-	/*
-	 * Disable the hard lockup detector if _any_ CPU fails to set up
-	 * set up the hardware perf event. The watchdog() function checks
-	 * the NMI_WATCHDOG_ENABLED bit periodically.
-	 *
-	 * The barriers are for syncing up watchdog_enabled across all the
-	 * cpus, as clear_bit() does not use barriers.
-	 */
-	smp_mb__before_atomic();
-	clear_bit(NMI_WATCHDOG_ENABLED_BIT, &watchdog_enabled);
-	smp_mb__after_atomic();
-
 	/* skip displaying the same error again */
 	if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
 		return PTR_ERR(event);
@@ -658,7 +642,36 @@ out:
 	return 0;
 }
 
-static void watchdog_nmi_disable(unsigned int cpu)
+static int watchdog_nmi_enable(unsigned int cpu)
+{
+	int err;
+
+	/* nothing to do if the hard lockup detector is disabled */
+	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
+		return 0;
+
+	err = arch_watchdog_nmi_enable(cpu);
+
+	if (err) {
+		/*
+		 * Disable the hard lockup detector if _any_ CPU fails to set up
+		 * set up the hardware perf event. The watchdog() function checks
+		 * the NMI_WATCHDOG_ENABLED bit periodically.
+		 *
+		 * The barriers are for syncing up watchdog_enabled across all the
+		 * cpus, as clear_bit() does not use barriers.
+		 */
+		smp_mb__before_atomic();
+		clear_bit(NMI_WATCHDOG_ENABLED_BIT, &watchdog_enabled);
+		smp_mb__after_atomic();
+
+		return err;
+	}
+
+	return 0;
+}
+
+void __weak arch_watchdog_nmi_disable(unsigned int cpu)
 {
 	struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
@@ -675,6 +688,11 @@ static void watchdog_nmi_disable(unsigned int cpu)
 	}
 }
 
+static void watchdog_nmi_disable(unsigned int cpu)
+{
+	arch_watchdog_nmi_disable(cpu);
+}
+
 #else
 static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
 static void watchdog_nmi_disable(unsigned int cpu) { return; }

Re: [PATCH 0/2] Introduce update_arch_nmi_watchdog for arch specific handlers

[Babu Moger]

On 10/7/2016 10:51 AM, Don Zickus wrote:
> On Thu, Oct 06, 2016 at 03:16:41PM -0700, Babu Moger wrote:
>> During our testing we noticed that nmi watchdogs in sparc could not be disabled or
>> enabled dynamically using sysctl/proc interface. Sparc uses its own arch specific
>> nmi watchdogs. There is a sysctl and proc interface(proc/sys/kernel/nmi_watchdog)
>> to enable/disable nmi watchdogs. However, that is not working for sparc. There
>> is no interface to feed this parameter to arch specific nmi watchdogs.
>>
>> These patches extend the same sysctl/proc interface to enable or disable
>> these arch specific nmi watchdogs dynamically. Introduced new function
>> update_arch_nmi_watchdog which can be implemented in arch specific handlers.
>> If you think there is a better way to do this. Please advice.
>>
>> Tested on sparc. Compile tested on x86.
> Hi Babu,
>
> Thanks for the patch.  Yeah, I don't test sparc at all (lack of hardware).
> Sorry about that.
>
> We did spend quite a bit of time trying to get various soft/hard lockup
> logic going for the /proc stuff and I am wondering if your patches are to
> simple and expose some of the races we tried to fix.
>
> Therefore I am wondering if we could re-use some of our logic for your case.
>
> The perf stuff is really the x86 equivalent of arch_watchdog_enable.  I am
> wondering if we break that out as a __weak default function and then have
> sparc override it with its own enable/disable functions.  Something along
> the lines below (compiled on x86 but untested)?
Hi Don, Sorry for the late response. I ran into issues with the setups 
and new approach.
  I could not use your patches as is. Reason is sparc does not define 
CONFIG_HARDLOCKUP_DETECTOR.
  So, defining default __weak function did not  work for me. However, I 
have used your idea to
define __weak functions  arch_watchdog_nmi_enable and 
arch_watchdog_nmi_disable when
CONFIG_HARDLOCKUP_DETECTOR  is not defined. Sending v2 version now.
Please take a look. Thanks for your inputs.
>
> Cheers,
> Don
>
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 9acb29f..55cd2d3 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
>    */
>   static unsigned long cpu0_err;
>   
> -static int watchdog_nmi_enable(unsigned int cpu)
> +int __weak arch_watchdog_nmi_enable(unsigned int cpu)
>   {
>   	struct perf_event_attr *wd_attr;
>   	struct perf_event *event = per_cpu(watchdog_ev, cpu);
>   
> -	/* nothing to do if the hard lockup detector is disabled */
> -	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> -		goto out;
> -
>   	/* is it already setup and enabled? */
>   	if (event && event->state > PERF_EVENT_STATE_OFF)
>   		goto out;
> @@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
>   		goto out_save;
>   	}
>   
> -	/*
> -	 * Disable the hard lockup detector if _any_ CPU fails to set up
> -	 * set up the hardware perf event. The watchdog() function checks
> -	 * the NMI_WATCHDOG_ENABLED bit periodically.
> -	 *
> -	 * The barriers are for syncing up watchdog_enabled across all the
> -	 * cpus, as clear_bit() does not use barriers.
> -	 */
> -	smp_mb__before_atomic();
> -	clear_bit(NMI_WATCHDOG_ENABLED_BIT, &watchdog_enabled);
> -	smp_mb__after_atomic();
> -
>   	/* skip displaying the same error again */
>   	if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
>   		return PTR_ERR(event);
> @@ -658,7 +642,36 @@ out:
>   	return 0;
>   }
>   
> -static void watchdog_nmi_disable(unsigned int cpu)
> +static int watchdog_nmi_enable(unsigned int cpu)
> +{
> +	int err;
> +
> +	/* nothing to do if the hard lockup detector is disabled */
> +	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> +		return 0;
> +
> +	err = arch_watchdog_nmi_enable(cpu);
> +
> +	if (err) {
> +		/*
> +		 * Disable the hard lockup detector if _any_ CPU fails to set up
> +		 * set up the hardware perf event. The watchdog() function checks
> +		 * the NMI_WATCHDOG_ENABLED bit periodically.
> +		 *
> +		 * The barriers are for syncing up watchdog_enabled across all the
> +		 * cpus, as clear_bit() does not use barriers.
> +		 */
> +		smp_mb__before_atomic();
> +		clear_bit(NMI_WATCHDOG_ENABLED_BIT, &watchdog_enabled);
> +		smp_mb__after_atomic();
> +
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +void __weak arch_watchdog_nmi_disable(unsigned int cpu)
>   {
>   	struct perf_event *event = per_cpu(watchdog_ev, cpu);
>   
> @@ -675,6 +688,11 @@ static void watchdog_nmi_disable(unsigned int cpu)
>   	}
>   }
>   
> +static void watchdog_nmi_disable(unsigned int cpu)
> +{
> +	arch_watchdog_nmi_disable(cpu);
> +}
> +
>   #else
>   static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
>   static void watchdog_nmi_disable(unsigned int cpu) { return; }