Improve watchdog config for arch watchdogs

For architectures that define HAVE_NMI_WATCHDOG, instead of having them provide the complete touch_nmi_watchdog() function, just have them provide arch_touch_nmi_watchdog().

This gives the generic code more flexibility in implementing this function, and arch implementations don't miss out on touching the softlockup watchdog or other generic details.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/blackfin/include/asm/nmi.h            |  2 ++
 arch/blackfin/kernel/nmi.c                 |  2 +-
 arch/mn10300/include/asm/nmi.h             |  2 ++
 arch/mn10300/kernel/mn10300-watchdog-low.S |  8 ++++----
 arch/mn10300/kernel/mn10300-watchdog.c     |  2 +-
 arch/sparc/include/asm/nmi.h               |  1 +
 arch/sparc/kernel/nmi.c                    |  6 ++----
 include/linux/nmi.h                        | 26 +++++++++++++++-----------
 kernel/watchdog_hld.c                      |  5 ++---
 9 files changed, 30 insertions(+), 24 deletions(-)

arch/blackfin/include/asm/nmi.h +2 -0

index b9caac4fcfd8..6e7047ceec79 100644
@@ -7,6 +7,8 @@
 #ifndef _BFIN_NMI_H_
 #define _BFIN_NMI_H_
 
+void arch_touch_nmi_watchdog(void);
+
 #include <linux/nmi.h>
 
 #endif

arch/blackfin/kernel/nmi.c +1 -1

index 633c37083e87..1e714329fe8a 100644
@@ -190,7 +190,7 @@ static int __init init_nmi_wdt(void)
 }
 device_initcall(init_nmi_wdt);
 
-void touch_nmi_watchdog(void)
+void arch_touch_nmi_watchdog(void)
 {
 	atomic_set(&nmi_touched[smp_processor_id()], 1);
 }

arch/mn10300/include/asm/nmi.h +2 -0

index f3671cbbc117..aee8aa22d9ee 100644
@@ -11,4 +11,6 @@
 #ifndef _ASM_NMI_H
 #define _ASM_NMI_H
 
+void arch_touch_nmi_watchdog(void);
+
 #endif /* _ASM_NMI_H */

arch/mn10300/kernel/mn10300-watchdog-low.S +4 -4

index f2f5c9cfaabd..34f8773de7d0 100644
@@ -50,9 +50,9 @@ watchdog_handler:
 #   we can't inline it)
 #
 ###############################################################################
-	.globl	touch_nmi_watchdog
-	.type	touch_nmi_watchdog,@function
-touch_nmi_watchdog:
+	.globl	arch_touch_nmi_watchdog
+	.type	arch_touch_nmi_watchdog,@function
+arch_touch_nmi_watchdog:
 	clr	d0
 	clr	d1
 	mov	watchdog_alert_counter, a0

@@ -63,4 +63,4 @@ touch_nmi_watchdog:
 	lne
 	ret	[],0
 
-	.size	touch_nmi_watchdog,.-touch_nmi_watchdog
+	.size	arch_touch_nmi_watchdog,.-arch_touch_nmi_watchdog

arch/mn10300/kernel/mn10300-watchdog.c +1 -1

index a2d8e6938d67..0d5641beadf5 100644
@@ -31,7 +31,7 @@ static unsigned int watchdog;
 static unsigned int watchdog_hz = 1;
 unsigned int watchdog_alert_counter[NR_CPUS];
 
-EXPORT_SYMBOL(touch_nmi_watchdog);
+EXPORT_SYMBOL(arch_touch_nmi_watchdog);
 
 /*
  * the best way to detect whether a CPU has a 'hard lockup' problem

arch/sparc/include/asm/nmi.h +1 -0

index 26ad2b2607c6..284eac3ffaf2 100644
@@ -7,6 +7,7 @@ void nmi_adjust_hz(unsigned int new_hz);
 
 extern atomic_t nmi_active;
 
+void arch_touch_nmi_watchdog(void);
 void start_nmi_watchdog(void *unused);
 void stop_nmi_watchdog(void *unused);
 

arch/sparc/kernel/nmi.c +2 -4

index 95e73c63c99d..048ad783ea3f 100644
@@ -51,7 +51,7 @@ static DEFINE_PER_CPU(unsigned int, last_irq_sum);
 static DEFINE_PER_CPU(long, alert_counter);
 static DEFINE_PER_CPU(int, nmi_touch);
 
-void touch_nmi_watchdog(void)
+void arch_touch_nmi_watchdog(void)
 {
 	if (atomic_read(&nmi_active)) {
 		int cpu;

@@ -61,10 +61,8 @@ void touch_nmi_watchdog(void)
 				per_cpu(nmi_touch, cpu) = 1;
 		}
 	}
-
-	touch_softlockup_watchdog();
 }
-EXPORT_SYMBOL(touch_nmi_watchdog);
+EXPORT_SYMBOL(arch_touch_nmi_watchdog);
 
 static void die_nmi(const char *str, struct pt_regs *regs, int do_panic)
 {

include/linux/nmi.h +15 -11

index 5e2e57536d98..6ea465a842a1 100644
@@ -6,6 +6,9 @@
 
 #include <linux/sched.h>
 #include <asm/irq.h>
+#if defined(CONFIG_HAVE_NMI_WATCHDOG)
+#include <asm/nmi.h>
+#endif
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 extern void touch_softlockup_watchdog_sched(void);

@@ -58,6 +61,14 @@ static inline void reset_hung_task_detector(void)
 #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
 #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
 
+#if defined(CONFIG_HARDLOCKUP_DETECTOR)
+extern void hld_touch_nmi_watchdog(void);
+extern void hardlockup_detector_disable(void);
+#else
+static inline void hld_touch_nmi_watchdog(void) {}
+static inline void hardlockup_detector_disable(void) {}
+#endif
+
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
  * 

@@ -65,21 +76,14 @@ static inline void reset_hung_task_detector(void)
  * may be used to reset the timeout - for code which intentionally
  * disables interrupts for a long time. This call is stateless.
  */
-#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
-#include <asm/nmi.h>
-extern void touch_nmi_watchdog(void);
-#else
 static inline void touch_nmi_watchdog(void)
 {
+#if defined(CONFIG_HAVE_NMI_WATCHDOG)
+	arch_touch_nmi_watchdog();
+#endif
+	hld_touch_nmi_watchdog();
 	touch_softlockup_watchdog();
 }
-#endif
-
-#if defined(CONFIG_HARDLOCKUP_DETECTOR)
-extern void hardlockup_detector_disable(void);
-#else
-static inline void hardlockup_detector_disable(void) {}
-#endif
 
 /*
  * Create trigger_all_cpu_backtrace() out of the arch-provided

kernel/watchdog_hld.c +2 -3

index 54a427d1f344..e0d7a7c43fb5 100644
@@ -56,7 +56,7 @@ static int __init hardlockup_panic_setup(char *str)
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
 
-void touch_nmi_watchdog(void)
+void hld_touch_nmi_watchdog(void)
 {
 	/*
 	 * Using __raw here because some code paths have

@@ -66,9 +66,8 @@ void touch_nmi_watchdog(void)
 	 * going off.
 	 */
 	raw_cpu_write(watchdog_nmi_touch, true);
-	touch_softlockup_watchdog();
 }
-EXPORT_SYMBOL(touch_nmi_watchdog);
+EXPORT_SYMBOL(hld_touch_nmi_watchdog);
 
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,

Signature · 1 line

2.11.0

Quoted 4 lines · Nicholas Piggin

For architectures that define HAVE_NMI_WATCHDOG, instead of having them provide the complete touch_nmi_watchdog() function, just have them provide arch_touch_nmi_watchdog().

Quoted 3 lines

This gives the generic code more flexibility in implementing this function, and arch implementations don't miss out on touching the softlockup watchdog or other generic details.

The idea makes sense. I don't think you can have hld_touch_nmi_watchdog defined with arch_touch_nmi_watchdog, so I am wondering if it makes sense to combine them somehow. Though renaming hld_touch_nmi_watchdog to arch_touch_nmi_watchdog sounds odd, I think it mimics the idea.

Quoted 5 lines · On Thu, 25 May 2017 09:55:59 -0400 Don Zickus <[email protected]> wrote:

Quoted 4 lines · Nicholas Piggin

For architectures that define HAVE_NMI_WATCHDOG, instead of having them provide the complete touch_nmi_watchdog() function, just have them provide arch_touch_nmi_watchdog().

Quoted 8 lines

Quoted 3 lines

This gives the generic code more flexibility in implementing this function, and arch implementations don't miss out on touching the softlockup watchdog or other generic details.

The idea makes sense. I don't think you can have hld_touch_nmi_watchdog defined with arch_touch_nmi_watchdog, so I am wondering if it makes sense to combine them somehow. Though renaming hld_touch_nmi_watchdog to arch_touch_nmi_watchdog sounds odd, I think it mimics the idea.

Yeah I agree it's not quite right, and I think using arch_touch_nmi_watchdog would be fine for the hld, which makes sense if you think of it as a utility or library function for architectures that want a hardlockup watchdog and can use perf for it.

I can change that if you prefer. BTW the 0day picked up another Kconfig compile bug, so I'll respin the series and include any changes you like.

Thanks,
Nick

After reconfiguring watchdog sysctls etc., architecture specific watchdogs may not get all their parameters updated.

watchdog_reconfigure() can be implemented to pull the new values in and set the arch NMI watchdog.

Signed-off-by Nicholas Piggin <[email protected]>

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

kernel/watchdog.c +24 -4

index deb010505646..d2996f5bf551 100644
@@ -120,6 +120,11 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
 {
 }
 
+void __weak watchdog_nmi_reconfigure(void)
+{
+}
+
+
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
 /* Helper for online, unparked cpus. */

@@ -597,6 +602,12 @@ static void watchdog_disable_all_cpus(void)
 	}
 }
 
+static int watchdog_update_cpus(void)
+{
+	return smpboot_update_cpumask_percpu_thread(
+		    &watchdog_threads, &watchdog_cpumask);
+}
+
 #else /* SOFTLOCKUP */
 static int watchdog_park_threads(void)
 {

@@ -616,6 +627,10 @@ static void watchdog_disable_all_cpus(void)
 {
 }
 
+static int watchdog_update_cpus(void)
+{
+}
+
 static void set_sample_period(void)
 {
 }

@@ -648,6 +663,8 @@ int lockup_detector_suspend(void)
 		watchdog_enabled = 0;
 	}
 
+	watchdog_nmi_reconfigure();
+
 	mutex_unlock(&watchdog_proc_mutex);
 
 	return ret;

@@ -668,6 +685,8 @@ void lockup_detector_resume(void)
 	if (watchdog_running && !watchdog_suspended)
 		watchdog_unpark_threads();
 
+	watchdog_nmi_reconfigure();
+
 	mutex_unlock(&watchdog_proc_mutex);
 	put_online_cpus();
 }

@@ -693,6 +712,8 @@ static int proc_watchdog_update(void)
 	else
 		watchdog_disable_all_cpus();
 
+	watchdog_nmi_reconfigure();
+
 	return err;
 
 }

@@ -878,12 +899,11 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 			 * a temporary cpumask, so we are likely not in a
 			 * position to do much else to make things better.
 			 */
-#ifdef CONFIG_SOFTLOCKUP_DETECTOR
-			if (smpboot_update_cpumask_percpu_thread(
-				    &watchdog_threads, &watchdog_cpumask) != 0)
+			if (watchdog_update_cpus() != 0)
 				pr_err("cpumask update failed\n");
-#endif
 		}
+
+		watchdog_nmi_reconfigure();
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);

Signature · 1 line

2.11.0

Quoted 5 lines · Nicholas Piggin

After reconfiguring watchdog sysctls etc., architecture specific watchdogs may not get all their parameters updated.

watchdog_reconfigure() can be implemented to pull the new values in and set the arch NMI watchdog.

I understand the reason for this patch and I don't have any real objection on how it was implemented within the constraints of all the current logic.

I just wonder if the current logic should be adjusted to make the hardlockup detector, namely the perf implementation more separate so it can handle what you would like more cleanly.

The watchdog_nmi_reconfigure is sort of hackish, but it is hard to fault you based on how things are designed. I am going to poke at it a little bit, but I will probably not find time to do much and accept what you have for now.

Quoted 14 lines · On Thu, 25 May 2017 10:08:33 -0400 Don Zickus <[email protected]> wrote:

Quoted 5 lines · Nicholas Piggin

After reconfiguring watchdog sysctls etc., architecture specific watchdogs may not get all their parameters updated.

watchdog_reconfigure() can be implemented to pull the new values in and set the arch NMI watchdog.

I understand the reason for this patch and I don't have any real objection on how it was implemented within the constraints of all the current logic.

I just wonder if the current logic should be adjusted to make the hardlockup detector, namely the perf implementation more separate so it can handle what you would like more cleanly.

Quoted 4 lines

The watchdog_nmi_reconfigure is sort of hackish, but it is hard to fault you based on how things are designed. I am going to poke at it a little bit, but I will probably not find time to do much and accept what you have for now.

I actually agree with you. These patches are basically an initial bridge to get us to decoupling hld-perf from hld-arch, but the code could definitely use several more passes to clean things up.

One thing we want to be mindful of is some watchdogs are very light weight, minimal, and some may not even want to call C code (at least from the NMI

and touch-watchdog paths). But having said that, it may not be a bad idea to have implementations provide a watchdog driver struct with some of the methods and reconfiguration they support. E.g., suspend/resume, stop/start on CPUs, adjust timeouts, etc.).

I didn't want to go the whole hog and over-engineer something that doesn't work though, so I'm hoping we can get the powerpc watchdog in, and then keep working on the apis.

Let me know what you think after you poke at it though.