NMI watch dog notify patch

Quoted 4 lines · George Anzinger

This patch adds a notify to the nmi watchdog to notify that the system is about to be taken down by the watchdog. If the notify is handled with a NOTIFY_STOP return, the system is given a new lease on life.

It looks sensible, but as there aren't actually any in-kernel uses for this I'd have thought it would be better for it to live out-of-tree?

Quoted 8 lines · Andrew Morton

Quoted 4 lines · George Anzinger

This patch adds a notify to the nmi watchdog to notify that the system is about to be taken down by the watchdog. If the notify is handled with a NOTIFY_STOP return, the system is given a new lease on life.

Quoted 2 lines

It looks sensible, but as there aren't actually any in-kernel uses for this I'd have thought it would be better for it to live out-of-tree?

I should just bundle it with the kgdb patch then?

I have been doing some work on kgdb to pull a few of it "fingers" out of various places in the kernel. This is the final location where we have a kgdb intercept not covered by a notify.

Quoted 1 line · George Anzinger

I have been doing some work on kgdb to pull a few of it "fingers" out of various places in the kernel. This is the final location where we have a kgdb intercept not covered by a notify.

I like the idea, but the hook should be in die_nmi(), not in the watchdog, using the reason that is already passed into die_nmi. die_nmi() is also called for a real NMI.

Quoted 8 lines · Keith Owens

Quoted 3 lines · George Anzinger

I have been doing some work on kgdb to pull a few of it "fingers" out of various places in the kernel. This is the final location where we have a kgdb intercept not covered by a notify.

Quoted 4 lines

I like the idea, but the hook should be in die_nmi(), not in the watchdog, using the reason that is already passed into die_nmi. die_nmi() is also called for a real NMI.

I had though that too, but it does not allow recovery (i.e. lets reset the watchdog and try again).

Hmm.. just looked at traps.c. Seems die_nmi is NOT called from the nmi trap, only from the watchdog. Also, there is a notify in the path to the other nmi stuff.

Quoted 3 lines

I had though that too, but it does not allow recovery (i.e. lets reset the watchdog and try again).

die_nmi() returns to nmi_watchdog_tick(), nmi_watchdog_tick does the reset and continues. Patch below.

Quoted 3 lines

Hmm.. just looked at traps.c. Seems die_nmi is NOT called from the nmi trap, only from the watchdog. Also, there is a notify in the path to the other nmi stuff.

I was looking at unknown_nmi_panic_callback(), which also calls die_nmi().

traps.c already has several notify_die() calls, nmi.c has none. It is cleaner to keep all the notification in traps.c, with this small change to nmi.c to cope with die_nmi() returning.

Index: linux/arch/i386/kernel/nmi.c
===================================================================
--- linux.orig/arch/i386/kernel/nmi.c 2005-07-28 17:22:06.735038510 +1000
+++ linux/arch/i386/kernel/nmi.c 2005-07-29 15:19:00.371196596 +1000
@@ -494,8 +494,10 @@ void nmi_watchdog_tick (struct pt_regs *
* wait a few IRQs (5 seconds) before doing the oops ... */
alert_counter[cpu]++;
- if (alert_counter[cpu] == 5*nmi_hz)
+ if (alert_counter[cpu] == 5*nmi_hz) {
die_nmi(regs, "NMI Watchdog detected LOCKUP");
+ alert_counter[cpu] = 0;
+ }
} else {
last_irq_sums[cpu] = sum;
alert_counter[cpu] = 0;

Quoted 27 lines · Keith Owens

Quoted 3 lines

I had though that too, but it does not allow recovery (i.e. lets reset

the watchdog and try again).

die_nmi() returns to nmi_watchdog_tick(), nmi_watchdog_tick does the reset and continues. Patch below.

Quoted 1 line

Hmm.. just looked at traps.c. Seems die_nmi is NOT called from the nmi trap, only from the watchdog. Also, there is a notify in the path to the other nmi stuff.

I was looking at unknown_nmi_panic_callback(), which also calls die_nmi().

traps.c already has several notify_die() calls, nmi.c has none. It is cleaner to keep all the notification in traps.c, with this small change to nmi.c to cope with die_nmi() returning.

Index: linux/arch/i386/kernel/nmi.c
===================================================================

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

@@ -494,8 +494,10 @@ void nmi_watchdog_tick (struct pt_regs *
 		 * wait a few IRQs (5 seconds) before doing the oops ...
 		 */
 		alert_counter[cpu]++;
-		if (alert_counter[cpu] == 5*nmi_hz)
+		if (alert_counter[cpu] == 5*nmi_hz) {
 			die_nmi(regs, "NMI Watchdog detected LOCKUP");

arch/i386/kernel/nmi.c void nmi_watchdog_tick (struct pt_regs * · line 503

Quoted 5 lines

arch/i386/kernel/nmi.c +2 -0

+			alert_counter[cpu] = 0;
+		}
 	} else {
 		last_irq_sums[cpu] = sum;
 		alert_counter[cpu] = 0;

That all makes sense - let's go that way?

-

This patch adds a notify to the die_nmi notify that the system is about to be taken down. If the notify is handled with a NOTIFY_STOP return, the system is given a new lease on life.

void die_nmi (struct pt_regs *regs, const char *msg) {

arch/i386/kernel/traps.c +4 -0

+	if (notify_die(DIE_NMIWATCHDOG, "nmi_watchdog", regs, 
+		       0, 0, SIGINT) == NOTIFY_STOP)
+		return;
+

	spin_lock(&nmi_print_lock);
	/*
	* We are in trouble anyway, lets at least try

Quoted 4 lines · George Anzinger

This patch adds a notify to the die_nmi notify that the system is about to be taken down. If the notify is handled with a NOTIFY_STOP return, the system is given a new lease on life.

Quoted 9 lines

void die_nmi (struct pt_regs *regs, const char *msg) {

(quoted diff) +4 -0

+	if (notify_die(DIE_NMIWATCHDOG, "nmi_watchdog", regs, 
+		       0, 0, SIGINT) == NOTIFY_STOP)
+		return;
+

	spin_lock(&nmi_print_lock);
	/*
	* We are in trouble anyway, lets at least try

Minor nitpick. die_nmi() already gets a message passed in to distinguish between different types of nmi. Pass that message to notify_die(), on the off chance that the notified routines can use that difference.

Also your patch adds a trailing whitespace on the call to notify_die().