Skip to contentSkip to conversation

lockup_detector: Cure hotplug deadlocks and replace duct tape

PATCH In review Thomas Gleixner 7 participants 77 messages Sep 12, 2017 – Oct 5, 2017

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

v2 cover lockup_detector: Cure hotplug deadlocks and replace duct tape

The lockup detector is broken is several ways:

- It's deadlock prone vs. CPU hotplug in various ways. Some of these are due to recursive cpus_read_lock() others are due to cpus_read_lock() from CPU hotplug callbacks which immediately lock the machine because cpus are write locked.

- The handling of the cpu hotplug threads happens sideways to the smpboot thread infrastructure, which is racy and pointless

- The handling of the user space sysctl interface is a complete trainwreck as it fiddles directly with variables which can be modified or evaluated by the running watchdogs.

- The perf event initialization is a trainwreck as it tries to create perf events over and over even if perf is not functional (no hardware, ....). To avoid excessive dmesg spam it contains magic printk ratelimiting along with either wrong or useless messages.

- The code structure is horrible as ifdef sections are scattered all over the place which makes it unreadable

- There is more wreckage, but see the changelogs for the ugly details.

The following series sanitizes the facility and addresses the problems.

Changes since V1:

- Wrapped the perf specific calls into the weak watchdog_nmi_* functions

- Fixed the compile error pointed out by Don

- Fixed the reconfiguration parameter inconsistency which broke powerpc

- Picked up the updated version of patch 11/29

Delta patch below.

Thanks,

tglx
---
Diffstat for the series:

 arch/parisc/kernel/process.c   |    2 
 arch/powerpc/kernel/watchdog.c |   22 -
 arch/x86/events/intel/core.c   |   11 
 include/linux/nmi.h            |  121 +++----
 include/linux/smpboot.h        |    4 
 kernel/cpu.c                   |    6 
 kernel/smpboot.c               |   22 -
 kernel/sysctl.c                |   22 -
 kernel/watchdog.c              |  633 ++++++++++++++---------------------------
 kernel/watchdog_hld.c          |  193 ++++++------
 10 files changed, 434 insertions(+), 602 deletions(-)

Delta patch vs. V1
8<------------------------

arch/powerpc/kernel/watchdog.c +2 -2
@@ -355,12 +355,12 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(bool stop)
+void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
 	cpus_read_lock();
-	if (stop) {
+	if (!run) {
 		for_each_cpu(cpu, &wd_cpus_enabled)
 			stop_wd_on_cpu(cpu);
 	} else {
include/linux/nmi.h +1 -1
@@ -102,7 +102,7 @@ static inline void hardlockup_detector_p
 static inline void hardlockup_detector_perf_enable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
 # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
-static int hardlockup_detector_perf_init(void) { return -ENODEV; }
+static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
 static inline void arch_touch_nmi_watchdog(void) {}
 # else
 static int hardlockup_detector_perf_init(void) { return 0; }
kernel/watchdog.c +25 -18

89 lines not on this page · raw message

#1 lore raw

PATCH v2 1/29 hardlockup_detector: Provide interface to stop/restart perf events

From: Peter Zijlstra <[email protected]>

Provide an interface to stop and restart perf NMI watchdog events on all CPUs. This is only usable during init and especially for handling the perf HT bug on Intel machines. It's safe to use it this way as nothing can start/stop the NMI watchdog in parallel.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h   |    4 ++++
 kernel/watchdog_hld.c |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
include/linux/nmi.h +4 -0
@@ -75,7 +75,11 @@ static inline void hardlockup_detector_d
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
+extern void hardlockup_detector_perf_stop(void);
+extern void hardlockup_detector_perf_restart(void);
 #else
+static inline void hardlockup_detector_perf_stop(void) { }
+static inline void hardlockup_detector_perf_restart(void) { }
 #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline void arch_touch_nmi_watchdog(void) {}
 #endif
kernel/watchdog_hld.c +41 -0

44 lines not on this page · raw message

#2 lore raw

[tip:core/urgent] watchdog/hardlockup: Provide interface to stop/restart perf events

watchdog/hardlockup: Provide interface to stop/restart perf events
Commit-ID:  d0b6e0a8ef24b1b07078ababe5d91bcdf4f4264a
Gitweb:     http://git.kernel.org/tip/d0b6e0a8ef24b1b07078ababe5d91bcdf4f4264a
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:36:55 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:03 +0200

watchdog/hardlockup: Provide interface to stop/restart perf events

Provide an interface to stop and restart perf NMI watchdog events on all CPUs. This is only usable during init and especially for handling the perf HT bug on Intel machines. It's safe to use it this way as nothing can start/stop the NMI watchdog in parallel.

---
 include/linux/nmi.h   |  4 ++++
 kernel/watchdog_hld.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
include/linux/nmi.h +4 -0
index a36abe2..b24d4a5 100644
@@ -75,7 +75,11 @@ static inline void hardlockup_detector_disable(void) {}
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
+extern void hardlockup_detector_perf_stop(void);
+extern void hardlockup_detector_perf_restart(void);
 #else
+static inline void hardlockup_detector_perf_stop(void) { }
+static inline void hardlockup_detector_perf_restart(void) { }
 #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline void arch_touch_nmi_watchdog(void) {}
 #endif
kernel/watchdog_hld.c +41 -0

44 lines not on this page · raw message

#3 lore raw

PATCH v2 2/29 perf/x86/intel: Sanitize PMU HT bug workaround

In reply to Thomas Gleixner

From: Peter Zijlstra <[email protected]>

The lockup_detector_suspend/resume() interface is broken in several ways especially as it results in recursive locking of the CPU hotplug lock.

Use the new stop/restart interface in the perf NMI watchdog to temporarily disable and reenable the already active watchdog events. That's enough to handle it.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 arch/x86/events/intel/core.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/events/intel/core.c +4 -7
@@ -4409,10 +4409,9 @@ static __init int fixup_ht_bug(void)
 		return 0;
 	}
 
-	if (lockup_detector_suspend() != 0) {
-		pr_debug("failed to disable PMU erratum BJ122, BV98, HSD29 workaround\n");
-		return 0;
-	}
+	cpus_read_lock();
+
+	hardlockup_detector_perf_stop();
 
 	x86_pmu.flags &= ~(PMU_FL_EXCL_CNTRS | PMU_FL_EXCL_ENABLED);
 
@@ -4420,9 +4419,7 @@ static __init int fixup_ht_bug(void)
 	x86_pmu.commit_scheduling = NULL;
 	x86_pmu.stop_scheduling = NULL;
 
-	lockup_detector_resume();
-
-	cpus_read_lock();
+	hardlockup_detector_perf_restart();
 
 	for_each_online_cpu(c)
 		free_excl_cntrs(c);
#4 lore raw

[tip:core/urgent] perf/x86/intel, watchdog/core: Sanitize PMU HT bug workaround

perf/x86/intel, watchdog/core: Sanitize PMU HT bug workaround
Commit-ID:  2406e3b166eee42777a6b0b38f52f924454474d7
Gitweb:     http://git.kernel.org/tip/2406e3b166eee42777a6b0b38f52f924454474d7
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:36:56 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:03 +0200

perf/x86/intel, watchdog/core: Sanitize PMU HT bug workaround

The lockup_detector_suspend/resume() interface is broken in several ways especially as it results in recursive locking of the CPU hotplug lock.

Use the new stop/restart interface in the perf NMI watchdog to temporarily disable and reenable the already active watchdog events. That's enough to handle it.

---
 arch/x86/events/intel/core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/events/intel/core.c +4 -7
index 829e89c..9fb9a1f 100644
@@ -4409,10 +4409,9 @@ static __init int fixup_ht_bug(void)
 		return 0;
 	}
 
-	if (lockup_detector_suspend() != 0) {
-		pr_debug("failed to disable PMU erratum BJ122, BV98, HSD29 workaround\n");
-		return 0;
-	}
+	cpus_read_lock();
+
+	hardlockup_detector_perf_stop();
 
 	x86_pmu.flags &= ~(PMU_FL_EXCL_CNTRS | PMU_FL_EXCL_ENABLED);
 
@@ -4420,9 +4419,7 @@ static __init int fixup_ht_bug(void)
 	x86_pmu.commit_scheduling = NULL;
 	x86_pmu.stop_scheduling = NULL;
 
-	lockup_detector_resume();
-
-	cpus_read_lock();
+	hardlockup_detector_perf_restart();
 
 	for_each_online_cpu(c)
 		free_excl_cntrs(c);
#5 lore raw

PATCH v2 3/29 lockup_detector: Provide interface to stop from poweroff()

In reply to Thomas Gleixner

PARISC has a a busy looping power off routine. If the watchdog is enabled the watchdog timer will still fire, but the thread is not running, which causes the softlockup watchdog to trigger.

Provide a interface which allows to turn the watchdog off.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Helge Deller <[email protected]>
---
 include/linux/nmi.h |    6 +++---
 kernel/watchdog.c   |   14 +++++++++++++-
 2 files changed, 16 insertions(+), 4 deletions(-)
include/linux/nmi.h +3 -3
@@ -12,10 +12,10 @@
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 void lockup_detector_init(void);
+void lockup_detector_soft_poweroff(void);
 #else
-static inline void lockup_detector_init(void)
-{
-}
+static inline void lockup_detector_init(void) { }
+static inline void lockup_detector_soft_poweroff(void) { }
 #endif
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
kernel/watchdog.c +13 -1
@@ -333,7 +333,8 @@ static enum hrtimer_restart watchdog_tim
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
 
-	if (atomic_read(&watchdog_park_in_progress) != 0)
+	if (!watchdog_enabled ||
+	    atomic_read(&watchdog_park_in_progress) != 0)
 		return HRTIMER_NORESTART;
 
 	/* kick the hardlockup detector */
@@ -660,6 +661,17 @@ static void set_sample_period(void)
 }
 #endif /* SOFTLOCKUP */
 
+/**
+ * lockup_detector_soft_poweroff - Interface to stop lockup detector(s)
+ *
+ * Special interface for parisc. It prevents lockup detector warnings from
+ * the default pm_poweroff() function which busy loops forever.
+ */
+void lockup_detector_soft_poweroff(void)
+{
+	watchdog_enabled = 0;
+}
+
 /*
  * Suspend the hard and soft lockup detector by parking the watchdog threads.
  */
#6 lore raw

[tip:core/urgent] watchdog/core: Provide interface to stop from poweroff()

watchdog/core: Provide interface to stop from poweroff()
Commit-ID:  6554fd8cf06db86f861bb24d7487b2873ca444c4
Gitweb:     http://git.kernel.org/tip/6554fd8cf06db86f861bb24d7487b2873ca444c4
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:36:57 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:03 +0200

watchdog/core: Provide interface to stop from poweroff()

PARISC has a a busy looping power off routine. If the watchdog is enabled the watchdog timer will still fire, but the thread is not running, which causes the softlockup watchdog to trigger.

Provide a interface which allows to turn the watchdog off.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Helge Deller <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h |  6 +++---
 kernel/watchdog.c   | 14 +++++++++++++-
 2 files changed, 16 insertions(+), 4 deletions(-)
include/linux/nmi.h +3 -3
index b24d4a5..85bb268b 100644
@@ -12,10 +12,10 @@
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 void lockup_detector_init(void);
+void lockup_detector_soft_poweroff(void);
 #else
-static inline void lockup_detector_init(void)
-{
-}
+static inline void lockup_detector_init(void) { }
+static inline void lockup_detector_soft_poweroff(void) { }
 #endif
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
kernel/watchdog.c +13 -1
index f5d5202..f23e373 100644
@@ -333,7 +333,8 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
 
-	if (atomic_read(&watchdog_park_in_progress) != 0)
+	if (!watchdog_enabled ||
+	    atomic_read(&watchdog_park_in_progress) != 0)
 		return HRTIMER_NORESTART;
 
 	/* kick the hardlockup detector */
@@ -660,6 +661,17 @@ static void set_sample_period(void)
 }
 #endif /* SOFTLOCKUP */
 
+/**
+ * lockup_detector_soft_poweroff - Interface to stop lockup detector(s)
+ *
+ * Special interface for parisc. It prevents lockup detector warnings from
+ * the default pm_poweroff() function which busy loops forever.
+ */
+void lockup_detector_soft_poweroff(void)
+{
+	watchdog_enabled = 0;
+}
+
 /*
  * Suspend the hard and soft lockup detector by parking the watchdog threads.
  */
#7 lore raw

PATCH v2 4/29 parisc: Use lockup_detector_stop()

In reply to Thomas Gleixner

The broken lockup_detector_suspend/resume() interface is going away. Use the new lockup_detector_soft_poweroff() interface to stop the watchdog from the busy looping power off routine.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Helge Deller <[email protected]>
---
 arch/parisc/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/parisc/kernel/process.c +1 -1
@@ -146,7 +146,7 @@ void machine_power_off(void)
 
 	/* prevent soft lockup/stalled CPU messages for endless loop. */
 	rcu_sysrq_start();
-	lockup_detector_suspend();
+	lockup_detector_soft_poweroff();
 	for (;;);
 }
 
maintainer Acked-by on v2 4/29
#8 lore raw

Re: [patch V2 04/29] parisc: Use lockup_detector_stop()

* Thomas Gleixner <[email protected]>:

Quoted 24 lines

The broken lockup_detector_suspend/resume() interface is going away. Use the new lockup_detector_soft_poweroff() interface to stop the watchdog from the busy looping power off routine.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Helge Deller <[email protected]>
---
 arch/parisc/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/parisc/kernel/process.c void machine_power_off(void) · line 151

Quoted 7 lines
arch/parisc/kernel/process.c +1 -1
@@ -146,7 +146,7 @@ void machine_power_off(void)
 
 	/* prevent soft lockup/stalled CPU messages for endless loop. */
 	rcu_sysrq_start();
-	lockup_detector_suspend();
+	lockup_detector_soft_poweroff();
 	for (;;);
 }

Thomas, thanks for cleaning that up.
You may add to patches 03/04:

On a side-note, there is sadly no general function like
turn_off_all_kind_of_runtime_hang_detectors()
which turns off *all* detectors at once (including soft lockup detector). I've seen another detector complaing at runtime that we were hanging here. I would need to dig up more info if you are interested...

Helge

#9 lore raw

Re: [patch V2 04/29] parisc: Use lockup_detector_stop()

Quoted 35 lines · Helge Deller

* Thomas Gleixner <[email protected]>:

Quoted 24 lines

The broken lockup_detector_suspend/resume() interface is going away. Use the new lockup_detector_soft_poweroff() interface to stop the watchdog from the busy looping power off routine.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Helge Deller <[email protected]>
---
 arch/parisc/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/parisc/kernel/process.c void machine_power_off(void) · line 151

Quoted 7 lines
arch/parisc/kernel/process.c +1 -1
@@ -146,7 +146,7 @@ void machine_power_off(void)
 
 	/* prevent soft lockup/stalled CPU messages for endless loop. */
 	rcu_sysrq_start();
-	lockup_detector_suspend();
+	lockup_detector_soft_poweroff();
 	for (;;);
 }

Thomas, thanks for cleaning that up.
You may add to patches 03/04:

Quoted 8 lines

On a side-note, there is sadly no general function like
turn_off_all_kind_of_runtime_hang_detectors()
which turns off *all* detectors at once (including soft lockup detector). I've seen another detector complaing at runtime that we were hanging here. I would need to dig up more info if you are interested...

There are numerous detectors I have seen over the years: rcu, clocksource, hard/soft, hang, fs, network, wq?, etc.. I am not sure it is easy to put them all in one place or makes sense.

I know working with the kvm folks, when they swap back in, the real clock can do a massive jump forward and causes a flood of warnings such that they had to 'touch' all of them before running the vm again.

Cheers,
Don

#10 lore raw

[tip:core/urgent] parisc, watchdog/core: Use lockup_detector_stop()

In reply to Thomas Gleixner
parisc, watchdog/core: Use lockup_detector_stop()
Commit-ID:  47bb4baf7df43ac8bbc51c24022466972ba29ef1
Gitweb:     http://git.kernel.org/tip/47bb4baf7df43ac8bbc51c24022466972ba29ef1
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:36:58 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:03 +0200

parisc, watchdog/core: Use lockup_detector_stop()

The broken lockup_detector_suspend/resume() interface is going away. Use the new lockup_detector_soft_poweroff() interface to stop the watchdog from the busy looping power off routine.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Helge Deller <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 arch/parisc/kernel/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/parisc/kernel/process.c +1 -1
index a45a67d..30f9239 100644
@@ -146,7 +146,7 @@ void machine_power_off(void)
 
 	/* prevent soft lockup/stalled CPU messages for endless loop. */
 	rcu_sysrq_start();
-	lockup_detector_suspend();
+	lockup_detector_soft_poweroff();
 	for (;;);
 }
 
#11 lore raw

PATCH v2 5/29 lockup_detector: Remove broken suspend/resume interfaces

In reply to Thomas Gleixner

This interface has several issues:

- It's causing recursive locking of the hotplug lock.

- It's complete overkill to teardown all threads and then recreate them

The same can be achieved with the simple hardlockup_detector_perf_stop / restart() interfaces. The abuse from the busy looping poweroff() loop of PARISC has been solved as well.

Remove the cruft.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 arch/powerpc/kernel/watchdog.c |    3 -
 include/linux/nmi.h            |   12 -----
 kernel/watchdog.c              |   89 -----------------------------------------
 3 files changed, 1 insertion(+), 103 deletions(-)
arch/powerpc/kernel/watchdog.c +0 -3
@@ -310,9 +310,6 @@ static int start_wd_on_cpu(unsigned int
 	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		return 0;
 
-	if (watchdog_suspended)
-		return 0;
-
 	if (!cpumask_test_cpu(cpu, &watchdog_cpumask))
 		return 0;
 
include/linux/nmi.h +0 -12
@@ -164,7 +164,6 @@ extern int watchdog_thresh;
 extern unsigned long watchdog_enabled;
 extern struct cpumask watchdog_cpumask;
 extern unsigned long *watchdog_cpumask_bits;
-extern int __read_mostly watchdog_suspended;
 #ifdef CONFIG_SMP
 extern int sysctl_softlockup_all_cpu_backtrace;
 extern int sysctl_hardlockup_all_cpu_backtrace;
@@ -192,17 +191,6 @@ extern int proc_watchdog_thresh(struct c
 				void __user *, size_t *, loff_t *);
 extern int proc_watchdog_cpumask(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
-extern int lockup_detector_suspend(void);
-extern void lockup_detector_resume(void);
-#else
-static inline int lockup_detector_suspend(void)
-{
-	return 0;
-}
-
-static inline void lockup_detector_resume(void)
-{
-}
 #endif
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
kernel/watchdog.c +1 -88

131 lines not on this page · raw message

#12 lore raw

[tip:core/urgent] watchdog/core: Remove broken suspend/resume interfaces

watchdog/core: Remove broken suspend/resume interfaces
Commit-ID:  5490125d77a43016b26f629d4b485e2c62172551
Gitweb:     http://git.kernel.org/tip/5490125d77a43016b26f629d4b485e2c62172551
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:36:59 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:04 +0200

watchdog/core: Remove broken suspend/resume interfaces

This interface has several issues:

- It's causing recursive locking of the hotplug lock.

- It's complete overkill to teardown all threads and then recreate them

The same can be achieved with the simple hardlockup_detector_perf_stop / restart() interfaces. The abuse from the busy looping poweroff() loop of PARISC has been solved as well.

Remove the cruft.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 arch/powerpc/kernel/watchdog.c |  3 --
 include/linux/nmi.h            | 12 ------
 kernel/watchdog.c              | 89 +-----------------------------------------
 3 files changed, 1 insertion(+), 103 deletions(-)
arch/powerpc/kernel/watchdog.c +0 -3
index 2f6eadd..5ded171 100644
@@ -310,9 +310,6 @@ static int start_wd_on_cpu(unsigned int cpu)
 	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		return 0;
 
-	if (watchdog_suspended)
-		return 0;
-
 	if (!cpumask_test_cpu(cpu, &watchdog_cpumask))
 		return 0;
 
include/linux/nmi.h +0 -12
index 85bb268b..7eefe7a 100644
@@ -164,7 +164,6 @@ extern int watchdog_thresh;
 extern unsigned long watchdog_enabled;
 extern struct cpumask watchdog_cpumask;
 extern unsigned long *watchdog_cpumask_bits;
-extern int __read_mostly watchdog_suspended;
 #ifdef CONFIG_SMP
 extern int sysctl_softlockup_all_cpu_backtrace;
 extern int sysctl_hardlockup_all_cpu_backtrace;
@@ -192,17 +191,6 @@ extern int proc_watchdog_thresh(struct ctl_table *, int ,
 				void __user *, size_t *, loff_t *);
 extern int proc_watchdog_cpumask(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
-extern int lockup_detector_suspend(void);
-extern void lockup_detector_resume(void);
-#else
-static inline int lockup_detector_suspend(void)
-{
-	return 0;
-}
-
-static inline void lockup_detector_resume(void)
-{
-}
 #endif
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
kernel/watchdog.c +1 -88

131 lines not on this page · raw message

#13 lore raw

PATCH v2 6/29 lockup_detector: Rework cpu hotplug locking

In reply to Thomas Gleixner

The watchdog proc interface causes extensive recursive locking of the cpu hotplug percpu rwsem, which is deadlock prone.

Replace the get/put_online_cpus() pairs with cpu_hotplug_disable()/enable() calls for now. Later patches will remove that requirement completely.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/watchdog.c +6 -6
@@ -703,7 +703,7 @@ static int proc_watchdog_common(int whic
 	int err, old, new;
 	int *watchdog_param = (int *)table->data;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	/*
@@ -752,7 +752,7 @@ static int proc_watchdog_common(int whic
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
@@ -794,7 +794,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 {
 	int err, old, new;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	old = ACCESS_ONCE(watchdog_thresh);
@@ -818,7 +818,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
@@ -833,7 +833,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 {
 	int err;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
@@ -856,7 +856,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 	}
 
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
#14 lore raw

[tip:core/urgent] watchdog/core: Rework CPU hotplug locking

watchdog/core: Rework CPU hotplug locking
Commit-ID:  b7a349819d4b9b5db64e523351e66a79a758eaa5
Gitweb:     http://git.kernel.org/tip/b7a349819d4b9b5db64e523351e66a79a758eaa5
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:00 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:04 +0200

watchdog/core: Rework CPU hotplug locking

The watchdog proc interface causes extensive recursive locking of the CPU hotplug percpu rwsem, which is deadlock prone.

Replace the get/put_online_cpus() pairs with cpu_hotplug_disable()/enable() calls for now. Later patches will remove that requirement completely.

---
 kernel/watchdog.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/watchdog.c +6 -6
index b2d4675..cd79f64 100644
@@ -703,7 +703,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 	int err, old, new;
 	int *watchdog_param = (int *)table->data;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	/*
@@ -752,7 +752,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
@@ -794,7 +794,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 {
 	int err, old, new;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	old = ACCESS_ONCE(watchdog_thresh);
@@ -818,7 +818,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
@@ -833,7 +833,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 {
 	int err;
 
-	get_online_cpus();
+	cpu_hotplug_disable();
 	mutex_lock(&watchdog_proc_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
@@ -856,7 +856,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	}
 
 	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
+	cpu_hotplug_enable();
 	return err;
 }
 
#15 lore raw

PATCH v2 7/29 lockup_detector: Rename watchdog_proc_mutex

In reply to Thomas Gleixner

Following patches will use the mutex for other purposes as well. Rename it as it is not longer a proc specific thing.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
kernel/watchdog.c +7 -8
@@ -29,8 +29,7 @@
 #include <linux/kvm_para.h>
 #include <linux/kthread.h>
 
-/* Watchdog configuration */
-static DEFINE_MUTEX(watchdog_proc_mutex);
+static DEFINE_MUTEX(watchdog_mutex);
 
 int __read_mostly nmi_watchdog_enabled;
 
@@ -704,7 +703,7 @@ static int proc_watchdog_common(int whic
 	int *watchdog_param = (int *)table->data;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	/*
 	 * If the parameter is being read return the state of the corresponding
@@ -751,7 +750,7 @@ static int proc_watchdog_common(int whic
 		err = proc_watchdog_update();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -795,7 +794,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 	int err, old, new;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	old = ACCESS_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
@@ -817,7 +816,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 		set_sample_period();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -834,7 +833,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 	int err;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write) {
@@ -855,7 +854,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 		watchdog_nmi_reconfigure();
 	}
 
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
#16 lore raw

[tip:core/urgent] watchdog/core: Rename watchdog_proc_mutex

watchdog/core: Rename watchdog_proc_mutex
Commit-ID:  946d197794b23202b8b46c43016747c72fe23393
Gitweb:     http://git.kernel.org/tip/946d197794b23202b8b46c43016747c72fe23393
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:01 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:04 +0200

watchdog/core: Rename watchdog_proc_mutex

Following patches will use the mutex for other purposes as well. Rename it as it is not longer a proc specific thing.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
kernel/watchdog.c +7 -8
index cd79f64..7c3a0a7 100644
@@ -29,8 +29,7 @@
 #include <linux/kvm_para.h>
 #include <linux/kthread.h>
 
-/* Watchdog configuration */
-static DEFINE_MUTEX(watchdog_proc_mutex);
+static DEFINE_MUTEX(watchdog_mutex);
 
 int __read_mostly nmi_watchdog_enabled;
 
@@ -704,7 +703,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 	int *watchdog_param = (int *)table->data;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	/*
 	 * If the parameter is being read return the state of the corresponding
@@ -751,7 +750,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 		err = proc_watchdog_update();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -795,7 +794,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	int err, old, new;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	old = ACCESS_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
@@ -817,7 +816,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 		set_sample_period();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -834,7 +833,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	int err;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write) {
@@ -855,7 +854,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 		watchdog_nmi_reconfigure();
 	}
 
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
#17 lore raw

PATCH v2 8/29 lockup_detector: Mark hardlockup_detector_disable() __init

In reply to Thomas Gleixner

The function is only used by the KVM init code. Mark it __init to prevent creative abuse.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/watchdog.c +1 -1
@@ -55,7 +55,7 @@ unsigned int __read_mostly hardlockup_pa
  * kernel command line parameters are parsed, because otherwise it is not
  * possible to override this in hardlockup_panic_setup().
  */
-void hardlockup_detector_disable(void)
+void __init hardlockup_detector_disable(void)
 {
 	watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
 }
#18 lore raw

[tip:core/urgent] watchdog/core: Mark hardlockup_detector_disable() __init

watchdog/core: Mark hardlockup_detector_disable() __init
Commit-ID:  7a3558200739e1378800a7a6d7f63c031115f7a4
Gitweb:     http://git.kernel.org/tip/7a3558200739e1378800a7a6d7f63c031115f7a4
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:02 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:04 +0200

watchdog/core: Mark hardlockup_detector_disable() __init

The function is only used by the KVM init code. Mark it __init to prevent creative abuse.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/watchdog.c +1 -1
index 7c3a0a7..1c185d9 100644
@@ -55,7 +55,7 @@ unsigned int __read_mostly hardlockup_panic =
  * kernel command line parameters are parsed, because otherwise it is not
  * possible to override this in hardlockup_panic_setup().
  */
-void hardlockup_detector_disable(void)
+void __init hardlockup_detector_disable(void)
 {
 	watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
 }
#19 lore raw

PATCH v2 9/29 lockup_detector/perf: Remove broken self disable on failure

In reply to Thomas Gleixner

The self disabling feature is broken vs. CPU hotplug locking:

CPU 0 CPU 1
cpus_write_lock();
cpu_up(1)
wait_for_completion()

			   ....
			   unpark_watchdog()
			   ->unpark()
			     perf_event_create() <- fails
			       watchdog_enable &= ~NMI_WATCHDOG;
			   ....    

cpus_write_unlock();
CPU 2
cpus_write_lock()
cpu_down(2)
wait_for_completion()

			   wakeup(watchdog);
			     watchdog()
			     if (!(watchdog_enable & NMI_WATCHDOG))
    			     	watchdog_nmi_disable()
      				  perf_event_disable()
				  ....
				  cpus_read_lock();

			   stop_smpboot_threads()
			     park_watchdog();
			       wait_for_completion(watchdog->parked);

Result: End of hotplug and instantaneous full lockup of the machine.

There is a similar problem with disabling the watchdog via the user space interface as the sysctl function fiddles with watchdog_enable directly.

It's very debatable whether this is required at all. If the watchdog works nicely on N CPUs and it fails to enable on the N + 1 CPU either during hotplug or because the user space interface disabled it via sysctl cpumask and then some perf user grabbed the counter which is then unavailable for the watchdog when the sysctl cpumask gets changed back.

There is no real justification for this.

One of the reasons WHY this is done is the utter stupidity of the init code of the perf NMI watchdog. Instead of checking upfront at boot whether PERF is available and functional at all, it just does this check at run time over and over when user space fiddles with the sysctl. That's broken beyond repair along with the idiotic error code dependent warn level printks and the even more silly printk rate limiting.

If the init code checks whether perf works at boot time, then this mess can be more or less avoided completely. Perf does not come magically into life at runtime. Brain usage while coding is overrated.

Remove the cruft and add a temporary safe guard which gets removed later.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c     |   15 ---------------
 kernel/watchdog_hld.c |   20 +++++++-------------
 2 files changed, 7 insertions(+), 28 deletions(-)
kernel/watchdog.c +0 -15
@@ -485,21 +485,6 @@ static void watchdog(unsigned int cpu)
 	__this_cpu_write(soft_lockup_hrtimer_cnt,
 			 __this_cpu_read(hrtimer_interrupts));
 	__touch_watchdog();
-
-	/*
-	 * watchdog_nmi_enable() clears the NMI_WATCHDOG_ENABLED bit in the
-	 * failure path. Check for failures that can occur asynchronously -
-	 * for example, when CPUs are on-lined - and shut down the hardware
-	 * perf event on each CPU accordingly.
-	 *
-	 * The only non-obvious place this bit can be cleared is through
-	 * watchdog_nmi_enable(), so a pr_info() is placed there.  Placing a
-	 * pr_info here would be too noisy as it would result in a message
-	 * every few seconds if the hardlockup was disabled but the softlockup
-	 * enabled.
-	 */
-	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
-		watchdog_nmi_disable(cpu);
 }
 
 static struct smp_hotplug_thread watchdog_threads = {
kernel/watchdog_hld.c +7 -13
@@ -23,6 +23,7 @@ static DEFINE_PER_CPU(bool, watchdog_nmi
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
 
 static unsigned long hardlockup_allcpu_dumped;
+static bool hardlockup_detector_disabled;
 
 void arch_touch_nmi_watchdog(void)
 {
@@ -178,6 +179,10 @@ int watchdog_nmi_enable(unsigned int cpu
 	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		goto out;
 
+	/* A failure disabled the hardlockup detector permanently */
+	if (hardlockup_detector_disabled)
+		return -ENODEV;
+
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -206,18 +211,6 @@ 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 (!firstcpu && (PTR_ERR(event) == firstcpu_err))
 		return PTR_ERR(event);
@@ -232,7 +225,8 @@ int watchdog_nmi_enable(unsigned int cpu
 		pr_err("disabled (cpu%i): unable to create perf event: %ld\n",
 			cpu, PTR_ERR(event));
 
-	pr_info("Shutting down hard lockup detector on all cpus\n");
+	pr_info("Disabling hard lockup detector permanently\n");
+	hardlockup_detector_disabled = true;
 
 	return PTR_ERR(event);
 
#20 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Remove broken self disable on failure

watchdog/hardlockup/perf: Remove broken self disable on failure
Commit-ID:  20d853fd0703b1d73c35a22024c0d4fcbcc57c8c
Gitweb:     http://git.kernel.org/tip/20d853fd0703b1d73c35a22024c0d4fcbcc57c8c
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:03 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:04 +0200

watchdog/hardlockup/perf: Remove broken self disable on failure

The self disabling feature is broken vs. CPU hotplug locking:

CPU 0 CPU 1
cpus_write_lock();
cpu_up(1)
wait_for_completion()

			   ....
			   unpark_watchdog()
			   ->unpark()
			     perf_event_create() <- fails
			       watchdog_enable &= ~NMI_WATCHDOG;
			   ....

cpus_write_unlock();
CPU 2
cpus_write_lock()
cpu_down(2)
wait_for_completion()

			   wakeup(watchdog);
			     watchdog()
			     if (!(watchdog_enable & NMI_WATCHDOG))
				watchdog_nmi_disable()
				  perf_event_disable()
				  ....
				  cpus_read_lock();

			   stop_smpboot_threads()
			     park_watchdog();
			       wait_for_completion(watchdog->parked);

Result: End of hotplug and instantaneous full lockup of the machine.

There is a similar problem with disabling the watchdog via the user space interface as the sysctl function fiddles with watchdog_enable directly.

It's very debatable whether this is required at all. If the watchdog works nicely on N CPUs and it fails to enable on the N + 1 CPU either during hotplug or because the user space interface disabled it via sysctl cpumask and then some perf user grabbed the counter which is then unavailable for the watchdog when the sysctl cpumask gets changed back.

There is no real justification for this.

One of the reasons WHY this is done is the utter stupidity of the init code of the perf NMI watchdog. Instead of checking upfront at boot whether PERF is available and functional at all, it just does this check at run time over and over when user space fiddles with the sysctl. That's broken beyond repair along with the idiotic error code dependent warn level printks and the even more silly printk rate limiting.

If the init code checks whether perf works at boot time, then this mess can be more or less avoided completely. Perf does not come magically into life at runtime. Brain usage while coding is overrated.

Remove the cruft and add a temporary safe guard which gets removed later.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c     | 15 ---------------
 kernel/watchdog_hld.c | 20 +++++++-------------
 2 files changed, 7 insertions(+), 28 deletions(-)
kernel/watchdog.c +0 -15
index 1c185d9..af00095 100644
@@ -485,21 +485,6 @@ static void watchdog(unsigned int cpu)
 	__this_cpu_write(soft_lockup_hrtimer_cnt,
 			 __this_cpu_read(hrtimer_interrupts));
 	__touch_watchdog();
-
-	/*
-	 * watchdog_nmi_enable() clears the NMI_WATCHDOG_ENABLED bit in the
-	 * failure path. Check for failures that can occur asynchronously -
-	 * for example, when CPUs are on-lined - and shut down the hardware
-	 * perf event on each CPU accordingly.
-	 *
-	 * The only non-obvious place this bit can be cleared is through
-	 * watchdog_nmi_enable(), so a pr_info() is placed there.  Placing a
-	 * pr_info here would be too noisy as it would result in a message
-	 * every few seconds if the hardlockup was disabled but the softlockup
-	 * enabled.
-	 */
-	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
-		watchdog_nmi_disable(cpu);
 }
 
 static struct smp_hotplug_thread watchdog_threads = {
kernel/watchdog_hld.c +7 -13
index c9586eb..7b60271 100644
@@ -23,6 +23,7 @@ static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
 
 static unsigned long hardlockup_allcpu_dumped;
+static bool hardlockup_detector_disabled;
 
 void arch_touch_nmi_watchdog(void)
 {
@@ -178,6 +179,10 @@ int watchdog_nmi_enable(unsigned int cpu)
 	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		goto out;
 
+	/* A failure disabled the hardlockup detector permanently */
+	if (hardlockup_detector_disabled)
+		return -ENODEV;
+
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -206,18 +211,6 @@ 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 (!firstcpu && (PTR_ERR(event) == firstcpu_err))
 		return PTR_ERR(event);
@@ -232,7 +225,8 @@ int watchdog_nmi_enable(unsigned int cpu)
 		pr_err("disabled (cpu%i): unable to create perf event: %ld\n",
 			cpu, PTR_ERR(event));
 
-	pr_info("Shutting down hard lockup detector on all cpus\n");
+	pr_info("Disabling hard lockup detector permanently\n");
+	hardlockup_detector_disabled = true;
 
 	return PTR_ERR(event);
 
#21 lore raw

PATCH v2 10/29 lockup_detector/perf: Prevent cpu hotplug deadlock

In reply to Thomas Gleixner

The following deadlock is possible in the watchdog hotplug code:

  cpus_write_lock()
    ...
      takedown_cpu()
        smpboot_park_threads()
          smpboot_park_thread()
            kthread_park()
              ->park() := watchdog_disable()
                watchdog_nmi_disable()
                  perf_event_release_kernel();
                    put_event()
                      _free_event()
                        ->destroy() := hw_perf_event_destroy()
                          x86_release_hardware()
                            release_ds_buffers()
                              get_online_cpus()

when a per cpu watchdog perf event is destroyed which drops the last reference to the PMU hardware. The cleanup code there invokes get_online_cpus() which instantly deadlocks because the hotplug percpu rwsem is write locked.

To solve this add a deferring mechanism:

cpus_write_lock()

			   kthread_park()
			    watchdog_nmi_disable(deferred)
			      perf_event_disable(event);
			      move_event_to_deferred(event);

....
cpus_write_unlock()
cleaup_deferred_events()
perf_event_release_kernel()

This is still properly serialized against concurrent hotplug via the cpu_add_remove_lock, which is held by the task which initiated the hotplug event.

This is also used to handle event destruction when the watchdog threads are parked via other mechanisms than CPU hotplug.

---
 include/linux/nmi.h   |    6 ++++++
 kernel/cpu.c          |    6 ++++++
 kernel/watchdog.c     |   25 +++++++++++++++++++++++++
 kernel/watchdog_hld.c |   34 ++++++++++++++++++++++++++++------
 4 files changed, 65 insertions(+), 6 deletions(-)
include/linux/nmi.h +6 -0
@@ -13,9 +13,11 @@
 #ifdef CONFIG_LOCKUP_DETECTOR
 void lockup_detector_init(void);
 void lockup_detector_soft_poweroff(void);
+void lockup_detector_cleanup(void);
 #else
 static inline void lockup_detector_init(void) { }
 static inline void lockup_detector_soft_poweroff(void) { }
+static inline void lockup_detector_cleanup(void) { }
 #endif
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
@@ -77,9 +79,13 @@ static inline void hardlockup_detector_d
 extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
+extern void hardlockup_detector_perf_disable(void);
+extern void hardlockup_detector_perf_cleanup(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
+static inline void hardlockup_detector_perf_disable(void) { }
+static inline void hardlockup_detector_perf_cleanup(void) { }
 #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline void arch_touch_nmi_watchdog(void) {}
 #endif
kernel/cpu.c +6 -0
@@ -24,6 +24,7 @@
 #include <linux/lockdep.h>
 #include <linux/tick.h>
 #include <linux/irq.h>
+#include <linux/nmi.h>
 #include <linux/smpboot.h>
 #include <linux/relay.h>
 #include <linux/slab.h>
@@ -734,6 +735,11 @@ static int __ref _cpu_down(unsigned int
 
 out:
 	cpus_write_unlock();
+	/*
+	 * Do post unplug cleanup. This is still protected against
+	 * concurrent CPU hotplug via cpu_add_remove_lock.
+	 */
+	lockup_detector_cleanup();
 	return ret;
 }
 
kernel/watchdog.c +25 -0
@@ -109,8 +109,10 @@ int __weak watchdog_nmi_enable(unsigned
 {
 	return 0;
 }
+
 void __weak watchdog_nmi_disable(unsigned int cpu)
 {
+	hardlockup_detector_perf_disable();
 }
 
 /*
@@ -193,6 +195,8 @@ static int __init hardlockup_all_cpu_bac
 #endif
 #endif
 
+static void __lockup_detector_cleanup(void);
+
 /*
  * Hard-lockup warnings should be triggered after just a few seconds. Soft-
  * lockups can have false positives under extreme conditions. So we generally
@@ -631,6 +635,24 @@ static void set_sample_period(void)
 }
 #endif /* SOFTLOCKUP */
 
+static void __lockup_detector_cleanup(void)
+{
+	lockdep_assert_held(&watchdog_mutex);
+	hardlockup_detector_perf_cleanup();
+}
+
+/**
+ * lockup_detector_cleanup - Cleanup after cpu hotplug or sysctl changes
+ *
+ * Caller must not hold the cpu hotplug rwsem.
+ */
+void lockup_detector_cleanup(void)
+{
+	mutex_lock(&watchdog_mutex);
+	__lockup_detector_cleanup();
+	mutex_unlock(&watchdog_mutex);
+}
+
 /**
  * lockup_detector_soft_poweroff - Interface to stop lockup detector(s)
  *
@@ -665,6 +687,8 @@ static int proc_watchdog_update(void)
 
 	watchdog_nmi_reconfigure();
 
+	__lockup_detector_cleanup();
+
 	return err;
 
 }
@@ -837,6 +861,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 		}
 
 		watchdog_nmi_reconfigure();
+		__lockup_detector_cleanup();
 	}
 
 	mutex_unlock(&watchdog_mutex);
kernel/watchdog_hld.c +28 -6
@@ -21,6 +21,8 @@
 static DEFINE_PER_CPU(bool, hard_watchdog_warn);
 static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
+static DEFINE_PER_CPU(struct perf_event *, dead_event);
+static struct cpumask dead_events_mask;
 
 static unsigned long hardlockup_allcpu_dumped;
 static bool hardlockup_detector_disabled;
@@ -239,16 +241,18 @@ int watchdog_nmi_enable(unsigned int cpu
 	return 0;
 }
 
-void watchdog_nmi_disable(unsigned int cpu)
+/**
+ * hardlockup_detector_perf_disable - Disable the local event
+ */
+void hardlockup_detector_perf_disable(void)
 {
-	struct perf_event *event = per_cpu(watchdog_ev, cpu);
+	struct perf_event *event = this_cpu_read(watchdog_ev);
 
 	if (event) {
 		perf_event_disable(event);
-		per_cpu(watchdog_ev, cpu) = NULL;
-
-		/* should be in cleanup, but blocks oprofile */
-		perf_event_release_kernel(event);
+		this_cpu_write(watchdog_ev, NULL);
+		this_cpu_write(dead_event, event);
+		cpumask_set_cpu(smp_processor_id(), &dead_events_mask);
 
 		/* watchdog_nmi_enable() expects this to be zero initially. */
 		if (atomic_dec_and_test(&watchdog_cpus))
@@ -257,6 +261,24 @@ void watchdog_nmi_disable(unsigned int c
 }
 
 /**
+ * hardlockup_detector_perf_cleanup - Cleanup disabled events and destroy them
+ *
+ * Called from lockup_detector_cleanup(). Serialized by the caller.
+ */
+void hardlockup_detector_perf_cleanup(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, &dead_events_mask) {
+		struct perf_event *event = per_cpu(dead_event, cpu);
+
+		per_cpu(dead_event, cpu) = NULL;
+		perf_event_release_kernel(event);
+	}
+	cpumask_clear(&dead_events_mask);
+}
+
+/**
  * hardlockup_detector_perf_stop - Globally stop watchdog events
  *
  * Special interface for x86 to handle the perf HT bug.
#22 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Prevent CPU hotplug deadlock

watchdog/hardlockup/perf: Prevent CPU hotplug deadlock
Commit-ID:  941154bd6937a710ae9193a3c733c0029e5ae7b8
Gitweb:     http://git.kernel.org/tip/941154bd6937a710ae9193a3c733c0029e5ae7b8
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:04 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:05 +0200

watchdog/hardlockup/perf: Prevent CPU hotplug deadlock

The following deadlock is possible in the watchdog hotplug code:

  cpus_write_lock()
    ...
      takedown_cpu()
        smpboot_park_threads()
          smpboot_park_thread()
            kthread_park()
              ->park() := watchdog_disable()
                watchdog_nmi_disable()
                  perf_event_release_kernel();
                    put_event()
                      _free_event()
                        ->destroy() := hw_perf_event_destroy()
                          x86_release_hardware()
                            release_ds_buffers()
                              get_online_cpus()

when a per cpu watchdog perf event is destroyed which drops the last reference to the PMU hardware. The cleanup code there invokes get_online_cpus() which instantly deadlocks because the hotplug percpu rwsem is write locked.

To solve this add a deferring mechanism:

cpus_write_lock()

			   kthread_park()
			    watchdog_nmi_disable(deferred)
			      perf_event_disable(event);
			      move_event_to_deferred(event);
			   ....

cpus_write_unlock()
cleaup_deferred_events()
perf_event_release_kernel()

This is still properly serialized against concurrent hotplug via the cpu_add_remove_lock, which is held by the task which initiated the hotplug event.

This is also used to handle event destruction when the watchdog threads are parked via other mechanisms than CPU hotplug.

---
 include/linux/nmi.h   |  6 ++++++
 kernel/cpu.c          |  6 ++++++
 kernel/watchdog.c     | 25 +++++++++++++++++++++++++
 kernel/watchdog_hld.c | 34 ++++++++++++++++++++++++++++------
 4 files changed, 65 insertions(+), 6 deletions(-)
include/linux/nmi.h +6 -0
index 7eefe7a..80354e6 100644
@@ -13,9 +13,11 @@
 #ifdef CONFIG_LOCKUP_DETECTOR
 void lockup_detector_init(void);
 void lockup_detector_soft_poweroff(void);
+void lockup_detector_cleanup(void);
 #else
 static inline void lockup_detector_init(void) { }
 static inline void lockup_detector_soft_poweroff(void) { }
+static inline void lockup_detector_cleanup(void) { }
 #endif
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
@@ -77,9 +79,13 @@ static inline void hardlockup_detector_disable(void) {}
 extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
+extern void hardlockup_detector_perf_disable(void);
+extern void hardlockup_detector_perf_cleanup(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
+static inline void hardlockup_detector_perf_disable(void) { }
+static inline void hardlockup_detector_perf_cleanup(void) { }
 #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline void arch_touch_nmi_watchdog(void) {}
 #endif
kernel/cpu.c +6 -0
index acf5308..a96b348 100644
@@ -24,6 +24,7 @@
 #include <linux/lockdep.h>
 #include <linux/tick.h>
 #include <linux/irq.h>
+#include <linux/nmi.h>
 #include <linux/smpboot.h>
 #include <linux/relay.h>
 #include <linux/slab.h>
@@ -734,6 +735,11 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 
 out:
 	cpus_write_unlock();
+	/*
+	 * Do post unplug cleanup. This is still protected against
+	 * concurrent CPU hotplug via cpu_add_remove_lock.
+	 */
+	lockup_detector_cleanup();
 	return ret;
 }
 
kernel/watchdog.c +25 -0
index af00095..dd1fd59 100644
@@ -109,8 +109,10 @@ int __weak watchdog_nmi_enable(unsigned int cpu)
 {
 	return 0;
 }
+
 void __weak watchdog_nmi_disable(unsigned int cpu)
 {
+	hardlockup_detector_perf_disable();
 }
 
 /*
@@ -193,6 +195,8 @@ __setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
 #endif
 #endif
 
+static void __lockup_detector_cleanup(void);
+
 /*
  * Hard-lockup warnings should be triggered after just a few seconds. Soft-
  * lockups can have false positives under extreme conditions. So we generally
@@ -631,6 +635,24 @@ static void set_sample_period(void)
 }
 #endif /* SOFTLOCKUP */
 
+static void __lockup_detector_cleanup(void)
+{
+	lockdep_assert_held(&watchdog_mutex);
+	hardlockup_detector_perf_cleanup();
+}
+
+/**
+ * lockup_detector_cleanup - Cleanup after cpu hotplug or sysctl changes
+ *
+ * Caller must not hold the cpu hotplug rwsem.
+ */
+void lockup_detector_cleanup(void)
+{
+	mutex_lock(&watchdog_mutex);
+	__lockup_detector_cleanup();
+	mutex_unlock(&watchdog_mutex);
+}
+
 /**
  * lockup_detector_soft_poweroff - Interface to stop lockup detector(s)
  *
@@ -665,6 +687,8 @@ static int proc_watchdog_update(void)
 
 	watchdog_nmi_reconfigure();
 
+	__lockup_detector_cleanup();
+
 	return err;
 
 }
@@ -837,6 +861,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 		}
 
 		watchdog_nmi_reconfigure();
+		__lockup_detector_cleanup();
 	}
 
 	mutex_unlock(&watchdog_mutex);
kernel/watchdog_hld.c +28 -6
index 7b60271..94111cc 100644
@@ -21,6 +21,8 @@
 static DEFINE_PER_CPU(bool, hard_watchdog_warn);
 static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
+static DEFINE_PER_CPU(struct perf_event *, dead_event);
+static struct cpumask dead_events_mask;
 
 static unsigned long hardlockup_allcpu_dumped;
 static bool hardlockup_detector_disabled;
@@ -239,16 +241,18 @@ out:
 	return 0;
 }
 
-void watchdog_nmi_disable(unsigned int cpu)
+/**
+ * hardlockup_detector_perf_disable - Disable the local event
+ */
+void hardlockup_detector_perf_disable(void)
 {
-	struct perf_event *event = per_cpu(watchdog_ev, cpu);
+	struct perf_event *event = this_cpu_read(watchdog_ev);
 
 	if (event) {
 		perf_event_disable(event);
-		per_cpu(watchdog_ev, cpu) = NULL;
-
-		/* should be in cleanup, but blocks oprofile */
-		perf_event_release_kernel(event);
+		this_cpu_write(watchdog_ev, NULL);
+		this_cpu_write(dead_event, event);
+		cpumask_set_cpu(smp_processor_id(), &dead_events_mask);
 
 		/* watchdog_nmi_enable() expects this to be zero initially. */
 		if (atomic_dec_and_test(&watchdog_cpus))
@@ -257,6 +261,24 @@ void watchdog_nmi_disable(unsigned int cpu)
 }
 
 /**
+ * hardlockup_detector_perf_cleanup - Cleanup disabled events and destroy them
+ *
+ * Called from lockup_detector_cleanup(). Serialized by the caller.
+ */
+void hardlockup_detector_perf_cleanup(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, &dead_events_mask) {
+		struct perf_event *event = per_cpu(dead_event, cpu);
+
+		per_cpu(dead_event, cpu) = NULL;
+		perf_event_release_kernel(event);
+	}
+	cpumask_clear(&dead_events_mask);
+}
+
+/**
  * hardlockup_detector_perf_stop - Globally stop watchdog events
  *
  * Special interface for x86 to handle the perf HT bug.
#23 lore raw

PATCH v2 11/29 lockup_detector: Remove park_in_progress obfuscation

In reply to Thomas Gleixner

b94f51183b06 ("kernel/watchdog: prevent false hardlockup on overloaded system") tries to fix the following issue:

proc_write()
set_sample_period() <--- New sample period becoms visible
<----- Broken starts
proc_watchdog_update()

     watchdog_enable_all_cpus() 	watchdog_hrtimer_fn()
     update_watchdog_all_cpus()		   restart_timer(sample_period)
        watchdog_park_threads()

					thread->park()
					  disable_nmi()
			  <----- Broken ends

The reason why this is broken is that the update of the watchdog threshold becomes immediately effective and visible for the hrtimer function which uses that value to rearm the timer. But the NMI/perf side still uses the old value up to the point where it is disabled. If the rate has been lowered then the NMI can run fast enough to 'detect' a hard lockup because the timer has not fired due to the longer period.

The patch 'fixed' this by adding a variable:

proc_write()
set_sample_period()
<----- Broken starts
proc_watchdog_update()
watchdog_enable_all_cpus() watchdog_hrtimer_fn()
update_watchdog_all_cpus() restart_timer(sample_period)
watchdog_park_threads()
park_in_progress = 1
<----- Broken ends

	  			        nmi_watchdog()
					  if (park_in_progress)
					     return;

The only effect of this variable was to make the window where the breakage can hit small enough that it was not longer observable in testing. From a correctness point of view it is a pointless bandaid which merily papers over the root cause: the unsychronized update of the variable.

Looking deeper into the related code pathes unearthed similar problems in the watchdog_start()/stop() functions.

watchdog_start()
perf_nmi_event_start()
hrtimer_start()

watchdog_stop()
hrtimer_cancel()
perf_nmi_event_stop()

In both cases the call order is wrong because if the tasks gets preempted or the VM gets scheduled out long enough after the first call, then there is a chance that the next NMI will see a stale hrtimer interrupt count and trigger a false positive hard lockup splat.

Get rid of park_in_progress so the code can be gradually deobfuscated and pruned from several layers of duct tape papering over the root cause, which has been either ignored or not understood at all.

Once this is removed the underlying problem will be fixed by rewriting the proc interface to do a proper synchronized update.

Address the start/stop() ordering problem as well by reverting the call order, so this part is at least correct now.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>

---

V2: Make the changelog technically and politically correct.

 include/linux/nmi.h   |    1 -
 kernel/watchdog.c     |   37 +++++++++++++++++--------------------
 kernel/watchdog_hld.c |    7 ++-----
 3 files changed, 19 insertions(+), 26 deletions(-)
include/linux/nmi.h +0 -1
@@ -27,7 +27,6 @@ extern void touch_softlockup_watchdog_sy
 extern void touch_all_softlockup_watchdogs(void);
 extern unsigned int  softlockup_panic;
 extern int soft_watchdog_enabled;
-extern atomic_t watchdog_park_in_progress;
 #else
 static inline void touch_softlockup_watchdog_sched(void)
 {
kernel/watchdog.c +17 -20
@@ -136,8 +136,6 @@ void __weak watchdog_nmi_reconfigure(voi
 #define for_each_watchdog_cpu(cpu) \
 	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
 
-atomic_t watchdog_park_in_progress = ATOMIC_INIT(0);
-
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -322,8 +320,7 @@ static enum hrtimer_restart watchdog_tim
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
 
-	if (!watchdog_enabled ||
-	    atomic_read(&watchdog_park_in_progress) != 0)
+	if (!watchdog_enabled)
 		return HRTIMER_NORESTART;
 
 	/* kick the hardlockup detector */
@@ -437,32 +434,37 @@ static void watchdog_set_prio(unsigned i
 
 static void watchdog_enable(unsigned int cpu)
 {
-	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
+	struct hrtimer *hrtimer = this_cpu_ptr(&watchdog_hrtimer);
 
-	/* kick off the timer for the hardlockup detector */
+	/*
+	 * Start the timer first to prevent the NMI watchdog triggering
+	 * before the timer has a chance to fire.
+	 */
 	hrtimer_init(hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	hrtimer->function = watchdog_timer_fn;
+	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
+		      HRTIMER_MODE_REL_PINNED);
 
+	/* Initialize timestamp */
+	__touch_watchdog();
 	/* Enable the perf event */
 	watchdog_nmi_enable(cpu);
 
-	/* done here because hrtimer_start can only pin to smp_processor_id() */
-	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
-		      HRTIMER_MODE_REL_PINNED);
-
-	/* initialize timestamp */
 	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
-	__touch_watchdog();
 }
 
 static void watchdog_disable(unsigned int cpu)
 {
-	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
+	struct hrtimer *hrtimer = this_cpu_ptr(&watchdog_hrtimer);
 
 	watchdog_set_prio(SCHED_NORMAL, 0);
-	hrtimer_cancel(hrtimer);
-	/* disable the perf event */
+	/*
+	 * Disable the perf event first. That prevents that a large delay
+	 * between disabling the timer and disabling the perf event causes
+	 * the perf NMI to detect a false positive.
+	 */
 	watchdog_nmi_disable(cpu);
+	hrtimer_cancel(hrtimer);
 }
 
 static void watchdog_cleanup(unsigned int cpu, bool online)
@@ -518,16 +520,11 @@ static int watchdog_park_threads(void)
 {
 	int cpu, ret = 0;
 
-	atomic_set(&watchdog_park_in_progress, 1);
-
 	for_each_watchdog_cpu(cpu) {
 		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
 		if (ret)
 			break;
 	}
-
-	atomic_set(&watchdog_park_in_progress, 0);
-
 	return ret;
 }
 
kernel/watchdog_hld.c +2 -5
@@ -106,15 +106,12 @@ static struct perf_event_attr wd_hw_attr
 
 /* Callback function for perf event subsystem */
 static void watchdog_overflow_callback(struct perf_event *event,
-		 struct perf_sample_data *data,
-		 struct pt_regs *regs)
+				       struct perf_sample_data *data,
+				       struct pt_regs *regs)
 {
 	/* Ensure the watchdog never gets throttled */
 	event->hw.interrupts = 0;
 
-	if (atomic_read(&watchdog_park_in_progress) != 0)
-		return;
-
 	if (__this_cpu_read(watchdog_nmi_touch) == true) {
 		__this_cpu_write(watchdog_nmi_touch, false);
 		return;
#24 lore raw

PATCH v2 12/29 lockup_detector: Cleanup stub functions

In reply to Thomas Gleixner

Having stub functions which take a full page is not helping the readablility of code.

Condense them and move the doubled #ifdef variant into the SYSFS section.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   68 +++++++++++++++++-------------------------------------
 1 file changed, 22 insertions(+), 46 deletions(-)
kernel/watchdog.c +22 -46
@@ -125,10 +125,7 @@ void __weak watchdog_nmi_disable(unsigne
  * - sysctl_hardlockup_all_cpu_backtrace
  * - hardlockup_panic
  */
-void __weak watchdog_nmi_reconfigure(void)
-{
-}
-
+void __weak watchdog_nmi_reconfigure(void) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -136,6 +133,11 @@ void __weak watchdog_nmi_reconfigure(voi
 #define for_each_watchdog_cpu(cpu) \
 	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
 
+/* Global variables, exported for sysctl */
+unsigned int __read_mostly softlockup_panic =
+			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
+int __read_mostly soft_watchdog_enabled;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -149,13 +151,9 @@ static DEFINE_PER_CPU(struct task_struct
 static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
 static unsigned long soft_lockup_nmi_warn;
 
-unsigned int __read_mostly softlockup_panic =
-			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
-
 static int __init softlockup_panic_setup(char *str)
 {
 	softlockup_panic = simple_strtoul(str, NULL, 0);
-
 	return 1;
 }
 __setup("softlockup_panic=", softlockup_panic_setup);
@@ -593,44 +591,13 @@ static void watchdog_disable_all_cpus(vo
 	}
 }
 
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return smpboot_update_cpumask_percpu_thread(
-		    &watchdog_threads, &watchdog_cpumask);
-}
-#endif
-
-#else /* SOFTLOCKUP */
-static int watchdog_park_threads(void)
-{
-	return 0;
-}
-
-static void watchdog_unpark_threads(void)
-{
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	return 0;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-}
-
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return 0;
-}
-#endif
-
-static void set_sample_period(void)
-{
-}
-#endif /* SOFTLOCKUP */
+#else /* CONFIG_SOFTLOCKUP_DETECTOR */
+static inline int watchdog_park_threads(void) { return 0; }
+static inline void watchdog_unpark_threads(void) { }
+static inline int watchdog_enable_all_cpus(void) { return 0; }
+static inline void watchdog_disable_all_cpus(void) { }
+static inline void set_sample_period(void) { }
+#endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
 {
@@ -827,6 +794,15 @@ int proc_watchdog_thresh(struct ctl_tabl
 	return err;
 }
 
+static int watchdog_update_cpus(void)
+{
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
+		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+							    &watchdog_cpumask);
+	}
+	return 0;
+}
+
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
#25 lore raw

[tip:core/urgent] watchdog/core: Clean up stub functions

watchdog/core: Clean up stub functions
Commit-ID:  2b9d7f233b835663cbc7b6b3f88dd20f61118d1e
Gitweb:     http://git.kernel.org/tip/2b9d7f233b835663cbc7b6b3f88dd20f61118d1e
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:06 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:05 +0200

watchdog/core: Clean up stub functions

Having stub functions which take a full page is not helping the readablility of code.

Condense them and move the doubled #ifdef variant into the SYSFS section.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 68 ++++++++++++++++++-------------------------------------
 1 file changed, 22 insertions(+), 46 deletions(-)
kernel/watchdog.c +22 -46
index c290135..af37c04 100644
@@ -125,10 +125,7 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
  * - sysctl_hardlockup_all_cpu_backtrace
  * - hardlockup_panic
  */
-void __weak watchdog_nmi_reconfigure(void)
-{
-}
-
+void __weak watchdog_nmi_reconfigure(void) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -136,6 +133,11 @@ void __weak watchdog_nmi_reconfigure(void)
 #define for_each_watchdog_cpu(cpu) \
 	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
 
+/* Global variables, exported for sysctl */
+unsigned int __read_mostly softlockup_panic =
+			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
+int __read_mostly soft_watchdog_enabled;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -149,13 +151,9 @@ static DEFINE_PER_CPU(struct task_struct *, softlockup_task_ptr_saved);
 static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
 static unsigned long soft_lockup_nmi_warn;
 
-unsigned int __read_mostly softlockup_panic =
-			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
-
 static int __init softlockup_panic_setup(char *str)
 {
 	softlockup_panic = simple_strtoul(str, NULL, 0);
-
 	return 1;
 }
 __setup("softlockup_panic=", softlockup_panic_setup);
@@ -593,44 +591,13 @@ static void watchdog_disable_all_cpus(void)
 	}
 }
 
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return smpboot_update_cpumask_percpu_thread(
-		    &watchdog_threads, &watchdog_cpumask);
-}
-#endif
-
-#else /* SOFTLOCKUP */
-static int watchdog_park_threads(void)
-{
-	return 0;
-}
-
-static void watchdog_unpark_threads(void)
-{
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	return 0;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-}
-
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return 0;
-}
-#endif
-
-static void set_sample_period(void)
-{
-}
-#endif /* SOFTLOCKUP */
+#else /* CONFIG_SOFTLOCKUP_DETECTOR */
+static inline int watchdog_park_threads(void) { return 0; }
+static inline void watchdog_unpark_threads(void) { }
+static inline int watchdog_enable_all_cpus(void) { return 0; }
+static inline void watchdog_disable_all_cpus(void) { }
+static inline void set_sample_period(void) { }
+#endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
 {
@@ -827,6 +794,15 @@ out:
 	return err;
 }
 
+static int watchdog_update_cpus(void)
+{
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
+		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+							    &watchdog_cpumask);
+	}
+	return 0;
+}
+
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
#26 lore raw

PATCH v2 13/29 lockup_detector: Cleanup the ifdef maze

In reply to Thomas Gleixner

The #ifdef maze in this file is horrible, Group stuff at least a bit so one can figure out what belongs to what.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)
kernel/watchdog.c +13 -20
@@ -41,7 +41,6 @@ unsigned long __read_mostly watchdog_ena
 #endif
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
-/* boot commands */
 /*
  * Should we panic when a soft-lockup or hard-lockup occurs:
  */
@@ -74,19 +73,21 @@ static int __init hardlockup_panic_setup
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
 
-#endif
+# ifdef CONFIG_SMP
+int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
 
-#ifdef CONFIG_SOFTLOCKUP_DETECTOR
-int __read_mostly soft_watchdog_enabled;
-#endif
+static int __init hardlockup_all_cpu_backtrace_setup(char *str)
+{
+	sysctl_hardlockup_all_cpu_backtrace = !!simple_strtol(str, NULL, 0);
+	return 1;
+}
+__setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
+# endif /* CONFIG_SMP */
+#endif /* CONFIG_HARDLOCKUP_DETECTOR */
 
 int __read_mostly watchdog_user_enabled;
 int __read_mostly watchdog_thresh = 10;
 
-#ifdef CONFIG_SMP
-int __read_mostly sysctl_softlockup_all_cpu_backtrace;
-int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
-#endif
 struct cpumask watchdog_cpumask __read_mostly;
 unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
 
@@ -173,22 +174,14 @@ static int __init nosoftlockup_setup(cha
 __setup("nosoftlockup", nosoftlockup_setup);
 
 #ifdef CONFIG_SMP
+int __read_mostly sysctl_softlockup_all_cpu_backtrace;
+
 static int __init softlockup_all_cpu_backtrace_setup(char *str)
 {
-	sysctl_softlockup_all_cpu_backtrace =
-		!!simple_strtol(str, NULL, 0);
+	sysctl_softlockup_all_cpu_backtrace = !!simple_strtol(str, NULL, 0);
 	return 1;
 }
 __setup("softlockup_all_cpu_backtrace=", softlockup_all_cpu_backtrace_setup);
-#ifdef CONFIG_HARDLOCKUP_DETECTOR
-static int __init hardlockup_all_cpu_backtrace_setup(char *str)
-{
-	sysctl_hardlockup_all_cpu_backtrace =
-		!!simple_strtol(str, NULL, 0);
-	return 1;
-}
-__setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
-#endif
 #endif
 
 static void __lockup_detector_cleanup(void);
#27 lore raw

[tip:core/urgent] watchdog/core: Clean up the #ifdef maze

watchdog/core: Clean up the #ifdef maze
Commit-ID:  368a7e2ce8ff0ddcdcb37eadb76530b033f6eb2d
Gitweb:     http://git.kernel.org/tip/368a7e2ce8ff0ddcdcb37eadb76530b033f6eb2d
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:07 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:05 +0200

watchdog/core: Clean up the #ifdef maze

The #ifdef maze in this file is horrible, group stuff at least a bit so one can figure out what belongs to what.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)
kernel/watchdog.c +13 -20
index af37c04..a9bdfde 100644
@@ -41,7 +41,6 @@ unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
 #endif
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
-/* boot commands */
 /*
  * Should we panic when a soft-lockup or hard-lockup occurs:
  */
@@ -74,19 +73,21 @@ static int __init hardlockup_panic_setup(char *str)
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
 
-#endif
+# ifdef CONFIG_SMP
+int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
 
-#ifdef CONFIG_SOFTLOCKUP_DETECTOR
-int __read_mostly soft_watchdog_enabled;
-#endif
+static int __init hardlockup_all_cpu_backtrace_setup(char *str)
+{
+	sysctl_hardlockup_all_cpu_backtrace = !!simple_strtol(str, NULL, 0);
+	return 1;
+}
+__setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
+# endif /* CONFIG_SMP */
+#endif /* CONFIG_HARDLOCKUP_DETECTOR */
 
 int __read_mostly watchdog_user_enabled;
 int __read_mostly watchdog_thresh = 10;
 
-#ifdef CONFIG_SMP
-int __read_mostly sysctl_softlockup_all_cpu_backtrace;
-int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
-#endif
 struct cpumask watchdog_cpumask __read_mostly;
 unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
 
@@ -173,22 +174,14 @@ static int __init nosoftlockup_setup(char *str)
 __setup("nosoftlockup", nosoftlockup_setup);
 
 #ifdef CONFIG_SMP
+int __read_mostly sysctl_softlockup_all_cpu_backtrace;
+
 static int __init softlockup_all_cpu_backtrace_setup(char *str)
 {
-	sysctl_softlockup_all_cpu_backtrace =
-		!!simple_strtol(str, NULL, 0);
+	sysctl_softlockup_all_cpu_backtrace = !!simple_strtol(str, NULL, 0);
 	return 1;
 }
 __setup("softlockup_all_cpu_backtrace=", softlockup_all_cpu_backtrace_setup);
-#ifdef CONFIG_HARDLOCKUP_DETECTOR
-static int __init hardlockup_all_cpu_backtrace_setup(char *str)
-{
-	sysctl_hardlockup_all_cpu_backtrace =
-		!!simple_strtol(str, NULL, 0);
-	return 1;
-}
-__setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
-#endif
 #endif
 
 static void __lockup_detector_cleanup(void);
#28 lore raw

PATCH v2 14/29 lockup_detector: Split out cpumask write function

In reply to Thomas Gleixner

Split the write part of the cpumask proc handler out into a separate helper to avoid deep indentation. This also reduces the patch complexity in the following cleanups.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)
kernel/watchdog.c +21 -19
@@ -792,10 +792,29 @@ static int watchdog_update_cpus(void)
 	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
 		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
 							    &watchdog_cpumask);
+		__lockup_detector_cleanup();
 	}
 	return 0;
 }
 
+static void proc_watchdog_cpumask_update(void)
+{
+	/* Remove impossible cpus to keep sysctl output clean. */
+	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
+
+	if (watchdog_running) {
+		/*
+		 * Failure would be due to being unable to allocate a
+		 * temporary cpumask, so we are likely not in a position to
+		 * do much else to make things better.
+		 */
+		if (watchdog_update_cpus() != 0)
+			pr_err("cpumask update failed\n");
+	}
+
+	watchdog_nmi_reconfigure();
+}
+
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
@@ -811,30 +830,13 @@ int proc_watchdog_cpumask(struct ctl_tab
 	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
-	if (!err && write) {
-		/* Remove impossible cpus to keep sysctl output cleaner. */
-		cpumask_and(&watchdog_cpumask, &watchdog_cpumask,
-			    cpu_possible_mask);
-
-		if (watchdog_running) {
-			/*
-			 * Failure would be due to being unable to allocate
-			 * a temporary cpumask, so we are likely not in a
-			 * position to do much else to make things better.
-			 */
-			if (watchdog_update_cpus() != 0)
-				pr_err("cpumask update failed\n");
-		}
-
-		watchdog_nmi_reconfigure();
-		__lockup_detector_cleanup();
-	}
+	if (!err && write)
+		proc_watchdog_cpumask_update();
 
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
-
 #endif /* CONFIG_SYSCTL */
 
 void __init lockup_detector_init(void)
#29 lore raw

[tip:core/urgent] watchdog/core: Split out cpumask write function

watchdog/core: Split out cpumask write function
Commit-ID:  05ba3de74a3f499dcaa37b186220aaf174c95a4b
Gitweb:     http://git.kernel.org/tip/05ba3de74a3f499dcaa37b186220aaf174c95a4b
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:08 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:06 +0200

watchdog/core: Split out cpumask write function

Split the write part of the cpumask proc handler out into a separate helper to avoid deep indentation. This also reduces the patch complexity in the following cleanups.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)
kernel/watchdog.c +21 -19
index a9bdfde..cedf45a 100644
@@ -792,10 +792,29 @@ static int watchdog_update_cpus(void)
 	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
 		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
 							    &watchdog_cpumask);
+		__lockup_detector_cleanup();
 	}
 	return 0;
 }
 
+static void proc_watchdog_cpumask_update(void)
+{
+	/* Remove impossible cpus to keep sysctl output clean. */
+	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
+
+	if (watchdog_running) {
+		/*
+		 * Failure would be due to being unable to allocate a
+		 * temporary cpumask, so we are likely not in a position to
+		 * do much else to make things better.
+		 */
+		if (watchdog_update_cpus() != 0)
+			pr_err("cpumask update failed\n");
+	}
+
+	watchdog_nmi_reconfigure();
+}
+
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
@@ -811,30 +830,13 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
-	if (!err && write) {
-		/* Remove impossible cpus to keep sysctl output cleaner. */
-		cpumask_and(&watchdog_cpumask, &watchdog_cpumask,
-			    cpu_possible_mask);
-
-		if (watchdog_running) {
-			/*
-			 * Failure would be due to being unable to allocate
-			 * a temporary cpumask, so we are likely not in a
-			 * position to do much else to make things better.
-			 */
-			if (watchdog_update_cpus() != 0)
-				pr_err("cpumask update failed\n");
-		}
-
-		watchdog_nmi_reconfigure();
-		__lockup_detector_cleanup();
-	}
+	if (!err && write)
+		proc_watchdog_cpumask_update();
 
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
-
 #endif /* CONFIG_SYSCTL */
 
 void __init lockup_detector_init(void)
#30 lore raw

PATCH v2 15/29 smpboot/threads: Avoid runtime allocation

In reply to Thomas Gleixner

smpboot_update_cpumask_threads_percpu() allocates a temporary cpumask at runtime. This is suboptimal because the call site needs more code size for proper error handling than a statically allocated temporary mask requires data size.

Add static temporary cpumask. The function is globaly serialized, so no further protection required.

Remove the half baken error handling in the watchdog code and get rid of the export as there are no in tree modular users of that function.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/smpboot.h |    4 ++--
 kernel/smpboot.c        |   22 +++++++---------------
 kernel/watchdog.c       |   21 +++++----------------
 3 files changed, 14 insertions(+), 33 deletions(-)
include/linux/smpboot.h +2 -2
@@ -55,7 +55,7 @@ smpboot_register_percpu_thread(struct sm
 }
 
 void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *);
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *);
 
 #endif
kernel/smpboot.c +7 -15
@@ -344,39 +344,31 @@ EXPORT_SYMBOL_GPL(smpboot_unregister_per
  * by the client, but only by calling this function.
  * This function can only be called on a registered smp_hotplug_thread.
  */
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *new)
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *new)
 {
 	struct cpumask *old = plug_thread->cpumask;
-	cpumask_var_t tmp;
+	static struct cpumask tmp;
 	unsigned int cpu;
 
-	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
-		return -ENOMEM;
-
 	get_online_cpus();
 	mutex_lock(&smpboot_threads_lock);
 
 	/* Park threads that were exclusively enabled on the old mask. */
-	cpumask_andnot(tmp, old, new);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, old, new);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_park_thread(plug_thread, cpu);
 
 	/* Unpark threads that are exclusively enabled on the new mask. */
-	cpumask_andnot(tmp, new, old);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, new, old);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_unpark_thread(plug_thread, cpu);
 
 	cpumask_copy(old, new);
 
 	mutex_unlock(&smpboot_threads_lock);
 	put_online_cpus();
-
-	free_cpumask_var(tmp);
-
-	return 0;
 }
-EXPORT_SYMBOL_GPL(smpboot_update_cpumask_percpu_thread);
 
 static DEFINE_PER_CPU(atomic_t, cpu_hotplug_state) = ATOMIC_INIT(CPU_POST_DEAD);
 
kernel/watchdog.c +5 -16
@@ -787,31 +787,20 @@ int proc_watchdog_thresh(struct ctl_tabl
 	return err;
 }
 
-static int watchdog_update_cpus(void)
+static void watchdog_update_cpus(void)
 {
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
-		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-							    &watchdog_cpumask);
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
+		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+						     &watchdog_cpumask);
 		__lockup_detector_cleanup();
 	}
-	return 0;
 }
 
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-
-	if (watchdog_running) {
-		/*
-		 * Failure would be due to being unable to allocate a
-		 * temporary cpumask, so we are likely not in a position to
-		 * do much else to make things better.
-		 */
-		if (watchdog_update_cpus() != 0)
-			pr_err("cpumask update failed\n");
-	}
-
+	watchdog_update_cpus();
 	watchdog_nmi_reconfigure();
 }
 
#31 lore raw

[tip:core/urgent] smpboot/threads, watchdog/core: Avoid runtime allocation

smpboot/threads, watchdog/core: Avoid runtime allocation
Commit-ID:  0d85923c7a81719567311ba0eae8ecb2efd4c8a0
Gitweb:     http://git.kernel.org/tip/0d85923c7a81719567311ba0eae8ecb2efd4c8a0
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:09 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:06 +0200

smpboot/threads, watchdog/core: Avoid runtime allocation

smpboot_update_cpumask_threads_percpu() allocates a temporary cpumask at runtime. This is suboptimal because the call site needs more code size for proper error handling than a statically allocated temporary mask requires data size.

Add static temporary cpumask. The function is globaly serialized, so no further protection required.

Remove the half baken error handling in the watchdog code and get rid of the export as there are no in tree modular users of that function.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/smpboot.h |  4 ++--
 kernel/smpboot.c        | 22 +++++++---------------
 kernel/watchdog.c       | 21 +++++----------------
 3 files changed, 14 insertions(+), 33 deletions(-)
include/linux/smpboot.h +2 -2
index 12910cf..c149aa7 100644
@@ -55,7 +55,7 @@ smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
 }
 
 void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *);
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *);
 
 #endif
kernel/smpboot.c +7 -15
index 1d71c05..ed7507b 100644
@@ -344,39 +344,31 @@ EXPORT_SYMBOL_GPL(smpboot_unregister_percpu_thread);
  * by the client, but only by calling this function.
  * This function can only be called on a registered smp_hotplug_thread.
  */
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *new)
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *new)
 {
 	struct cpumask *old = plug_thread->cpumask;
-	cpumask_var_t tmp;
+	static struct cpumask tmp;
 	unsigned int cpu;
 
-	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
-		return -ENOMEM;
-
 	get_online_cpus();
 	mutex_lock(&smpboot_threads_lock);
 
 	/* Park threads that were exclusively enabled on the old mask. */
-	cpumask_andnot(tmp, old, new);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, old, new);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_park_thread(plug_thread, cpu);
 
 	/* Unpark threads that are exclusively enabled on the new mask. */
-	cpumask_andnot(tmp, new, old);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, new, old);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_unpark_thread(plug_thread, cpu);
 
 	cpumask_copy(old, new);
 
 	mutex_unlock(&smpboot_threads_lock);
 	put_online_cpus();
-
-	free_cpumask_var(tmp);
-
-	return 0;
 }
-EXPORT_SYMBOL_GPL(smpboot_update_cpumask_percpu_thread);
 
 static DEFINE_PER_CPU(atomic_t, cpu_hotplug_state) = ATOMIC_INIT(CPU_POST_DEAD);
 
kernel/watchdog.c +5 -16
index cedf45a..8935a3a 100644
@@ -787,31 +787,20 @@ out:
 	return err;
 }
 
-static int watchdog_update_cpus(void)
+static void watchdog_update_cpus(void)
 {
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
-		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-							    &watchdog_cpumask);
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
+		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+						     &watchdog_cpumask);
 		__lockup_detector_cleanup();
 	}
-	return 0;
 }
 
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-
-	if (watchdog_running) {
-		/*
-		 * Failure would be due to being unable to allocate a
-		 * temporary cpumask, so we are likely not in a position to
-		 * do much else to make things better.
-		 */
-		if (watchdog_update_cpus() != 0)
-			pr_err("cpumask update failed\n");
-	}
-
+	watchdog_update_cpus();
 	watchdog_nmi_reconfigure();
 }
 
#32 lore raw

PATCH v2 16/29 lockup_detector: Create new thread handling infrastructure

In reply to Thomas Gleixner

The lockup detector reconfiguration tears down all watchdog threads when the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an insane amount of resources, so it's enough to set them up at init time and keep them in parked position when the watchdog is disabled and unpark them when it is reenabled. The smpboot thread infrastructure takes care of keeping the force parked threads in place even across cpu hotplug.

Another horrible mechanism are the open coded park/unpark loops which are used for reconfiguration of the watchdog. The smpboot infrastructure allows exactly the same via smpboot_update_cpumask_thread_percpu(), which is cpu hotplug safe. Using that instead of the open coded loops allows to get rid of the hotplug locking mess in the watchdog code.

Implement a clean infrastructure which allows to replace the open coded nonsense.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
kernel/watchdog.c +75 -0
@@ -139,6 +139,9 @@ unsigned int __read_mostly softlockup_pa
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
 int __read_mostly soft_watchdog_enabled;
 
+struct cpumask watchdog_allowed_mask __read_mostly;
+static bool softlockup_threads_initialized __read_mostly;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -584,12 +587,84 @@ static void watchdog_disable_all_cpus(vo
 	}
 }
 
+static void softlockup_update_smpboot_threads(void)
+{
+	lockdep_assert_held(&watchdog_mutex);
+
+	if (!softlockup_threads_initialized)
+		return;
+
+	smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+					     &watchdog_allowed_mask);
+	__lockup_detector_cleanup();
+}
+
+/* Temporarily park all watchdog threads */
+static void softlockup_park_all_threads(void)
+{
+	cpumask_clear(&watchdog_allowed_mask);
+	softlockup_update_smpboot_threads();
+}
+
+/*
+ * Park threads which are not longer enabled and unpark threads which have
+ * been newly enabled.
+ */
+static void softlockup_update_threads(void)
+{
+	cpumask_copy(&watchdog_allowed_mask, &watchdog_cpumask);
+	softlockup_update_smpboot_threads();
+}
+
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	softlockup_park_all_threads();
+	set_sample_period();
+	if (enabled)
+		softlockup_update_threads();
+}
+
+/*
+ * Create the watchdog thread infrastructure.
+ *
+ * The threads are not unparked as watchdog_allowed_mask is empty.  When
+ * the threads are sucessfully initialized, take the proper locks and
+ * unpark the threads in the watchdog_cpumask if the watchdog is enabled.
+ */
+static __init void softlockup_init_threads(void)
+{
+	int ret;
+
+	/*
+	 * If sysctl is off and watchdog got disabled on the command line,
+	 * nothing to do here.
+	 */
+	if (!IS_ENABLED(CONFIG_SYSCTL) &&
+	    !(watchdog_enabled && watchdog_thresh))
+		return;
+
+	ret = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
+						     &watchdog_allowed_mask);
+	if (ret) {
+		pr_err("Failed to initialize soft lockup detector threads\n");
+		return;
+	}
+
+	mutex_lock(&watchdog_mutex);
+	softlockup_threads_initialized = true;
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	mutex_unlock(&watchdog_mutex);
+}
+
 #else /* CONFIG_SOFTLOCKUP_DETECTOR */
 static inline int watchdog_park_threads(void) { return 0; }
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void set_sample_period(void) { }
+static inline void softlockup_init_threads(void) { }
+static inline void softlockup_update_threads(void) { }
+static inline void softlockup_reconfigure_threads(bool enabled) { }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
#33 lore raw

[tip:core/urgent] watchdog/core: Create new thread handling infrastructure

watchdog/core: Create new thread handling infrastructure
Commit-ID:  2eb2527f847d1bd8d8fb9db1e8139db5d6eddb36
Gitweb:     http://git.kernel.org/tip/2eb2527f847d1bd8d8fb9db1e8139db5d6eddb36
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:10 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:06 +0200

watchdog/core: Create new thread handling infrastructure

The lockup detector reconfiguration tears down all watchdog threads when the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an insane amount of resources, so it's enough to set them up at init time and keep them in parked position when the watchdog is disabled and unpark them when it is reenabled. The smpboot thread infrastructure takes care of keeping the force parked threads in place even across cpu hotplug.

Another horrible mechanism are the open coded park/unpark loops which are used for reconfiguration of the watchdog. The smpboot infrastructure allows exactly the same via smpboot_update_cpumask_thread_percpu(), which is cpu hotplug safe. Using that instead of the open coded loops allows to get rid of the hotplug locking mess in the watchdog code.

Implement a clean infrastructure which allows to replace the open coded nonsense.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
kernel/watchdog.c +75 -0
index 8935a3a..b355183 100644
@@ -139,6 +139,9 @@ unsigned int __read_mostly softlockup_panic =
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
 int __read_mostly soft_watchdog_enabled;
 
+struct cpumask watchdog_allowed_mask __read_mostly;
+static bool softlockup_threads_initialized __read_mostly;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -584,12 +587,84 @@ static void watchdog_disable_all_cpus(void)
 	}
 }
 
+static void softlockup_update_smpboot_threads(void)
+{
+	lockdep_assert_held(&watchdog_mutex);
+
+	if (!softlockup_threads_initialized)
+		return;
+
+	smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+					     &watchdog_allowed_mask);
+	__lockup_detector_cleanup();
+}
+
+/* Temporarily park all watchdog threads */
+static void softlockup_park_all_threads(void)
+{
+	cpumask_clear(&watchdog_allowed_mask);
+	softlockup_update_smpboot_threads();
+}
+
+/*
+ * Park threads which are not longer enabled and unpark threads which have
+ * been newly enabled.
+ */
+static void softlockup_update_threads(void)
+{
+	cpumask_copy(&watchdog_allowed_mask, &watchdog_cpumask);
+	softlockup_update_smpboot_threads();
+}
+
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	softlockup_park_all_threads();
+	set_sample_period();
+	if (enabled)
+		softlockup_update_threads();
+}
+
+/*
+ * Create the watchdog thread infrastructure.
+ *
+ * The threads are not unparked as watchdog_allowed_mask is empty.  When
+ * the threads are sucessfully initialized, take the proper locks and
+ * unpark the threads in the watchdog_cpumask if the watchdog is enabled.
+ */
+static __init void softlockup_init_threads(void)
+{
+	int ret;
+
+	/*
+	 * If sysctl is off and watchdog got disabled on the command line,
+	 * nothing to do here.
+	 */
+	if (!IS_ENABLED(CONFIG_SYSCTL) &&
+	    !(watchdog_enabled && watchdog_thresh))
+		return;
+
+	ret = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
+						     &watchdog_allowed_mask);
+	if (ret) {
+		pr_err("Failed to initialize soft lockup detector threads\n");
+		return;
+	}
+
+	mutex_lock(&watchdog_mutex);
+	softlockup_threads_initialized = true;
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	mutex_unlock(&watchdog_mutex);
+}
+
 #else /* CONFIG_SOFTLOCKUP_DETECTOR */
 static inline int watchdog_park_threads(void) { return 0; }
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void set_sample_period(void) { }
+static inline void softlockup_init_threads(void) { }
+static inline void softlockup_update_threads(void) { }
+static inline void softlockup_reconfigure_threads(bool enabled) { }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
#34 lore raw

PATCH v2 17/29 lockup_detector: Get rid of the thread teardown/setup dance

In reply to Thomas Gleixner

The lockup detector reconfiguration tears down all watchdog threads when the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an insane amount of resources, so it's enough to set them up at init time and keep them in parked position when the watchdog is disabled and unpark them when it is reenabled. The smpboot thread infrastructure takes care of keeping the force parked threads in place even across cpu hotplug.

Aside of that the code implements the park/unpark facility of smp hotplug threads on its own, which is even more pointless. We have functionality in the smpboot thread code to do so.

Use the new thread management functions and get rid of the unholy mess.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |  190 +++++-------------------------------------------------
 1 file changed, 19 insertions(+), 171 deletions(-)
kernel/watchdog.c +19 -171
@@ -92,13 +92,6 @@ struct cpumask watchdog_cpumask __read_m
 unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
 
 /*
- * The 'watchdog_running' variable is set to 1 when the watchdog threads
- * are registered/started and is set to 0 when the watchdog threads are
- * unregistered/stopped, so it is an indicator whether the threads exist.
- */
-static int __read_mostly watchdog_running;
-
-/*
  * These functions can be overridden if an architecture implements its
  * own hardlockup detector.
  *
@@ -130,10 +123,6 @@ void __weak watchdog_nmi_reconfigure(voi
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
-/* Helper for online, unparked cpus. */
-#define for_each_watchdog_cpu(cpu) \
-	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
-
 /* Global variables, exported for sysctl */
 unsigned int __read_mostly softlockup_panic =
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
@@ -259,11 +248,15 @@ void touch_all_softlockup_watchdogs(void
 	int cpu;
 
 	/*
-	 * this is done lockless
-	 * do we care if a 0 races with a timestamp?
-	 * all it means is the softlock check starts one cycle later
+	 * watchdog_mutex cannpt be taken here, as this might be called
+	 * from (soft)interrupt context, so the access to
+	 * watchdog_allowed_cpumask might race with a concurrent update.
+	 *
+	 * The watchdog time stamp can race against a concurrent real
+	 * update as well, the only side effect might be a cycle delay for
+	 * the softlockup check.
 	 */
-	for_each_watchdog_cpu(cpu)
+	for_each_cpu(cpu, &watchdog_allowed_mask)
 		per_cpu(watchdog_touch_ts, cpu) = 0;
 	wq_watchdog_touch(-1);
 }
@@ -303,9 +296,6 @@ static void watchdog_interrupt_count(voi
 	__this_cpu_inc(hrtimer_interrupts);
 }
 
-static int watchdog_enable_all_cpus(void);
-static void watchdog_disable_all_cpus(void);
-
 /* watchdog kicker functions */
 static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 {
@@ -498,95 +488,6 @@ static struct smp_hotplug_thread watchdo
 	.unpark			= watchdog_enable,
 };
 
-/*
- * park all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function returns an error if kthread_park() of a watchdog thread
- * fails. In this situation, the watchdog threads of some CPUs can already
- * be parked and the watchdog threads of other CPUs can still be runnable.
- * Callers are expected to handle this special condition as appropriate in
- * their context.
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static int watchdog_park_threads(void)
-{
-	int cpu, ret = 0;
-
-	for_each_watchdog_cpu(cpu) {
-		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
-/*
- * unpark all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static void watchdog_unpark_threads(void)
-{
-	int cpu;
-
-	for_each_watchdog_cpu(cpu)
-		kthread_unpark(per_cpu(softlockup_watchdog, cpu));
-}
-
-static int update_watchdog_all_cpus(void)
-{
-	int ret;
-
-	ret = watchdog_park_threads();
-	if (ret)
-		return ret;
-
-	watchdog_unpark_threads();
-
-	return 0;
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	int err = 0;
-
-	if (!watchdog_running) {
-		err = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
-							     &watchdog_cpumask);
-		if (err)
-			pr_err("Failed to create watchdog threads, disabled\n");
-		else
-			watchdog_running = 1;
-	} else {
-		/*
-		 * Enable/disable the lockup detectors or
-		 * change the sample period 'on the fly'.
-		 */
-		err = update_watchdog_all_cpus();
-
-		if (err) {
-			watchdog_disable_all_cpus();
-			pr_err("Failed to update lockup detectors, disabled\n");
-		}
-	}
-
-	if (err)
-		watchdog_enabled = 0;
-
-	return err;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-	if (watchdog_running) {
-		watchdog_running = 0;
-		smpboot_unregister_percpu_thread(&watchdog_threads);
-	}
-}
-
 static void softlockup_update_smpboot_threads(void)
 {
 	lockdep_assert_held(&watchdog_mutex);
@@ -661,7 +562,6 @@ static inline int watchdog_park_threads(
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
-static inline void set_sample_period(void) { }
 static inline void softlockup_init_threads(void) { }
 static inline void softlockup_update_threads(void) { }
 static inline void softlockup_reconfigure_threads(bool enabled) { }
@@ -701,28 +601,10 @@ void lockup_detector_soft_poweroff(void)
 /*
  * Update the run state of the lockup detectors.
  */
-static int proc_watchdog_update(void)
+static void proc_watchdog_update(void)
 {
-	int err = 0;
-
-	/*
-	 * 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
-	 * period will be updated and the lockup detectors will be enabled
-	 * or disabled 'on the fly'.
-	 */
-	if (watchdog_enabled && watchdog_thresh)
-		err = watchdog_enable_all_cpus();
-	else
-		watchdog_disable_all_cpus();
-
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
 	watchdog_nmi_reconfigure();
-
-	__lockup_detector_cleanup();
-
-	return err;
-
 }
 
 /*
@@ -778,17 +660,8 @@ static int proc_watchdog_common(int whic
 				new = old & ~which;
 		} while (cmpxchg(&watchdog_enabled, old, new) != old);
 
-		/*
-		 * Update the run state of the lockup detectors. There is _no_
-		 * need to check the value returned by proc_watchdog_update()
-		 * and to restore the previous value of 'watchdog_enabled' as
-		 * both lockup detectors are disabled if proc_watchdog_update()
-		 * returns an error.
-		 */
-		if (old == new)
-			goto out;
-
-		err = proc_watchdog_update();
+		if (old != new)
+			proc_watchdog_update();
 	}
 out:
 	mutex_unlock(&watchdog_mutex);
@@ -832,50 +705,28 @@ int proc_soft_watchdog(struct ctl_table
 int proc_watchdog_thresh(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old, new;
+	int err, old;
 
 	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
-	old = ACCESS_ONCE(watchdog_thresh);
+	old = READ_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
-	if (err || !write)
-		goto out;
+	if (!err && write && old != READ_ONCE(watchdog_thresh))
+		proc_watchdog_update();
 
-	/*
-	 * Update the sample period. Restore on failure.
-	 */
-	new = ACCESS_ONCE(watchdog_thresh);
-	if (old == new)
-		goto out;
-
-	set_sample_period();
-	err = proc_watchdog_update();
-	if (err) {
-		watchdog_thresh = old;
-		set_sample_period();
-	}
-out:
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
 
-static void watchdog_update_cpus(void)
-{
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
-		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-						     &watchdog_cpumask);
-		__lockup_detector_cleanup();
-	}
-}
-
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-	watchdog_update_cpus();
+
+	softlockup_update_threads();
 	watchdog_nmi_reconfigure();
 }
 
@@ -905,8 +756,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 
 void __init lockup_detector_init(void)
 {
-	set_sample_period();
-
 #ifdef CONFIG_NO_HZ_FULL
 	if (tick_nohz_full_enabled()) {
 		pr_info("Disabling watchdog on nohz_full cores by default\n");
@@ -917,6 +766,5 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
-	if (watchdog_enabled)
-		watchdog_enable_all_cpus();
+	softlockup_init_threads();
 }
#35 lore raw

[tip:core/urgent] watchdog/core: Get rid of the thread teardown/setup dance

watchdog/core: Get rid of the thread teardown/setup dance
Commit-ID:  d57108d4f6791291e89d980e7f7a3566c32ab188
Gitweb:     http://git.kernel.org/tip/d57108d4f6791291e89d980e7f7a3566c32ab188
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:11 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:06 +0200

watchdog/core: Get rid of the thread teardown/setup dance

The lockup detector reconfiguration tears down all watchdog threads when the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an insane amount of resources, so it's enough to set them up at init time and keep them in parked position when the watchdog is disabled and unpark them when it is reenabled. The smpboot thread infrastructure takes care of keeping the force parked threads in place even across cpu hotplug.

Aside of that the code implements the park/unpark facility of smp hotplug threads on its own, which is even more pointless. We have functionality in the smpboot thread code to do so.

Use the new thread management functions and get rid of the unholy mess.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 190 ++++++------------------------------------------------
 1 file changed, 19 insertions(+), 171 deletions(-)
kernel/watchdog.c +19 -171
index b355183..762d3ed 100644
@@ -92,13 +92,6 @@ struct cpumask watchdog_cpumask __read_mostly;
 unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
 
 /*
- * The 'watchdog_running' variable is set to 1 when the watchdog threads
- * are registered/started and is set to 0 when the watchdog threads are
- * unregistered/stopped, so it is an indicator whether the threads exist.
- */
-static int __read_mostly watchdog_running;
-
-/*
  * These functions can be overridden if an architecture implements its
  * own hardlockup detector.
  *
@@ -130,10 +123,6 @@ void __weak watchdog_nmi_reconfigure(void) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
-/* Helper for online, unparked cpus. */
-#define for_each_watchdog_cpu(cpu) \
-	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
-
 /* Global variables, exported for sysctl */
 unsigned int __read_mostly softlockup_panic =
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
@@ -259,11 +248,15 @@ void touch_all_softlockup_watchdogs(void)
 	int cpu;
 
 	/*
-	 * this is done lockless
-	 * do we care if a 0 races with a timestamp?
-	 * all it means is the softlock check starts one cycle later
+	 * watchdog_mutex cannpt be taken here, as this might be called
+	 * from (soft)interrupt context, so the access to
+	 * watchdog_allowed_cpumask might race with a concurrent update.
+	 *
+	 * The watchdog time stamp can race against a concurrent real
+	 * update as well, the only side effect might be a cycle delay for
+	 * the softlockup check.
 	 */
-	for_each_watchdog_cpu(cpu)
+	for_each_cpu(cpu, &watchdog_allowed_mask)
 		per_cpu(watchdog_touch_ts, cpu) = 0;
 	wq_watchdog_touch(-1);
 }
@@ -303,9 +296,6 @@ static void watchdog_interrupt_count(void)
 	__this_cpu_inc(hrtimer_interrupts);
 }
 
-static int watchdog_enable_all_cpus(void);
-static void watchdog_disable_all_cpus(void);
-
 /* watchdog kicker functions */
 static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 {
@@ -498,95 +488,6 @@ static struct smp_hotplug_thread watchdog_threads = {
 	.unpark			= watchdog_enable,
 };
 
-/*
- * park all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function returns an error if kthread_park() of a watchdog thread
- * fails. In this situation, the watchdog threads of some CPUs can already
- * be parked and the watchdog threads of other CPUs can still be runnable.
- * Callers are expected to handle this special condition as appropriate in
- * their context.
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static int watchdog_park_threads(void)
-{
-	int cpu, ret = 0;
-
-	for_each_watchdog_cpu(cpu) {
-		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
-/*
- * unpark all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static void watchdog_unpark_threads(void)
-{
-	int cpu;
-
-	for_each_watchdog_cpu(cpu)
-		kthread_unpark(per_cpu(softlockup_watchdog, cpu));
-}
-
-static int update_watchdog_all_cpus(void)
-{
-	int ret;
-
-	ret = watchdog_park_threads();
-	if (ret)
-		return ret;
-
-	watchdog_unpark_threads();
-
-	return 0;
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	int err = 0;
-
-	if (!watchdog_running) {
-		err = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
-							     &watchdog_cpumask);
-		if (err)
-			pr_err("Failed to create watchdog threads, disabled\n");
-		else
-			watchdog_running = 1;
-	} else {
-		/*
-		 * Enable/disable the lockup detectors or
-		 * change the sample period 'on the fly'.
-		 */
-		err = update_watchdog_all_cpus();
-
-		if (err) {
-			watchdog_disable_all_cpus();
-			pr_err("Failed to update lockup detectors, disabled\n");
-		}
-	}
-
-	if (err)
-		watchdog_enabled = 0;
-
-	return err;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-	if (watchdog_running) {
-		watchdog_running = 0;
-		smpboot_unregister_percpu_thread(&watchdog_threads);
-	}
-}
-
 static void softlockup_update_smpboot_threads(void)
 {
 	lockdep_assert_held(&watchdog_mutex);
@@ -661,7 +562,6 @@ static inline int watchdog_park_threads(void) { return 0; }
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
-static inline void set_sample_period(void) { }
 static inline void softlockup_init_threads(void) { }
 static inline void softlockup_update_threads(void) { }
 static inline void softlockup_reconfigure_threads(bool enabled) { }
@@ -701,28 +601,10 @@ void lockup_detector_soft_poweroff(void)
 /*
  * Update the run state of the lockup detectors.
  */
-static int proc_watchdog_update(void)
+static void proc_watchdog_update(void)
 {
-	int err = 0;
-
-	/*
-	 * 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
-	 * period will be updated and the lockup detectors will be enabled
-	 * or disabled 'on the fly'.
-	 */
-	if (watchdog_enabled && watchdog_thresh)
-		err = watchdog_enable_all_cpus();
-	else
-		watchdog_disable_all_cpus();
-
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
 	watchdog_nmi_reconfigure();
-
-	__lockup_detector_cleanup();
-
-	return err;
-
 }
 
 /*
@@ -778,17 +660,8 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 				new = old & ~which;
 		} while (cmpxchg(&watchdog_enabled, old, new) != old);
 
-		/*
-		 * Update the run state of the lockup detectors. There is _no_
-		 * need to check the value returned by proc_watchdog_update()
-		 * and to restore the previous value of 'watchdog_enabled' as
-		 * both lockup detectors are disabled if proc_watchdog_update()
-		 * returns an error.
-		 */
-		if (old == new)
-			goto out;
-
-		err = proc_watchdog_update();
+		if (old != new)
+			proc_watchdog_update();
 	}
 out:
 	mutex_unlock(&watchdog_mutex);
@@ -832,50 +705,28 @@ int proc_soft_watchdog(struct ctl_table *table, int write,
 int proc_watchdog_thresh(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old, new;
+	int err, old;
 
 	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
-	old = ACCESS_ONCE(watchdog_thresh);
+	old = READ_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
-	if (err || !write)
-		goto out;
+	if (!err && write && old != READ_ONCE(watchdog_thresh))
+		proc_watchdog_update();
 
-	/*
-	 * Update the sample period. Restore on failure.
-	 */
-	new = ACCESS_ONCE(watchdog_thresh);
-	if (old == new)
-		goto out;
-
-	set_sample_period();
-	err = proc_watchdog_update();
-	if (err) {
-		watchdog_thresh = old;
-		set_sample_period();
-	}
-out:
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
 
-static void watchdog_update_cpus(void)
-{
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
-		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-						     &watchdog_cpumask);
-		__lockup_detector_cleanup();
-	}
-}
-
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-	watchdog_update_cpus();
+
+	softlockup_update_threads();
 	watchdog_nmi_reconfigure();
 }
 
@@ -905,8 +756,6 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 
 void __init lockup_detector_init(void)
 {
-	set_sample_period();
-
 #ifdef CONFIG_NO_HZ_FULL
 	if (tick_nohz_full_enabled()) {
 		pr_info("Disabling watchdog on nohz_full cores by default\n");
@@ -917,6 +766,5 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
-	if (watchdog_enabled)
-		watchdog_enable_all_cpus();
+	softlockup_init_threads();
 }
#36 lore raw

PATCH v2 18/29 lockup_detector: Further simplify sysctl handling

In reply to Thomas Gleixner

Use a single function to update sysctl changes. This is not a high frequency user space interface and it's root only.

Preparatory patch to cleanup the sysctl variable handling.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
kernel/watchdog.c +7 -20
@@ -507,11 +507,8 @@ static void softlockup_park_all_threads(
 	softlockup_update_smpboot_threads();
 }
 
-/*
- * Park threads which are not longer enabled and unpark threads which have
- * been newly enabled.
- */
-static void softlockup_update_threads(void)
+/* Unpark enabled threads */
+static void softlockup_unpark_threads(void)
 {
 	cpumask_copy(&watchdog_allowed_mask, &watchdog_cpumask);
 	softlockup_update_smpboot_threads();
@@ -522,7 +519,7 @@ static void softlockup_reconfigure_threa
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
-		softlockup_update_threads();
+		softlockup_unpark_threads();
 }
 
 /*
@@ -563,7 +560,6 @@ static inline void watchdog_unpark_threa
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_update_threads(void) { }
 static inline void softlockup_reconfigure_threads(bool enabled) { }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
@@ -598,11 +594,11 @@ void lockup_detector_soft_poweroff(void)
 
 #ifdef CONFIG_SYSCTL
 
-/*
- * Update the run state of the lockup detectors.
- */
+/* Propagate any changes to the watchdog threads */
 static void proc_watchdog_update(void)
 {
+	/* Remove impossible cpus to keep sysctl output clean. */
+	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
 	watchdog_nmi_reconfigure();
 }
@@ -721,15 +717,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 	return err;
 }
 
-static void proc_watchdog_cpumask_update(void)
-{
-	/* Remove impossible cpus to keep sysctl output clean. */
-	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-
-	softlockup_update_threads();
-	watchdog_nmi_reconfigure();
-}
-
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
@@ -746,7 +733,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write)
-		proc_watchdog_cpumask_update();
+		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
#37 lore raw

[tip:core/urgent] watchdog/core: Further simplify sysctl handling

watchdog/core: Further simplify sysctl handling
Commit-ID:  e8b62b2dd14f8f2427856ba24cb7db922bda9bfd
Gitweb:     http://git.kernel.org/tip/e8b62b2dd14f8f2427856ba24cb7db922bda9bfd
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:12 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:06 +0200

watchdog/core: Further simplify sysctl handling

Use a single function to update sysctl changes. This is not a high frequency user space interface and it's root only.

Preparatory patch to cleanup the sysctl variable handling.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
kernel/watchdog.c +7 -20
index 762d3ed..ca87472 100644
@@ -507,11 +507,8 @@ static void softlockup_park_all_threads(void)
 	softlockup_update_smpboot_threads();
 }
 
-/*
- * Park threads which are not longer enabled and unpark threads which have
- * been newly enabled.
- */
-static void softlockup_update_threads(void)
+/* Unpark enabled threads */
+static void softlockup_unpark_threads(void)
 {
 	cpumask_copy(&watchdog_allowed_mask, &watchdog_cpumask);
 	softlockup_update_smpboot_threads();
@@ -522,7 +519,7 @@ static void softlockup_reconfigure_threads(bool enabled)
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
-		softlockup_update_threads();
+		softlockup_unpark_threads();
 }
 
 /*
@@ -563,7 +560,6 @@ static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_update_threads(void) { }
 static inline void softlockup_reconfigure_threads(bool enabled) { }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
@@ -598,11 +594,11 @@ void lockup_detector_soft_poweroff(void)
 
 #ifdef CONFIG_SYSCTL
 
-/*
- * Update the run state of the lockup detectors.
- */
+/* Propagate any changes to the watchdog threads */
 static void proc_watchdog_update(void)
 {
+	/* Remove impossible cpus to keep sysctl output clean. */
+	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
 	watchdog_nmi_reconfigure();
 }
@@ -721,15 +717,6 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	return err;
 }
 
-static void proc_watchdog_cpumask_update(void)
-{
-	/* Remove impossible cpus to keep sysctl output clean. */
-	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-
-	softlockup_update_threads();
-	watchdog_nmi_reconfigure();
-}
-
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the
@@ -746,7 +733,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write)
-		proc_watchdog_cpumask_update();
+		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
#38 lore raw

PATCH v2 19/29 lockup_detector: Cleanup header mess

In reply to Thomas Gleixner

Having the same #ifdef in various places does not make it more readable. Collect stuff into one place.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h |   60 +++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)
include/linux/nmi.h +25 -35
@@ -14,11 +14,29 @@
 void lockup_detector_init(void);
 void lockup_detector_soft_poweroff(void);
 void lockup_detector_cleanup(void);
+bool is_hardlockup(void);
+
+extern int watchdog_user_enabled;
+extern int nmi_watchdog_enabled;
+extern int soft_watchdog_enabled;
+extern int watchdog_thresh;
+extern unsigned long watchdog_enabled;
+
+extern struct cpumask watchdog_cpumask;
+extern unsigned long *watchdog_cpumask_bits;
+#ifdef CONFIG_SMP
+extern int sysctl_softlockup_all_cpu_backtrace;
+extern int sysctl_hardlockup_all_cpu_backtrace;
 #else
+#define sysctl_softlockup_all_cpu_backtrace 0
+#define sysctl_hardlockup_all_cpu_backtrace 0
+#endif /* !CONFIG_SMP */
+
+#else /* CONFIG_LOCKUP_DETECTOR */
 static inline void lockup_detector_init(void) { }
 static inline void lockup_detector_soft_poweroff(void) { }
 static inline void lockup_detector_cleanup(void) { }
-#endif
+#endif /* !CONFIG_LOCKUP_DETECTOR */
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 extern void touch_softlockup_watchdog_sched(void);
@@ -26,28 +44,17 @@ extern void touch_softlockup_watchdog(vo
 extern void touch_softlockup_watchdog_sync(void);
 extern void touch_all_softlockup_watchdogs(void);
 extern unsigned int  softlockup_panic;
-extern int soft_watchdog_enabled;
 #else
-static inline void touch_softlockup_watchdog_sched(void)
-{
-}
-static inline void touch_softlockup_watchdog(void)
-{
-}
-static inline void touch_softlockup_watchdog_sync(void)
-{
-}
-static inline void touch_all_softlockup_watchdogs(void)
-{
-}
+static inline void touch_softlockup_watchdog_sched(void) { }
+static inline void touch_softlockup_watchdog(void) { }
+static inline void touch_softlockup_watchdog_sync(void) { }
+static inline void touch_all_softlockup_watchdogs(void) { }
 #endif
 
 #ifdef CONFIG_DETECT_HUNG_TASK
 void reset_hung_task_detector(void);
 #else
-static inline void reset_hung_task_detector(void)
-{
-}
+static inline void reset_hung_task_detector(void) { }
 #endif
 
 /*
@@ -92,7 +99,7 @@ static inline void arch_touch_nmi_watchd
 
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
- * 
+ *
  * If the architecture supports the NMI watchdog, touch_nmi_watchdog()
  * may be used to reset the timeout - for code which intentionally
  * disables interrupts for a long time. This call is stateless.
@@ -162,21 +169,6 @@ static inline bool trigger_single_cpu_ba
 u64 hw_nmi_get_sample_period(int watchdog_thresh);
 #endif
 
-#ifdef CONFIG_LOCKUP_DETECTOR
-extern int nmi_watchdog_enabled;
-extern int watchdog_user_enabled;
-extern int watchdog_thresh;
-extern unsigned long watchdog_enabled;
-extern struct cpumask watchdog_cpumask;
-extern unsigned long *watchdog_cpumask_bits;
-#ifdef CONFIG_SMP
-extern int sysctl_softlockup_all_cpu_backtrace;
-extern int sysctl_hardlockup_all_cpu_backtrace;
-#else
-#define sysctl_softlockup_all_cpu_backtrace 0
-#define sysctl_hardlockup_all_cpu_backtrace 0
-#endif
-
 #if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \
     defined(CONFIG_HARDLOCKUP_DETECTOR)
 void watchdog_update_hrtimer_threshold(u64 period);
@@ -184,7 +176,6 @@ void watchdog_update_hrtimer_threshold(u
 static inline void watchdog_update_hrtimer_threshold(u64 period) { }
 #endif
 
-extern bool is_hardlockup(void);
 struct ctl_table;
 extern int proc_watchdog(struct ctl_table *, int ,
 			 void __user *, size_t *, loff_t *);
@@ -196,7 +187,6 @@ extern int proc_watchdog_thresh(struct c
 				void __user *, size_t *, loff_t *);
 extern int proc_watchdog_cpumask(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
-#endif
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 #include <asm/nmi.h>
#39 lore raw

[tip:core/urgent] watchdog/core: Clean up header mess

watchdog/core: Clean up header mess
Commit-ID:  3b371b5936e7777c819619c00ca60f196a8e13fa
Gitweb:     http://git.kernel.org/tip/3b371b5936e7777c819619c00ca60f196a8e13fa
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:13 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:07 +0200

watchdog/core: Clean up header mess

Having the same #ifdef in various places does not make it more readable. Collect stuff into one place.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h | 60 ++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)
include/linux/nmi.h +25 -35
index 91a3a4a..cfebb3b 100644
@@ -14,11 +14,29 @@
 void lockup_detector_init(void);
 void lockup_detector_soft_poweroff(void);
 void lockup_detector_cleanup(void);
+bool is_hardlockup(void);
+
+extern int watchdog_user_enabled;
+extern int nmi_watchdog_enabled;
+extern int soft_watchdog_enabled;
+extern int watchdog_thresh;
+extern unsigned long watchdog_enabled;
+
+extern struct cpumask watchdog_cpumask;
+extern unsigned long *watchdog_cpumask_bits;
+#ifdef CONFIG_SMP
+extern int sysctl_softlockup_all_cpu_backtrace;
+extern int sysctl_hardlockup_all_cpu_backtrace;
 #else
+#define sysctl_softlockup_all_cpu_backtrace 0
+#define sysctl_hardlockup_all_cpu_backtrace 0
+#endif /* !CONFIG_SMP */
+
+#else /* CONFIG_LOCKUP_DETECTOR */
 static inline void lockup_detector_init(void) { }
 static inline void lockup_detector_soft_poweroff(void) { }
 static inline void lockup_detector_cleanup(void) { }
-#endif
+#endif /* !CONFIG_LOCKUP_DETECTOR */
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 extern void touch_softlockup_watchdog_sched(void);
@@ -26,28 +44,17 @@ extern void touch_softlockup_watchdog(void);
 extern void touch_softlockup_watchdog_sync(void);
 extern void touch_all_softlockup_watchdogs(void);
 extern unsigned int  softlockup_panic;
-extern int soft_watchdog_enabled;
 #else
-static inline void touch_softlockup_watchdog_sched(void)
-{
-}
-static inline void touch_softlockup_watchdog(void)
-{
-}
-static inline void touch_softlockup_watchdog_sync(void)
-{
-}
-static inline void touch_all_softlockup_watchdogs(void)
-{
-}
+static inline void touch_softlockup_watchdog_sched(void) { }
+static inline void touch_softlockup_watchdog(void) { }
+static inline void touch_softlockup_watchdog_sync(void) { }
+static inline void touch_all_softlockup_watchdogs(void) { }
 #endif
 
 #ifdef CONFIG_DETECT_HUNG_TASK
 void reset_hung_task_detector(void);
 #else
-static inline void reset_hung_task_detector(void)
-{
-}
+static inline void reset_hung_task_detector(void) { }
 #endif
 
 /*
@@ -92,7 +99,7 @@ static inline void arch_touch_nmi_watchdog(void) {}
 
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
- * 
+ *
  * If the architecture supports the NMI watchdog, touch_nmi_watchdog()
  * may be used to reset the timeout - for code which intentionally
  * disables interrupts for a long time. This call is stateless.
@@ -162,21 +169,6 @@ static inline bool trigger_single_cpu_backtrace(int cpu)
 u64 hw_nmi_get_sample_period(int watchdog_thresh);
 #endif
 
-#ifdef CONFIG_LOCKUP_DETECTOR
-extern int nmi_watchdog_enabled;
-extern int watchdog_user_enabled;
-extern int watchdog_thresh;
-extern unsigned long watchdog_enabled;
-extern struct cpumask watchdog_cpumask;
-extern unsigned long *watchdog_cpumask_bits;
-#ifdef CONFIG_SMP
-extern int sysctl_softlockup_all_cpu_backtrace;
-extern int sysctl_hardlockup_all_cpu_backtrace;
-#else
-#define sysctl_softlockup_all_cpu_backtrace 0
-#define sysctl_hardlockup_all_cpu_backtrace 0
-#endif
-
 #if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \
     defined(CONFIG_HARDLOCKUP_DETECTOR)
 void watchdog_update_hrtimer_threshold(u64 period);
@@ -184,7 +176,6 @@ void watchdog_update_hrtimer_threshold(u64 period);
 static inline void watchdog_update_hrtimer_threshold(u64 period) { }
 #endif
 
-extern bool is_hardlockup(void);
 struct ctl_table;
 extern int proc_watchdog(struct ctl_table *, int ,
 			 void __user *, size_t *, loff_t *);
@@ -196,7 +187,6 @@ extern int proc_watchdog_thresh(struct ctl_table *, int ,
 				void __user *, size_t *, loff_t *);
 extern int proc_watchdog_cpumask(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
-#endif
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 #include <asm/nmi.h>
#40 lore raw

PATCH v2 20/29 lockup_detector/sysctl: Get rid of the ifdeffery

In reply to Thomas Gleixner

The sysctl of the nmi_watchdog file prevents writes by setting
min = max = 0

if none of the users is enabled. That involves ifdeffery and is competely non obvious.

If none of the facilities is enabeld, then the file can simply be made read only. Move the ifdeffery into the header and use a constant for file permissions.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h |    6 ++++++
 kernel/sysctl.c     |    6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)
include/linux/nmi.h +6 -0
@@ -81,6 +81,12 @@ extern unsigned int hardlockup_panic;
 static inline void hardlockup_detector_disable(void) {}
 #endif
 
+#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
+# define NMI_WATCHDOG_SYSCTL_PERM	0644
+#else
+# define NMI_WATCHDOG_SYSCTL_PERM	0444
+#endif
+
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
kernel/sysctl.c +1 -5
@@ -891,14 +891,10 @@ static struct ctl_table kern_table[] = {
 		.procname       = "nmi_watchdog",
 		.data           = &nmi_watchdog_enabled,
 		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
 		.proc_handler   = proc_nmi_watchdog,
 		.extra1		= &zero,
-#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
 		.extra2		= &one,
-#else
-		.extra2		= &zero,
-#endif
 	},
 	{
 		.procname	= "watchdog_cpumask",
#41 lore raw

[tip:core/urgent] watchdog/sysctl: Get rid of the #ifdeffery

watchdog/sysctl: Get rid of the #ifdeffery
Commit-ID:  51d4052b01ca555e0d1d5fe297b309beb6c64aa0
Gitweb:     http://git.kernel.org/tip/51d4052b01ca555e0d1d5fe297b309beb6c64aa0
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:14 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:07 +0200

watchdog/sysctl: Get rid of the #ifdeffery

The sysctl of the nmi_watchdog file prevents writes by setting:

min = max = 0

if none of the users is enabled. That involves ifdeffery and is competely non obvious.

If none of the facilities is enabeld, then the file can simply be made read only. Move the ifdeffery into the header and use a constant for file permissions.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h | 6 ++++++
 kernel/sysctl.c     | 6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)
include/linux/nmi.h +6 -0
index cfebb3b..5774b443 100644
@@ -81,6 +81,12 @@ extern unsigned int hardlockup_panic;
 static inline void hardlockup_detector_disable(void) {}
 #endif
 
+#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
+# define NMI_WATCHDOG_SYSCTL_PERM	0644
+#else
+# define NMI_WATCHDOG_SYSCTL_PERM	0444
+#endif
+
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
kernel/sysctl.c +1 -5
index 6648fbb..539cb4e 100644
@@ -891,14 +891,10 @@ static struct ctl_table kern_table[] = {
 		.procname       = "nmi_watchdog",
 		.data           = &nmi_watchdog_enabled,
 		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
 		.proc_handler   = proc_nmi_watchdog,
 		.extra1		= &zero,
-#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
 		.extra2		= &one,
-#else
-		.extra2		= &zero,
-#endif
 	},
 	{
 		.procname	= "watchdog_cpumask",
#42 lore raw

PATCH v2 21/29 lockup_detector: Cleanup sysctl variable name space

In reply to Thomas Gleixner

Reflect that these variables are user interface related and remove the whitespace damage in the sysctl table while at it.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h |   16 ++++++++--------
 kernel/sysctl.c     |   16 ++++++++--------
 kernel/watchdog.c   |   41 ++++++++++++++++++++---------------------
 3 files changed, 36 insertions(+), 37 deletions(-)
include/linux/nmi.h +8 -8
@@ -17,8 +17,8 @@ void lockup_detector_cleanup(void);
 bool is_hardlockup(void);
 
 extern int watchdog_user_enabled;
-extern int nmi_watchdog_enabled;
-extern int soft_watchdog_enabled;
+extern int nmi_watchdog_user_enabled;
+extern int soft_watchdog_user_enabled;
 extern int watchdog_thresh;
 extern unsigned long watchdog_enabled;
 
@@ -62,12 +62,12 @@ static inline void reset_hung_task_detec
  * 'watchdog_enabled' variable. Each lockup detector has its dedicated bit -
  * bit 0 for the hard lockup detector and bit 1 for the soft lockup detector.
  *
- * 'watchdog_user_enabled', 'nmi_watchdog_enabled' and 'soft_watchdog_enabled'
- * are variables that are only used as an 'interface' between the parameters
- * in /proc/sys/kernel and the internal state bits in 'watchdog_enabled'. The
- * 'watchdog_thresh' variable is handled differently because its value is not
- * boolean, and the lockup detectors are 'suspended' while 'watchdog_thresh'
- * is equal zero.
+ * 'watchdog_user_enabled', 'nmi_watchdog_user_enabled' and
+ * 'soft_watchdog_user_enabled' are variables that are only used as an
+ * 'interface' between the parameters in /proc/sys/kernel and the internal
+ * state bits in 'watchdog_enabled'. The 'watchdog_thresh' variable is
+ * handled differently because its value is not boolean, and the lockup
+ * detectors are 'suspended' while 'watchdog_thresh' is equal zero.
  */
 #define NMI_WATCHDOG_ENABLED_BIT   0
 #define SOFT_WATCHDOG_ENABLED_BIT  1
kernel/sysctl.c +8 -8
@@ -871,9 +871,9 @@ static struct ctl_table kern_table[] = {
 #if defined(CONFIG_LOCKUP_DETECTOR)
 	{
 		.procname       = "watchdog",
-		.data           = &watchdog_user_enabled,
-		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.data		= &watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
 		.proc_handler   = proc_watchdog,
 		.extra1		= &zero,
 		.extra2		= &one,
@@ -889,8 +889,8 @@ static struct ctl_table kern_table[] = {
 	},
 	{
 		.procname       = "nmi_watchdog",
-		.data           = &nmi_watchdog_enabled,
-		.maxlen         = sizeof (int),
+		.data		= &nmi_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
 		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
 		.proc_handler   = proc_nmi_watchdog,
 		.extra1		= &zero,
@@ -906,9 +906,9 @@ static struct ctl_table kern_table[] = {
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 	{
 		.procname       = "soft_watchdog",
-		.data           = &soft_watchdog_enabled,
-		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.data		= &soft_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
 		.proc_handler   = proc_soft_watchdog,
 		.extra1		= &zero,
 		.extra2		= &one,
kernel/watchdog.c +20 -21

77 lines not on this page · raw message

#43 lore raw

[tip:core/urgent] watchdog/sysctl: Clean up sysctl variable name space

watchdog/sysctl: Clean up sysctl variable name space
Commit-ID:  7feeb9cd4f5b34476ffb9e6d58d58c5416375b19
Gitweb:     http://git.kernel.org/tip/7feeb9cd4f5b34476ffb9e6d58d58c5416375b19
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:15 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:07 +0200

watchdog/sysctl: Clean up sysctl variable name space

Reflect that these variables are user interface related and remove the whitespace damage in the sysctl table while at it.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h | 16 ++++++++--------
 kernel/sysctl.c     | 16 ++++++++--------
 kernel/watchdog.c   | 41 ++++++++++++++++++++---------------------
 3 files changed, 36 insertions(+), 37 deletions(-)
include/linux/nmi.h +8 -8
index 5774b443..4a8d103 100644
@@ -17,8 +17,8 @@ void lockup_detector_cleanup(void);
 bool is_hardlockup(void);
 
 extern int watchdog_user_enabled;
-extern int nmi_watchdog_enabled;
-extern int soft_watchdog_enabled;
+extern int nmi_watchdog_user_enabled;
+extern int soft_watchdog_user_enabled;
 extern int watchdog_thresh;
 extern unsigned long watchdog_enabled;
 
@@ -62,12 +62,12 @@ static inline void reset_hung_task_detector(void) { }
  * 'watchdog_enabled' variable. Each lockup detector has its dedicated bit -
  * bit 0 for the hard lockup detector and bit 1 for the soft lockup detector.
  *
- * 'watchdog_user_enabled', 'nmi_watchdog_enabled' and 'soft_watchdog_enabled'
- * are variables that are only used as an 'interface' between the parameters
- * in /proc/sys/kernel and the internal state bits in 'watchdog_enabled'. The
- * 'watchdog_thresh' variable is handled differently because its value is not
- * boolean, and the lockup detectors are 'suspended' while 'watchdog_thresh'
- * is equal zero.
+ * 'watchdog_user_enabled', 'nmi_watchdog_user_enabled' and
+ * 'soft_watchdog_user_enabled' are variables that are only used as an
+ * 'interface' between the parameters in /proc/sys/kernel and the internal
+ * state bits in 'watchdog_enabled'. The 'watchdog_thresh' variable is
+ * handled differently because its value is not boolean, and the lockup
+ * detectors are 'suspended' while 'watchdog_thresh' is equal zero.
  */
 #define NMI_WATCHDOG_ENABLED_BIT   0
 #define SOFT_WATCHDOG_ENABLED_BIT  1
kernel/sysctl.c +8 -8
index 539cb4e..4c08ed4 100644
@@ -871,9 +871,9 @@ static struct ctl_table kern_table[] = {
 #if defined(CONFIG_LOCKUP_DETECTOR)
 	{
 		.procname       = "watchdog",
-		.data           = &watchdog_user_enabled,
-		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.data		= &watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
 		.proc_handler   = proc_watchdog,
 		.extra1		= &zero,
 		.extra2		= &one,
@@ -889,8 +889,8 @@ static struct ctl_table kern_table[] = {
 	},
 	{
 		.procname       = "nmi_watchdog",
-		.data           = &nmi_watchdog_enabled,
-		.maxlen         = sizeof (int),
+		.data		= &nmi_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
 		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
 		.proc_handler   = proc_nmi_watchdog,
 		.extra1		= &zero,
@@ -906,9 +906,9 @@ static struct ctl_table kern_table[] = {
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 	{
 		.procname       = "soft_watchdog",
-		.data           = &soft_watchdog_enabled,
-		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.data		= &soft_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
 		.proc_handler   = proc_soft_watchdog,
 		.extra1		= &zero,
 		.extra2		= &one,
kernel/watchdog.c +20 -21

77 lines not on this page · raw message

#44 lore raw

PATCH v2 22/29 lockup_detector: Make watchdog_nmi_reconfigure() two stage

In reply to Thomas Gleixner

Both the perf reconfiguration and the powerpc watchdog_nmi_reconfigure() need to be done in two steps.

1) Stop all NMIs
2) Read the new parameters and start NMIs

Right now watchdog_nmi_reconfigure() is a combination of both. To allow a clean reconfiguration add a 'run' argument and split the functionality in powerpc.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Andrew Morton <[email protected]>
---
 arch/powerpc/kernel/watchdog.c |   17 +++++++++--------
 include/linux/nmi.h            |    2 ++
 kernel/watchdog.c              |   31 ++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 17 deletions(-)
arch/powerpc/kernel/watchdog.c +9 -8
@@ -355,17 +355,18 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(void)
+void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
-	watchdog_calc_timeouts();
-
-	for_each_cpu(cpu, &wd_cpus_enabled)
-		stop_wd_on_cpu(cpu);
-
-	for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
-		start_wd_on_cpu(cpu);
+	if (!run) {
+		for_each_cpu(cpu, &wd_cpus_enabled)
+			stop_wd_on_cpu(cpu);
+	} else {
+		watchdog_calc_timeouts();
+		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
+			start_wd_on_cpu(cpu);
+	}
 }
 
 /*
include/linux/nmi.h +2 -0
@@ -103,6 +103,8 @@ static inline void arch_touch_nmi_watchd
 #endif
 #endif
 
+void watchdog_nmi_reconfigure(bool run);
+
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
  *
kernel/watchdog.c +22 -9
@@ -112,17 +112,25 @@ void __weak watchdog_nmi_disable(unsigne
 	hardlockup_detector_perf_disable();
 }
 
-/*
- * watchdog_nmi_reconfigure can be implemented to be notified after any
- * watchdog configuration change. The arch hardlockup watchdog should
- * respond to the following variables:
+/**
+ * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
+ * @run:	If false stop the watchdogs on all enabled CPUs
+ *		If true start the watchdogs on all enabled CPUs
+ *
+ * The core call order is:
+ * watchdog_nmi_reconfigure(false);
+ * update_variables();
+ * watchdog_nmi_reconfigure(true);
+ *
+ * The second call which starts the watchdogs again guarantees that the
+ * following variables are stable across the call.
  * - watchdog_enabled
  * - watchdog_thresh
  * - watchdog_cpumask
- * - sysctl_hardlockup_all_cpu_backtrace
- * - hardlockup_panic
+ *
+ * After the call the variables can be changed again.
  */
-void __weak watchdog_nmi_reconfigure(void) { }
+void __weak watchdog_nmi_reconfigure(bool run) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -515,10 +523,12 @@ static void softlockup_unpark_threads(vo
 
 static void softlockup_reconfigure_threads(bool enabled)
 {
+	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
 		softlockup_unpark_threads();
+	watchdog_nmi_reconfigure(true);
 }
 
 /*
@@ -559,7 +569,11 @@ static inline void watchdog_unpark_threa
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_reconfigure_threads(bool enabled) { }
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	watchdog_nmi_reconfigure(false);
+	watchdog_nmi_reconfigure(true);
+}
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
@@ -599,7 +613,6 @@ static void proc_watchdog_update(void)
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
-	watchdog_nmi_reconfigure();
 }
 
 /*
#45 lore raw

[tip:core/urgent] watchdog/core, powerpc: Make watchdog_nmi_reconfigure() two stage

watchdog/core, powerpc: Make watchdog_nmi_reconfigure() two stage
Commit-ID:  6592ad2fcc8f15b4f99b36c1db7d9f65510c203b
Gitweb:     http://git.kernel.org/tip/6592ad2fcc8f15b4f99b36c1db7d9f65510c203b
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:16 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:07 +0200

watchdog/core, powerpc: Make watchdog_nmi_reconfigure() two stage

Both the perf reconfiguration and the powerpc watchdog_nmi_reconfigure() need to be done in two steps.

1) Stop all NMIs
2) Read the new parameters and start NMIs

Right now watchdog_nmi_reconfigure() is a combination of both. To allow a clean reconfiguration add a 'run' argument and split the functionality in powerpc.

Cc Andrew Morton <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 arch/powerpc/kernel/watchdog.c | 17 +++++++++--------
 include/linux/nmi.h            |  2 ++
 kernel/watchdog.c              | 31 ++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 17 deletions(-)
arch/powerpc/kernel/watchdog.c +9 -8
index 5ded171..291af79 100644
@@ -355,17 +355,18 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(void)
+void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
-	watchdog_calc_timeouts();
-
-	for_each_cpu(cpu, &wd_cpus_enabled)
-		stop_wd_on_cpu(cpu);
-
-	for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
-		start_wd_on_cpu(cpu);
+	if (!run) {
+		for_each_cpu(cpu, &wd_cpus_enabled)
+			stop_wd_on_cpu(cpu);
+	} else {
+		watchdog_calc_timeouts();
+		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
+			start_wd_on_cpu(cpu);
+	}
 }
 
 /*
include/linux/nmi.h +2 -0
index 4a8d103..eee255b 100644
@@ -103,6 +103,8 @@ static inline void arch_touch_nmi_watchdog(void) {}
 #endif
 #endif
 
+void watchdog_nmi_reconfigure(bool run);
+
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
  *
kernel/watchdog.c +22 -9
index baae9fc..5693afd 100644
@@ -112,17 +112,25 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
 	hardlockup_detector_perf_disable();
 }
 
-/*
- * watchdog_nmi_reconfigure can be implemented to be notified after any
- * watchdog configuration change. The arch hardlockup watchdog should
- * respond to the following variables:
+/**
+ * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
+ * @run:	If false stop the watchdogs on all enabled CPUs
+ *		If true start the watchdogs on all enabled CPUs
+ *
+ * The core call order is:
+ * watchdog_nmi_reconfigure(false);
+ * update_variables();
+ * watchdog_nmi_reconfigure(true);
+ *
+ * The second call which starts the watchdogs again guarantees that the
+ * following variables are stable across the call.
  * - watchdog_enabled
  * - watchdog_thresh
  * - watchdog_cpumask
- * - sysctl_hardlockup_all_cpu_backtrace
- * - hardlockup_panic
+ *
+ * After the call the variables can be changed again.
  */
-void __weak watchdog_nmi_reconfigure(void) { }
+void __weak watchdog_nmi_reconfigure(bool run) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -515,10 +523,12 @@ static void softlockup_unpark_threads(void)
 
 static void softlockup_reconfigure_threads(bool enabled)
 {
+	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
 		softlockup_unpark_threads();
+	watchdog_nmi_reconfigure(true);
 }
 
 /*
@@ -559,7 +569,11 @@ static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_reconfigure_threads(bool enabled) { }
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	watchdog_nmi_reconfigure(false);
+	watchdog_nmi_reconfigure(true);
+}
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
@@ -599,7 +613,6 @@ static void proc_watchdog_update(void)
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
-	watchdog_nmi_reconfigure();
 }
 
 /*
#46 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

In reply to Thomas Gleixner

Hi Thomas,

Quoted 24 lines · Thomas Gleixner

Both the perf reconfiguration and the powerpc watchdog_nmi_reconfigure() need to be done in two steps.

1) Stop all NMIs
2) Read the new parameters and start NMIs

Right now watchdog_nmi_reconfigure() is a combination of both. To allow a clean reconfiguration add a 'run' argument and split the functionality in powerpc.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Andrew Morton <[email protected]>
Quoted 5 lines
---
 arch/powerpc/kernel/watchdog.c |   17 +++++++++--------
 include/linux/nmi.h            |    2 ++
 kernel/watchdog.c              |   31 ++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 17 deletions(-)

Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

I'm not sure what the easiest fix is. One option would be to just drop the WARN_ON, it's just there for paranoia AFAICS.

cheers

Quoted 110 lines
arch/powerpc/kernel/watchdog.c +9 -8
@@ -355,17 +355,18 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(void)
+void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
-	watchdog_calc_timeouts();
-
-	for_each_cpu(cpu, &wd_cpus_enabled)
-		stop_wd_on_cpu(cpu);
-
-	for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
-		start_wd_on_cpu(cpu);
+	if (!run) {
+		for_each_cpu(cpu, &wd_cpus_enabled)
+			stop_wd_on_cpu(cpu);
+	} else {
+		watchdog_calc_timeouts();
+		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
+			start_wd_on_cpu(cpu);
+	}
 }
 
 /*
include/linux/nmi.h +2 -0
@@ -103,6 +103,8 @@ static inline void arch_touch_nmi_watchd
 #endif
 #endif
 
+void watchdog_nmi_reconfigure(bool run);
+
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
  *
kernel/watchdog.c +22 -9
@@ -112,17 +112,25 @@ void __weak watchdog_nmi_disable(unsigne
 	hardlockup_detector_perf_disable();
 }
 
-/*
- * watchdog_nmi_reconfigure can be implemented to be notified after any
- * watchdog configuration change. The arch hardlockup watchdog should
- * respond to the following variables:
+/**
+ * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
+ * @run:	If false stop the watchdogs on all enabled CPUs
+ *		If true start the watchdogs on all enabled CPUs
+ *
+ * The core call order is:
+ * watchdog_nmi_reconfigure(false);
+ * update_variables();
+ * watchdog_nmi_reconfigure(true);
+ *
+ * The second call which starts the watchdogs again guarantees that the
+ * following variables are stable across the call.
  * - watchdog_enabled
  * - watchdog_thresh
  * - watchdog_cpumask
- * - sysctl_hardlockup_all_cpu_backtrace
- * - hardlockup_panic
+ *
+ * After the call the variables can be changed again.
  */
-void __weak watchdog_nmi_reconfigure(void) { }
+void __weak watchdog_nmi_reconfigure(bool run) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -515,10 +523,12 @@ static void softlockup_unpark_threads(vo
 
 static void softlockup_reconfigure_threads(bool enabled)
 {
+	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
 		softlockup_unpark_threads();
+	watchdog_nmi_reconfigure(true);
 }
 
 /*
@@ -559,7 +569,11 @@ static inline void watchdog_unpark_threa
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_reconfigure_threads(bool enabled) { }
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	watchdog_nmi_reconfigure(false);
+	watchdog_nmi_reconfigure(true);
+}
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
@@ -599,7 +613,6 @@ static void proc_watchdog_update(void)
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
-	watchdog_nmi_reconfigure();
 }
 
 /*
#47 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 24 lines · Michael Ellerman

Hi Thomas,
Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

Quoted 2 lines

I'm not sure what the easiest fix is. One option would be to just drop the WARN_ON, it's just there for paranoia AFAICS.

The straight forward way is to make use of the new probe function. Patch below.

Thanks,

tglx

8<------------------

arch/powerpc/kernel/watchdog.c +4 -6
@@ -375,20 +375,18 @@ void watchdog_nmi_start(void)
 /*
  * This runs after lockup_detector_init() which sets up watchdog_cpumask.
  */
-static int __init powerpc_watchdog_init(void)
+int __init watchdog_nmi_probe(void)
 {
 	int err;
 
-	watchdog_calc_timeouts();
-
-	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powerpc/watchdog:online",
-				start_wd_on_cpu, stop_wd_on_cpu);
+	err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					"powerpc/watchdog:online",
+					start_wd_on_cpu, stop_wd_on_cpu);
 	if (err < 0)
 		pr_warn("Watchdog could not be initialized");
 
 	return 0;
 }
-arch_initcall(powerpc_watchdog_init);
 
 static void handle_backtrace_ipi(struct pt_regs *regs)
 {
kernel/watchdog.c +4 -1
@@ -608,7 +608,6 @@ static inline int watchdog_park_threads(
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
-static inline void softlockup_init_threads(void) { }
 static void softlockup_reconfigure_threads(void)
 {
 	cpus_read_lock();
@@ -617,6 +616,10 @@ static void softlockup_reconfigure_threa
 	watchdog_nmi_start();
 	cpus_read_unlock();
 }
+static inline void softlockup_init_threads(void)
+{
+	softlockup_reconfigure_threads();
+}
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
#48 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 25 lines · Thomas Gleixner
Quoted 24 lines · Michael Ellerman

Hi Thomas,
Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc
Quoted 6 lines
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

Which leads me to the question why you need the hotplug state at all if the softlockup detector is enabled. Wouldn't it make more sense to only register the state if softlockup detector is turned off in Kconfig and actually move it to the core code?

Thanks,

tglx

#49 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 26 lines · On Tue, 3 Oct 2017 09:04:03 +0200 (CEST) Thomas Gleixner <[email protected]>…
Quoted 25 lines · Thomas Gleixner
Quoted 24 lines · Michael Ellerman

Hi Thomas,
Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

I had the idea that it watchdog_nmi_reconfigure() being only called with get_online_cpus held would prevent hotplug callbacks running.

Quoted 5 lines

Which leads me to the question why you need the hotplug state at all if the softlockup detector is enabled. Wouldn't it make more sense to only register the state if softlockup detector is turned off in Kconfig and actually move it to the core code?

I don't understand what you mean exactly, but it was done to avoid relying on the softlockup detector at all, because it wasn't needed for anything else (unlike the perf lockup detector).

Thanks,
Nick

#50 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 40 lines · Nicholas Piggin
Quoted 26 lines · On Tue, 3 Oct 2017 09:04:03 +0200 (CEST) Thomas Gleixner <[email protected]>…
Quoted 25 lines · Thomas Gleixner
Quoted 24 lines · Michael Ellerman

Hi Thomas,
Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

I had the idea that it watchdog_nmi_reconfigure() being only called with get_online_cpus held would prevent hotplug callbacks running.

Quoted 1 line
Quoted 8 lines
Quoted 4 lines

Which leads me to the question why you need the hotplug state at all if the softlockup detector is enabled. Wouldn't it make more sense to only register the state if softlockup detector is turned off in Kconfig and actually move it to the core code?

I don't understand what you mean exactly, but it was done to avoid relying on the softlockup detector at all, because it wasn't needed for anything else (unlike the perf lockup detector).

If the softlockup detector is enabled along with your hardlockup detector then the current code in mainline invokes watchdog_nmi_enable(cpu), which is a weak function and as I just noticed not implemented by powerpc. So it's a non issue because it's not implemented.

Thanks,

tglx

#51 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

In reply to Thomas Gleixner
Quoted 25 lines · Thomas Gleixner
Quoted 24 lines · Michael Ellerman

Hi Thomas,
Unfortunately this is hitting the WARN_ON in start_wd_cpu() on powerpc because we're calling it multiple times for the boot CPU.

The first call is via:

  start_wd_on_cpu+0x80/0x2f0
  watchdog_nmi_reconfigure+0x124/0x170
  softlockup_reconfigure_threads+0x110/0x130
  lockup_detector_init+0xbc/0xe0
  kernel_init_freeable+0x18c/0x37c
  kernel_init+0x2c/0x160
  ret_from_kernel_thread+0x5c/0xbc

And then again via the CPU hotplug registration:

  start_wd_on_cpu+0x80/0x2f0
  cpuhp_invoke_callback+0x194/0x620
  cpuhp_thread_fun+0x7c/0x1b0
  smpboot_thread_fn+0x290/0x2a0
  kthread+0x168/0x1b0
  ret_from_kernel_thread+0x5c/0xbc
Quoted 6 lines
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

I don't see it with CPU hotplug.

AFAICS that's because softlockup_reconfigure_threads() isn't called for CPU hotplug. Unless there's a path I'm missing?

Quoted 5 lines
Quoted 2 lines

I'm not sure what the easiest fix is. One option would be to just drop the WARN_ON, it's just there for paranoia AFAICS.

The straight forward way is to make use of the new probe function. Patch below.

Thanks.

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

So seems there may be some more gremlins. Will test more in the morning.

cheers

#52 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 4 lines · Michael Ellerman
Quoted 3 lines · Thomas Gleixner
Quoted 2 lines

The first call is new because previously watchdog_nmi_reconfigure() wasn't called from softlockup_reconfigure_threads().

Quoted 8 lines
Quoted 3 lines

Hmm, don't you have the same problem with CPU hotplug or do you just get lucky because the hotplug callback in your code is ordered vs. the softlockup thread hotplug callback in a way that this does not hit?

I don't see it with CPU hotplug.

AFAICS that's because softlockup_reconfigure_threads() isn't called for CPU hotplug. Unless there's a path I'm missing?

As I said in the other reply, I assumed that its called via watchdog_nmi_enable(cpu), but that's a weak function which is not implemented on power. So no issue.

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines

I'm not sure what the easiest fix is. One option would be to just drop the WARN_ON, it's just there for paranoia AFAICS.

The straight forward way is to make use of the new probe function. Patch below.

Quoted 7 lines

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

So seems there may be some more gremlins. Will test more in the morning.

Hrm. That's weird. I'll have a look and send a proper patch series on top of next.

Thanks,

tglx

#53 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 11 lines · Thomas Gleixner
Quoted 7 lines · Michael Ellerman

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

So seems there may be some more gremlins. Will test more in the morning.

Hrm. That's weird. I'll have a look and send a proper patch series on top of next.

The major difference is that the reworked code utilizes watchdog_nmi_reconfigure() for both init and the sysctl updates, but I can't for my life figure out why that doesn't work.

Thanks,

tglx

#54 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 8 lines · Thomas Gleixner
Quoted 7 lines · Thomas Gleixner
Quoted 6 lines · Michael Ellerman

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

So seems there may be some more gremlins. Will test more in the morning.

Hrm. That's weird. I'll have a look and send a proper patch series on top of next.

The major difference is that the reworked code utilizes watchdog_nmi_reconfigure() for both init and the sysctl updates, but I can't for my life figure out why that doesn't work.

I collected the changes which Linus requested along with the nmi_probe() one and pushed them into:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.core/urgent

That's based on 4.13 final so it neither contains 4.14 nor -next material.

Thanks,

tglx

reviewer Acked-by
#55 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Quoted 18 lines · Thomas Gleixner
Quoted 8 lines · Thomas Gleixner
Quoted 7 lines · Thomas Gleixner
Quoted 6 lines · Michael Ellerman

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

So seems there may be some more gremlins. Will test more in the morning.

Hrm. That's weird. I'll have a look and send a proper patch series on top of next.

The major difference is that the reworked code utilizes watchdog_nmi_reconfigure() for both init and the sysctl updates, but I can't for my life figure out why that doesn't work.

Quoted 6 lines

I collected the changes which Linus requested along with the nmi_probe() one and pushed them into:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.core/urgent

That's based on 4.13 final so it neither contains 4.14 nor -next material.

Thanks. I tested that here and it seems fine. The warning at boot is gone and it is correctly catching a hard lockup triggered via LKDTM, eg:

  # mount -t debugfs none /sys/kernel/debug
  # echo HARDLOCKUP > /sys/kernel/debug/provoke-crash/DIRECT
  lkdtm: Performing direct entry HARDLOCKUP
  Watchdog CPU:0 Hard LOCKUP
  Modules linked in:
  CPU: 0 PID: 1215 Comm: sh Not tainted 4.13.0-gcc6-11846-g86be5ee #162
  task: c0000000f1fc4c00 task.stack: c0000000ee3ac000
  NIP:  c0000000007205a4 LR: c00000000071f950 CTR: c000000000720570
  REGS: c00000003ffffd80 TRAP: 0900   Not tainted  (4.13.0-gcc6-11846-g86be5ee)
  MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 28002228  XER: 00000000
  CFAR: c0000000007205a8 SOFTE: 0 
  GPR00: c00000000071f950 c0000000ee3afbb0 c00000000107cf00 c0000000010604f0 
  GPR04: c0000000ffa05d90 c0000000ffa1c968 0000000000000000 0000000000000000 
  GPR08: 0000000000000007 0000000000000001 0000000000000000 9000000030001003 
  GPR12: c000000000720570 c00000000fd40000 0000000000000000 0000000000000000 
  GPR16: 0000000000000000 0000000000000000 0000000000000000 00000000100b8fd0 
  GPR20: 000001002f5a3485 00000000100b8f90 0000000000000000 0000000000000000 
  GPR24: c000000001060778 c0000000ee3afe00 c0000000ee3afe00 c0000000010603b0 
  GPR28: 000000000000000b c0000000f1fe0000 0000000000000140 c0000000010604f0 
  NIP [c0000000007205a4] lkdtm_HARDLOCKUP+0x34/0x40
  LR [c00000000071f950] lkdtm_do_action+0x50/0x70
  Call Trace:
  [c0000000ee3afbb0] [0000000000000140] 0x140 (unreliable)
  [c0000000ee3afbd0] [c00000000071f950] lkdtm_do_action+0x50/0x70
  [c0000000ee3afc00] [c00000000071fdc0] direct_entry+0x110/0x1b0
  [c0000000ee3afc90] [c00000000050141c] full_proxy_write+0x9c/0x110
  [c0000000ee3afcf0] [c000000000336a3c] __vfs_write+0x6c/0x210
  [c0000000ee3afd90] [c000000000338960] vfs_write+0xd0/0x270
  [c0000000ee3afde0] [c00000000033a93c] SyS_write+0x6c/0x110
  [c0000000ee3afe30] [c00000000000b220] system_call+0x58/0x6c
  Instruction dump:
  3842c990 7c0802a6 f8010010 f821ffe1 60000000 60000000 39400000 892d027a 
  994d027a 60000000 60420000 7c210b78 <7c421378> 4bfffff8 60420000 3c4c0096 
  Kernel panic - not syncing: Hard LOCKUP

cheers

#56 lore raw

Re: [patch V2 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

In reply to Thomas Gleixner
Quoted 18 lines · Thomas Gleixner
Quoted 8 lines · Thomas Gleixner
Quoted 7 lines · Thomas Gleixner
Quoted 6 lines · Michael Ellerman

Hmm, I tried that patch, it makes the warning go away. But then I triggered a deliberate hard lockup and got nothing.

Then I went back to the existing code (in linux-next), and I still get no warning from a deliberate hard lockup.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

So seems there may be some more gremlins. Will test more in the morning.

Hrm. That's weird. I'll have a look and send a proper patch series on top of next.

The major difference is that the reworked code utilizes watchdog_nmi_reconfigure() for both init and the sysctl updates, but I can't for my life figure out why that doesn't work.

Quoted 6 lines

I collected the changes which Linus requested along with the nmi_probe() one and pushed them into:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.core/urgent

That's based on 4.13 final so it neither contains 4.14 nor -next material.

Tested your changes on 4.14-rc3 and it passes my tests. Thanks!

Tested-and-Reviewed-by Don Zickus <[email protected]>
#57 lore raw

PATCH v2 23/29 lockup_detector: Get rid of the racy update loop

In reply to Thomas Gleixner

Letting user space poke directly at variables which are used at run time is stupid and causes a lot of race conditions and other issues.

Seperate the user variables and on change invoke the reconfiguration, which then stops the watchdogs, reevaluates the new user value and restarts the watchdogs with the new parameters.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   95 ++++++++++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 48 deletions(-)
kernel/watchdog.c +47 -48
@@ -32,15 +32,17 @@
 static DEFINE_MUTEX(watchdog_mutex);
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
-unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED |
-						NMI_WATCHDOG_ENABLED;
+# define WATCHDOG_DEFAULT	(SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED)
+# define NMI_WATCHDOG_DEFAULT	1
 #else
-unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
+# define WATCHDOG_DEFAULT	(SOFT_WATCHDOG_ENABLED)
+# define NMI_WATCHDOG_DEFAULT	0
 #endif
 
-int __read_mostly nmi_watchdog_user_enabled;
-int __read_mostly soft_watchdog_user_enabled;
-int __read_mostly watchdog_user_enabled;
+unsigned long __read_mostly watchdog_enabled;
+int __read_mostly watchdog_user_enabled = 1;
+int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
+int __read_mostly soft_watchdog_user_enabled = 1;
 int __read_mostly watchdog_thresh = 10;
 
 struct cpumask watchdog_allowed_mask __read_mostly;
@@ -65,7 +67,7 @@ unsigned int __read_mostly hardlockup_pa
  */
 void __init hardlockup_detector_disable(void)
 {
-	watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
+	nmi_watchdog_user_enabled = 0;
 }
 
 static int __init hardlockup_panic_setup(char *str)
@@ -75,9 +77,9 @@ static int __init hardlockup_panic_setup
 	else if (!strncmp(str, "nopanic", 7))
 		hardlockup_panic = 0;
 	else if (!strncmp(str, "0", 1))
-		watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
+		nmi_watchdog_user_enabled = 0;
 	else if (!strncmp(str, "1", 1))
-		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
+		nmi_watchdog_user_enabled = 1;
 	return 1;
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
@@ -132,6 +134,23 @@ void __weak watchdog_nmi_disable(unsigne
  */
 void __weak watchdog_nmi_reconfigure(bool run) { }
 
+/**
+ * lockup_detector_update_enable - Update the sysctl enable bit
+ *
+ * Caller needs to make sure that the NMI/perf watchdogs are off, so this
+ * can't race with watchdog_nmi_disable().
+ */
+static void lockup_detector_update_enable(void)
+{
+	watchdog_enabled = 0;
+	if (!watchdog_user_enabled)
+		return;
+	if (nmi_watchdog_user_enabled)
+		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
+	if (soft_watchdog_user_enabled)
+		watchdog_enabled |= SOFT_WATCHDOG_ENABLED;
+}
+
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
 /* Global variables, exported for sysctl */
@@ -160,14 +179,14 @@ static int __init softlockup_panic_setup
 
 static int __init nowatchdog_setup(char *str)
 {
-	watchdog_enabled = 0;
+	watchdog_user_enabled = 0;
 	return 1;
 }
 __setup("nowatchdog", nowatchdog_setup);
 
 static int __init nosoftlockup_setup(char *str)
 {
-	watchdog_enabled &= ~SOFT_WATCHDOG_ENABLED;
+	soft_watchdog_user_enabled = 0;
 	return 1;
 }
 __setup("nosoftlockup", nosoftlockup_setup);
@@ -521,12 +540,13 @@ static void softlockup_unpark_threads(vo
 	softlockup_update_smpboot_threads();
 }
 
-static void softlockup_reconfigure_threads(bool enabled)
+static void softlockup_reconfigure_threads(void)
 {
 	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
-	if (enabled)
+	lockup_detector_update_enable();
+	if (watchdog_enabled && watchdog_thresh)
 		softlockup_unpark_threads();
 	watchdog_nmi_reconfigure(true);
 }
@@ -546,6 +566,8 @@ static __init void softlockup_init_threa
 	 * If sysctl is off and watchdog got disabled on the command line,
 	 * nothing to do here.
 	 */
+	lockup_detector_update_enable();
+
 	if (!IS_ENABLED(CONFIG_SYSCTL) &&
 	    !(watchdog_enabled && watchdog_thresh))
 		return;
@@ -559,7 +581,7 @@ static __init void softlockup_init_threa
 
 	mutex_lock(&watchdog_mutex);
 	softlockup_threads_initialized = true;
-	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	softlockup_reconfigure_threads();
 	mutex_unlock(&watchdog_mutex);
 }
 
@@ -569,9 +591,10 @@ static inline void watchdog_unpark_threa
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static void softlockup_reconfigure_threads(bool enabled)
+static void softlockup_reconfigure_threads(void)
 {
 	watchdog_nmi_reconfigure(false);
+	lockup_detector_update_enable();
 	watchdog_nmi_reconfigure(true);
 }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
@@ -612,7 +635,7 @@ static void proc_watchdog_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	softlockup_reconfigure_threads();
 }
 
 /*
@@ -630,48 +653,24 @@ static void proc_watchdog_update(void)
 static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 				void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old, new;
-	int *watchdog_param = (int *)table->data;
+	int err, old, *param = table->data;
 
 	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
-	/*
-	 * If the parameter is being read return the state of the corresponding
-	 * bit(s) in 'watchdog_enabled', else update 'watchdog_enabled' and the
-	 * run state of the lockup detectors.
-	 */
 	if (!write) {
-		*watchdog_param = (watchdog_enabled & which) != 0;
+		/*
+		 * On read synchronize the userspace interface. This is a
+		 * racy snapshot.
+		 */
+		*param = (watchdog_enabled & which) != 0;
 		err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	} else {
+		old = READ_ONCE(*param);
 		err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
-		if (err)
-			goto out;
-
-		/*
-		 * There is a race window between fetching the current value
-		 * from 'watchdog_enabled' and storing the new value. During
-		 * this race window, watchdog_nmi_enable() can sneak in and
-		 * clear the NMI_WATCHDOG_ENABLED bit in 'watchdog_enabled'.
-		 * The 'cmpxchg' detects this race and the loop retries.
-		 */
-		do {
-			old = watchdog_enabled;
-			/*
-			 * If the parameter value is not zero set the
-			 * corresponding bit(s), else clear it(them).
-			 */
-			if (*watchdog_param)
-				new = old | which;
-			else
-				new = old & ~which;
-		} while (cmpxchg(&watchdog_enabled, old, new) != old);
-
-		if (old != new)
+		if (!err && old != READ_ONCE(*param))
 			proc_watchdog_update();
 	}
-out:
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
#58 lore raw

[tip:core/urgent] watchdog/core: Get rid of the racy update loop

watchdog/core: Get rid of the racy update loop
Commit-ID:  091549858ed881e5f3054374af4f5b1cac681d50
Gitweb:     http://git.kernel.org/tip/091549858ed881e5f3054374af4f5b1cac681d50
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:17 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:07 +0200

watchdog/core: Get rid of the racy update loop

Letting user space poke directly at variables which are used at run time is stupid and causes a lot of race conditions and other issues.

Seperate the user variables and on change invoke the reconfiguration, which then stops the watchdogs, reevaluates the new user value and restarts the watchdogs with the new parameters.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 95 +++++++++++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 48 deletions(-)
kernel/watchdog.c +47 -48
index 5693afd..8488631 100644
@@ -32,15 +32,17 @@
 static DEFINE_MUTEX(watchdog_mutex);
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
-unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED |
-						NMI_WATCHDOG_ENABLED;
+# define WATCHDOG_DEFAULT	(SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED)
+# define NMI_WATCHDOG_DEFAULT	1
 #else
-unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
+# define WATCHDOG_DEFAULT	(SOFT_WATCHDOG_ENABLED)
+# define NMI_WATCHDOG_DEFAULT	0
 #endif
 
-int __read_mostly nmi_watchdog_user_enabled;
-int __read_mostly soft_watchdog_user_enabled;
-int __read_mostly watchdog_user_enabled;
+unsigned long __read_mostly watchdog_enabled;
+int __read_mostly watchdog_user_enabled = 1;
+int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
+int __read_mostly soft_watchdog_user_enabled = 1;
 int __read_mostly watchdog_thresh = 10;
 
 struct cpumask watchdog_allowed_mask __read_mostly;
@@ -65,7 +67,7 @@ unsigned int __read_mostly hardlockup_panic =
  */
 void __init hardlockup_detector_disable(void)
 {
-	watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
+	nmi_watchdog_user_enabled = 0;
 }
 
 static int __init hardlockup_panic_setup(char *str)
@@ -75,9 +77,9 @@ static int __init hardlockup_panic_setup(char *str)
 	else if (!strncmp(str, "nopanic", 7))
 		hardlockup_panic = 0;
 	else if (!strncmp(str, "0", 1))
-		watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
+		nmi_watchdog_user_enabled = 0;
 	else if (!strncmp(str, "1", 1))
-		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
+		nmi_watchdog_user_enabled = 1;
 	return 1;
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
@@ -132,6 +134,23 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
  */
 void __weak watchdog_nmi_reconfigure(bool run) { }
 
+/**
+ * lockup_detector_update_enable - Update the sysctl enable bit
+ *
+ * Caller needs to make sure that the NMI/perf watchdogs are off, so this
+ * can't race with watchdog_nmi_disable().
+ */
+static void lockup_detector_update_enable(void)
+{
+	watchdog_enabled = 0;
+	if (!watchdog_user_enabled)
+		return;
+	if (nmi_watchdog_user_enabled)
+		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
+	if (soft_watchdog_user_enabled)
+		watchdog_enabled |= SOFT_WATCHDOG_ENABLED;
+}
+
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
 /* Global variables, exported for sysctl */
@@ -160,14 +179,14 @@ __setup("softlockup_panic=", softlockup_panic_setup);
 
 static int __init nowatchdog_setup(char *str)
 {
-	watchdog_enabled = 0;
+	watchdog_user_enabled = 0;
 	return 1;
 }
 __setup("nowatchdog", nowatchdog_setup);
 
 static int __init nosoftlockup_setup(char *str)
 {
-	watchdog_enabled &= ~SOFT_WATCHDOG_ENABLED;
+	soft_watchdog_user_enabled = 0;
 	return 1;
 }
 __setup("nosoftlockup", nosoftlockup_setup);
@@ -521,12 +540,13 @@ static void softlockup_unpark_threads(void)
 	softlockup_update_smpboot_threads();
 }
 
-static void softlockup_reconfigure_threads(bool enabled)
+static void softlockup_reconfigure_threads(void)
 {
 	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
-	if (enabled)
+	lockup_detector_update_enable();
+	if (watchdog_enabled && watchdog_thresh)
 		softlockup_unpark_threads();
 	watchdog_nmi_reconfigure(true);
 }
@@ -546,6 +566,8 @@ static __init void softlockup_init_threads(void)
 	 * If sysctl is off and watchdog got disabled on the command line,
 	 * nothing to do here.
 	 */
+	lockup_detector_update_enable();
+
 	if (!IS_ENABLED(CONFIG_SYSCTL) &&
 	    !(watchdog_enabled && watchdog_thresh))
 		return;
@@ -559,7 +581,7 @@ static __init void softlockup_init_threads(void)
 
 	mutex_lock(&watchdog_mutex);
 	softlockup_threads_initialized = true;
-	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	softlockup_reconfigure_threads();
 	mutex_unlock(&watchdog_mutex);
 }
 
@@ -569,9 +591,10 @@ static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static void softlockup_reconfigure_threads(bool enabled)
+static void softlockup_reconfigure_threads(void)
 {
 	watchdog_nmi_reconfigure(false);
+	lockup_detector_update_enable();
 	watchdog_nmi_reconfigure(true);
 }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
@@ -612,7 +635,7 @@ static void proc_watchdog_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	softlockup_reconfigure_threads();
 }
 
 /*
@@ -630,48 +653,24 @@ static void proc_watchdog_update(void)
 static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 				void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old, new;
-	int *watchdog_param = (int *)table->data;
+	int err, old, *param = table->data;
 
 	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
-	/*
-	 * If the parameter is being read return the state of the corresponding
-	 * bit(s) in 'watchdog_enabled', else update 'watchdog_enabled' and the
-	 * run state of the lockup detectors.
-	 */
 	if (!write) {
-		*watchdog_param = (watchdog_enabled & which) != 0;
+		/*
+		 * On read synchronize the userspace interface. This is a
+		 * racy snapshot.
+		 */
+		*param = (watchdog_enabled & which) != 0;
 		err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	} else {
+		old = READ_ONCE(*param);
 		err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
-		if (err)
-			goto out;
-
-		/*
-		 * There is a race window between fetching the current value
-		 * from 'watchdog_enabled' and storing the new value. During
-		 * this race window, watchdog_nmi_enable() can sneak in and
-		 * clear the NMI_WATCHDOG_ENABLED bit in 'watchdog_enabled'.
-		 * The 'cmpxchg' detects this race and the loop retries.
-		 */
-		do {
-			old = watchdog_enabled;
-			/*
-			 * If the parameter value is not zero set the
-			 * corresponding bit(s), else clear it(them).
-			 */
-			if (*watchdog_param)
-				new = old | which;
-			else
-				new = old & ~which;
-		} while (cmpxchg(&watchdog_enabled, old, new) != old);
-
-		if (old != new)
+		if (!err && old != READ_ONCE(*param))
 			proc_watchdog_update();
 	}
-out:
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
#59 lore raw

PATCH v2 24/29 lockup_detector/perf: Implement init time perf validation

In reply to Thomas Gleixner

The watchdog tries to create perf events even after it figured out that perf is not functional or the requested event is not supported.

That's braindead as this can be done once at init time and if not supported the NMI watchdog can be turned off unconditonally.

Implement the perf hardlockup detector functionality for that. This creates a new event create function, which will replace the unholy mess of the existing one in later patches.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h   |    8 ++++++--
 kernel/watchdog_hld.c |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)
include/linux/nmi.h +6 -2
@@ -93,14 +93,18 @@ extern void hardlockup_detector_perf_sto
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_detector_perf_disable(void);
 extern void hardlockup_detector_perf_cleanup(void);
+extern int hardlockup_detector_perf_init(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
 static inline void hardlockup_detector_perf_disable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
-#if !defined(CONFIG_HAVE_NMI_WATCHDOG)
+# if !defined(CONFIG_HAVE_NMI_WATCHDOG)
+static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
 static inline void arch_touch_nmi_watchdog(void) {}
-#endif
+# else
+static int hardlockup_detector_perf_init(void) { return 0; }
+# endif
 #endif
 
 void watchdog_nmi_reconfigure(bool run);
kernel/watchdog_hld.c +37 -0
@@ -238,6 +238,27 @@ int watchdog_nmi_enable(unsigned int cpu
 	return 0;
 }
 
+static int hardlockup_detector_event_create(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct perf_event_attr *wd_attr;
+	struct perf_event *evt;
+
+	wd_attr = &wd_hw_attr;
+	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
+
+	/* Try to register using hardware perf events */
+	evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
+					       watchdog_overflow_callback, NULL);
+	if (IS_ERR(evt)) {
+		pr_info("Perf event create on CPU %d failed with %ld\n", cpu,
+			PTR_ERR(evt));
+		return PTR_ERR(evt);
+	}
+	this_cpu_write(watchdog_ev, evt);
+	return 0;
+}
+
 /**
  * hardlockup_detector_perf_disable - Disable the local event
  */
@@ -315,3 +336,19 @@ void __init hardlockup_detector_perf_res
 			perf_event_enable(event);
 	}
 }
+
+/**
+ * hardlockup_detector_perf_init - Probe whether NMI event is available at all
+ */
+int __init hardlockup_detector_perf_init(void)
+{
+	int ret = hardlockup_detector_event_create();
+
+	if (ret) {
+		pr_info("Perf NMI watchdog permanetely disabled\n");
+	} else {
+		perf_event_release_kernel(this_cpu_read(watchdog_ev));
+		this_cpu_write(watchdog_ev, NULL);
+	}
+	return ret;
+}
#60 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Implement init time perf validation

watchdog/hardlockup/perf: Implement init time perf validation
Commit-ID:  178b9f7a36d2c74a38274b66dd89f53611298a19
Gitweb:     http://git.kernel.org/tip/178b9f7a36d2c74a38274b66dd89f53611298a19
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:18 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:08 +0200

watchdog/hardlockup/perf: Implement init time perf validation

The watchdog tries to create perf events even after it figured out that perf is not functional or the requested event is not supported.

That's braindead as this can be done once at init time and if not supported the NMI watchdog can be turned off unconditonally.

Implement the perf hardlockup detector functionality for that. This creates a new event create function, which will replace the unholy mess of the existing one in later patches.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h   |  8 ++++++--
 kernel/watchdog_hld.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)
include/linux/nmi.h +6 -2
index eee255b..72c62a8 100644
@@ -93,14 +93,18 @@ extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_detector_perf_disable(void);
 extern void hardlockup_detector_perf_cleanup(void);
+extern int hardlockup_detector_perf_init(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
 static inline void hardlockup_detector_perf_disable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
-#if !defined(CONFIG_HAVE_NMI_WATCHDOG)
+# if !defined(CONFIG_HAVE_NMI_WATCHDOG)
+static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
 static inline void arch_touch_nmi_watchdog(void) {}
-#endif
+# else
+static inline int hardlockup_detector_perf_init(void) { return 0; }
+# endif
 #endif
 
 void watchdog_nmi_reconfigure(bool run);
kernel/watchdog_hld.c +37 -0
index 0aa191e..f7e752e 100644
@@ -238,6 +238,27 @@ out:
 	return 0;
 }
 
+static int hardlockup_detector_event_create(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct perf_event_attr *wd_attr;
+	struct perf_event *evt;
+
+	wd_attr = &wd_hw_attr;
+	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
+
+	/* Try to register using hardware perf events */
+	evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
+					       watchdog_overflow_callback, NULL);
+	if (IS_ERR(evt)) {
+		pr_info("Perf event create on CPU %d failed with %ld\n", cpu,
+			PTR_ERR(evt));
+		return PTR_ERR(evt);
+	}
+	this_cpu_write(watchdog_ev, evt);
+	return 0;
+}
+
 /**
  * hardlockup_detector_perf_disable - Disable the local event
  */
@@ -315,3 +336,19 @@ void __init hardlockup_detector_perf_restart(void)
 			perf_event_enable(event);
 	}
 }
+
+/**
+ * hardlockup_detector_perf_init - Probe whether NMI event is available at all
+ */
+int __init hardlockup_detector_perf_init(void)
+{
+	int ret = hardlockup_detector_event_create();
+
+	if (ret) {
+		pr_info("Perf NMI watchdog permanetely disabled\n");
+	} else {
+		perf_event_release_kernel(this_cpu_read(watchdog_ev));
+		this_cpu_write(watchdog_ev, NULL);
+	}
+	return ret;
+}
#61 lore raw

PATCH v2 25/29 lockup_detector: Implement init time detection of perf

In reply to Thomas Gleixner

Use the init time detection of the perf NMI watchdog to determine whether the perf NMI watchdog is functional. If not disable it permanentely. It won't come back magically at runtime.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
kernel/watchdog.c +12 -1
@@ -44,6 +44,7 @@ int __read_mostly watchdog_user_enabled
 int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
 int __read_mostly soft_watchdog_user_enabled = 1;
 int __read_mostly watchdog_thresh = 10;
+int __read_mostly nmi_watchdog_available;
 
 struct cpumask watchdog_allowed_mask __read_mostly;
 static bool softlockup_threads_initialized __read_mostly;
@@ -114,6 +115,12 @@ void __weak watchdog_nmi_disable(unsigne
 	hardlockup_detector_perf_disable();
 }
 
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init void watchdog_nmi_probe(void)
+{
+	return hardlockup_detector_perf_init();
+}
+
 /**
  * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
  * @run:	If false stop the watchdogs on all enabled CPUs
@@ -145,7 +152,7 @@ static void lockup_detector_update_enabl
 	watchdog_enabled = 0;
 	if (!watchdog_user_enabled)
 		return;
-	if (nmi_watchdog_user_enabled)
+	if (nmi_watchdog_available && nmi_watchdog_user_enabled)
 		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
 	if (soft_watchdog_user_enabled)
 		watchdog_enabled |= SOFT_WATCHDOG_ENABLED;
@@ -692,6 +699,8 @@ int proc_watchdog(struct ctl_table *tabl
 int proc_nmi_watchdog(struct ctl_table *table, int write,
 		      void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	if (!nmi_watchdog_available && write)
+		return -ENOTSUPP;
 	return proc_watchdog_common(NMI_WATCHDOG_ENABLED,
 				    table, write, buffer, lenp, ppos);
 }
@@ -764,5 +773,7 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
+	if (!watchdog_nmi_probe())
+		nmi_watchdog_available = true;
 	softlockup_init_threads();
 }
#62 lore raw

Re: [patch V2 25/29] lockup_detector: Implement init time detection of perf

Quoted 30 lines · Thomas Gleixner

Use the init time detection of the perf NMI watchdog to determine whether the perf NMI watchdog is functional. If not disable it permanentely. It won't come back magically at runtime.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
kernel/watchdog.c +1 -0
@@ -44,6 +44,7 @@ int __read_mostly watchdog_user_enabled
 int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
 int __read_mostly soft_watchdog_user_enabled = 1;
 int __read_mostly watchdog_thresh = 10;
+int __read_mostly nmi_watchdog_available;
 
 struct cpumask watchdog_allowed_mask __read_mostly;
 static bool softlockup_threads_initialized __read_mostly;

kernel/watchdog.c void __weak watchdog_nmi_disable(unsigne · line 119

Quoted 5 lines
kernel/watchdog.c +2 -0
@@ -114,6 +115,12 @@ void __weak watchdog_nmi_disable(unsigne
 	hardlockup_detector_perf_disable();
 }
 
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init void watchdog_nmi_probe(void)

^^^ compile error, can't have 'int' and 'void'. Removing 'void' fixes it.

Cheers,
Don

Quoted 35 lines
kernel/watchdog.c +9 -1
+{
+	return hardlockup_detector_perf_init();
+}
+
 /**
  * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
  * @run:	If false stop the watchdogs on all enabled CPUs
@@ -145,7 +152,7 @@ static void lockup_detector_update_enabl
 	watchdog_enabled = 0;
 	if (!watchdog_user_enabled)
 		return;
-	if (nmi_watchdog_user_enabled)
+	if (nmi_watchdog_available && nmi_watchdog_user_enabled)
 		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
 	if (soft_watchdog_user_enabled)
 		watchdog_enabled |= SOFT_WATCHDOG_ENABLED;
@@ -692,6 +699,8 @@ int proc_watchdog(struct ctl_table *tabl
 int proc_nmi_watchdog(struct ctl_table *table, int write,
 		      void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	if (!nmi_watchdog_available && write)
+		return -ENOTSUPP;
 	return proc_watchdog_common(NMI_WATCHDOG_ENABLED,
 				    table, write, buffer, lenp, ppos);
 }
@@ -764,5 +773,7 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
+	if (!watchdog_nmi_probe())
+		nmi_watchdog_available = true;
 	softlockup_init_threads();
 }
#63 lore raw

Re: [patch V2 25/29] lockup_detector: Implement init time detection of perf

Quoted 4 lines · Don Zickus

kernel/watchdog.c void __weak watchdog_nmi_disable(unsigne · line 119

Quoted 2 lines
kernel/watchdog.c +2 -0
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init void watchdog_nmi_probe(void)

^^^ compile error, can't have 'int' and 'void'. Removing 'void' fixes it.

Yes, I know. I feel stupid because I'm 100% sure that I compiled it ....

#64 lore raw

Re: [patch V2 25/29] lockup_detector: Implement init time detection of perf

Quoted 7 lines · Thomas Gleixner
Quoted 4 lines · Don Zickus

kernel/watchdog.c void __weak watchdog_nmi_disable(unsigne · line 119

Quoted 2 lines
kernel/watchdog.c +2 -0
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init void watchdog_nmi_probe(void)

^^^ compile error, can't have 'int' and 'void'. Removing 'void' fixes it.

Yes, I know. I feel stupid because I'm 100% sure that I compiled it ....

So there a Kconfig dependency on that - defconfig won't show it, which might be why you didn't see it?

Thanks,

Ingo

#65 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Implement init time detection of perf

In reply to Thomas Gleixner
watchdog/hardlockup/perf: Implement init time detection of perf
Commit-ID:  a994a3147e4c0c9c50a46e6cace7586254975e20
Gitweb:     http://git.kernel.org/tip/a994a3147e4c0c9c50a46e6cace7586254975e20
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:19 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:08 +0200

watchdog/hardlockup/perf: Implement init time detection of perf

Use the init time detection of the perf NMI watchdog to determine whether the perf NMI watchdog is functional. If not disable it permanentely. It won't come back magically at runtime.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
kernel/watchdog.c +12 -1
index 8488631..fd8a998 100644
@@ -44,6 +44,7 @@ int __read_mostly watchdog_user_enabled = 1;
 int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
 int __read_mostly soft_watchdog_user_enabled = 1;
 int __read_mostly watchdog_thresh = 10;
+int __read_mostly nmi_watchdog_available;
 
 struct cpumask watchdog_allowed_mask __read_mostly;
 static bool softlockup_threads_initialized __read_mostly;
@@ -114,6 +115,12 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
 	hardlockup_detector_perf_disable();
 }
 
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init watchdog_nmi_probe(void)
+{
+	return hardlockup_detector_perf_init();
+}
+
 /**
  * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
  * @run:	If false stop the watchdogs on all enabled CPUs
@@ -145,7 +152,7 @@ static void lockup_detector_update_enable(void)
 	watchdog_enabled = 0;
 	if (!watchdog_user_enabled)
 		return;
-	if (nmi_watchdog_user_enabled)
+	if (nmi_watchdog_available && nmi_watchdog_user_enabled)
 		watchdog_enabled |= NMI_WATCHDOG_ENABLED;
 	if (soft_watchdog_user_enabled)
 		watchdog_enabled |= SOFT_WATCHDOG_ENABLED;
@@ -692,6 +699,8 @@ int proc_watchdog(struct ctl_table *table, int write,
 int proc_nmi_watchdog(struct ctl_table *table, int write,
 		      void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	if (!nmi_watchdog_available && write)
+		return -ENOTSUPP;
 	return proc_watchdog_common(NMI_WATCHDOG_ENABLED,
 				    table, write, buffer, lenp, ppos);
 }
@@ -764,5 +773,7 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
+	if (!watchdog_nmi_probe())
+		nmi_watchdog_available = true;
 	softlockup_init_threads();
 }
#66 lore raw

PATCH v2 26/29 lockup_detector/perf: Implement CPU enable replacement

In reply to Thomas Gleixner

watchdog_nmi_enable() is an unparseable mess, Provide a clean perf specific implementation, which will be used when the existing setup/teardown mess is replaced.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/nmi.h   |    2 ++
 kernel/watchdog_hld.c |   11 +++++++++++
 2 files changed, 13 insertions(+)
include/linux/nmi.h +2 -0
@@ -92,12 +92,14 @@ extern void arch_touch_nmi_watchdog(void
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_detector_perf_disable(void);
+extern void hardlockup_detector_perf_enable(void);
 extern void hardlockup_detector_perf_cleanup(void);
 extern int hardlockup_detector_perf_init(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
 static inline void hardlockup_detector_perf_disable(void) { }
+static inline void hardlockup_detector_perf_enable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
 # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
kernel/watchdog_hld.c +11 -0
@@ -260,6 +260,17 @@ static int hardlockup_detector_event_cre
 }
 
 /**
+ * hardlockup_detector_perf_enable - Enable the local event
+ */
+void hardlockup_detector_perf_enable(void)
+{
+	if (hardlockup_detector_event_create())
+		return;
+
+	perf_event_enable(this_cpu_read(watchdog_ev));
+}
+
+/**
  * hardlockup_detector_perf_disable - Disable the local event
  */
 void hardlockup_detector_perf_disable(void)
#67 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Implement CPU enable replacement

watchdog/hardlockup/perf: Implement CPU enable replacement
Commit-ID:  2a1b8ee4f5665b4291e43e4a25d964c3eb2f4c32
Gitweb:     http://git.kernel.org/tip/2a1b8ee4f5665b4291e43e4a25d964c3eb2f4c32
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:20 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:08 +0200

watchdog/hardlockup/perf: Implement CPU enable replacement

watchdog_nmi_enable() is an unparseable mess, Provide a clean perf specific implementation, which will be used when the existing setup/teardown mess is replaced.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 include/linux/nmi.h   |  2 ++
 kernel/watchdog_hld.c | 11 +++++++++++
 2 files changed, 13 insertions(+)
include/linux/nmi.h +2 -0
index 72c62a8..89ba8b2 100644
@@ -92,12 +92,14 @@ extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_detector_perf_disable(void);
+extern void hardlockup_detector_perf_enable(void);
 extern void hardlockup_detector_perf_cleanup(void);
 extern int hardlockup_detector_perf_init(void);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
 static inline void hardlockup_detector_perf_disable(void) { }
+static inline void hardlockup_detector_perf_enable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
 # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
kernel/watchdog_hld.c +11 -0
index f7e752e..99a3f22 100644
@@ -260,6 +260,17 @@ static int hardlockup_detector_event_create(void)
 }
 
 /**
+ * hardlockup_detector_perf_enable - Enable the local event
+ */
+void hardlockup_detector_perf_enable(void)
+{
+	if (hardlockup_detector_event_create())
+		return;
+
+	perf_event_enable(this_cpu_read(watchdog_ev));
+}
+
+/**
  * hardlockup_detector_perf_disable - Disable the local event
  */
 void hardlockup_detector_perf_disable(void)
#68 lore raw

PATCH v2 27/29 lockup_detector: Use new perf CPU enable mechanism

In reply to Thomas Gleixner

Get rid of the hodgepodge which tries to be smart about perf being unavailable and error printout rate limiting.

That's all not required simply because this is never invoked when the perf NMI watchdog is not functional.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog.c     |    4 +-
 kernel/watchdog_hld.c |   88 ++------------------------------------------------
 2 files changed, 8 insertions(+), 84 deletions(-)
kernel/watchdog.c +3 -1
@@ -107,6 +107,7 @@ static int __init hardlockup_all_cpu_bac
  */
 int __weak watchdog_nmi_enable(unsigned int cpu)
 {
+	hardlockup_detector_perf_enable();
 	return 0;
 }
 
@@ -465,7 +466,8 @@ static void watchdog_enable(unsigned int
 	/* Initialize timestamp */
 	__touch_watchdog();
 	/* Enable the perf event */
-	watchdog_nmi_enable(cpu);
+	if (watchdog_enabled & NMI_WATCHDOG_ENABLED)
+		watchdog_nmi_enable(cpu);
 
 	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 }
kernel/watchdog_hld.c +5 -83

112 lines not on this page · raw message

#69 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Use new perf CPU enable mechanism

watchdog/hardlockup/perf: Use new perf CPU enable mechanism
Commit-ID:  146c9d0e9dfdb62ed6afd43cc263efafbbfd1dcf
Gitweb:     http://git.kernel.org/tip/146c9d0e9dfdb62ed6afd43cc263efafbbfd1dcf
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:21 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:08 +0200

watchdog/hardlockup/perf: Use new perf CPU enable mechanism

Get rid of the hodgepodge which tries to be smart about perf being unavailable and error printout rate limiting.

That's all not required simply because this is never invoked when the perf NMI watchdog is not functional.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog.c     |  4 ++-
 kernel/watchdog_hld.c | 88 +++------------------------------------------------
 2 files changed, 8 insertions(+), 84 deletions(-)
kernel/watchdog.c +3 -1
index fd8a998..5eb1196 100644
@@ -107,6 +107,7 @@ __setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
  */
 int __weak watchdog_nmi_enable(unsigned int cpu)
 {
+	hardlockup_detector_perf_enable();
 	return 0;
 }
 
@@ -465,7 +466,8 @@ static void watchdog_enable(unsigned int cpu)
 	/* Initialize timestamp */
 	__touch_watchdog();
 	/* Enable the perf event */
-	watchdog_nmi_enable(cpu);
+	if (watchdog_enabled & NMI_WATCHDOG_ENABLED)
+		watchdog_nmi_enable(cpu);
 
 	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 }
kernel/watchdog_hld.c +5 -83

112 lines not on this page · raw message

#70 lore raw

PATCH v2 28/29 lockup_detector/perf: Simplify deferred event destroy

In reply to Thomas Gleixner

Now that all functionality is properly serialized against CPU hotplug, remove the extra per cpu storage which holds the disabled events for cleanup. The core makes sure that cleanup happens before new events are created.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 kernel/watchdog_hld.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
kernel/watchdog_hld.c +2 -5
@@ -21,7 +21,6 @@
 static DEFINE_PER_CPU(bool, hard_watchdog_warn);
 static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
-static DEFINE_PER_CPU(struct perf_event *, dead_event);
 static struct cpumask dead_events_mask;
 
 static unsigned long hardlockup_allcpu_dumped;
@@ -204,8 +203,6 @@ void hardlockup_detector_perf_disable(vo
 
 	if (event) {
 		perf_event_disable(event);
-		this_cpu_write(watchdog_ev, NULL);
-		this_cpu_write(dead_event, event);
 		cpumask_set_cpu(smp_processor_id(), &dead_events_mask);
 		watchdog_cpus--;
 	}
@@ -221,9 +218,9 @@ void hardlockup_detector_perf_cleanup(vo
 	int cpu;
 
 	for_each_cpu(cpu, &dead_events_mask) {
-		struct perf_event *event = per_cpu(dead_event, cpu);
+		struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
-		per_cpu(dead_event, cpu) = NULL;
+		per_cpu(watchdog_ev, cpu) = NULL;
 		perf_event_release_kernel(event);
 	}
 	cpumask_clear(&dead_events_mask);
#71 lore raw

[tip:core/urgent] watchdog/hardlockup/perf: Simplify deferred event destroy

watchdog/hardlockup/perf: Simplify deferred event destroy
Commit-ID:  a33d44843d4574ec05bec39527d8a87b7af2072c
Gitweb:     http://git.kernel.org/tip/a33d44843d4574ec05bec39527d8a87b7af2072c
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:22 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:08 +0200

watchdog/hardlockup/perf: Simplify deferred event destroy

Now that all functionality is properly serialized against CPU hotplug, remove the extra per cpu storage which holds the disabled events for cleanup. The core makes sure that cleanup happens before new events are created.

Cc Andrew Morton <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 kernel/watchdog_hld.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
kernel/watchdog_hld.c +2 -5
index 509bb6b..b293115 100644
@@ -21,7 +21,6 @@
 static DEFINE_PER_CPU(bool, hard_watchdog_warn);
 static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
 static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
-static DEFINE_PER_CPU(struct perf_event *, dead_event);
 static struct cpumask dead_events_mask;
 
 static unsigned long hardlockup_allcpu_dumped;
@@ -204,8 +203,6 @@ void hardlockup_detector_perf_disable(void)
 
 	if (event) {
 		perf_event_disable(event);
-		this_cpu_write(watchdog_ev, NULL);
-		this_cpu_write(dead_event, event);
 		cpumask_set_cpu(smp_processor_id(), &dead_events_mask);
 		watchdog_cpus--;
 	}
@@ -221,9 +218,9 @@ void hardlockup_detector_perf_cleanup(void)
 	int cpu;
 
 	for_each_cpu(cpu, &dead_events_mask) {
-		struct perf_event *event = per_cpu(dead_event, cpu);
+		struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
-		per_cpu(dead_event, cpu) = NULL;
+		per_cpu(watchdog_ev, cpu) = NULL;
 		perf_event_release_kernel(event);
 	}
 	cpumask_clear(&dead_events_mask);
#72 lore raw

PATCH v2 29/29 lockup_detector: Cleanup hotplug locking mess

In reply to Thomas Gleixner

All watchdog thread related functions are delegated to the smpboot thread infrastructure, which handles serialization against CPU hotplug correctly.

The sysctl interface is completely decoupled from anything which requires CPU hotplug protection.

No need to protect the sysctl writes against cpu hotplug anymore. Remove it and add the now required protection to the powerpc arch_nmi_watchdog implementation.

Cc Don Zickus <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Andrew Morton <[email protected]>
---
 arch/powerpc/kernel/watchdog.c |    2 ++
 kernel/watchdog.c              |    6 ------
 2 files changed, 2 insertions(+), 6 deletions(-)
arch/powerpc/kernel/watchdog.c +2 -0
@@ -359,6 +359,7 @@ void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
+	cpus_read_lock();
 	if (!run) {
 		for_each_cpu(cpu, &wd_cpus_enabled)
 			stop_wd_on_cpu(cpu);
@@ -367,6 +368,7 @@ void watchdog_nmi_reconfigure(bool run)
 		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
 			start_wd_on_cpu(cpu);
 	}
+	cpus_read_unlock();
 }
 
 /*
kernel/watchdog.c +0 -6
@@ -664,7 +664,6 @@ static int proc_watchdog_common(int whic
 {
 	int err, old, *param = table->data;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	if (!write) {
@@ -681,7 +680,6 @@ static int proc_watchdog_common(int whic
 			proc_watchdog_update();
 	}
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -725,7 +723,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 {
 	int err, old;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	old = READ_ONCE(watchdog_thresh);
@@ -735,7 +732,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -750,7 +746,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 {
 	int err;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
@@ -758,7 +753,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 #endif /* CONFIG_SYSCTL */
#73 lore raw

[tip:core/urgent] watchdog/hardlockup: Clean up hotplug locking mess

watchdog/hardlockup: Clean up hotplug locking mess
Commit-ID:  ab5fe3ff38ff9653490910cc71dbbedc95a86e41
Gitweb:     http://git.kernel.org/tip/ab5fe3ff38ff9653490910cc71dbbedc95a86e41
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Tue, 12 Sep 2017 21:37:23 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 14 Sep 2017 11:41:09 +0200

watchdog/hardlockup: Clean up hotplug locking mess

All watchdog thread related functions are delegated to the smpboot thread infrastructure, which handles serialization against CPU hotplug correctly.

The sysctl interface is completely decoupled from anything which requires CPU hotplug protection.

No need to protect the sysctl writes against cpu hotplug anymore. Remove it and add the now required protection to the powerpc arch_nmi_watchdog implementation.

Cc Andrew Morton <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Siewior <[email protected]>
Cc Ulrich Obergfell <[email protected]>
---
 arch/powerpc/kernel/watchdog.c | 2 ++
 kernel/watchdog.c              | 6 ------
 2 files changed, 2 insertions(+), 6 deletions(-)
arch/powerpc/kernel/watchdog.c +2 -0
index 291af79..dfb0677 100644
@@ -359,6 +359,7 @@ void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
+	cpus_read_lock();
 	if (!run) {
 		for_each_cpu(cpu, &wd_cpus_enabled)
 			stop_wd_on_cpu(cpu);
@@ -367,6 +368,7 @@ void watchdog_nmi_reconfigure(bool run)
 		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
 			start_wd_on_cpu(cpu);
 	}
+	cpus_read_unlock();
 }
 
 /*
kernel/watchdog.c +0 -6
index 5eb1196..f6ef163 100644
@@ -664,7 +664,6 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 {
 	int err, old, *param = table->data;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	if (!write) {
@@ -681,7 +680,6 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 			proc_watchdog_update();
 	}
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -725,7 +723,6 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 {
 	int err, old;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	old = READ_ONCE(watchdog_thresh);
@@ -735,7 +732,6 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -750,7 +746,6 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 {
 	int err;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
@@ -758,7 +753,6 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 #endif /* CONFIG_SYSCTL */
Reviewed-by on the v2 series
#74 lore raw
In reply to Thomas Gleixner
Quoted 19 lines · Thomas Gleixner

The lockup detector is broken is several ways:

- It's deadlock prone vs. CPU hotplug in various ways. Some of these are due to recursive cpus_read_lock() others are due to cpus_read_lock() from CPU hotplug callbacks which immediately lock the machine because cpus are write locked.

- The handling of the cpu hotplug threads happens sideways to the smpboot thread infrastructure, which is racy and pointless

- The handling of the user space sysctl interface is a complete trainwreck as it fiddles directly with variables which can be modified or evaluated by the running watchdogs.

- The perf event initialization is a trainwreck as it tries to create perf events over and over even if perf is not functional (no hardware, ....). To avoid excessive dmesg spam it contains magic printk ratelimiting along with either wrong or useless messages.

Quoted 5 lines

- The code structure is horrible as ifdef sections are scattered all over the place which makes it unreadable

- There is more wreckage, but see the changelogs for the ugly details.

Aside from the simple compile issue in patch 25. I have no issues with this patchset. Thanks Thomas!

Quoted 164 lines

The following series sanitizes the facility and addresses the problems.

Changes since V1:

- Wrapped the perf specific calls into the weak watchdog_nmi_* functions

- Fixed the compile error pointed out by Don

- Fixed the reconfiguration parameter inconsistency which broke powerpc

- Picked up the updated version of patch 11/29

Delta patch below.

Thanks,

tglx
---
Diffstat for the series:

 arch/parisc/kernel/process.c   |    2 
 arch/powerpc/kernel/watchdog.c |   22 -
 arch/x86/events/intel/core.c   |   11 
 include/linux/nmi.h            |  121 +++----
 include/linux/smpboot.h        |    4 
 kernel/cpu.c                   |    6 
 kernel/smpboot.c               |   22 -
 kernel/sysctl.c                |   22 -
 kernel/watchdog.c              |  633 ++++++++++++++---------------------------
 kernel/watchdog_hld.c          |  193 ++++++------
 10 files changed, 434 insertions(+), 602 deletions(-)

Delta patch vs. V1
8<------------------------

arch/powerpc/kernel/watchdog.c +2 -2
@@ -355,12 +355,12 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(bool stop)
+void watchdog_nmi_reconfigure(bool run)
 {
 	int cpu;
 
 	cpus_read_lock();
-	if (stop) {
+	if (!run) {
 		for_each_cpu(cpu, &wd_cpus_enabled)
 			stop_wd_on_cpu(cpu);
 	} else {
include/linux/nmi.h +1 -1
@@ -102,7 +102,7 @@ static inline void hardlockup_detector_p
 static inline void hardlockup_detector_perf_enable(void) { }
 static inline void hardlockup_detector_perf_cleanup(void) { }
 # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
-static int hardlockup_detector_perf_init(void) { return -ENODEV; }
+static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
 static inline void arch_touch_nmi_watchdog(void) {}
 # else
 static int hardlockup_detector_perf_init(void) { return 0; }
kernel/watchdog.c +25 -18
@@ -105,18 +105,32 @@ static int __init hardlockup_all_cpu_bac
  * softlockup watchdog threads start and stop. The arch must select the
  * SOFTLOCKUP_DETECTOR Kconfig.
  */
-int __weak watchdog_nmi_enable(unsigned int cpu) { return 0; }
-void __weak watchdog_nmi_disable(unsigned int cpu) { }
+int __weak watchdog_nmi_enable(unsigned int cpu)
+{
+	hardlockup_detector_perf_enable();
+	return 0;
+}
+
+void __weak watchdog_nmi_disable(unsigned int cpu)
+{
+	hardlockup_detector_perf_disable();
+}
+
+/* Return 0, if a NMI watchdog is available. Error code otherwise */
+int __weak __init void watchdog_nmi_probe(void)
+{
+	return hardlockup_detector_perf_init();
+}
 
 /**
  * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
- * @stop:	If true stop the watchdogs on all enabled CPUs
- *		If false start the watchdogs on all enabled CPUs
+ * @run:	If false stop the watchdogs on all enabled CPUs
+ *		If true start the watchdogs on all enabled CPUs
  *
  * The core call order is:
- * watchdog_nmi_reconfigure(true);
- * update_variables();
  * watchdog_nmi_reconfigure(false);
+ * update_variables();
+ * watchdog_nmi_reconfigure(true);
  *
  * The second call which starts the watchdogs again guarantees that the
  * following variables are stable across the call.
@@ -126,13 +140,13 @@ void __weak watchdog_nmi_disable(unsigne
  *
  * After the call the variables can be changed again.
  */
-void __weak watchdog_nmi_reconfigure(bool stop) { }
+void __weak watchdog_nmi_reconfigure(bool run) { }
 
 /**
  * lockup_detector_update_enable - Update the sysctl enable bit
  *
  * Caller needs to make sure that the NMI/perf watchdogs are off, so this
- * can't race with hardlockup_detector_disable().
+ * can't race with watchdog_nmi_disable().
  */
 static void lockup_detector_update_enable(void)
 {
@@ -453,8 +467,7 @@ static void watchdog_enable(unsigned int
 	__touch_watchdog();
 	/* Enable the perf event */
 	if (watchdog_enabled & NMI_WATCHDOG_ENABLED)
-		hardlockup_detector_perf_enable();
-	watchdog_nmi_enable(cpu);
+		watchdog_nmi_enable(cpu);
 
 	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 }
@@ -469,7 +482,6 @@ static void watchdog_disable(unsigned in
 	 * between disabling the timer and disabling the perf event causes
 	 * the perf NMI to detect a false positive.
 	 */
-	hardlockup_detector_perf_disable();
 	watchdog_nmi_disable(cpu);
 	hrtimer_cancel(hrtimer);
 }
@@ -745,12 +757,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 }
 #endif /* CONFIG_SYSCTL */
 
-static __init void detect_nmi_watchdog(void)
-{
-	if (!hardlockup_detector_perf_init())
-		nmi_watchdog_available = true;
-}
-
 void __init lockup_detector_init(void)
 {
 #ifdef CONFIG_NO_HZ_FULL
@@ -763,6 +769,7 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
-	detect_nmi_watchdog();
+	if (!watchdog_nmi_probe())
+		nmi_watchdog_available = true;
 	softlockup_init_threads();
 }
#75 lore raw
Quoted 23 lines · Don Zickus
Quoted 22 lines · Thomas Gleixner

The lockup detector is broken is several ways:

- It's deadlock prone vs. CPU hotplug in various ways. Some of these are due to recursive cpus_read_lock() others are due to cpus_read_lock() from CPU hotplug callbacks which immediately lock the machine because cpus are write locked.

- The handling of the cpu hotplug threads happens sideways to the smpboot thread infrastructure, which is racy and pointless

- The handling of the user space sysctl interface is a complete trainwreck as it fiddles directly with variables which can be modified or evaluated by the running watchdogs.

- The perf event initialization is a trainwreck as it tries to create perf events over and over even if perf is not functional (no hardware, ....). To avoid excessive dmesg spam it contains magic printk ratelimiting along with either wrong or useless messages.

- The code structure is horrible as ifdef sections are scattered all over the place which makes it unreadable

Quoted 7 lines
Quoted 2 lines

- There is more wreckage, but see the changelogs for the ugly details.

Aside from the simple compile issue in patch 25. I have no issues with this patchset. Thanks Thomas!

Thanks guys! I've applied the series and will push it out after some testing.

Thanks,

Ingo