[patch] serial console vs NMI watchdog

[patch] serial console vs NMI watchdog

[Andrew Morton]

SYSRQ-T on serial console can crash the machine.  This
is because a large amount of output is sent to a slow
device while interrupts are disabled.  The NMI
watchdog triggers.

The interrupt disabling happens in pc_keyb.c:keyboard_interrupt().
Changing this code to *not* disable interrupts looks complex.

I see two ways of fixing this.  One is to do the sysrq
stuff outside the spin_lock_irq(), with:

static void keyboard_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
+	extern void (*sysrq_handler)(void);
+	void (*my_sysrq_handler)(void);

        spin_lock_irq(&kbd_controller_lock);
        handle_kbd_event();
+	my_sysrq_handler = sysrq_handler;
+	sysrq_handler = 0;
        spin_unlock_irq(&kbd_controller_lock);
+	if (my_sysrq_handler)
+		(*my_sysrq_handler)();
}

But I didn't do that, because I suspect there are other
places in the kernel (development and debug stuff) where
we want to turn the NMI watchdog handler off for a while.

So this patch creates a new API function

	enable_nmi_watchdog(int yes);

and uses it within the sysrq code.

BTW: NMI watchdog is now disabled by default in 2.4.3-pre3.
The `nmi_watchdog=1' boot option is needed to enable it.



--- linux-2.4.2-ac16/include/linux/irq.h	Fri Mar  9 17:11:17 2001
+++ linux-ac/include/linux/irq.h	Sat Mar 10 01:02:12 2001
@@ -56,6 +56,20 @@
 
 #include <asm/hw_irq.h> /* the arch dependent stuff */
 
+/**
+ * enable_nmi_watchdog - enables/disables NMI watchdog checking.
+ * @yes: If zero, disable
+ * 
+ * If the architecture supports the NMI watchdog, enable_nmi_watchdog() may be used
+ * to temporarily disable it.  Calls to enable_nmi_watchdog() may be nested - it is
+ * implemented as an up/down counter, so the calls must be balanced.
+ */
+#ifdef ARCH_HAS_NMI_WATCHDOG
+extern void enable_nmi_watchdog(int yes);
+#else
+#define enable_nmi_watchdog(yes) do{} while(0)
+#endif
+
 extern int handle_IRQ_event(unsigned int, struct pt_regs *, struct irqaction *);
 extern int setup_irq(unsigned int , struct irqaction * );
 
--- linux-2.4.2-ac16/include/asm-i386/irq.h	Fri Oct  8 03:17:09 1999
+++ linux-ac/include/asm-i386/irq.h	Fri Mar  9 22:59:15 2001
@@ -32,4 +32,8 @@
 extern void disable_irq_nosync(unsigned int);
 extern void enable_irq(unsigned int);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+#define ARCH_HAS_NMI_WATCHDOG		/* See include/linux/irq.h */
+#endif
+
 #endif /* _ASM_IRQ_H */
--- linux-2.4.2-ac16/drivers/char/sysrq.c	Sun Feb 25 17:37:04 2001
+++ linux-ac/drivers/char/sysrq.c	Fri Mar  9 23:00:39 2001
@@ -23,6 +23,7 @@
 #include <linux/quotaops.h>
 #include <linux/smp_lock.h>
 #include <linux/module.h>
+#include <linux/irq.h>
 
 #include <asm/ptrace.h>
 
@@ -69,6 +70,11 @@
 	if (!key)
 		return;
 
+	/*
+	 * Interrupts are disabled, and serial consoles are slow. So
+	 * Let's suspend the NMI watchdog.
+	 */
+	enable_nmi_watchdog(0);
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq: ");
 	switch (key) {
@@ -152,6 +158,7 @@
 		/* Don't use 'A' as it's handled specially on the Sparc */
 	}
 
+	enable_nmi_watchdog(1);
 	console_loglevel = orig_log_level;
 }
 
--- linux-2.4.2-ac16/arch/i386/kernel/nmi.c	Fri Mar  9 17:10:51 2001
+++ linux-ac/arch/i386/kernel/nmi.c	Sat Mar 10 01:10:50 2001
@@ -226,6 +226,15 @@
 }
 
 static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
+static atomic_t nmi_watchdog_enabled = ATOMIC_INIT(0);	/* 0 == enabled */
+
+void enable_nmi_watchdog(int yes)
+{
+	if (yes)
+		atomic_inc(&nmi_watchdog_enabled);
+	else
+		atomic_dec(&nmi_watchdog_enabled);
+}
 
 void nmi_watchdog_tick (struct pt_regs * regs)
 {
@@ -255,7 +264,7 @@
 
 	sum = apic_timer_irqs[cpu];
 
-	if (last_irq_sums[cpu] == sum) {
+	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_enabled) == 0) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...

Re: [patch] serial console vs NMI watchdog

[Ion Badulescu]

On Sat, 10 Mar 2001 01:21:25 +1100, Andrew Morton <andrewm@uow.edu.au> wrote:

> +/**
> + * enable_nmi_watchdog - enables/disables NMI watchdog checking.
> + * @yes: If zero, disable

Ugh. I have a feeling that your chances to get Linus to accept this are
extremely slim.

Just have two functions, enable_nmi_watchdog and disable_nmi_watchdog.
You can make them inline, or even macros...

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [patch] serial console vs NMI watchdog

[Andrew Morton]

Ion Badulescu wrote:
> 
> On Sat, 10 Mar 2001 01:21:25 +1100, Andrew Morton <andrewm@uow.edu.au> wrote:
> 
> > +/**
> > + * enable_nmi_watchdog - enables/disables NMI watchdog checking.
> > + * @yes: If zero, disable
> 
> Ugh. I have a feeling that your chances to get Linus to accept this are
> extremely slim.
> 
> Just have two functions, enable_nmi_watchdog and disable_nmi_watchdog.
> You can make them inline, or even macros...

You're right.



--- linux-2.4.2-ac16/include/linux/irq.h	Fri Mar  9 17:11:17 2001
+++ linux-ac/include/linux/irq.h	Sat Mar 10 13:34:22 2001
@@ -56,6 +56,21 @@
 
 #include <asm/hw_irq.h> /* the arch dependent stuff */
 
+/**
+ * nmi_watchdog_disable - disable NMI watchdog checking.
+ * 
+ * If the architecture supports the NMI watchdog, nmi_watchdog_disable() may be used
+ * to temporarily disable it.  Use nmi_watchdog_enable() later on.  It is implemented
+ * via an up/down counter, so you must keep the calls balanced.
+ */
+#ifdef ARCH_HAS_NMI_WATCHDOG
+extern void nmi_watchdog_disable(void);
+extern void nmi_watchdog_enable(void);
+#else
+#define nmi_watchdog_disable() do{} while(0)
+#define nmi_watchdog_enable() do{} while(0)
+#endif
+
 extern int handle_IRQ_event(unsigned int, struct pt_regs *, struct irqaction *);
 extern int setup_irq(unsigned int , struct irqaction * );
 
--- linux-2.4.2-ac16/include/asm-i386/irq.h	Fri Oct  8 03:17:09 1999
+++ linux-ac/include/asm-i386/irq.h	Sat Mar 10 02:17:47 2001
@@ -32,4 +32,8 @@
 extern void disable_irq_nosync(unsigned int);
 extern void enable_irq(unsigned int);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+#define ARCH_HAS_NMI_WATCHDOG		/* See include/linux/irq.h */
+#endif
+
 #endif /* _ASM_IRQ_H */
--- linux-2.4.2-ac16/drivers/char/sysrq.c	Sun Feb 25 17:37:04 2001
+++ linux-ac/drivers/char/sysrq.c	Sat Mar 10 13:07:46 2001
@@ -23,6 +23,7 @@
 #include <linux/quotaops.h>
 #include <linux/smp_lock.h>
 #include <linux/module.h>
+#include <linux/irq.h>
 
 #include <asm/ptrace.h>
 
@@ -69,6 +70,11 @@
 	if (!key)
 		return;
 
+	/*
+	 * Interrupts are disabled, and serial consoles are slow. So
+	 * Let's suspend the NMI watchdog.
+	 */
+	nmi_watchdog_disable();
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq: ");
 	switch (key) {
@@ -152,6 +158,7 @@
 		/* Don't use 'A' as it's handled specially on the Sparc */
 	}
 
+	nmi_watchdog_enable();
 	console_loglevel = orig_log_level;
 }
 
--- linux-2.4.2-ac16/arch/i386/kernel/nmi.c	Fri Mar  9 17:10:51 2001
+++ linux-ac/arch/i386/kernel/nmi.c	Sat Mar 10 13:21:59 2001
@@ -226,6 +226,17 @@
 }
 
 static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
+static atomic_t nmi_watchdog_disabled = ATOMIC_INIT(0);
+
+void nmi_watchdog_disable(void)
+{
+	atomic_inc(&nmi_watchdog_disabled);
+}
+
+void nmi_watchdog_enable(void)
+{
+	atomic_dec(&nmi_watchdog_disabled);
+}
 
 void nmi_watchdog_tick (struct pt_regs * regs)
 {
@@ -255,7 +266,7 @@
 
 	sum = apic_timer_irqs[cpu];
 
-	if (last_irq_sums[cpu] == sum) {
+	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_disabled) == 0) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...

Re: [patch] serial console vs NMI watchdog

[Robert Read]

On Sat, Mar 10, 2001 at 01:21:25AM +1100, Andrew Morton wrote:
> +static atomic_t nmi_watchdog_enabled = ATOMIC_INIT(0);	/* 0 == enabled */
> +
> +void enable_nmi_watchdog(int yes)
> +{
> +	if (yes)
> +		atomic_inc(&nmi_watchdog_enabled);
> +	else
> +		atomic_dec(&nmi_watchdog_enabled);
> +}
>  
>  void nmi_watchdog_tick (struct pt_regs * regs)
>  {
> @@ -255,7 +264,7 @@
>  
>  	sum = apic_timer_irqs[cpu];
>  
> -	if (last_irq_sums[cpu] == sum) {
> +	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_enabled) == 0) {

Shouldn't that be atomic_read(&nmi_watchdog_enabled) != 0?

Re: [patch] serial console vs NMI watchdog

[Ingo Molnar]

Andrew,

your patch looks too complex, and doesnt cover the case of the serial
driver deadlocking. Why not add a "touch_nmi_watchdog_counter()" function
that just changes last_irq_sums instead of adding locking? This way
deadlocks will be caught in the serial code too. (because touch_nmi() will
only "postpone" the NMI watchdog lockup event, not disable it.) This
should be a matter of 5 lines ...

	Ingo

Re: [patch] serial console vs NMI watchdog

[Keith Owens]

On Sun, 11 Mar 2001 08:44:24 +0100 (CET), 
Ingo Molnar <mingo@elte.hu> wrote:
>Andrew,
>
>your patch looks too complex, and doesnt cover the case of the serial
>driver deadlocking. Why not add a "touch_nmi_watchdog_counter()" function
>that just changes last_irq_sums instead of adding locking? This way
>deadlocks will be caught in the serial code too. (because touch_nmi() will
>only "postpone" the NMI watchdog lockup event, not disable it.)

kdb has to completely disable the nmi counter while it is in control.
All interrupts are disabled, all but one cpus are spinning, the control
cpu does busy wait while it polls the input devices.  With that model
there is no alternative to a complete disable.

Re: [patch] serial console vs NMI watchdog

[Ingo Molnar]

On Sun, 11 Mar 2001, Keith Owens wrote:

> kdb has to completely disable the nmi counter while it is in control.
> All interrupts are disabled, all but one cpus are spinning, the
> control cpu does busy wait while it polls the input devices.  With
> that model there is no alternative to a complete disable.

it sure has an alternative. The 'cpus spinning' code calls touch_nmi()
within the busy loop, the polling code on the control CPU too. This is
sure more robust and catches lockup bugs in kdb too ...

	Ingo

Re: [patch] serial console vs NMI watchdog

[Keith Owens]

On Sun, 11 Mar 2001 08:53:40 +0100 (CET), 
Ingo Molnar <mingo@elte.hu> wrote:
>it sure has an alternative. The 'cpus spinning' code calls touch_nmi()
>within the busy loop, the polling code on the control CPU too. This is
>sure more robust and catches lockup bugs in kdb too ...

Works for me.  It even makes kdb simpler.

Re: [patch] serial console vs NMI watchdog

[Andrew Morton]

Keith Owens wrote:
> 
> On Sun, 11 Mar 2001 08:53:40 +0100 (CET),
> Ingo Molnar <mingo@elte.hu> wrote:
> >it sure has an alternative. The 'cpus spinning' code calls touch_nmi()
> >within the busy loop, the polling code on the control CPU too. This is
> >sure more robust and catches lockup bugs in kdb too ...
> 
> Works for me.  It even makes kdb simpler.

humm...

OK, this seems doable in the case of serial console.
For CONFIG_LP_CONSOLE (which has the same problem) it
looks like we can just call touch_nmi() in lp_console_write().

I'll see what Tim has to say.

-

[patch] nmi-watchdog-2.4.2-A1

[Ingo Molnar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 670 bytes --]


On Sun, 11 Mar 2001, Keith Owens wrote:

> Works for me.  It even makes kdb simpler.

agreed. Also, touch_nmi_watchdog() is stateless and is thus much less
prone to locking bugs.

i've attached nmi-watchdog-2.4.2-A1 that implements touch_nmi_watchdog(),
ontop of 2.4.2-ac18, and changes show_state() to use this interface. (the
patch also takes the NMI counters out of the obscure in-function place
they used to be.)

the patch compiles & boots just fine on 2.4.2-ac18 in both SMP and
APIC-less-UP mode. The NMI watchdog is functional, and SysRq-T does not
cause a lockup if used with a slow serial console that takes more than 5
seconds to output the tasklist.

	Ingo

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4127 bytes --]

--- linux/include/linux/irq.h.orig	Sun Mar 11 11:20:21 2001
+++ linux/include/linux/irq.h	Sun Mar 11 11:22:27 2001
@@ -57,18 +57,16 @@
 #include <asm/hw_irq.h> /* the arch dependent stuff */
 
 /**
- * nmi_watchdog_disable - disable NMI watchdog checking.
+ * touch_nmi_watchdog - restart NMI watchdog timeout.
  * 
- * If the architecture supports the NMI watchdog, nmi_watchdog_disable() may be used
- * to temporarily disable it.  Use nmi_watchdog_enable() later on.  It is implemented
- * via an up/down counter, so you must keep the calls balanced.
+ * 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.
  */
 #ifdef ARCH_HAS_NMI_WATCHDOG
-extern void nmi_watchdog_disable(void);
-extern void nmi_watchdog_enable(void);
+extern void touch_nmi_watchdog(void);
 #else
-#define nmi_watchdog_disable() do{} while(0)
-#define nmi_watchdog_enable() do{} while(0)
+# define touch_nmi_watchdog() do { } while(0)
 #endif
 
 extern int handle_IRQ_event(unsigned int, struct pt_regs *, struct irqaction *);
--- linux/drivers/char/sysrq.c.orig	Sun Mar 11 11:30:46 2001
+++ linux/drivers/char/sysrq.c	Sun Mar 11 11:31:12 2001
@@ -72,9 +72,9 @@
 
 	/*
 	 * Interrupts are disabled, and serial consoles are slow. So
-	 * Let's suspend the NMI watchdog.
+	 * let's re-start the NMI watchdog timeout.
 	 */
-	nmi_watchdog_disable();
+	touch_nmi_watchdog();
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq: ");
 	switch (key) {
@@ -158,7 +158,6 @@
 		/* Don't use 'A' as it's handled specially on the Sparc */
 	}
 
-	nmi_watchdog_enable();
 	console_loglevel = orig_log_level;
 }
 
--- linux/arch/i386/kernel/nmi.c.orig	Sun Mar 11 11:24:34 2001
+++ linux/arch/i386/kernel/nmi.c	Sun Mar 11 11:30:06 2001
@@ -226,37 +226,36 @@
 }
 
 static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
-static atomic_t nmi_watchdog_disabled = ATOMIC_INIT(0);
 
-void nmi_watchdog_disable(void)
-{
-	atomic_inc(&nmi_watchdog_disabled);
-}
+/*
+ * the best way to detect wether a CPU has a 'hard lockup' problem
+ * is to check it's local APIC timer IRQ counts. If they are not
+ * changing then that CPU has some problem.
+ *
+ * as these watchdog NMI IRQs are generated on every CPU, we only
+ * have to check the current processor.
+ *
+ * since NMIs dont listen to _any_ locks, we have to be extremely
+ * careful not to rely on unsafe variables. The printk might lock
+ * up though, so we have to break up any console locks first ...
+ * [when there will be more tty-related locks, break them up
+ *  here too!]
+ */
+
+static unsigned int
+	last_irq_sums [NR_CPUS],
+	alert_counter [NR_CPUS];
 
-void nmi_watchdog_enable(void)
+void touch_nmi_watchdog (void)
 {
-	atomic_dec(&nmi_watchdog_disabled);
+	/*
+	 * Just reset the alert counter:
+	 */
+	alert_counter[smp_processor_id()] = 0;
 }
 
 void nmi_watchdog_tick (struct pt_regs * regs)
 {
-	/*
-	 * the best way to detect wether a CPU has a 'hard lockup' problem
-	 * is to check it's local APIC timer IRQ counts. If they are not
-	 * changing then that CPU has some problem.
-	 *
-	 * as these watchdog NMI IRQs are broadcasted to every CPU, here
-	 * we only have to check the current processor.
-	 *
-	 * since NMIs dont listen to _any_ locks, we have to be extremely
-	 * careful not to rely on unsafe variables. The printk might lock
-	 * up though, so we have to break up any console locks first ...
-	 * [when there will be more tty-related locks, break them up
-	 *  here too!]
-	 */
-
-	static unsigned int last_irq_sums [NR_CPUS],
-				alert_counter [NR_CPUS];
 
 	/*
 	 * Since current-> is always on the stack, and we always switch
@@ -266,7 +265,7 @@
 
 	sum = apic_timer_irqs[cpu];
 
-	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_disabled) == 0) {
+	if (last_irq_sums[cpu] == sum) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...

Re: [patch] nmi-watchdog-2.4.2-A1

[Andrew Morton]

Ingo Molnar wrote:
> 
> On Sun, 11 Mar 2001, Keith Owens wrote:
> 
> > Works for me.  It even makes kdb simpler.
> 
> agreed. Also, touch_nmi_watchdog() is stateless and is thus much less
> prone to locking bugs.
> 
> i've attached nmi-watchdog-2.4.2-A1 that implements touch_nmi_watchdog(),
> ontop of 2.4.2-ac18, and changes show_state() to use this interface. (the
> patch also takes the NMI counters out of the obscure in-function place
> they used to be.)
> 
> the patch compiles & boots just fine on 2.4.2-ac18 in both SMP and
> APIC-less-UP mode. The NMI watchdog is functional, and SysRq-T does not
> cause a lockup if used with a slow serial console that takes more than 5
> seconds to output the tasklist.
> 

Sorry, this doesn't look right. Are you sure you booted
with `nmi_watchdog=1'?  It was turned off by default in -ac18.

Two things:

- CPU A could be doing the SYSRQ printing, while
  CPU B is spinning on a lock which CPU A holds. The
  NMI watchdog will then whack CPU B.  So touch_nmi_watchdog()
  needs to touch *all* CPUs.  (kbd_controller_lock, for example).

- We need to touch the NMI more than once during the 
  SYSRQ-T output - five seconds isn't enough.

  The correctest way is, I think, to touch_nmi() in
  rs285_console_write(), lp_console_write() and serial_console_write().
  We _could_ just touch it in show_state(), but that means
  we still get whacked if we do a lot of printk()s with interrupts
  disabled from some random place in the kernel.


--- linux-2.4.2-ac18/arch/i386/kernel/nmi.c	Sun Mar 11 15:12:31 2001
+++ linux-ac/arch/i386/kernel/nmi.c	Sun Mar 11 21:03:18 2001
@@ -226,37 +226,39 @@
 }
 
 static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
-static atomic_t nmi_watchdog_disabled = ATOMIC_INIT(0);
 
-void nmi_watchdog_disable(void)
-{
-	atomic_inc(&nmi_watchdog_disabled);
-}
+/*
+ * The best way to detect whether a CPU has a 'hard lockup' problem
+ * is to check its local APIC timer IRQ counts. If they are not
+ * changing then that CPU has some problem.
+ *
+ * as these watchdog NMI IRQs are generated on every CPU, we only
+ * have to check the current processor.
+ *
+ * since NMIs dont listen to _any_ locks, we have to be extremely
+ * careful not to rely on unsafe variables. The printk might lock
+ * up though, so we have to break up any console locks first ...
+ * [when there will be more tty-related locks, break them up
+ *  here too!]
+ */
+
+static unsigned int
+	last_irq_sums [NR_CPUS],
+	alert_counter [NR_CPUS];
 
-void nmi_watchdog_enable(void)
+void touch_nmi_watchdog (void)
 {
-	atomic_dec(&nmi_watchdog_disabled);
+	/*
+	 * Just reset the alert counters.
+	 */
+	int cpu;
+
+	for (cpu = 0; cpu < smp_num_cpus; cpu++)
+		alert_counter[cpu] = 0;
 }
 
 void nmi_watchdog_tick (struct pt_regs * regs)
 {
-	/*
-	 * the best way to detect wether a CPU has a 'hard lockup' problem
-	 * is to check it's local APIC timer IRQ counts. If they are not
-	 * changing then that CPU has some problem.
-	 *
-	 * as these watchdog NMI IRQs are broadcasted to every CPU, here
-	 * we only have to check the current processor.
-	 *
-	 * since NMIs dont listen to _any_ locks, we have to be extremely
-	 * careful not to rely on unsafe variables. The printk might lock
-	 * up though, so we have to break up any console locks first ...
-	 * [when there will be more tty-related locks, break them up
-	 *  here too!]
-	 */
-
-	static unsigned int last_irq_sums [NR_CPUS],
-				alert_counter [NR_CPUS];
 
 	/*
 	 * Since current-> is always on the stack, and we always switch
@@ -266,7 +268,7 @@
 
 	sum = apic_timer_irqs[cpu];
 
-	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_disabled) == 0) {
+	if (last_irq_sums[cpu] == sum) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...
--- linux-2.4.2-ac18/drivers/char/sysrq.c	Sun Mar 11 15:12:34 2001
+++ linux-ac/drivers/char/sysrq.c	Sun Mar 11 20:57:30 2001
@@ -70,11 +70,6 @@
 	if (!key)
 		return;
 
-	/*
-	 * Interrupts are disabled, and serial consoles are slow. So
-	 * Let's suspend the NMI watchdog.
-	 */
-	nmi_watchdog_disable();
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq: ");
 	switch (key) {
@@ -158,7 +153,6 @@
 		/* Don't use 'A' as it's handled specially on the Sparc */
 	}
 
-	nmi_watchdog_enable();
 	console_loglevel = orig_log_level;
 }
 
--- linux-2.4.2-ac18/drivers/char/serial.c	Sun Mar 11 15:12:34 2001
+++ linux-ac/drivers/char/serial.c	Sun Mar 11 20:58:58 2001
@@ -5478,10 +5478,11 @@
 /*
  *	Wait for transmitter & holding register to empty
  */
-static inline void wait_for_xmitr(struct async_struct *info)
+static void wait_for_xmitr(struct async_struct *info)
 {
 	unsigned int status, tmout = 1000000;
 
+	touch_nmi_watchdog();
 	do {
 		status = serial_in(info, UART_LSR);
 
--- linux-2.4.2-ac18/drivers/char/serial_21285.c	Sun Feb 25 17:37:03 2001
+++ linux-ac/drivers/char/serial_21285.c	Sun Mar 11 21:00:26 2001
@@ -385,6 +385,7 @@
 			while (*CSR_UARTFLG & 0x20);
 			*CSR_UARTDR = '\r';
 		}
+		touch_nmi_watchdog();
 	}
 	enable_irq(IRQ_CONTX);
 }
--- linux-2.4.2-ac18/drivers/char/lp.c	Sun Mar 11 15:12:34 2001
+++ linux-ac/drivers/char/lp.c	Sun Mar 11 21:01:29 2001
@@ -576,8 +576,8 @@
 			canwrite = lf - s;
 
 		if (canwrite > 0) {
+			touch_nmi_watchdog();
 			written = parport_write (port, s, canwrite);
-
 			if (written <= 0)
 				continue;
 
@@ -594,6 +594,7 @@
 			s++;
 			count--;
 			do {
+				touch_nmi_watchdog();
 				written = parport_write (port, crlf, i);
 				if (written > 0)
 					i -= written, crlf += written;
--- linux-2.4.2-ac18/include/linux/irq.h	Sun Mar 11 15:12:48 2001
+++ linux-ac/include/linux/irq.h	Sun Mar 11 20:37:16 2001
@@ -57,18 +57,16 @@
 #include <asm/hw_irq.h> /* the arch dependent stuff */
 
 /**
- * nmi_watchdog_disable - disable NMI watchdog checking.
+ * touch_nmi_watchdog - restart NMI watchdog timeout.
  * 
- * If the architecture supports the NMI watchdog, nmi_watchdog_disable() may be used
- * to temporarily disable it.  Use nmi_watchdog_enable() later on.  It is implemented
- * via an up/down counter, so you must keep the calls balanced.
+ * 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.
  */
 #ifdef ARCH_HAS_NMI_WATCHDOG
-extern void nmi_watchdog_disable(void);
-extern void nmi_watchdog_enable(void);
+extern void touch_nmi_watchdog(void);
 #else
-#define nmi_watchdog_disable() do{} while(0)
-#define nmi_watchdog_enable() do{} while(0)
+# define touch_nmi_watchdog() do { } while(0)
 #endif
 
 extern int handle_IRQ_event(unsigned int, struct pt_regs *, struct irqaction *);

[patch] nmi-watchdog-2.4.2-A2

[Ingo Molnar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1358 bytes --]


On Sun, 11 Mar 2001, Andrew Morton wrote:

> Sorry, this doesn't look right. Are you sure you booted with
> `nmi_watchdog=1'? It was turned off by default in -ac18.

of course i did ...

> Two things:
>
> - CPU A could be doing the SYSRQ printing, while
>   CPU B is spinning on a lock which CPU A holds. The
>   NMI watchdog will then whack CPU B.  So touch_nmi_watchdog()
>   needs to touch *all* CPUs.  (kbd_controller_lock, for example).

yep, agreed.

> - We need to touch the NMI more than once during the
>   SYSRQ-T output - five seconds isn't enough.
>
>   The correctest way is, I think, to touch_nmi() in
>   rs285_console_write(), lp_console_write() and
>   serial_console_write().

nope:

>   We _could_ just touch it in show_state(), but that means
>   we still get whacked if we do a lot of printk()s with interrupts
>   disabled from some random place in the kernel.

exactly, and that is a feature. We want to find all those places, because
disabling IRQs for too long can cause problems in unrelated kernel code.
SysRq-T is a special case so touch_nmi() is justified in that and only
that case. The NMI watchdog is something that gives security, and we want
to be very conservative disabling its effect.

[i've attached nmi-watchdog-2.4.2-A2 (against -ac18) which adds your fix
to clear all alert counters in touch_nmi_watchdog().]

	Ingo

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4714 bytes --]

--- linux/kernel/sched.c.orig	Sun Mar 11 11:49:00 2001
+++ linux/kernel/sched.c	Sun Mar 11 11:51:37 2001
@@ -1183,8 +1183,14 @@
 	printk("  task                 PC        stack   pid father child younger older\n");
 #endif
 	read_lock(&tasklist_lock);
-	for_each_task(p)
+	for_each_task(p) {
+		/*
+		 * reset the NMI-timeout, listing all files on a slow
+		 * console might take alot of time:
+		 */
+		touch_nmi_watchdog();
 		show_task(p);
+	}
 	read_unlock(&tasklist_lock);
 }
 
--- linux/include/linux/irq.h.orig	Sun Mar 11 11:20:21 2001
+++ linux/include/linux/irq.h	Sun Mar 11 12:02:23 2001
@@ -57,18 +57,16 @@
 #include <asm/hw_irq.h> /* the arch dependent stuff */
 
 /**
- * nmi_watchdog_disable - disable NMI watchdog checking.
+ * touch_nmi_watchdog - restart NMI watchdog timeout.
  * 
- * If the architecture supports the NMI watchdog, nmi_watchdog_disable() may be used
- * to temporarily disable it.  Use nmi_watchdog_enable() later on.  It is implemented
- * via an up/down counter, so you must keep the calls balanced.
+ * 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.
  */
 #ifdef ARCH_HAS_NMI_WATCHDOG
-extern void nmi_watchdog_disable(void);
-extern void nmi_watchdog_enable(void);
+extern void touch_nmi_watchdog(void);
 #else
-#define nmi_watchdog_disable() do{} while(0)
-#define nmi_watchdog_enable() do{} while(0)
+# define touch_nmi_watchdog() do { } while(0)
 #endif
 
 extern int handle_IRQ_event(unsigned int, struct pt_regs *, struct irqaction *);
--- linux/drivers/char/sysrq.c.orig	Sun Mar 11 11:30:46 2001
+++ linux/drivers/char/sysrq.c	Sun Mar 11 11:49:19 2001
@@ -70,11 +70,6 @@
 	if (!key)
 		return;
 
-	/*
-	 * Interrupts are disabled, and serial consoles are slow. So
-	 * Let's suspend the NMI watchdog.
-	 */
-	nmi_watchdog_disable();
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq: ");
 	switch (key) {
@@ -158,7 +153,6 @@
 		/* Don't use 'A' as it's handled specially on the Sparc */
 	}
 
-	nmi_watchdog_enable();
 	console_loglevel = orig_log_level;
 }
 
--- linux/arch/i386/kernel/nmi.c.orig	Sun Mar 11 11:24:34 2001
+++ linux/arch/i386/kernel/nmi.c	Sun Mar 11 17:57:41 2001
@@ -226,37 +226,40 @@
 }
 
 static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
-static atomic_t nmi_watchdog_disabled = ATOMIC_INIT(0);
 
-void nmi_watchdog_disable(void)
-{
-	atomic_inc(&nmi_watchdog_disabled);
-}
+/*
+ * the best way to detect wether a CPU has a 'hard lockup' problem
+ * is to check it's local APIC timer IRQ counts. If they are not
+ * changing then that CPU has some problem.
+ *
+ * as these watchdog NMI IRQs are generated on every CPU, we only
+ * have to check the current processor.
+ *
+ * since NMIs dont listen to _any_ locks, we have to be extremely
+ * careful not to rely on unsafe variables. The printk might lock
+ * up though, so we have to break up any console locks first ...
+ * [when there will be more tty-related locks, break them up
+ *  here too!]
+ */
+
+static unsigned int
+	last_irq_sums [NR_CPUS],
+	alert_counter [NR_CPUS];
 
-void nmi_watchdog_enable(void)
+void touch_nmi_watchdog (void)
 {
-	atomic_dec(&nmi_watchdog_disabled);
-}
+	int i;
 
-void nmi_watchdog_tick (struct pt_regs * regs)
-{
 	/*
-	 * the best way to detect wether a CPU has a 'hard lockup' problem
-	 * is to check it's local APIC timer IRQ counts. If they are not
-	 * changing then that CPU has some problem.
-	 *
-	 * as these watchdog NMI IRQs are broadcasted to every CPU, here
-	 * we only have to check the current processor.
-	 *
-	 * since NMIs dont listen to _any_ locks, we have to be extremely
-	 * careful not to rely on unsafe variables. The printk might lock
-	 * up though, so we have to break up any console locks first ...
-	 * [when there will be more tty-related locks, break them up
-	 *  here too!]
+	 * Just reset the alert counters, (other CPUs might be
+	 * spinning on locks we hold):
 	 */
+	for (i = 0; i < smp_num_cpus; i++)
+		alert_counter[i] = 0;
+}
 
-	static unsigned int last_irq_sums [NR_CPUS],
-				alert_counter [NR_CPUS];
+void nmi_watchdog_tick (struct pt_regs * regs)
+{
 
 	/*
 	 * Since current-> is always on the stack, and we always switch
@@ -266,7 +269,7 @@
 
 	sum = apic_timer_irqs[cpu];
 
-	if (last_irq_sums[cpu] == sum && atomic_read(&nmi_watchdog_disabled) == 0) {
+	if (last_irq_sums[cpu] == sum) {
 		/*
 		 * Ayiee, looks like this CPU is stuck ...
 		 * wait a few IRQs (5 seconds) before doing the oops ...

Re: [patch] serial console vs NMI watchdog

[george anzinger]

Keith Owens wrote:
> 
> On Sun, 11 Mar 2001 08:44:24 +0100 (CET),
> Ingo Molnar <mingo@elte.hu> wrote:
> >Andrew,
> >
> >your patch looks too complex, and doesnt cover the case of the serial
> >driver deadlocking. Why not add a "touch_nmi_watchdog_counter()" function
> >that just changes last_irq_sums instead of adding locking? This way
> >deadlocks will be caught in the serial code too. (because touch_nmi() will
> >only "postpone" the NMI watchdog lockup event, not disable it.)
> 
> kdb has to completely disable the nmi counter while it is in control.
> All interrupts are disabled, all but one cpus are spinning, the control
> cpu does busy wait while it polls the input devices.  With that model
> there is no alternative to a complete disable.
> 
Consider this.  Why not use the NMI to sync the cpus.  Kdb would have a
function that is called each NMI.  If it is doing nothing, just return
false, else, if waiting for this cpu, well here it is, put it in spin
AFTER saving where it came from so the operator can figure out what it
is doing.  In kgdb I just put the interrupt registers in the task_struct
where they are put when a context switch is done.  Then the debugger can
do a trace, etc. on that task.  A global var that the debugger can see
is also set to the cpus, "current".  

If the cpu is already spinning, return to the nmi code with a true flag
which will cause it to ignore the nmi.  Same thing if it is the cpu that
is doing debug i/o.

I went to this for kgdb after the system failed to return from the call
to force the other cpus to execute a function (which means they have to
be alive).  For extra safety I also time the sync.  If one or more
expected cpus, don't show while looping reading the cycle counter, the
code just continues with out the sync.

George

Re: [patch] serial console vs NMI watchdog

[Keith Owens]

On Sun, 11 Mar 2001 20:43:16 -0800, 
george anzinger <george@mvista.com> wrote:
>Consider this.  Why not use the NMI to sync the cpus.  Kdb would have a
>function that is called each NMI.

kdb uses NMI IPI to get the other cpu's attention.  One cpu is in
control and may or may not be accepting NMI, it depends on the event
that entered kdb.  The other cpus end up in kdb code, spinning waiting
for a cpu switch.  Initially they are not receiving NMI because they
were invoked via NMI which is masked until they exit.  However if the
user does a cpu switch then single steps the interrupted code, the cpu
has to return from the NMI handler to the interrupted code at which
time this cpu starts receiving NMI again.

The kdb context can change from ignoring NMI to accepting NMI.  It is
easier to bring all the cpus into kdb and let the kdb code decide if it
ignores any NMI that is being received.

Re: [patch] serial console vs NMI watchdog

[george anzinger]

Keith Owens wrote:
> 
> On Sun, 11 Mar 2001 20:43:16 -0800,
> george anzinger <george@mvista.com> wrote:
> >Consider this.  Why not use the NMI to sync the cpus.  Kdb would have a
> >function that is called each NMI.
> 
> kdb uses NMI IPI to get the other cpu's attention.  One cpu is in
> control and may or may not be accepting NMI, it depends on the event
> that entered kdb.  The other cpus end up in kdb code, spinning waiting
> for a cpu switch.  Initially they are not receiving NMI because they
> were invoked via NMI which is masked until they exit.  However if the
> user does a cpu switch then single steps the interrupted code, the cpu
> has to return from the NMI handler to the interrupted code at which
> time this cpu starts receiving NMI again.

Are you actually twiddling the hardware, or just changing what happens
on NMI?
> 
> The kdb context can change from ignoring NMI to accepting NMI.  It is
> easier to bring all the cpus into kdb and let the kdb code decide if it
> ignores any NMI that is being received.

Yes. Exactly.

George

Re: [patch] serial console vs NMI watchdog

[Keith Owens]

On Mon, 12 Mar 2001 00:27:14 -0800, 
george anzinger <george@mvista.com> wrote:
>Keith Owens wrote:
>> kdb uses NMI IPI to get the other cpu's attention.  One cpu is in
>> control and may or may not be accepting NMI, it depends on the event
>> that entered kdb.  The other cpus end up in kdb code, spinning waiting
>> for a cpu switch.  Initially they are not receiving NMI because they
>> were invoked via NMI which is masked until they exit.
>
>Are you actually twiddling the hardware, or just changing what happens
>on NMI?

No hardware twiddling.  One cpu gets an event which triggers kdb, that
event may or may not be via NMI.  kdb on ix86 then uses NMI to get the
attention of the other cpus, even if they are in a disabled spinloop.
ix86 hardware will not deliver another NMI to a cpu until the cpu
issues iret to return from the NMI handler.

Initially all but one cpu is forced into kdb via the NMI handler so no
more NMI events will occur on those cpus.  The first cpu may or may not
be receiving NMI, it depends on how kdb was invoked on the first cpu.
To do single stepping of code, kdb allows one cpu out of kdb state so
it can execute one instruction at the point it was interrupted.  If the
cpu was entered via NMI then that means exiting from the NMI handler
back to the original code, do single step then back into kdb again.

Having exited the NMI handler, that cpu will start receiving NMI events
again, even though it is still under kdb control.  So some cpus will
get NMI, some will not, depending on user actions.  kdb uses a software
mechanism to selectively disable the NMI watchdog.