[PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

The new nmi_watchdog (which uses the perf event subsystem) is very
similar in structure to the softlockup detector.  Using Ingo's suggestion,
I combined the two functionalities into one file, kernel/watchdog.c.

Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
sit on top of the perf event subsystem, which is run every 60 seconds or so
to see if there are any lockups.

To detect hardlockups, cpus not responding to interrupts, I implemented an
hrtimer that runs 5 times for every perf event overflow event.  If that stops
counting on a cpu, then the cpu is most likely in trouble.

To detect softlockups, tasks not yielding to the scheduler, I used the
previous kthread idea that now gets kicked every time the hrtimer fires.
If the kthread isn't being scheduled neither is anyone else and the
warning is printed to the console.

I tested this on x86_64 and both the softlockup and hardlockup paths work.

V2:
- cleaned up the Kconfig and softlockup combination
- surrounded hardlockup cases with #ifdef CONFIG_PERF_EVENTS_NMI
- seperated out the softlockup case from perf event subsystem
- re-arranged the enabling/disabling nmi watchdog from proc space
- added cpumasks for hardlockup failure cases
- removed fallback to soft events if no PMU exists for hard events

TODO:
- figure out how to make an arch-agnostic clock2cycles call (if possible)
  to feed into perf events as a sample period
- probably implement some missing procfs stuff

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/apic/hw_nmi.c |    2 +-
 include/linux/sched.h         |    6 +
 init/Kconfig                  |    1 +
 kernel/Makefile               |    4 +-
 kernel/sysctl.c               |    9 +
 kernel/watchdog.c             |  570 +++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig.debug             |   21 +-
 7 files changed, 605 insertions(+), 8 deletions(-)
 create mode 100644 kernel/watchdog.c

diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index e8b78a0..79425f9 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -89,7 +89,7 @@ int hw_nmi_is_cpu_stuck(struct pt_regs *regs)
 
 u64 hw_nmi_get_sample_period(void)
 {
-	return cpu_khz * 1000;
+	return (u64)(cpu_khz) * 1000 * 60;
 }
 
 #ifdef ARCH_HAS_NMI_WATCHDOG
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6f7bba9..83be6d7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -338,6 +338,12 @@ extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
 					 size_t *lenp, loff_t *ppos);
 #endif
 
+#ifdef CONFIG_NMI_WATCHDOG
+extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
+				  void __user *buffer,
+				  size_t *lenp, loff_t *ppos);
+#endif
+
 /* Attach to any functions which should be ignored in wchan output. */
 #define __sched		__attribute__((__section__(".sched.text")))
 
diff --git a/init/Kconfig b/init/Kconfig
index 7331a16..0b83612 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -948,6 +948,7 @@ config PERF_USE_VMALLOC
 
 config PERF_EVENTS_NMI
 	bool
+	depends on PERF_EVENTS
 	help
 	  Arch has support for nmi_watchdog
 
diff --git a/kernel/Makefile b/kernel/Makefile
index 8a5abe5..56ba99d 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -75,9 +75,11 @@ obj-$(CONFIG_GCOV_KERNEL) += gcov/
 obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
 obj-$(CONFIG_KPROBES) += kprobes.o
 obj-$(CONFIG_KGDB) += kgdb.o
+ifneq ($(CONFIG_NMI_WATCHDOG),y)
 obj-$(CONFIG_DETECT_SOFTLOCKUP) += softlockup.o
-obj-$(CONFIG_NMI_WATCHDOG) += nmi_watchdog.o
+endif
 obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
+obj-$(CONFIG_NMI_WATCHDOG) += watchdog.o
 obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
 obj-$(CONFIG_SECCOMP) += seccomp.o
 obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ac72c9e..2165b22 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -704,6 +704,15 @@ static struct ctl_table kern_table[] = {
 		.mode           = 0644,
 		.proc_handler   = proc_nmi_enabled,
 	},
+	{
+		.procname	= "watchdog_thresh",
+		.data		= &softlockup_thresh,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dowatchdog_thresh,
+		.extra1		= &neg_one,
+		.extra2		= &sixty,
+	},
 #endif
 #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86) && !defined(CONFIG_NMI_WATCHDOG)
 	{
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
new file mode 100644
index 0000000..fad5b1a
--- /dev/null
+++ b/kernel/watchdog.c
@@ -0,0 +1,570 @@
+/*
+ * Detect Hard/Soft Lockups using the NMI
+ *
+ * started by Don Zickus, Copyright (C) 2010 Red Hat, Inc.
+ *
+ * this code detects hard lockups: incidents in where on a CPU
+ * the kernel does not respond to anything except NMI.
+ *
+ * Note: Most of this code is borrowed heavily from softlockup.c,
+ * so thanks to Ingo for the initial implementation.
+ * Some chunks also taken from arch/x86/kernel/apic/nmi.c, thanks
+ * to those contributors as well.
+ */
+
+#include <linux/mm.h>
+#include <linux/cpu.h>
+#include <linux/nmi.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
+#include <linux/lockdep.h>
+#include <linux/notifier.h>
+#include <linux/module.h>
+#include <linux/sysctl.h>
+
+#include <asm/irq_regs.h>
+#include <linux/perf_event.h>
+
+int nmi_watchdog_enabled;
+int __read_mostly softlockup_thresh = 60;
+
+static DECLARE_BITMAP(hardlockup_mask, CONFIG_NR_CPUS) __read_mostly;
+static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
+static DEFINE_PER_CPU(struct task_struct *, softlockup_watchdog);
+static DEFINE_PER_CPU(struct hrtimer, watchdog_hrtimer);
+#ifdef CONFIG_PERF_EVENTS_NMI
+static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts);
+static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
+static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
+#endif
+
+static int __read_mostly did_panic;
+static int __initdata no_watchdog;
+
+
+/* boot commands */
+/*
+ * Should we panic when a soft-lockup or hard-lockup occurs:
+ */
+#ifdef CONFIG_PERF_EVENTS_NMI
+static int hardlockup_panic;
+
+static int __init hardlockup_panic_setup(char *str)
+{
+	if (!strncmp(str, "panic", 5))
+		hardlockup_panic = 1;
+	return 1;
+}
+__setup("nmi_watchdog=", hardlockup_panic_setup);
+#endif
+
+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);
+
+static int __init no_watchdog_setup(char *str)
+{
+	no_watchdog = 1;
+	return 1;
+}
+__setup("no_watchdog", no_watchdog_setup);
+
+/* deprecated */
+static int __init nosoftlockup_setup(char *str)
+{
+	no_watchdog = 1;
+	return 1;
+}
+__setup("nosoftlockup", nosoftlockup_setup);
+static int __init nonmi_watchdog_setup(char *str)
+{
+	no_watchdog = 1;
+	return 1;
+}
+__setup("nonmi_watchdog", nonmi_watchdog_setup);
+/*  */
+
+
+/*
+ * Returns seconds, approximately.  We don't need nanosecond
+ * resolution, and we don't need to waste time with a big divide when
+ * 2^30ns == 1.074s.
+ */
+static unsigned long get_timestamp(int this_cpu)
+{
+	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
+}
+
+static unsigned long get_sample_period(void)
+{
+	/*
+	 * convert softlockup_thresh from seconds to ns
+	 * the divide by 5 is to give hrtimer 5 chances to
+	 * increment before the hardlockup detector generates
+	 * a warning
+	 */
+	return softlockup_thresh / 5 * NSEC_PER_SEC;
+}
+
+/* Commands for resetting the watchdog */
+static void __touch_watchdog(void)
+{
+	int this_cpu = raw_smp_processor_id();
+
+	__raw_get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+}
+
+void touch_watchdog(void)
+{
+	__raw_get_cpu_var(watchdog_touch_ts) = 0;
+}
+EXPORT_SYMBOL(touch_watchdog);
+
+void touch_all_watchdog(void)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu)
+		per_cpu(watchdog_touch_ts, cpu) = 0;
+}
+
+/* deprecated functions */
+void touch_nmi_watchdog(void)
+{
+	touch_watchdog();
+}
+EXPORT_SYMBOL(touch_nmi_watchdog);
+
+void touch_all_nmi_watchdog(void)
+{
+	touch_all_watchdog();
+}
+
+void touch_softlockup_watchdog(void)
+{
+	touch_watchdog();
+}
+
+void touch_all_softlockup_watchdogs(void)
+{
+	touch_all_watchdog();
+}
+
+void softlockup_tick(void)
+{
+}
+/* end of deprecated functions */
+
+#ifdef CONFIG_PERF_EVENTS_NMI
+/* watchdog detector functions */
+static int is_hardlockup(int cpu)
+{
+	unsigned long hrint = per_cpu(hrtimer_interrupts, cpu);
+
+	if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
+		return 1;
+
+	per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
+	return 0;
+}
+#endif
+
+static int is_softlockup(unsigned long touch_ts, int cpu)
+{
+	unsigned long now = get_timestamp(cpu);
+
+	/* Warn about unreasonable delays: */
+	if (now > (touch_ts + softlockup_thresh))
+		return now - touch_ts;
+
+	return 0;
+}
+
+static int
+watchdog_panic(struct notifier_block *this, unsigned long event, void *ptr)
+{
+	did_panic = 1;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = watchdog_panic,
+};
+
+#ifdef CONFIG_PERF_EVENTS_NMI
+struct perf_event_attr wd_hw_attr = {
+	.type		= PERF_TYPE_HARDWARE,
+	.config		= PERF_COUNT_HW_CPU_CYCLES,
+	.size		= sizeof(struct perf_event_attr),
+	.pinned		= 1,
+	.disabled	= 1,
+};
+
+struct perf_event_attr wd_sw_attr = {
+	.type		= PERF_TYPE_SOFTWARE,
+	.config		= PERF_COUNT_SW_CPU_CLOCK,
+	.size		= sizeof(struct perf_event_attr),
+	.pinned		= 1,
+	.disabled	= 1,
+};
+
+/* Callback function for perf event subsystem */
+void watchdog_overflow_callback(struct perf_event *event, int nmi,
+		 struct perf_sample_data *data,
+		 struct pt_regs *regs)
+{
+	int this_cpu = smp_processor_id();
+	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
+
+	if (touch_ts == 0) {
+		__touch_watchdog();
+		return;
+	}
+
+	/* check for a hardlockup
+	 * This is done by making sure our timer interrupt
+	 * is incrementing.  The timer interrupt should have
+	 * fired multiple times before we overflow'd.  If it hasn't
+	 * then this is a good indication the cpu is stuck
+	 */
+	if (is_hardlockup(this_cpu)) {
+		/* only print hardlockups once */
+		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
+			return;
+
+		if (hardlockup_panic)
+			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
+		else
+			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
+
+		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
+		return;
+	}
+
+	cpumask_clear_cpu(this_cpu, to_cpumask(hardlockup_mask));
+	return;
+}
+#endif /* CONFIG_PERF_EVENTS_NMI */
+
+/* watchdog kicker functions */
+static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
+{
+	int this_cpu = smp_processor_id();
+	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
+	struct pt_regs *regs = get_irq_regs();
+	int duration;
+
+#ifdef CONFIG_PERF_EVENTS_NMI
+	/* kick the hardlockup detector */
+	__get_cpu_var(hrtimer_interrupts)++;
+#endif
+
+	/* kick the softlockup detector */
+	wake_up_process(__get_cpu_var(softlockup_watchdog));
+
+	/* .. and repeat */
+	hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
+
+	if (touch_ts == 0) {
+		__touch_watchdog();
+		return HRTIMER_RESTART;
+	}
+
+	/* check for a softlockup
+	 * This is done by making sure a high priority task is
+	 * being scheduled.  The task touches the watchdog to
+	 * indicate it is getting cpu time.  If it hasn't then
+	 * this is a good indication some task is hogging the cpu
+	 */
+	duration = is_softlockup(touch_ts, this_cpu);
+	if (duration) {
+		printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n",
+			this_cpu, duration,
+			current->comm, task_pid_nr(current));
+		print_modules();
+		print_irqtrace_events(current);
+		if (regs)
+			show_regs(regs);
+		else
+			dump_stack();
+
+		if (softlockup_panic)
+			panic("softlockup: hung tasks");
+	}
+
+	return HRTIMER_RESTART;
+}
+
+
+/*
+ * The watchdog thread - touches the timestamp.
+ */
+static int watchdog(void *__bind_cpu)
+{
+	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
+	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, (unsigned long)__bind_cpu);
+
+	sched_setscheduler(current, SCHED_FIFO, &param);
+
+	/* initialize timestamp */
+	__touch_watchdog();
+
+	/* kick off the timer for the hardlockup detector */
+	/* done here because hrtimer_start can only pin to smp_processor_id() */
+	hrtimer_start(hrtimer, ns_to_ktime(get_sample_period()),
+		      HRTIMER_MODE_REL_PINNED);
+
+	set_current_state(TASK_INTERRUPTIBLE);
+	/*
+	 * Run briefly once per second to reset the softlockup timestamp.
+	 * If this gets delayed for more than 60 seconds then the
+	 * debug-printout triggers in softlockup_tick().
+	 */
+	while (!kthread_should_stop()) {
+		__touch_watchdog();
+		schedule();
+
+		if (kthread_should_stop())
+			break;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+	}
+	__set_current_state(TASK_RUNNING);
+
+	return 0;
+}
+
+
+/* prepare/enable/disable routines */
+static int watchdog_prepare_cpu(int cpu)
+{
+	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
+
+	BUG_ON(per_cpu(softlockup_watchdog, cpu));
+	hrtimer_init(hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer->function = watchdog_timer_fn;
+
+	return 0;
+}
+
+static int watchdog_enable(int cpu)
+{
+	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
+#ifdef CONFIG_PERF_EVENTS_NMI
+	struct perf_event_attr *wd_attr;
+	struct perf_event *event = per_cpu(watchdog_ev, cpu);
+
+	/* is it already setup and enabled? */
+	if (event && event->state > PERF_EVENT_STATE_OFF)
+		goto out;
+
+	/* it is setup but not enabled */
+	if (event != NULL)
+		goto out_enable;
+
+	/* Try to register using hardware perf events */
+	wd_attr = &wd_hw_attr;
+	wd_attr->sample_period = hw_nmi_get_sample_period();
+	event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback);
+	if (!IS_ERR(event)) {
+		printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n");
+		goto out_save;
+	}
+
+	printk(KERN_ERR "NMI watchdog failed to create perf event on cpu%i: %p\n", cpu, event);
+	return -1;
+
+	/* success path */
+out_save:
+	per_cpu(watchdog_ev, cpu) = event;
+out_enable:
+	perf_event_enable(per_cpu(watchdog_ev, cpu));
+out:
+#endif /* CONFIG_PERF_EVENTS_NMI */
+
+	/* create the watchdog thread */
+	if (!p) {
+		p = kthread_create(watchdog, (void *)(unsigned long)cpu, "watchdog/%d", cpu);
+		if (IS_ERR(p)) {
+			printk(KERN_ERR "softlockup watchdog for %i failed\n", cpu);
+			return -1;
+		}
+		kthread_bind(p, cpu);
+		per_cpu(watchdog_touch_ts, cpu) = 0;
+		per_cpu(softlockup_watchdog, cpu) = p;
+		wake_up_process(p);
+	}
+
+	return 0;
+}
+
+static void watchdog_disable(int cpu)
+{
+	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
+	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
+#ifdef CONFIG_PERF_EVENTS_NMI
+	struct perf_event *event = per_cpu(watchdog_ev, cpu);
+#endif
+
+	/*
+	 * cancel the timer first to stop incrementing the stats
+	 * and waking up the kthread
+	 */
+	hrtimer_cancel(hrtimer);
+
+#ifdef CONFIG_PERF_EVENTS_NMI
+	if (event) {
+		perf_event_disable(event);
+		per_cpu(watchdog_ev, cpu) = NULL;
+
+		/* should be in cleanup, but blocks oprofile */
+		perf_event_release_kernel(event);
+	}
+#endif
+
+	if (p) {
+		per_cpu(softlockup_watchdog, cpu) = NULL;
+		kthread_stop(p);
+	}
+
+	/* if any cpu succeeds, watchdog is considered enabled for the system */
+	nmi_watchdog_enabled = 1;
+}
+
+static void watchdog_cleanup(int cpu)
+{
+
+}
+
+static void watchdog_enable_all_cpus(void)
+{
+	int cpu;
+	int result;
+
+	for_each_online_cpu(cpu)
+		result += watchdog_enable(cpu);
+
+	if (result)
+		printk(KERN_ERR "watchdog: failed to be enabled on some cpus\n");
+
+}
+
+static void watchdog_disable_all_cpus(void)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu)
+		watchdog_disable(cpu);
+
+	/* if all watchdogs are disabled, then they are disabled for the system */
+	nmi_watchdog_enabled = 0;
+}
+
+
+/* sysctl functions */
+#ifdef CONFIG_SYSCTL
+/*
+ * proc handler for /proc/sys/kernel/nmi_watchdog
+ */
+
+int proc_nmi_enabled(struct ctl_table *table, int write,
+		     void __user *buffer, size_t *length, loff_t *ppos)
+{
+	touch_all_watchdog();
+	proc_dointvec(table, write, buffer, length, ppos);
+
+	if (nmi_watchdog_enabled)
+		watchdog_enable_all_cpus();
+	else
+		watchdog_disable_all_cpus();
+	return 0;
+}
+
+int proc_dowatchdog_thresh(struct ctl_table *table, int write,
+			     void __user *buffer,
+			     size_t *lenp, loff_t *ppos)
+{
+	touch_all_watchdog();
+	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+}
+
+/* stub functions */
+int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
+			     void __user *buffer,
+			     size_t *lenp, loff_t *ppos)
+{
+	return proc_dowatchdog_thresh(table, write, buffer, lenp, ppos);
+}
+/* end of stub functions */
+#endif /* CONFIG_SYSCTL */
+
+
+/*
+ * Create/destroy watchdog threads as CPUs come and go:
+ */
+static int __cpuinit
+cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+	int hotcpu = (unsigned long)hcpu;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+	case CPU_UP_PREPARE_FROZEN:
+		if (watchdog_prepare_cpu(hotcpu))
+			return NOTIFY_BAD;
+		break;
+	case CPU_ONLINE:
+	case CPU_ONLINE_FROZEN:
+		if (watchdog_enable(hotcpu))
+			return NOTIFY_BAD;
+		break;
+#ifdef CONFIG_HOTPLUG_CPU
+	case CPU_UP_CANCELED:
+	case CPU_UP_CANCELED_FROZEN:
+		watchdog_disable(hotcpu);
+		break;
+	case CPU_DEAD:
+	case CPU_DEAD_FROZEN:
+		watchdog_disable(hotcpu);
+		watchdog_cleanup(hotcpu);
+		break;
+#endif /* CONFIG_HOTPLUG_CPU */
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __cpuinitdata cpu_nfb = {
+	.notifier_call = cpu_callback
+};
+
+static int __init spawn_watchdog_task(void)
+{
+	void *cpu = (void *)(long)smp_processor_id();
+	int err;
+
+	if (no_watchdog)
+		return 0;
+
+	err = cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);
+	if (err == NOTIFY_BAD) {
+		BUG();
+		return 1;
+	}
+	cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
+	register_cpu_notifier(&cpu_nfb);
+
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
+
+	return 0;
+}
+early_initcall(spawn_watchdog_task);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e2e73cc..280794a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -171,15 +171,24 @@ config DETECT_SOFTLOCKUP
 	   support it.)
 
 config NMI_WATCHDOG
-	bool "Detect Hard Lockups with an NMI Watchdog"
-	depends on DEBUG_KERNEL && PERF_EVENTS && PERF_EVENTS_NMI
+	bool "Detect Hard and Soft Lockups"
+	depends on DEBUG_KERNEL
+	default DETECT_SOFTLOCKUP
 	help
 	  Say Y here to enable the kernel to use the NMI as a watchdog
-	  to detect hard lockups.  This is useful when a cpu hangs for no
-	  reason but can still respond to NMIs.  A backtrace is displayed
-	  for reviewing and reporting.
+	  to detect hard and soft lockups.
+
+	  Softlockups are bugs that cause the kernel to loop in kernel
+	  mode for more than 60 seconds, without giving other tasks a
+	  chance to run.  The current stack trace is displayed upon
+	  detection and the system will stay locked up.
+
+	  Hardlockups are bugs that cause the cpu to loop in kernel mode
+	  for more than 60 seconds, without letting other interrupts a
+	  chance to run.  The current stack trace is displayed upon detection
+	  and the system will stay locked up.
 
-	  The overhead should be minimal, just an extra NMI every few
+	  The overhead should me minimal, just an extra NMI every few
 	  seconds.
 
 config BOOTPARAM_SOFTLOCKUP_PANIC
-- 
1.6.5.2

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Randy Dunlap]

On Thu, 15 Apr 2010 17:25:10 -0400 Don Zickus wrote:

>  arch/x86/kernel/apic/hw_nmi.c |    2 +-
>  include/linux/sched.h         |    6 +
>  init/Kconfig                  |    1 +
>  kernel/Makefile               |    4 +-
>  kernel/sysctl.c               |    9 +
>  kernel/watchdog.c             |  570 +++++++++++++++++++++++++++++++++++++++++
>  lib/Kconfig.debug             |   21 +-
>  7 files changed, 605 insertions(+), 8 deletions(-)
>  create mode 100644 kernel/watchdog.c

Updates to Documentation/kernel-parameters.txt ??


> +static int __init no_watchdog_setup(char *str)
> +{
> +	no_watchdog = 1;
> +	return 1;
> +}
> +__setup("no_watchdog", no_watchdog_setup);

New, please document.

> +/* deprecated */
> +static int __init nosoftlockup_setup(char *str)
> +{
> +	no_watchdog = 1;
> +	return 1;
> +}

That's not marked as deprecated anywhere?

> +__setup("nosoftlockup", nosoftlockup_setup);
> +static int __init nonmi_watchdog_setup(char *str)
> +{
> +	no_watchdog = 1;
> +	return 1;
> +}
> +__setup("nonmi_watchdog", nonmi_watchdog_setup);

New, please document.


> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e2e73cc..280794a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -171,15 +171,24 @@ config DETECT_SOFTLOCKUP
>  	   support it.)
>  
>  config NMI_WATCHDOG
> -	bool "Detect Hard Lockups with an NMI Watchdog"
> -	depends on DEBUG_KERNEL && PERF_EVENTS && PERF_EVENTS_NMI
> +	bool "Detect Hard and Soft Lockups"
> +	depends on DEBUG_KERNEL
> +	default DETECT_SOFTLOCKUP
>  	help
>  	  Say Y here to enable the kernel to use the NMI as a watchdog
> -	  to detect hard lockups.  This is useful when a cpu hangs for no

	                                  preferably:    CPU

> -	  reason but can still respond to NMIs.  A backtrace is displayed
> -	  for reviewing and reporting.
> +	  to detect hard and soft lockups.
> +
> +	  Softlockups are bugs that cause the kernel to loop in kernel
> +	  mode for more than 60 seconds, without giving other tasks a
> +	  chance to run.  The current stack trace is displayed upon
> +	  detection and the system will stay locked up.
> +
> +	  Hardlockups are bugs that cause the cpu to loop in kernel mode

ditto

> +	  for more than 60 seconds, without letting other interrupts a

	                                                             have a

> +	  chance to run.  The current stack trace is displayed upon detection
> +	  and the system will stay locked up.
>  
> -	  The overhead should be minimal, just an extra NMI every few
> +	  The overhead should me minimal, just an extra NMI every few

	                      be

>  	  seconds.
>  
>  config BOOTPARAM_SOFTLOCKUP_PANIC


---
~Randy

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:
> The new nmi_watchdog (which uses the perf event subsystem) is very
> similar in structure to the softlockup detector.  Using Ingo's suggestion,
> I combined the two functionalities into one file, kernel/watchdog.c.
> 
> Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
> sit on top of the perf event subsystem, which is run every 60 seconds or so
> to see if there are any lockups.
> 
> To detect hardlockups, cpus not responding to interrupts, I implemented an
> hrtimer that runs 5 times for every perf event overflow event.  If that stops
> counting on a cpu, then the cpu is most likely in trouble.
> 
> To detect softlockups, tasks not yielding to the scheduler, I used the
> previous kthread idea that now gets kicked every time the hrtimer fires.
> If the kthread isn't being scheduled neither is anyone else and the
> warning is printed to the console.
> 
> I tested this on x86_64 and both the softlockup and hardlockup paths work.
> 
> V2:
> - cleaned up the Kconfig and softlockup combination
> - surrounded hardlockup cases with #ifdef CONFIG_PERF_EVENTS_NMI
> - seperated out the softlockup case from perf event subsystem
> - re-arranged the enabling/disabling nmi watchdog from proc space
> - added cpumasks for hardlockup failure cases
> - removed fallback to soft events if no PMU exists for hard events
> 
> TODO:
> - figure out how to make an arch-agnostic clock2cycles call (if possible)
>   to feed into perf events as a sample period



In fact we also have the sample_freq thing that let you run
against a frequency (events per sec) rather than a sample
period.

We do some calculations that recompute the actual sample
period on top of this frequency in a regular base as the
events come.

It's rather unfortunate we can't have 0.xxxx frequencies
but may be we can work that out by adding a freq_unscale
field, which would basically produce the frequency like
this:

	freq = event->attr.sample_freq * (10 ^ -event->attr.freq_unscale)

This is not trivial though, so let's rather focus on the
real matter for now :)


> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index e8b78a0..79425f9 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -89,7 +89,7 @@ int hw_nmi_is_cpu_stuck(struct pt_regs *regs)
>  
>  u64 hw_nmi_get_sample_period(void)
>  {
> -	return cpu_khz * 1000;
> +	return (u64)(cpu_khz) * 1000 * 60;
>  }
>  
>  #ifdef ARCH_HAS_NMI_WATCHDOG
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 6f7bba9..83be6d7 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -338,6 +338,12 @@ extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
>  					 size_t *lenp, loff_t *ppos);
>  #endif
>  
> +#ifdef CONFIG_NMI_WATCHDOG
> +extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> +				  void __user *buffer,
> +				  size_t *lenp, loff_t *ppos);
> +#endif
> +
>  /* Attach to any functions which should be ignored in wchan output. */
>  #define __sched		__attribute__((__section__(".sched.text")))
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 7331a16..0b83612 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -948,6 +948,7 @@ config PERF_USE_VMALLOC
>  
>  config PERF_EVENTS_NMI
>  	bool
> +	depends on PERF_EVENTS
>  	help
>  	  Arch has support for nmi_watchdog



That looks too general. It's more about the fact the arch supports
cpu cycle events and generates NMIs on overflow.


  
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 8a5abe5..56ba99d 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -75,9 +75,11 @@ obj-$(CONFIG_GCOV_KERNEL) += gcov/
>  obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
>  obj-$(CONFIG_KPROBES) += kprobes.o
>  obj-$(CONFIG_KGDB) += kgdb.o
> +ifneq ($(CONFIG_NMI_WATCHDOG),y)
>  obj-$(CONFIG_DETECT_SOFTLOCKUP) += softlockup.o
> -obj-$(CONFIG_NMI_WATCHDOG) += nmi_watchdog.o
> +endif



I'm confused, do we have two versions of the softlockup
detector now? You should drop the older one.



>  obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
> +obj-$(CONFIG_NMI_WATCHDOG) += watchdog.o
>  obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
>  obj-$(CONFIG_SECCOMP) += seccomp.o
>  obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ac72c9e..2165b22 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -704,6 +704,15 @@ static struct ctl_table kern_table[] = {
>  		.mode           = 0644,
>  		.proc_handler   = proc_nmi_enabled,
>  	},
> +	{
> +		.procname	= "watchdog_thresh",
> +		.data		= &softlockup_thresh,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dowatchdog_thresh,
> +		.extra1		= &neg_one,
> +		.extra2		= &sixty,
> +	},
>  #endif
>  #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86) && !defined(CONFIG_NMI_WATCHDOG)
>  	{
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> new file mode 100644
> index 0000000..fad5b1a
> --- /dev/null
> +++ b/kernel/watchdog.c
> @@ -0,0 +1,570 @@
> +/*
> + * Detect Hard/Soft Lockups using the NMI



Well, softlockups detection doesn't use NMI.



> +/*
> + * Returns seconds, approximately.  We don't need nanosecond
> + * resolution, and we don't need to waste time with a big divide when
> + * 2^30ns == 1.074s.
> + */
> +static unsigned long get_timestamp(int this_cpu)
> +{
> +	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
> +}
> +
> +static unsigned long get_sample_period(void)
> +{
> +	/*
> +	 * convert softlockup_thresh from seconds to ns
> +	 * the divide by 5 is to give hrtimer 5 chances to
> +	 * increment before the hardlockup detector generates
> +	 * a warning
> +	 */
> +	return softlockup_thresh / 5 * NSEC_PER_SEC;
> +}
> +
> +/* Commands for resetting the watchdog */
> +static void __touch_watchdog(void)
> +{
> +	int this_cpu = raw_smp_processor_id();
> +
> +	__raw_get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);



Please use __get_cpu_var() instead so that we keep the preempt disabled
check from smp_processor_id()



> +}
> +
> +void touch_watchdog(void)
> +{
> +	__raw_get_cpu_var(watchdog_touch_ts) = 0;


Same here.



> +}
> +EXPORT_SYMBOL(touch_watchdog);
> +
> +void touch_all_watchdog(void)
> +{
> +	int cpu;
> +
> +	for_each_online_cpu(cpu)
> +		per_cpu(watchdog_touch_ts, cpu) = 0;
> +}
> +
> +/* deprecated functions */
> +void touch_nmi_watchdog(void)
> +{
> +	touch_watchdog();
> +}
> +EXPORT_SYMBOL(touch_nmi_watchdog);
> +
> +void touch_all_nmi_watchdog(void)
> +{
> +	touch_all_watchdog();
> +}
> +
> +void touch_softlockup_watchdog(void)
> +{
> +	touch_watchdog();
> +}
> +
> +void touch_all_softlockup_watchdogs(void)
> +{
> +	touch_all_watchdog();
> +}
> +
> +void softlockup_tick(void)
> +{
> +}
> +/* end of deprecated functions */



You should replace the call sites directly.



> +#ifdef CONFIG_PERF_EVENTS_NMI
> +/* watchdog detector functions */
> +static int is_hardlockup(int cpu)
> +{
> +	unsigned long hrint = per_cpu(hrtimer_interrupts, cpu);
> +
> +	if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> +		return 1;
> +
> +	per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
> +	return 0;
> +}
> +#endif
> +
> +static int is_softlockup(unsigned long touch_ts, int cpu)
> +{
> +	unsigned long now = get_timestamp(cpu);
> +
> +	/* Warn about unreasonable delays: */
> +	if (now > (touch_ts + softlockup_thresh))
> +		return now - touch_ts;
> +
> +	return 0;
> +}
> +
> +static int
> +watchdog_panic(struct notifier_block *this, unsigned long event, void *ptr)
> +{
> +	did_panic = 1;
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block panic_block = {
> +	.notifier_call = watchdog_panic,
> +};
> +
> +#ifdef CONFIG_PERF_EVENTS_NMI
> +struct perf_event_attr wd_hw_attr = {
> +	.type		= PERF_TYPE_HARDWARE,
> +	.config		= PERF_COUNT_HW_CPU_CYCLES,
> +	.size		= sizeof(struct perf_event_attr),
> +	.pinned		= 1,
> +	.disabled	= 1,
> +};
> +
> +struct perf_event_attr wd_sw_attr = {
> +	.type		= PERF_TYPE_SOFTWARE,
> +	.config		= PERF_COUNT_SW_CPU_CLOCK,
> +	.size		= sizeof(struct perf_event_attr),
> +	.pinned		= 1,
> +	.disabled	= 1,
> +};




Why do you keep the software clock, I don't see how it can be
useful to detect hardlockups. It triggers in a regular irq
not an NMI.



> +
> +/* Callback function for perf event subsystem */
> +void watchdog_overflow_callback(struct perf_event *event, int nmi,
> +		 struct perf_sample_data *data,
> +		 struct pt_regs *regs)
> +{
> +	int this_cpu = smp_processor_id();
> +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> +
> +	if (touch_ts == 0) {
> +		__touch_watchdog();
> +		return;
> +	}
> +
> +	/* check for a hardlockup
> +	 * This is done by making sure our timer interrupt
> +	 * is incrementing.  The timer interrupt should have
> +	 * fired multiple times before we overflow'd.  If it hasn't
> +	 * then this is a good indication the cpu is stuck
> +	 */
> +	if (is_hardlockup(this_cpu)) {
> +		/* only print hardlockups once */
> +		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
> +			return;
> +
> +		if (hardlockup_panic)
> +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> +		else
> +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> +
> +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));



May be have an arch spin lock there to update your cpu mask safely.



> +		return;
> +	}
> +
> +	cpumask_clear_cpu(this_cpu, to_cpumask(hardlockup_mask));



Hmm...this is probably not necessary.



> +	return;
> +}
> +#endif /* CONFIG_PERF_EVENTS_NMI */
> +
> +/* watchdog kicker functions */
> +static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> +{
> +	int this_cpu = smp_processor_id();
> +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> +	struct pt_regs *regs = get_irq_regs();
> +	int duration;
> +
> +#ifdef CONFIG_PERF_EVENTS_NMI
> +	/* kick the hardlockup detector */
> +	__get_cpu_var(hrtimer_interrupts)++;
> +#endif



Please avoid such ifdefs in the middle of the code.

It's better to gather hardlockup matters in a single ifdef block:

#ifdef CONFIG_PERF_EVENTS_NMI
define your functions here
#else
define (if needed) your off case functions here (static inline stubs)
#endif



> +
> +	/* kick the softlockup detector */
> +	wake_up_process(__get_cpu_var(softlockup_watchdog));
> +
> +	/* .. and repeat */
> +	hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> +
> +	if (touch_ts == 0) {
> +		__touch_watchdog();
> +		return HRTIMER_RESTART;
> +	}
> +
> +	/* check for a softlockup
> +	 * This is done by making sure a high priority task is
> +	 * being scheduled.  The task touches the watchdog to
> +	 * indicate it is getting cpu time.  If it hasn't then
> +	 * this is a good indication some task is hogging the cpu
> +	 */
> +	duration = is_softlockup(touch_ts, this_cpu);
> +	if (duration) {
> +		printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n",
> +			this_cpu, duration,
> +			current->comm, task_pid_nr(current));
> +		print_modules();
> +		print_irqtrace_events(current);
> +		if (regs)
> +			show_regs(regs);
> +		else
> +			dump_stack();
> +
> +		if (softlockup_panic)
> +			panic("softlockup: hung tasks");



You probably want a backtrace cpu mask here as well
(but better don't use the same than the hardlockup thing)



> +	}
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +
> +/*
> + * The watchdog thread - touches the timestamp.
> + */
> +static int watchdog(void *__bind_cpu)
> +{
> +	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
> +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, (unsigned long)__bind_cpu);
> +
> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +
> +	/* initialize timestamp */
> +	__touch_watchdog();
> +
> +	/* kick off the timer for the hardlockup detector */
> +	/* done here because hrtimer_start can only pin to smp_processor_id() */
> +	hrtimer_start(hrtimer, ns_to_ktime(get_sample_period()),
> +		      HRTIMER_MODE_REL_PINNED);
> +
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	/*
> +	 * Run briefly once per second to reset the softlockup timestamp.
> +	 * If this gets delayed for more than 60 seconds then the
> +	 * debug-printout triggers in softlockup_tick().
> +	 */
> +	while (!kthread_should_stop()) {
> +		__touch_watchdog();
> +		schedule();
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		set_current_state(TASK_INTERRUPTIBLE);
> +	}
> +	__set_current_state(TASK_RUNNING);
> +
> +	return 0;
> +}
> +
> +
> +/* prepare/enable/disable routines */
> +static int watchdog_prepare_cpu(int cpu)
> +{
> +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
> +
> +	BUG_ON(per_cpu(softlockup_watchdog, cpu));



Please warn here instead, nothing seriously dangerous is going to
happen.



> +	hrtimer_init(hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer->function = watchdog_timer_fn;
> +
> +	return 0;
> +}
> +
> +static int watchdog_enable(int cpu)
> +{
> +	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
> +#ifdef CONFIG_PERF_EVENTS_NMI
> +	struct perf_event_attr *wd_attr;
> +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> +
> +	/* is it already setup and enabled? */
> +	if (event && event->state > PERF_EVENT_STATE_OFF)
> +		goto out;
> +
> +	/* it is setup but not enabled */
> +	if (event != NULL)
> +		goto out_enable;
> +
> +	/* Try to register using hardware perf events */
> +	wd_attr = &wd_hw_attr;
> +	wd_attr->sample_period = hw_nmi_get_sample_period();
> +	event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback);
> +	if (!IS_ERR(event)) {
> +		printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n");
> +		goto out_save;
> +	}
> +
> +	printk(KERN_ERR "NMI watchdog failed to create perf event on cpu%i: %p\n", cpu, event);
> +	return -1;
> +
> +	/* success path */
> +out_save:
> +	per_cpu(watchdog_ev, cpu) = event;
> +out_enable:
> +	perf_event_enable(per_cpu(watchdog_ev, cpu));
> +out:
> +#endif /* CONFIG_PERF_EVENTS_NMI */



Especially such kind of idef in a function plus goto in the middle, we really
want to avoid that.

You want a watchdog_nmi_enable() call instead that does nothing
if !CONFIG_PERF_EVENTS_NMI.



> +
> +	/* create the watchdog thread */
> +	if (!p) {
> +		p = kthread_create(watchdog, (void *)(unsigned long)cpu, "watchdog/%d", cpu);
> +		if (IS_ERR(p)) {
> +			printk(KERN_ERR "softlockup watchdog for %i failed\n", cpu);
> +			return -1;
> +		}
> +		kthread_bind(p, cpu);
> +		per_cpu(watchdog_touch_ts, cpu) = 0;
> +		per_cpu(softlockup_watchdog, cpu) = p;
> +		wake_up_process(p);
> +	}
> +
> +	return 0;
> +}
> +
> +static void watchdog_disable(int cpu)
> +{
> +	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
> +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
> +#ifdef CONFIG_PERF_EVENTS_NMI
> +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> +#endif
> +
> +	/*
> +	 * cancel the timer first to stop incrementing the stats
> +	 * and waking up the kthread
> +	 */
> +	hrtimer_cancel(hrtimer);
> +
> +#ifdef CONFIG_PERF_EVENTS_NMI
> +	if (event) {
> +		perf_event_disable(event);
> +		per_cpu(watchdog_ev, cpu) = NULL;
> +
> +		/* should be in cleanup, but blocks oprofile */
> +		perf_event_release_kernel(event);
> +	}
> +#endif
> +
> +	if (p) {
> +		per_cpu(softlockup_watchdog, cpu) = NULL;
> +		kthread_stop(p);
> +	}
> +
> +	/* if any cpu succeeds, watchdog is considered enabled for the system */
> +	nmi_watchdog_enabled = 1;
> +}
> +
> +static void watchdog_cleanup(int cpu)
> +{
> +
> +}
> +
> +static void watchdog_enable_all_cpus(void)
> +{
> +	int cpu;
> +	int result;
> +
> +	for_each_online_cpu(cpu)
> +		result += watchdog_enable(cpu);
> +
> +	if (result)
> +		printk(KERN_ERR "watchdog: failed to be enabled on some cpus\n");
> +
> +}
> +
> +static void watchdog_disable_all_cpus(void)
> +{
> +	int cpu;
> +
> +	for_each_online_cpu(cpu)
> +		watchdog_disable(cpu);
> +
> +	/* if all watchdogs are disabled, then they are disabled for the system */
> +	nmi_watchdog_enabled = 0;
> +}
> +
> +
> +/* sysctl functions */
> +#ifdef CONFIG_SYSCTL
> +/*
> + * proc handler for /proc/sys/kernel/nmi_watchdog
> + */
> +
> +int proc_nmi_enabled(struct ctl_table *table, int write,
> +		     void __user *buffer, size_t *length, loff_t *ppos)
> +{
> +	touch_all_watchdog();



Why do you need to touch watchdogs here?



> +	proc_dointvec(table, write, buffer, length, ppos);
> +
> +	if (nmi_watchdog_enabled)
> +		watchdog_enable_all_cpus();
> +	else
> +		watchdog_disable_all_cpus();
> +	return 0;
> +}
> +
> +int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> +			     void __user *buffer,
> +			     size_t *lenp, loff_t *ppos)
> +{
> +	touch_all_watchdog();



Same here?


> +	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +}
> +
> +/* stub functions */
> +int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
> +			     void __user *buffer,
> +			     size_t *lenp, loff_t *ppos)
> +{
> +	return proc_dowatchdog_thresh(table, write, buffer, lenp, ppos);
> +}
> +/* end of stub functions */
> +#endif /* CONFIG_SYSCTL */
> +
> +
> +/*
> + * Create/destroy watchdog threads as CPUs come and go:
> + */
> +static int __cpuinit
> +cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
> +{
> +	int hotcpu = (unsigned long)hcpu;
> +
> +	switch (action) {
> +	case CPU_UP_PREPARE:
> +	case CPU_UP_PREPARE_FROZEN:
> +		if (watchdog_prepare_cpu(hotcpu))
> +			return NOTIFY_BAD;
> +		break;
> +	case CPU_ONLINE:
> +	case CPU_ONLINE_FROZEN:
> +		if (watchdog_enable(hotcpu))
> +			return NOTIFY_BAD;
> +		break;
> +#ifdef CONFIG_HOTPLUG_CPU
> +	case CPU_UP_CANCELED:
> +	case CPU_UP_CANCELED_FROZEN:
> +		watchdog_disable(hotcpu);
> +		break;
> +	case CPU_DEAD:
> +	case CPU_DEAD_FROZEN:
> +		watchdog_disable(hotcpu);
> +		watchdog_cleanup(hotcpu);
> +		break;
> +#endif /* CONFIG_HOTPLUG_CPU */
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block __cpuinitdata cpu_nfb = {
> +	.notifier_call = cpu_callback
> +};
> +
> +static int __init spawn_watchdog_task(void)
> +{
> +	void *cpu = (void *)(long)smp_processor_id();
> +	int err;
> +
> +	if (no_watchdog)
> +		return 0;
> +
> +	err = cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);
> +	if (err == NOTIFY_BAD) {
> +		BUG();



Please warn instead, there is nothing fatal here.



> +		return 1;
> +	}
> +	cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
> +	register_cpu_notifier(&cpu_nfb);
> +
> +	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
> +
> +	return 0;
> +}
> +early_initcall(spawn_watchdog_task);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e2e73cc..280794a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -171,15 +171,24 @@ config DETECT_SOFTLOCKUP
>  	   support it.)
>  
>  config NMI_WATCHDOG
> -	bool "Detect Hard Lockups with an NMI Watchdog"
> -	depends on DEBUG_KERNEL && PERF_EVENTS && PERF_EVENTS_NMI
> +	bool "Detect Hard and Soft Lockups"
> +	depends on DEBUG_KERNEL
> +	default DETECT_SOFTLOCKUP



I'd suggest to drop the NMI prefix, this is not anymore
about detecting hard lockups only.

May be config WATCHDOG or config LOCKUP_DETECTOR if it's taken
already.


Also you should half-drop the DETECT_SOFTLOCKUP thing:
keep it's definition but drop the ability to choose it from
the prompt:

config DETECT_SOFTLOCKUP
	bool
	depends on DEBUG_KERNEL && !S390
	default y

This way we keep it for compatibility with def_configs, it will
enable the WATCHDOG by default if it is "y", we can schedule
its removal later.


Thanks.

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:
> > The new nmi_watchdog (which uses the perf event subsystem) is very
> > similar in structure to the softlockup detector.  Using Ingo's suggestion,
> > I combined the two functionalities into one file, kernel/watchdog.c.
> > 
> > Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
> > sit on top of the perf event subsystem, which is run every 60 seconds or so
> > to see if there are any lockups.
> > 
> > To detect hardlockups, cpus not responding to interrupts, I implemented an
> > hrtimer that runs 5 times for every perf event overflow event.  If that stops
> > counting on a cpu, then the cpu is most likely in trouble.
> > 
> > To detect softlockups, tasks not yielding to the scheduler, I used the
> > previous kthread idea that now gets kicked every time the hrtimer fires.
> > If the kthread isn't being scheduled neither is anyone else and the
> > warning is printed to the console.
> > 
> > I tested this on x86_64 and both the softlockup and hardlockup paths work.
> > 
> > V2:
> > - cleaned up the Kconfig and softlockup combination
> > - surrounded hardlockup cases with #ifdef CONFIG_PERF_EVENTS_NMI
> > - seperated out the softlockup case from perf event subsystem
> > - re-arranged the enabling/disabling nmi watchdog from proc space
> > - added cpumasks for hardlockup failure cases
> > - removed fallback to soft events if no PMU exists for hard events
> > 
> > TODO:
> > - figure out how to make an arch-agnostic clock2cycles call (if possible)
> >   to feed into perf events as a sample period
> 
> 
> 
> In fact we also have the sample_freq thing that let you run
> against a frequency (events per sec) rather than a sample
> period.
> 
> We do some calculations that recompute the actual sample
> period on top of this frequency in a regular base as the
> events come.
> 
> It's rather unfortunate we can't have 0.xxxx frequencies
> but may be we can work that out by adding a freq_unscale
> field, which would basically produce the frequency like
> this:
> 
> 	freq = event->attr.sample_freq * (10 ^ -event->attr.freq_unscale)
> 
> This is not trivial though, so let's rather focus on the
> real matter for now :)

Interesting thanks.

> 
> 
> > diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> > index e8b78a0..79425f9 100644
> > --- a/arch/x86/kernel/apic/hw_nmi.c
> > +++ b/arch/x86/kernel/apic/hw_nmi.c
> > @@ -89,7 +89,7 @@ int hw_nmi_is_cpu_stuck(struct pt_regs *regs)
> >  
> >  u64 hw_nmi_get_sample_period(void)
> >  {
> > -	return cpu_khz * 1000;
> > +	return (u64)(cpu_khz) * 1000 * 60;
> >  }
> >  
> >  #ifdef ARCH_HAS_NMI_WATCHDOG
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 6f7bba9..83be6d7 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -338,6 +338,12 @@ extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
> >  					 size_t *lenp, loff_t *ppos);
> >  #endif
> >  
> > +#ifdef CONFIG_NMI_WATCHDOG
> > +extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> > +				  void __user *buffer,
> > +				  size_t *lenp, loff_t *ppos);
> > +#endif
> > +
> >  /* Attach to any functions which should be ignored in wchan output. */
> >  #define __sched		__attribute__((__section__(".sched.text")))
> >  
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 7331a16..0b83612 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -948,6 +948,7 @@ config PERF_USE_VMALLOC
> >  
> >  config PERF_EVENTS_NMI
> >  	bool
> > +	depends on PERF_EVENTS
> >  	help
> >  	  Arch has support for nmi_watchdog
> 
> 
> 
> That looks too general. It's more about the fact the arch supports
> cpu cycle events and generates NMIs on overflow.

I was trying to figure out a way to add the PERF_EVENTS dependency as I
didn't want to impose it on the CONFIG_NMI_WATCHDOG if that config
supported softlockup (which doesn't need the PERF_EVENTS).

> 
> 
>   
> > diff --git a/kernel/Makefile b/kernel/Makefile
> > index 8a5abe5..56ba99d 100644
> > --- a/kernel/Makefile
> > +++ b/kernel/Makefile
> > @@ -75,9 +75,11 @@ obj-$(CONFIG_GCOV_KERNEL) += gcov/
> >  obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
> >  obj-$(CONFIG_KPROBES) += kprobes.o
> >  obj-$(CONFIG_KGDB) += kgdb.o
> > +ifneq ($(CONFIG_NMI_WATCHDOG),y)
> >  obj-$(CONFIG_DETECT_SOFTLOCKUP) += softlockup.o
> > -obj-$(CONFIG_NMI_WATCHDOG) += nmi_watchdog.o
> > +endif
> 
> 
> 
> I'm confused, do we have two versions of the softlockup
> detector now? You should drop the older one.

Originally Ingo talked about a migration path, so I was going to support
the older one in case the new one was having issues, sort of like what he
suggested about moving the nmi code from arch/x86/kernel/apic/nmi.c to
kernel/watchdog.c.  But I can probably drop the softlockup case as the
migration isn't as tricky as the nmi case.

> 
> 
> 
> >  obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
> > +obj-$(CONFIG_NMI_WATCHDOG) += watchdog.o
> >  obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
> >  obj-$(CONFIG_SECCOMP) += seccomp.o
> >  obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index ac72c9e..2165b22 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -704,6 +704,15 @@ static struct ctl_table kern_table[] = {
> >  		.mode           = 0644,
> >  		.proc_handler   = proc_nmi_enabled,
> >  	},
> > +	{
> > +		.procname	= "watchdog_thresh",
> > +		.data		= &softlockup_thresh,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0644,
> > +		.proc_handler	= proc_dowatchdog_thresh,
> > +		.extra1		= &neg_one,
> > +		.extra2		= &sixty,
> > +	},
> >  #endif
> >  #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86) && !defined(CONFIG_NMI_WATCHDOG)
> >  	{
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > new file mode 100644
> > index 0000000..fad5b1a
> > --- /dev/null
> > +++ b/kernel/watchdog.c
> > @@ -0,0 +1,570 @@
> > +/*
> > + * Detect Hard/Soft Lockups using the NMI
> 
> 
> 
> Well, softlockups detection doesn't use NMI.

Yeah, sorry.  I dropped part of the code per your suggestion and forget to
update the comments.

> 
> 
> 
> > +/*
> > + * Returns seconds, approximately.  We don't need nanosecond
> > + * resolution, and we don't need to waste time with a big divide when
> > + * 2^30ns == 1.074s.
> > + */
> > +static unsigned long get_timestamp(int this_cpu)
> > +{
> > +	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
> > +}
> > +
> > +static unsigned long get_sample_period(void)
> > +{
> > +	/*
> > +	 * convert softlockup_thresh from seconds to ns
> > +	 * the divide by 5 is to give hrtimer 5 chances to
> > +	 * increment before the hardlockup detector generates
> > +	 * a warning
> > +	 */
> > +	return softlockup_thresh / 5 * NSEC_PER_SEC;
> > +}
> > +
> > +/* Commands for resetting the watchdog */
> > +static void __touch_watchdog(void)
> > +{
> > +	int this_cpu = raw_smp_processor_id();
> > +
> > +	__raw_get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
> 
> 
> 
> Please use __get_cpu_var() instead so that we keep the preempt disabled
> check from smp_processor_id()

ok.

> 
> 
> 
> > +}
> > +
> > +void touch_watchdog(void)
> > +{
> > +	__raw_get_cpu_var(watchdog_touch_ts) = 0;
> 
> 
> Same here.
> 
> 
> 
> > +}
> > +EXPORT_SYMBOL(touch_watchdog);
> > +
> > +void touch_all_watchdog(void)
> > +{
> > +	int cpu;
> > +
> > +	for_each_online_cpu(cpu)
> > +		per_cpu(watchdog_touch_ts, cpu) = 0;
> > +}
> > +
> > +/* deprecated functions */
> > +void touch_nmi_watchdog(void)
> > +{
> > +	touch_watchdog();
> > +}
> > +EXPORT_SYMBOL(touch_nmi_watchdog);
> > +
> > +void touch_all_nmi_watchdog(void)
> > +{
> > +	touch_all_watchdog();
> > +}
> > +
> > +void touch_softlockup_watchdog(void)
> > +{
> > +	touch_watchdog();
> > +}
> > +
> > +void touch_all_softlockup_watchdogs(void)
> > +{
> > +	touch_all_watchdog();
> > +}
> > +
> > +void softlockup_tick(void)
> > +{
> > +}
> > +/* end of deprecated functions */
> 
> 
> 
> You should replace the call sites directly.

ok.

> 
> 
> 
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +/* watchdog detector functions */
> > +static int is_hardlockup(int cpu)
> > +{
> > +	unsigned long hrint = per_cpu(hrtimer_interrupts, cpu);
> > +
> > +	if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> > +		return 1;
> > +
> > +	per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static int is_softlockup(unsigned long touch_ts, int cpu)
> > +{
> > +	unsigned long now = get_timestamp(cpu);
> > +
> > +	/* Warn about unreasonable delays: */
> > +	if (now > (touch_ts + softlockup_thresh))
> > +		return now - touch_ts;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +watchdog_panic(struct notifier_block *this, unsigned long event, void *ptr)
> > +{
> > +	did_panic = 1;
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block panic_block = {
> > +	.notifier_call = watchdog_panic,
> > +};
> > +
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +struct perf_event_attr wd_hw_attr = {
> > +	.type		= PERF_TYPE_HARDWARE,
> > +	.config		= PERF_COUNT_HW_CPU_CYCLES,
> > +	.size		= sizeof(struct perf_event_attr),
> > +	.pinned		= 1,
> > +	.disabled	= 1,
> > +};
> > +
> > +struct perf_event_attr wd_sw_attr = {
> > +	.type		= PERF_TYPE_SOFTWARE,
> > +	.config		= PERF_COUNT_SW_CPU_CLOCK,
> > +	.size		= sizeof(struct perf_event_attr),
> > +	.pinned		= 1,
> > +	.disabled	= 1,
> > +};
> 
> 
> 
> 
> Why do you keep the software clock, I don't see how it can be
> useful to detect hardlockups. It triggers in a regular irq
> not an NMI.

sorry, I dropped part of the code and forgot to drop this too.

> 
> 
> 
> > +
> > +/* Callback function for perf event subsystem */
> > +void watchdog_overflow_callback(struct perf_event *event, int nmi,
> > +		 struct perf_sample_data *data,
> > +		 struct pt_regs *regs)
> > +{
> > +	int this_cpu = smp_processor_id();
> > +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> > +
> > +	if (touch_ts == 0) {
> > +		__touch_watchdog();
> > +		return;
> > +	}
> > +
> > +	/* check for a hardlockup
> > +	 * This is done by making sure our timer interrupt
> > +	 * is incrementing.  The timer interrupt should have
> > +	 * fired multiple times before we overflow'd.  If it hasn't
> > +	 * then this is a good indication the cpu is stuck
> > +	 */
> > +	if (is_hardlockup(this_cpu)) {
> > +		/* only print hardlockups once */
> > +		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
> > +			return;
> > +
> > +		if (hardlockup_panic)
> > +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > +		else
> > +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > +
> > +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
> 
> 
> 
> May be have an arch spin lock there to update your cpu mask safely.

ok.

> 
> 
> 
> > +		return;
> > +	}
> > +
> > +	cpumask_clear_cpu(this_cpu, to_cpumask(hardlockup_mask));
> 
> 
> 
> Hmm...this is probably not necessary.

I was just thinking of the case where dispite the WARN above, the cpu
actually recovered and then failed again separately.  But I probably won't
spend anymore time defending it. :-)

> 
> 
> 
> > +	return;
> > +}
> > +#endif /* CONFIG_PERF_EVENTS_NMI */
> > +
> > +/* watchdog kicker functions */
> > +static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> > +{
> > +	int this_cpu = smp_processor_id();
> > +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> > +	struct pt_regs *regs = get_irq_regs();
> > +	int duration;
> > +
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +	/* kick the hardlockup detector */
> > +	__get_cpu_var(hrtimer_interrupts)++;
> > +#endif
> 
> 
> 
> Please avoid such ifdefs in the middle of the code.
> 
> It's better to gather hardlockup matters in a single ifdef block:
> 
> #ifdef CONFIG_PERF_EVENTS_NMI
> define your functions here
> #else
> define (if needed) your off case functions here (static inline stubs)
> #endif

ok.

> 
> 
> 
> > +
> > +	/* kick the softlockup detector */
> > +	wake_up_process(__get_cpu_var(softlockup_watchdog));
> > +
> > +	/* .. and repeat */
> > +	hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
> > +
> > +	if (touch_ts == 0) {
> > +		__touch_watchdog();
> > +		return HRTIMER_RESTART;
> > +	}
> > +
> > +	/* check for a softlockup
> > +	 * This is done by making sure a high priority task is
> > +	 * being scheduled.  The task touches the watchdog to
> > +	 * indicate it is getting cpu time.  If it hasn't then
> > +	 * this is a good indication some task is hogging the cpu
> > +	 */
> > +	duration = is_softlockup(touch_ts, this_cpu);
> > +	if (duration) {
> > +		printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n",
> > +			this_cpu, duration,
> > +			current->comm, task_pid_nr(current));
> > +		print_modules();
> > +		print_irqtrace_events(current);
> > +		if (regs)
> > +			show_regs(regs);
> > +		else
> > +			dump_stack();
> > +
> > +		if (softlockup_panic)
> > +			panic("softlockup: hung tasks");
> 
> 
> 
> You probably want a backtrace cpu mask here as well
> (but better don't use the same than the hardlockup thing)

yup.

> 
> 
> 
> > +	}
> > +
> > +	return HRTIMER_RESTART;
> > +}
> > +
> > +
> > +/*
> > + * The watchdog thread - touches the timestamp.
> > + */
> > +static int watchdog(void *__bind_cpu)
> > +{
> > +	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
> > +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, (unsigned long)__bind_cpu);
> > +
> > +	sched_setscheduler(current, SCHED_FIFO, &param);
> > +
> > +	/* initialize timestamp */
> > +	__touch_watchdog();
> > +
> > +	/* kick off the timer for the hardlockup detector */
> > +	/* done here because hrtimer_start can only pin to smp_processor_id() */
> > +	hrtimer_start(hrtimer, ns_to_ktime(get_sample_period()),
> > +		      HRTIMER_MODE_REL_PINNED);
> > +
> > +	set_current_state(TASK_INTERRUPTIBLE);
> > +	/*
> > +	 * Run briefly once per second to reset the softlockup timestamp.
> > +	 * If this gets delayed for more than 60 seconds then the
> > +	 * debug-printout triggers in softlockup_tick().
> > +	 */
> > +	while (!kthread_should_stop()) {
> > +		__touch_watchdog();
> > +		schedule();
> > +
> > +		if (kthread_should_stop())
> > +			break;
> > +
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +	}
> > +	__set_current_state(TASK_RUNNING);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +/* prepare/enable/disable routines */
> > +static int watchdog_prepare_cpu(int cpu)
> > +{
> > +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
> > +
> > +	BUG_ON(per_cpu(softlockup_watchdog, cpu));
> 
> 
> 
> Please warn here instead, nothing seriously dangerous is going to
> happen.
> 
> 
> 
> > +	hrtimer_init(hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +	hrtimer->function = watchdog_timer_fn;
> > +
> > +	return 0;
> > +}
> > +
> > +static int watchdog_enable(int cpu)
> > +{
> > +	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +	struct perf_event_attr *wd_attr;
> > +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> > +
> > +	/* is it already setup and enabled? */
> > +	if (event && event->state > PERF_EVENT_STATE_OFF)
> > +		goto out;
> > +
> > +	/* it is setup but not enabled */
> > +	if (event != NULL)
> > +		goto out_enable;
> > +
> > +	/* Try to register using hardware perf events */
> > +	wd_attr = &wd_hw_attr;
> > +	wd_attr->sample_period = hw_nmi_get_sample_period();
> > +	event = perf_event_create_kernel_counter(wd_attr, cpu, -1, watchdog_overflow_callback);
> > +	if (!IS_ERR(event)) {
> > +		printk(KERN_INFO "NMI watchdog enabled, takes one hw-pmu counter.\n");
> > +		goto out_save;
> > +	}
> > +
> > +	printk(KERN_ERR "NMI watchdog failed to create perf event on cpu%i: %p\n", cpu, event);
> > +	return -1;
> > +
> > +	/* success path */
> > +out_save:
> > +	per_cpu(watchdog_ev, cpu) = event;
> > +out_enable:
> > +	perf_event_enable(per_cpu(watchdog_ev, cpu));
> > +out:
> > +#endif /* CONFIG_PERF_EVENTS_NMI */
> 
> 
> 
> Especially such kind of idef in a function plus goto in the middle, we really
> want to avoid that.
> 
> You want a watchdog_nmi_enable() call instead that does nothing
> if !CONFIG_PERF_EVENTS_NMI.

ok.

> 
> 
> 
> > +
> > +	/* create the watchdog thread */
> > +	if (!p) {
> > +		p = kthread_create(watchdog, (void *)(unsigned long)cpu, "watchdog/%d", cpu);
> > +		if (IS_ERR(p)) {
> > +			printk(KERN_ERR "softlockup watchdog for %i failed\n", cpu);
> > +			return -1;
> > +		}
> > +		kthread_bind(p, cpu);
> > +		per_cpu(watchdog_touch_ts, cpu) = 0;
> > +		per_cpu(softlockup_watchdog, cpu) = p;
> > +		wake_up_process(p);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void watchdog_disable(int cpu)
> > +{
> > +	struct task_struct *p = per_cpu(softlockup_watchdog, cpu);
> > +	struct hrtimer *hrtimer = &per_cpu(watchdog_hrtimer, cpu);
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> > +#endif
> > +
> > +	/*
> > +	 * cancel the timer first to stop incrementing the stats
> > +	 * and waking up the kthread
> > +	 */
> > +	hrtimer_cancel(hrtimer);
> > +
> > +#ifdef CONFIG_PERF_EVENTS_NMI
> > +	if (event) {
> > +		perf_event_disable(event);
> > +		per_cpu(watchdog_ev, cpu) = NULL;
> > +
> > +		/* should be in cleanup, but blocks oprofile */
> > +		perf_event_release_kernel(event);
> > +	}
> > +#endif
> > +
> > +	if (p) {
> > +		per_cpu(softlockup_watchdog, cpu) = NULL;
> > +		kthread_stop(p);
> > +	}
> > +
> > +	/* if any cpu succeeds, watchdog is considered enabled for the system */
> > +	nmi_watchdog_enabled = 1;
> > +}
> > +
> > +static void watchdog_cleanup(int cpu)
> > +{
> > +
> > +}
> > +
> > +static void watchdog_enable_all_cpus(void)
> > +{
> > +	int cpu;
> > +	int result;
> > +
> > +	for_each_online_cpu(cpu)
> > +		result += watchdog_enable(cpu);
> > +
> > +	if (result)
> > +		printk(KERN_ERR "watchdog: failed to be enabled on some cpus\n");
> > +
> > +}
> > +
> > +static void watchdog_disable_all_cpus(void)
> > +{
> > +	int cpu;
> > +
> > +	for_each_online_cpu(cpu)
> > +		watchdog_disable(cpu);
> > +
> > +	/* if all watchdogs are disabled, then they are disabled for the system */
> > +	nmi_watchdog_enabled = 0;
> > +}
> > +
> > +
> > +/* sysctl functions */
> > +#ifdef CONFIG_SYSCTL
> > +/*
> > + * proc handler for /proc/sys/kernel/nmi_watchdog
> > + */
> > +
> > +int proc_nmi_enabled(struct ctl_table *table, int write,
> > +		     void __user *buffer, size_t *length, loff_t *ppos)
> > +{
> > +	touch_all_watchdog();
> 
> 
> 
> Why do you need to touch watchdogs here?

just legacy stuff I copied over.  I can remove.

> 
> 
> 
> > +	proc_dointvec(table, write, buffer, length, ppos);
> > +
> > +	if (nmi_watchdog_enabled)
> > +		watchdog_enable_all_cpus();
> > +	else
> > +		watchdog_disable_all_cpus();
> > +	return 0;
> > +}
> > +
> > +int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> > +			     void __user *buffer,
> > +			     size_t *lenp, loff_t *ppos)
> > +{
> > +	touch_all_watchdog();
> 
> 
> 
> Same here?
> 
> 
> > +	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > +}
> > +
> > +/* stub functions */
> > +int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
> > +			     void __user *buffer,
> > +			     size_t *lenp, loff_t *ppos)
> > +{
> > +	return proc_dowatchdog_thresh(table, write, buffer, lenp, ppos);
> > +}
> > +/* end of stub functions */
> > +#endif /* CONFIG_SYSCTL */
> > +
> > +
> > +/*
> > + * Create/destroy watchdog threads as CPUs come and go:
> > + */
> > +static int __cpuinit
> > +cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
> > +{
> > +	int hotcpu = (unsigned long)hcpu;
> > +
> > +	switch (action) {
> > +	case CPU_UP_PREPARE:
> > +	case CPU_UP_PREPARE_FROZEN:
> > +		if (watchdog_prepare_cpu(hotcpu))
> > +			return NOTIFY_BAD;
> > +		break;
> > +	case CPU_ONLINE:
> > +	case CPU_ONLINE_FROZEN:
> > +		if (watchdog_enable(hotcpu))
> > +			return NOTIFY_BAD;
> > +		break;
> > +#ifdef CONFIG_HOTPLUG_CPU
> > +	case CPU_UP_CANCELED:
> > +	case CPU_UP_CANCELED_FROZEN:
> > +		watchdog_disable(hotcpu);
> > +		break;
> > +	case CPU_DEAD:
> > +	case CPU_DEAD_FROZEN:
> > +		watchdog_disable(hotcpu);
> > +		watchdog_cleanup(hotcpu);
> > +		break;
> > +#endif /* CONFIG_HOTPLUG_CPU */
> > +	}
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block __cpuinitdata cpu_nfb = {
> > +	.notifier_call = cpu_callback
> > +};
> > +
> > +static int __init spawn_watchdog_task(void)
> > +{
> > +	void *cpu = (void *)(long)smp_processor_id();
> > +	int err;
> > +
> > +	if (no_watchdog)
> > +		return 0;
> > +
> > +	err = cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);
> > +	if (err == NOTIFY_BAD) {
> > +		BUG();
> 
> 
> 
> Please warn instead, there is nothing fatal here.
> 
> 
> 
> > +		return 1;
> > +	}
> > +	cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
> > +	register_cpu_notifier(&cpu_nfb);
> > +
> > +	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
> > +
> > +	return 0;
> > +}
> > +early_initcall(spawn_watchdog_task);
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index e2e73cc..280794a 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -171,15 +171,24 @@ config DETECT_SOFTLOCKUP
> >  	   support it.)
> >  
> >  config NMI_WATCHDOG
> > -	bool "Detect Hard Lockups with an NMI Watchdog"
> > -	depends on DEBUG_KERNEL && PERF_EVENTS && PERF_EVENTS_NMI
> > +	bool "Detect Hard and Soft Lockups"
> > +	depends on DEBUG_KERNEL
> > +	default DETECT_SOFTLOCKUP
> 
> 
> 
> I'd suggest to drop the NMI prefix, this is not anymore
> about detecting hard lockups only.
> 
> May be config WATCHDOG or config LOCKUP_DETECTOR if it's taken
> already.

yeah, I thought about that too.  Will work on it.

> 
> 
> Also you should half-drop the DETECT_SOFTLOCKUP thing:
> keep it's definition but drop the ability to choose it from
> the prompt:
> 
> config DETECT_SOFTLOCKUP
> 	bool
> 	depends on DEBUG_KERNEL && !S390
> 	default y
> 
> This way we keep it for compatibility with def_configs, it will
> enable the WATCHDOG by default if it is "y", we can schedule
> its removal later.

I understand the general idea but not quite the implementation idea.  I will work
on it and see what I come up with.

Thanks for the review!

Cheers,
Don

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Thu, Apr 15, 2010 at 03:32:51PM -0700, Randy Dunlap wrote:
> On Thu, 15 Apr 2010 17:25:10 -0400 Don Zickus wrote:
> 
> >  arch/x86/kernel/apic/hw_nmi.c |    2 +-
> >  include/linux/sched.h         |    6 +
> >  init/Kconfig                  |    1 +
> >  kernel/Makefile               |    4 +-
> >  kernel/sysctl.c               |    9 +
> >  kernel/watchdog.c             |  570 +++++++++++++++++++++++++++++++++++++++++
> >  lib/Kconfig.debug             |   21 +-
> >  7 files changed, 605 insertions(+), 8 deletions(-)
> >  create mode 100644 kernel/watchdog.c
> 
> Updates to Documentation/kernel-parameters.txt ??

Thanks for the comments, will try to get them added in the next spin.

Cheers,
Don

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Cyrill Gorcunov]

On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
[...]
> > +
> > +/* Callback function for perf event subsystem */
> > +void watchdog_overflow_callback(struct perf_event *event, int nmi,
> > +		 struct perf_sample_data *data,
> > +		 struct pt_regs *regs)
> > +{
> > +	int this_cpu = smp_processor_id();
> > +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> > +
> > +	if (touch_ts == 0) {
> > +		__touch_watchdog();
> > +		return;
> > +	}
> > +
> > +	/* check for a hardlockup
> > +	 * This is done by making sure our timer interrupt
> > +	 * is incrementing.  The timer interrupt should have
> > +	 * fired multiple times before we overflow'd.  If it hasn't
> > +	 * then this is a good indication the cpu is stuck
> > +	 */
> > +	if (is_hardlockup(this_cpu)) {
> > +		/* only print hardlockups once */
> > +		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
> > +			return;
> > +
> > +		if (hardlockup_panic)
> > +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > +		else
> > +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > +
> > +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
> 
> 
> 
> May be have an arch spin lock there to update your cpu mask safely.
> 

Hmm, this is NMI handler path so from what we protect this per-cpu data?
Do I miss something? /me confused

	-- Cyrill

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 10:12:13AM -0400, Don Zickus wrote:
> On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> > >  config PERF_EVENTS_NMI
> > >  	bool
> > > +	depends on PERF_EVENTS
> > >  	help
> > >  	  Arch has support for nmi_watchdog
> > 
> > 
> > 
> > That looks too general. It's more about the fact the arch supports
> > cpu cycle events and generates NMIs on overflow.
> 
> I was trying to figure out a way to add the PERF_EVENTS dependency as I
> didn't want to impose it on the CONFIG_NMI_WATCHDOG if that config
> supported softlockup (which doesn't need the PERF_EVENTS).



Yeah and this is fine. I was talking about the help description.


 
> > I'm confused, do we have two versions of the softlockup
> > detector now? You should drop the older one.
> 
> Originally Ingo talked about a migration path, so I was going to support
> the older one in case the new one was having issues, sort of like what he
> suggested about moving the nmi code from arch/x86/kernel/apic/nmi.c to
> kernel/watchdog.c.  But I can probably drop the softlockup case as the
> migration isn't as tricky as the nmi case.



Ok.

> > > +		return;
> > > +	}
> > > +
> > > +	cpumask_clear_cpu(this_cpu, to_cpumask(hardlockup_mask));
> > 
> > 
> > 
> > Hmm...this is probably not necessary.
> 
> I was just thinking of the case where dispite the WARN above, the cpu
> actually recovered and then failed again separately.  But I probably won't
> spend anymore time defending it. :-)



This is really just a corner case, I guess you don't need to
bother with that. It is actually racy against other cpus and adding
a spinlock here (in the everything is fine path) would be an overkill.

In fact, having two per cpu vars named hardlockup_warned and
softlockup_warned would be better than cpumasks. I'm sorry I
suggested you the cpumask, but such per cpu vars will avoid
you dealing with these synchonization issues. And one of the primary
rules is usually to never take a lock from NMIs if we can :)


 
> > You probably want a backtrace cpu mask here as well
> > (but better don't use the same than the hardlockup thing)
> 
> yup.


So actually, per_cpu softlockup_warned would be better :)


> > Also you should half-drop the DETECT_SOFTLOCKUP thing:
> > keep it's definition but drop the ability to choose it from
> > the prompt:
> > 
> > config DETECT_SOFTLOCKUP
> > 	bool
> > 	depends on DEBUG_KERNEL && !S390
> > 	default y
> > 
> > This way we keep it for compatibility with def_configs, it will
> > enable the WATCHDOG by default if it is "y", we can schedule
> > its removal later.
 
> I understand the general idea but not quite the implementation idea.  I will work
> on it and see what I come up with.


We current have:

config DETECT_SOFTLOCKUP
     bool "Blah"
     depends on DEBUG_KERNEL && !S390
     default y
     help
       .......

The idea is to remove the "Blah" so that the user can't select it
anymore from make menuconfig, and to remove the help too as it's useless
too.

So that config WATCHDOG can be default y if DETECT_SOFTLOCKUP.
Then if someone comes with a config that has DETECT_SOFTLOCKUP,
it's new implementation (WATCHDOG) will enabled by default.

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 06:32:32PM +0400, Cyrill Gorcunov wrote:
> On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> [...]
> > > +
> > > +/* Callback function for perf event subsystem */
> > > +void watchdog_overflow_callback(struct perf_event *event, int nmi,
> > > +		 struct perf_sample_data *data,
> > > +		 struct pt_regs *regs)
> > > +{
> > > +	int this_cpu = smp_processor_id();
> > > +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> > > +
> > > +	if (touch_ts == 0) {
> > > +		__touch_watchdog();
> > > +		return;
> > > +	}
> > > +
> > > +	/* check for a hardlockup
> > > +	 * This is done by making sure our timer interrupt
> > > +	 * is incrementing.  The timer interrupt should have
> > > +	 * fired multiple times before we overflow'd.  If it hasn't
> > > +	 * then this is a good indication the cpu is stuck
> > > +	 */
> > > +	if (is_hardlockup(this_cpu)) {
> > > +		/* only print hardlockups once */
> > > +		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
> > > +			return;
> > > +
> > > +		if (hardlockup_panic)
> > > +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > +		else
> > > +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > +
> > > +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
> > 
> > 
> > 
> > May be have an arch spin lock there to update your cpu mask safely.
> > 
> 
> Hmm, this is NMI handler path so from what we protect this per-cpu data?
> Do I miss something? /me confused


The cpu mask is not per cpu here, this is a shared bitmap, so you
can race against other cpus NMIs.

That said, as I suggested, having a per cpu var that we set when we
warned would be much better than a spinlock here.

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Fri, Apr 16, 2010 at 06:32:32PM +0400, Cyrill Gorcunov wrote:
> On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> [...]
> > > +
> > > +/* Callback function for perf event subsystem */
> > > +void watchdog_overflow_callback(struct perf_event *event, int nmi,
> > > +		 struct perf_sample_data *data,
> > > +		 struct pt_regs *regs)
> > > +{
> > > +	int this_cpu = smp_processor_id();
> > > +	unsigned long touch_ts = per_cpu(watchdog_touch_ts, this_cpu);
> > > +
> > > +	if (touch_ts == 0) {
> > > +		__touch_watchdog();
> > > +		return;
> > > +	}
> > > +
> > > +	/* check for a hardlockup
> > > +	 * This is done by making sure our timer interrupt
> > > +	 * is incrementing.  The timer interrupt should have
> > > +	 * fired multiple times before we overflow'd.  If it hasn't
> > > +	 * then this is a good indication the cpu is stuck
> > > +	 */
> > > +	if (is_hardlockup(this_cpu)) {
> > > +		/* only print hardlockups once */
> > > +		if (cpumask_test_cpu(this_cpu, to_cpumask(hardlockup_mask)))
> > > +			return;
> > > +
> > > +		if (hardlockup_panic)
> > > +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > +		else
> > > +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > +
> > > +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
> > 
> > 
> > 
> > May be have an arch spin lock there to update your cpu mask safely.
> > 
> 
> Hmm, this is NMI handler path so from what we protect this per-cpu data?
> Do I miss something? /me confused

It's not per-cpu which is the problem I believe.  If you have multiple
cpus dealing with hardlockups than they can overwrite each other's data.

Cheers,
Don

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Peter Zijlstra]

On Fri, 2010-04-16 at 16:46 +0200, Frederic Weisbecker wrote:
> > > May be have an arch spin lock there to update your cpu mask safely.
> > > 
> > 
> > Hmm, this is NMI handler path so from what we protect this per-cpu data?
> > Do I miss something? /me confused
> 
> 
> The cpu mask is not per cpu here, this is a shared bitmap, so you
> can race against other cpus NMIs.
> 
> That said, as I suggested, having a per cpu var that we set when we
> warned would be much better than a spinlock here. 

Every time you think NMI and spinlock think FAIL.

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Cyrill Gorcunov]

On Fri, Apr 16, 2010 at 04:46:17PM +0200, Frederic Weisbecker wrote:
...
> > > > +		if (hardlockup_panic)
> > > > +			panic("Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > > +		else
> > > > +			WARN(1, "Watchdog detected hard LOCKUP on cpu %d", this_cpu);
> > > > +
> > > > +		cpumask_set_cpu(this_cpu, to_cpumask(hardlockup_mask));
> > > 
> > > 
> > > 
> > > May be have an arch spin lock there to update your cpu mask safely.
> > > 
> > 
> > Hmm, this is NMI handler path so from what we protect this per-cpu data?
> > Do I miss something? /me confused
> 
> 
> The cpu mask is not per cpu here, this is a shared bitmap, so you
> can race against other cpus NMIs.
> 
> That said, as I suggested, having a per cpu var that we set when we
> warned would be much better than a spinlock here.
> 

yeah, saw DECLARE_BITMAP but read it as DEFINE_PER_CPU for some reason.
having any spinlock in irq handler is really under suspicious.

	-- Cyrill

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 04:53:11PM +0200, Peter Zijlstra wrote:
> On Fri, 2010-04-16 at 16:46 +0200, Frederic Weisbecker wrote:
> > > > May be have an arch spin lock there to update your cpu mask safely.
> > > > 
> > > 
> > > Hmm, this is NMI handler path so from what we protect this per-cpu data?
> > > Do I miss something? /me confused
> > 
> > 
> > The cpu mask is not per cpu here, this is a shared bitmap, so you
> > can race against other cpus NMIs.
> > 
> > That said, as I suggested, having a per cpu var that we set when we
> > warned would be much better than a spinlock here. 
> 
> Every time you think NMI and spinlock think FAIL.


In fact I was first inspired by the x86 nmi watchdog handler
that does this spinlock to protect cpumask, but realize just
after my FAIL ;-)

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Fri, Apr 16, 2010 at 04:43:04PM +0200, Frederic Weisbecker wrote:
> On Fri, Apr 16, 2010 at 10:12:13AM -0400, Don Zickus wrote:
> > On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> > > >  config PERF_EVENTS_NMI
> > > >  	bool
> > > > +	depends on PERF_EVENTS
> > > >  	help
> > > >  	  Arch has support for nmi_watchdog
> > > 
> > > 
> > > 
> > > That looks too general. It's more about the fact the arch supports
> > > cpu cycle events and generates NMIs on overflow.
> > 
> > I was trying to figure out a way to add the PERF_EVENTS dependency as I
> > didn't want to impose it on the CONFIG_NMI_WATCHDOG if that config
> > supported softlockup (which doesn't need the PERF_EVENTS).
> 
> 
> 
> Yeah and this is fine. I was talking about the help description.

Oh. heh.  ok, will expand that.

> 
> 
>  
> > > I'm confused, do we have two versions of the softlockup
> > > detector now? You should drop the older one.
> > 
> > Originally Ingo talked about a migration path, so I was going to support
> > the older one in case the new one was having issues, sort of like what he
> > suggested about moving the nmi code from arch/x86/kernel/apic/nmi.c to
> > kernel/watchdog.c.  But I can probably drop the softlockup case as the
> > migration isn't as tricky as the nmi case.
> 
> 
> 
> Ok.
> 
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	cpumask_clear_cpu(this_cpu, to_cpumask(hardlockup_mask));
> > > 
> > > 
> > > 
> > > Hmm...this is probably not necessary.
> > 
> > I was just thinking of the case where dispite the WARN above, the cpu
> > actually recovered and then failed again separately.  But I probably won't
> > spend anymore time defending it. :-)
> 
> 
> 
> This is really just a corner case, I guess you don't need to
> bother with that. It is actually racy against other cpus and adding
> a spinlock here (in the everything is fine path) would be an overkill.
> 
> In fact, having two per cpu vars named hardlockup_warned and
> softlockup_warned would be better than cpumasks. I'm sorry I
> suggested you the cpumask, but such per cpu vars will avoid
> you dealing with these synchonization issues. And one of the primary
> rules is usually to never take a lock from NMIs if we can :)

Yeah, I guess per cpu is better.  I agree that locks in NMI are frowned
upon but I wasn't sure of it was dealt with.

I'll try to implement this.  Any objections if I combined hardlockup and
softlockup with per cpu watchdog_warn and have bit masks for HARDLOCKUP
and SOFTLOCKUP?  I hate to just waste per cpu space for this.

> 
> 
>  
> > > You probably want a backtrace cpu mask here as well
> > > (but better don't use the same than the hardlockup thing)
> > 
> > yup.
> 
> 
> So actually, per_cpu softlockup_warned would be better :)
> 
> 
> > > Also you should half-drop the DETECT_SOFTLOCKUP thing:
> > > keep it's definition but drop the ability to choose it from
> > > the prompt:
> > > 
> > > config DETECT_SOFTLOCKUP
> > > 	bool
> > > 	depends on DEBUG_KERNEL && !S390
> > > 	default y
> > > 
> > > This way we keep it for compatibility with def_configs, it will
> > > enable the WATCHDOG by default if it is "y", we can schedule
> > > its removal later.
>  
> > I understand the general idea but not quite the implementation idea.  I will work
> > on it and see what I come up with.
> 
> 
> We current have:
> 
> config DETECT_SOFTLOCKUP
>      bool "Blah"
>      depends on DEBUG_KERNEL && !S390
>      default y
>      help
>        .......
> 
> The idea is to remove the "Blah" so that the user can't select it
> anymore from make menuconfig, and to remove the help too as it's useless
> too.
> 
> So that config WATCHDOG can be default y if DETECT_SOFTLOCKUP.
> Then if someone comes with a config that has DETECT_SOFTLOCKUP,
> it's new implementation (WATCHDOG) will enabled by default.

Ah, I missed the bool part.  I got it.  Thanks for the clarification.

Cheers,
Don

> 

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 11:04:07AM -0400, Don Zickus wrote:
> > This is really just a corner case, I guess you don't need to
> > bother with that. It is actually racy against other cpus and adding
> > a spinlock here (in the everything is fine path) would be an overkill.
> > 
> > In fact, having two per cpu vars named hardlockup_warned and
> > softlockup_warned would be better than cpumasks. I'm sorry I
> > suggested you the cpumask, but such per cpu vars will avoid
> > you dealing with these synchonization issues. And one of the primary
> > rules is usually to never take a lock from NMIs if we can :)
> 
> Yeah, I guess per cpu is better.  I agree that locks in NMI are frowned
> upon but I wasn't sure of it was dealt with.


They work in fact. They are just not checked by lockdep.
And mostly they are very dangerous: if something else can
take it (from interrupt, from context) then this is a deadlock.
And even though we ensure this is only taken from NMI, we tend
to avoid that.


 
> I'll try to implement this.  Any objections if I combined hardlockup and
> softlockup with per cpu watchdog_warn and have bit masks for HARDLOCKUP
> and SOFTLOCKUP?  I hate to just waste per cpu space for this.



Hmm, a hardlockup can come in after a softlockup.
Don't worry too much about memory: usually the more you have cpu,
the more you have memory :)
Plus this is debugging code, not something supposed to be enabled
in production.

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Fri, Apr 16, 2010 at 05:32:12PM +0200, Frederic Weisbecker wrote:
> > I'll try to implement this.  Any objections if I combined hardlockup and
> > softlockup with per cpu watchdog_warn and have bit masks for HARDLOCKUP
> > and SOFTLOCKUP?  I hate to just waste per cpu space for this.
> 
> 
> 
> Hmm, a hardlockup can come in after a softlockup.

Let me re-explain what I meant.  It was meant to do double duty.  The
softlockup code only checks the SOFTLOCKUP bit and the hardlockup only
ever checks the HARDLOCKUP bit.

ie if get_cpu_var(watchdog_warn) && HARDLOCKUP { return; }

> Don't worry too much about memory: usually the more you have cpu,
> the more you have memory :)
> Plus this is debugging code, not something supposed to be enabled
> in production.

Well depends on your POV.  In RHEL we enable both NMI_WATCHDOG and
SOFTLOCKUP on production systems (and we have customers that are
thankful for that :-) ).

Cheers,
Don

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 12:14:01PM -0400, Don Zickus wrote:
> On Fri, Apr 16, 2010 at 05:32:12PM +0200, Frederic Weisbecker wrote:
> > > I'll try to implement this.  Any objections if I combined hardlockup and
> > > softlockup with per cpu watchdog_warn and have bit masks for HARDLOCKUP
> > > and SOFTLOCKUP?  I hate to just waste per cpu space for this.
> > 
> > 
> > 
> > Hmm, a hardlockup can come in after a softlockup.
> 
> Let me re-explain what I meant.  It was meant to do double duty.  The
> softlockup code only checks the SOFTLOCKUP bit and the hardlockup only
> ever checks the HARDLOCKUP bit.
> 
> ie if get_cpu_var(watchdog_warn) && HARDLOCKUP { return; }


Ah right.



> 
> > Don't worry too much about memory: usually the more you have cpu,
> > the more you have memory :)
> > Plus this is debugging code, not something supposed to be enabled
> > in production.
> 
> Well depends on your POV.  In RHEL we enable both NMI_WATCHDOG and
> SOFTLOCKUP on production systems (and we have customers that are
> thankful for that :-) ).


Ok :)

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:
> > The new nmi_watchdog (which uses the perf event subsystem) is very
> > similar in structure to the softlockup detector.  Using Ingo's suggestion,
> > I combined the two functionalities into one file, kernel/watchdog.c.
> > 
> > Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
> > sit on top of the perf event subsystem, which is run every 60 seconds or so
> > to see if there are any lockups.

Hello all,

After making a bunch of cleanups, I am stuck debating whether to continue
updating this patch on the stale branch perf/nmi on Ingo's tree or just
repost the whole patch again (which isn't much bigger just adds the
arch/x86/kernel/apic/hw_nmi.c piece).

Part of the new patch series includes removing kernel/nmi_watchdog.c,
which seemed kinda silly because it was only an intermediate file until
things got shifted to kernel/watchdog.c

Thoughts?

Cheers,
Don

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> > On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:
> > > The new nmi_watchdog (which uses the perf event subsystem) is very
> > > similar in structure to the softlockup detector.  Using Ingo's suggestion,
> > > I combined the two functionalities into one file, kernel/watchdog.c.
> > > 
> > > Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
> > > sit on top of the perf event subsystem, which is run every 60 seconds or so
> > > to see if there are any lockups.
> 
> Hello all,
> 
> After making a bunch of cleanups, I am stuck debating whether to continue 
> updating this patch on the stale branch perf/nmi on Ingo's tree or just 
> repost the whole patch again (which isn't much bigger just adds the 
> arch/x86/kernel/apic/hw_nmi.c piece).
> 
> Part of the new patch series includes removing kernel/nmi_watchdog.c, which 
> seemed kinda silly because it was only an intermediate file until things got 
> shifted to kernel/watchdog.c
> 
> Thoughts?

I'd prefer relative patches as the current perf/nmi bits are tested quite 
well.

Intermediate stages are not a problem: 90% of the code in the kernel's Git 
history is 'intermediate' as well, in hindsight. What matters is that the 
workflow that resulted was clean and that the patches were (and are) clean.

	Ingo

Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

[Don Zickus]

On Mon, Apr 19, 2010 at 11:35:29PM +0200, Ingo Molnar wrote:
> 
> * Don Zickus <dzickus@redhat.com> wrote:
> 
> > On Fri, Apr 16, 2010 at 03:47:14AM +0200, Frederic Weisbecker wrote:
> > > On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:
> > > > The new nmi_watchdog (which uses the perf event subsystem) is very
> > > > similar in structure to the softlockup detector.  Using Ingo's suggestion,
> > > > I combined the two functionalities into one file, kernel/watchdog.c.
> > > > 
> > > > Now both the nmi_watchdog (or hardlockup detector) and softlockup detector
> > > > sit on top of the perf event subsystem, which is run every 60 seconds or so
> > > > to see if there are any lockups.
> > 
> > Hello all,
> > 
> > After making a bunch of cleanups, I am stuck debating whether to continue 
> > updating this patch on the stale branch perf/nmi on Ingo's tree or just 
> > repost the whole patch again (which isn't much bigger just adds the 
> > arch/x86/kernel/apic/hw_nmi.c piece).
> > 
> > Part of the new patch series includes removing kernel/nmi_watchdog.c, which 
> > seemed kinda silly because it was only an intermediate file until things got 
> > shifted to kernel/watchdog.c
> > 
> > Thoughts?
> 
> I'd prefer relative patches as the current perf/nmi bits are tested quite 
> well.
> 
> Intermediate stages are not a problem: 90% of the code in the kernel's Git 
> history is 'intermediate' as well, in hindsight. What matters is that the 
> workflow that resulted was clean and that the patches were (and are) clean.

Ok, I'll continue that then.  Thanks.

Cheers,
Don