Patch to 2.6.8.1-mm2 to allow multiple NMI handlers to be registered
The attached patch modifies the NMI handling to do the following (for x86 only):
* Allow multiple handlers to be registered and return if they have handled the NMI or not. oprofile and nmi_watchdog are modified to use this.
* Modifies the NMI watchdog code to use a function pointer instead of using if statements depending on CPU type. This made the change for the previous item easier.
* Moved the "unhandled NMI" code from nmi.c to traps.c, since nmi.c is really the handling for the nmi watchdog, not general nmi handling.
* Renames include/linux/nmi.h to include/linux/nmi_watchdog.h and creates a new include/linux/nmi.h for purely NMI handling.
This patch used to rename nmi.c to nmi_watchdog.c and create a new nmi.c file that was actually NMI handling. That's probably too big for now, but might be a useful change in the future.
I need this because the IPMI watchdog can generate NMIs as a pretimeout, to say "Hey, I'm about to fire the watchdog, do you want to do anything about it?". That way, the watchdog driver can panic thus generating useful information instead of just resetting the processor. I have seen other watchdog timers with this capability, too. And other special-purpose hardware that generates NMIs might find this useful, too.
I needed multiple handlers so the NMI watchdog and IPMI watchdog (and other NMI-related things) can peacefully coexist. It is not perfect because the hardware around NMIs sucks so badly, but it does work ok.
This code has been available as a patch as part of the IPMI driver for a while, so it has seen some testing.
-Corey
arch/i386/kernel/i386_ksyms.c +9 -3
@@ -33,6 +33,9 @@#include <asm/nmi.h> #include <asm/ist.h> #include <asm/kdebug.h> +#ifdef CONFIG_X86_LOCAL_APIC +#include <asm/apic.h> +#endif extern void dump_thread (struct pt_regs *,struct user *);extern spinlock_t rtc_lock;
@@ -88,6 +91,12 @@EXPORT_SYMBOL (cpu_khz);EXPORT_SYMBOL (apm_info); +EXPORT_SYMBOL (request_nmi); +EXPORT_SYMBOL (release_nmi); +#ifdef CONFIG_X86_LOCAL_APIC +EXPORT_SYMBOL (nmi_watchdog); +#endif +EXPORT_SYMBOL_NOVERS (__down_failed);EXPORT_SYMBOL_NOVERS (__down_failed_interruptible);EXPORT_SYMBOL_NOVERS (__down_failed_trylock);
@@ -171,9 +180,6 @@EXPORT_SYMBOL (rtc_lock); -EXPORT_SYMBOL_GPL (set_nmi_callback); -EXPORT_SYMBOL_GPL (unset_nmi_callback); -#undef memcmpextern int memcmp (const void *,const void *,__kernel_size_t );EXPORT_SYMBOL_NOVERS (memcmp);
arch/i386/kernel/irq.c +4 -0
@@ -143,6 +143,8 @@* Generic, controller-independent functions: */ +extern void nmi_append_user_names (struct seq_file *p); +int show_interrupts (struct seq_file *p,void *v) {int i = *(loff_t *) v, j;
@@ -181,6 +183,8 @@seq_printf (p,"NMI: " );for_each_cpu (j)seq_printf (p,"%10u " ,nmi_count (j)); +seq_printf (p," " ); +nmi_append_user_names (p);seq_putc (p,'\n' );#ifdef CONFIG_X86_LOCAL_APIC seq_printf (p,"LOC: " );
arch/i386/kernel/nmi.c +141 -79
320 lines not on this page · raw message
arch/i386/kernel/traps.c +219 -59
336 lines not on this page · raw message
arch/i386/oprofile/nmi_int.c +27 -4
@@ -82,11 +82,24 @@#endif /* CONFIG_PM */ -static int nmi_callback (struct pt_regs * regs,int cpu) +// FIXME: kernel_only +static int nmi_callback (void * dev_id,struct pt_regs * regs,int cpu,int handled) { -return model->check_ctrs (cpu, &cpu_msrs[cpu], regs); +if (model->check_ctrs (cpu, &cpu_msrs[cpu], regs)) +return NOTIFY_OK ; + +return NOTIFY_DONE ; } +static struct nmi_handler nmi_handler = +{ + .link =LIST_HEAD_INIT (nmi_handler.link), + .dev_name ="oprofile" , + .dev_id =NULL , + .handler = nmi_callback, + .priority =NMI_HANDLER_MAX_PRIORITY /* Highest possible priority */ +}; +static void nmi_cpu_save_registers (struct op_msrs * msrs) {
@@ -173,8 +186,12 @@ } +static void nmi_cpu_shutdown (void * dummy); +static int nmi_setup (void ) { +int rv; +if (!allocate_msrs ())return -ENOMEM ;
@@ -192,7 +209,13 @@*/ on_each_cpu (nmi_save_registers,NULL ,0 ,1 );on_each_cpu (nmi_cpu_setup,NULL ,0 ,1 ); -set_nmi_callback (nmi_callback); + rv =request_nmi (&nmi_handler); +if (rv) { +smp_call_function (nmi_cpu_shutdown,NULL ,0 ,1 ); +nmi_cpu_shutdown (0 ); +return rv; + } + nmi_enabled =1 ;return 0 ; }
@@ -243,7 +266,7 @@ { nmi_enabled =0 ;on_each_cpu (nmi_cpu_shutdown,NULL ,0 ,1 ); -unset_nmi_callback (); +release_nmi (&nmi_handler);release_lapic_nmi ();free_msrs (); }
arch/x86_64/kernel/nmi.c +1 -1
@@ -23,7 +23,7 @@#include <linux/kernel_stat.h> #include <linux/module.h> #include <linux/sysdev.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <asm/smp.h> #include <asm/mtrr.h>
drivers/acpi/osl.c +1 -1
@@ -35,7 +35,7 @@#include <linux/kmod.h> #include <linux/delay.h> #include <linux/workqueue.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <acpi/acpi.h> #include <asm/io.h> #include <acpi/acpi_bus.h>
include/asm-i386/apic.h +0 -1
@@ -102,7 +102,6 @@extern void release_lapic_nmi (void );extern void disable_timer_nmi_watchdog (void );extern void enable_timer_nmi_watchdog (void ); -extern void nmi_watchdog_tick (struct pt_regs * regs);extern int APIC_init_uniprocessor (void );extern void disable_APIC_timer (void );extern void enable_APIC_timer (void );
include/asm-i386/nmi.h +30 -14
54 lines not on this page · raw message
include/linux/nmi.h +7 -18
@@ -1,22 +1,11 @@/* -* linux/include/linux/nmi.h +* linux/include/linux/nmi.h +* +* (C) 2002 Corey Minyard <[email protected]> +* +* Include file for NMI handling. */ -#ifndef LINUX_NMI_H -#define LINUX_NMI_H - -#include <asm/irq.h> - -/** -* touch_nmi_watchdog - restart NMI watchdog timeout. -* -* If the architecture supports the NMI watchdog, touch_nmi_watchdog() -* may be used to reset the timeout - for code which intentionally -* disables interrupts for a long time. This call is stateless. -*/ -#ifdef ARCH_HAS_NMI_WATCHDOG -extern void touch_nmi_watchdog (void ); -#else -# define touch_nmi_watchdog () do { } while(0) -#endif +#if defined(__i386__) +#include <asm/nmi.h> #endif
include/linux/nmi_watchdog.h +22 -0
@@ -0,0 +1,22 @@ +/* +* linux/include/linux/nmi.h +*/ +#ifndef LINUX_NMI_WATCHDOG_H +#define LINUX_NMI_WATCHDOG_H + +#include <asm/irq.h> + +/** +* touch_nmi_watchdog - restart NMI watchdog timeout. +* +* If the architecture supports the NMI watchdog, touch_nmi_watchdog() +* may be used to reset the timeout - for code which intentionally +* disables interrupts for a long time. This call is stateless. +*/ +#ifdef ARCH_HAS_NMI_WATCHDOG +extern void touch_nmi_watchdog (void ); +#else +# define touch_nmi_watchdog () do { } while(0) +#endif + +#endif
kernel/panic.c +1 -1
@@ -18,7 +18,7 @@#include <linux/sysrq.h> #include <linux/syscalls.h> #include <linux/interrupt.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> int panic_timeout;int panic_on_oops;
kernel/sched.c +1 -1
@@ -20,7 +20,7 @@#include <linux/mm.h> #include <linux/module.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <linux/init.h> #include <asm/uaccess.h> #include <linux/highmem.h>
arch/i386/kernel/nmi.c line 380
Quoted 6 lines · Corey Minyard
arch/i386/kernel/nmi.c +6
-0+static int k7_watchdog_reset (int handled) +{ +unsigned int low, high; +int source; + +rdmsr (MSR_K7_PERFCTR0 , low, high);
Please use rdpmc() instead of rdmsr() when reading counter registers. Ditto in the other places.
(I know oprofile doesn't, but that's no excuse.)
arch/i386/kernel/nmi.c line 386
Quoted 6 lines
arch/i386/kernel/nmi.c +6
-0+/* +* If the timer has overflowed, this is certainly a watchdog +* source +*/ + source = (low & (1 <<31 )) ==0 ; +if (source)
Why not "if ((int)low >= 0)"?
arch/i386/kernel/nmi.c line 606
Quoted 8 lines
arch/i386/kernel/nmi.c +8
-0+/* +* The only thing that SHOULD be before us is the oprofile +* code. If it has handled an NMI, then we shouldn't. This +* is a rather unnatural relationship, it would much better to +* build a perf-counter handler and then tie both the +* watchdog and oprofile code to it. Then this ugliness +* could go away. +*/
Depending on the value of nmi_watchdog and how oprofile was set up, neither, just one, or both of them can cause NMIs. Only one of them can do it via the performance counters, however.
How do you handle multiple simultaneous NMIs from different sources?
Quoted 13 lines · Mikael Pettersson
arch/i386/kernel/nmi.c line 380
Quoted 6 lines · Corey Minyard
arch/i386/kernel/nmi.c +6
-0+static int k7_watchdog_reset (int handled) +{ +unsigned int low, high; +int source; + +rdmsr (MSR_K7_PERFCTR0 , low, high);Please use
rdpmc()instead ofrdmsr()when reading counter registers. Ditto in the other places.
(I know oprofile doesn't, but that's no excuse.)
Ok, no problem.
Quoted 10 lines
arch/i386/kernel/nmi.c line 386
Quoted 6 lines
arch/i386/kernel/nmi.c +6
-0+/* +* If the timer has overflowed, this is certainly a watchdog +* source +*/ + source = (low & (1 <<31 )) ==0 ; +if (source)Why not "if ((int)low >= 0)"?
IIRC, the docs state that timer goes off if the high bit is cleared in the register. I was just going with the documentation description. Not a big deal either way, I don't think.
Quoted 4 lines
Quoted 4 lines
arch/i386/kernel/nmi.c +4
-0+/* +* The only thing that SHOULD be before us is the oprofile +* code. If it has handled an NMI, then we shouldn't. This +* is a rather unnatural relationship, it would much better to
Quoted 10 lines
arch/i386/kernel/nmi.c line 606
Quoted 4 lines
arch/i386/kernel/nmi.c +4
-0+* build a perf-counter handler and then tie both the +* watchdog and oprofile code to it. Then this ugliness +* could go away. +*/ Depending on the value of
nmi_watchdogand how oprofile was set up, neither, just one, or both of them can cause NMIs. Only one of them can do it via the performance counters, however.
For nmi_watchdog=1, I can come from both the performance counters and the IOAPIC only if they both go off at almost exactly the same time (there is only one edge for both). Otherwise, the oprofile code can tell if it is the source, and it will return if it handled it or not. The race is small, but there. I guess I might have to implement the ugliness I describe below.
Quoted 3 lines
How do you handle multiple simultaneous NMIs from different sources?
That's why the current NMI hardware sucks so bad. In general, you cannot tell easily. However, you can tell if the NMI came from at least the NMI watchdog if nmi_watchdog=2. And you can usually tell if it was an ECC error or I/O error. I'm working on getting things changed in IPMI to be able to tell if one came from IPMI. At OLS, we talked about this for a while and people are going to start trying to push the hardware vendors to improve the NMI hardware so the source can always be known. So things are not perfect, but I believe they are usable.
You cannot tell (well, I think you can, but it would be tricky*) if the NMI came from the nmi_watchdog=1 (IOAPIC) because that requires claiming a lock to look at the timer registers. You cannot claim locks in an NMI, at least not general locks. Other sources depend on the capability of the hardware.
Thanks,
-Corey
* If the processor has compare-and-swap (486 and newer do, I believe), you can create an "processor-owned" lock that you can atomically claim and set ownership of. With that, the NMI handler could could first check and see if the processor it was running on owned the lock, and do the appropriate thing if it did. Otherwise it could claim the spinlock normally. Ugly, but possible.
Quoted 10 lines · Corey Minyard
Quoted 10 lines
arch/i386/kernel/nmi.c line 386
Quoted 6 lines
arch/i386/kernel/nmi.c +6
-0+/* +* If the timer has overflowed, this is certainly a watchdog +* source +*/ + source = (low & (1 <<31 )) ==0 ; +if (source)Why not "if ((int)low >= 0)"?
Quoted 3 lines
IIRC, the docs state that timer goes off if the high bit is cleared in the register. I was just going with the documentation description. Not a big deal either way, I don't think.
Which is the same as saying it occurs on the negative-to-non-negative transition. The "if ((int)low >= 0)" test is just a test and branch on sign, while your original requires either a (microcoded?) bit op, or constructing a large-magnitude mask, followed by some tests.
Yes this is a micro-optimisation, but I find it actually simplifies the code.
/Mikael
Quoted 4 lines · Mikael Pettersson
Which is the same as saying it occurs on the negative-to-non-negative transition. The "if ((int)low >= 0)" test is just a test and branch on sign, while your original requires either a (microcoded?) bit op, or constructing a large-magnitude mask, followed by some tests.
gcc should be able to convert one into the other depending on which one is faster.
Andreas.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Quoted 3 lines · Corey Minyard
Quoted 3 lines
Please use
rdpmc()instead ofrdmsr()when reading counter registers. Ditto in the other places.
(I know oprofile doesn't, but that's no excuse.)
I actually have no idea why we don't use rdpmc().
john
Quoted 7 lines · John Levon
Quoted 3 lines · Corey Minyard
Quoted 3 lines
Please use
rdpmc()instead ofrdmsr()when reading counter registers. Ditto in the other places.
(I know oprofile doesn't, but that's no excuse.)I actually have no idea why we don't use
rdpmc().
If i recall, not all the performance counters were accessible via rdpmc and being slower.
Quoted 12 lines · Mikael Pettersson
Review
Quoted 6 lines · Corey Minyard
(quoted diff) +6
-0+static int k7_watchdog_reset(int handled) +{ + unsigned int low, high; + int source; + + rdmsr(MSR_K7_PERFCTR0, low, high);Please use
rdpmc()instead ofrdmsr()when reading counter registers. Ditto in the other places.
(I know oprofile doesn't, but that's no excuse.)
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* + * If the timer has overflowed, this is certainly a watchdog + * source + */ + source = (low & (1 << 31)) == 0; + if (source)Why not "if ((int)low >= 0)"?
Ok, new patch with these fixed. Tested on P6 and P4. I don't have a K7 to test on, but it's a pretty straightforward change.
The "if ((int) low) >= 0" thing removed two pointless instructions on x86 (gcc 3.2). So much for gcc optimization on this one.
Thanks for your help.
-Corey
arch/i386/kernel/i386_ksyms.c +9 -3
@@ -33,6 +33,9 @@#include <asm/nmi.h> #include <asm/ist.h> #include <asm/kdebug.h> +#ifdef CONFIG_X86_LOCAL_APIC +#include <asm/apic.h> +#endif extern void dump_thread (struct pt_regs *,struct user *);extern spinlock_t rtc_lock;
@@ -88,6 +91,12 @@EXPORT_SYMBOL (cpu_khz);EXPORT_SYMBOL (apm_info); +EXPORT_SYMBOL (request_nmi); +EXPORT_SYMBOL (release_nmi); +#ifdef CONFIG_X86_LOCAL_APIC +EXPORT_SYMBOL (nmi_watchdog); +#endif +EXPORT_SYMBOL_NOVERS (__down_failed);EXPORT_SYMBOL_NOVERS (__down_failed_interruptible);EXPORT_SYMBOL_NOVERS (__down_failed_trylock);
@@ -171,9 +180,6 @@EXPORT_SYMBOL (rtc_lock); -EXPORT_SYMBOL_GPL (set_nmi_callback); -EXPORT_SYMBOL_GPL (unset_nmi_callback); -#undef memcmpextern int memcmp (const void *,const void *,__kernel_size_t );EXPORT_SYMBOL_NOVERS (memcmp);
arch/i386/kernel/irq.c +4 -0
@@ -143,6 +143,8 @@* Generic, controller-independent functions: */ +extern void nmi_append_user_names (struct seq_file *p); +int show_interrupts (struct seq_file *p,void *v) {int i = *(loff_t *) v, j;
@@ -181,6 +183,8 @@seq_printf (p,"NMI: " );for_each_cpu (j)seq_printf (p,"%10u " ,nmi_count (j)); +seq_printf (p," " ); +nmi_append_user_names (p);seq_putc (p,'\n' );#ifdef CONFIG_X86_LOCAL_APIC seq_printf (p,"LOC: " );
arch/i386/kernel/nmi.c +145 -81
350 lines not on this page · raw message
arch/i386/kernel/traps.c +219 -59
336 lines not on this page · raw message
arch/i386/oprofile/nmi_int.c +27 -4
@@ -82,11 +82,24 @@#endif /* CONFIG_PM */ -static int nmi_callback (struct pt_regs * regs,int cpu) +// FIXME: kernel_only +static int nmi_callback (void * dev_id,struct pt_regs * regs,int cpu,int handled) { -return model->check_ctrs (cpu, &cpu_msrs[cpu], regs); +if (model->check_ctrs (cpu, &cpu_msrs[cpu], regs)) +return NOTIFY_OK ; + +return NOTIFY_DONE ; } +static struct nmi_handler nmi_handler = +{ + .link =LIST_HEAD_INIT (nmi_handler.link), + .dev_name ="oprofile" , + .dev_id =NULL , + .handler = nmi_callback, + .priority =NMI_HANDLER_MAX_PRIORITY /* Highest possible priority */ +}; +static void nmi_cpu_save_registers (struct op_msrs * msrs) {
@@ -173,8 +186,12 @@ } +static void nmi_cpu_shutdown (void * dummy); +static int nmi_setup (void ) { +int rv; +if (!allocate_msrs ())return -ENOMEM ;
@@ -192,7 +209,13 @@*/ on_each_cpu (nmi_save_registers,NULL ,0 ,1 );on_each_cpu (nmi_cpu_setup,NULL ,0 ,1 ); -set_nmi_callback (nmi_callback); + rv =request_nmi (&nmi_handler); +if (rv) { +smp_call_function (nmi_cpu_shutdown,NULL ,0 ,1 ); +nmi_cpu_shutdown (0 ); +return rv; + } + nmi_enabled =1 ;return 0 ; }
@@ -243,7 +266,7 @@ { nmi_enabled =0 ;on_each_cpu (nmi_cpu_shutdown,NULL ,0 ,1 ); -unset_nmi_callback (); +release_nmi (&nmi_handler);release_lapic_nmi ();free_msrs (); }
arch/x86_64/kernel/nmi.c +1 -1
@@ -23,7 +23,7 @@#include <linux/kernel_stat.h> #include <linux/module.h> #include <linux/sysdev.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <asm/smp.h> #include <asm/mtrr.h>
drivers/acpi/osl.c +1 -1
@@ -35,7 +35,7 @@#include <linux/kmod.h> #include <linux/delay.h> #include <linux/workqueue.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <acpi/acpi.h> #include <asm/io.h> #include <acpi/acpi_bus.h>
include/asm-i386/apic.h +0 -1
@@ -102,7 +102,6 @@extern void release_lapic_nmi (void );extern void disable_timer_nmi_watchdog (void );extern void enable_timer_nmi_watchdog (void ); -extern void nmi_watchdog_tick (struct pt_regs * regs);extern int APIC_init_uniprocessor (void );extern void disable_APIC_timer (void );extern void enable_APIC_timer (void );
include/asm-i386/nmi.h +30 -14
54 lines not on this page · raw message
include/linux/nmi.h +7 -18
@@ -1,22 +1,11 @@/* -* linux/include/linux/nmi.h +* linux/include/linux/nmi.h +* +* (C) 2002 Corey Minyard <[email protected]> +* +* Include file for NMI handling. */ -#ifndef LINUX_NMI_H -#define LINUX_NMI_H - -#include <asm/irq.h> - -/** -* touch_nmi_watchdog - restart NMI watchdog timeout. -* -* If the architecture supports the NMI watchdog, touch_nmi_watchdog() -* may be used to reset the timeout - for code which intentionally -* disables interrupts for a long time. This call is stateless. -*/ -#ifdef ARCH_HAS_NMI_WATCHDOG -extern void touch_nmi_watchdog (void ); -#else -# define touch_nmi_watchdog () do { } while(0) -#endif +#if defined(__i386__) +#include <asm/nmi.h> #endif
include/linux/nmi_watchdog.h +22 -0
@@ -0,0 +1,22 @@ +/* +* linux/include/linux/nmi.h +*/ +#ifndef LINUX_NMI_WATCHDOG_H +#define LINUX_NMI_WATCHDOG_H + +#include <asm/irq.h> + +/** +* touch_nmi_watchdog - restart NMI watchdog timeout. +* +* If the architecture supports the NMI watchdog, touch_nmi_watchdog() +* may be used to reset the timeout - for code which intentionally +* disables interrupts for a long time. This call is stateless. +*/ +#ifdef ARCH_HAS_NMI_WATCHDOG +extern void touch_nmi_watchdog (void ); +#else +# define touch_nmi_watchdog () do { } while(0) +#endif + +#endif
kernel/panic.c +1 -1
@@ -18,7 +18,7 @@#include <linux/sysrq.h> #include <linux/syscalls.h> #include <linux/interrupt.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> int panic_timeout;int panic_on_oops;
kernel/sched.c +1 -1
@@ -20,7 +20,7 @@#include <linux/mm.h> #include <linux/module.h> -#include <linux/nmi.h> +#include <linux/nmi_watchdog.h> #include <linux/init.h> #include <asm/uaccess.h> #include <linux/highmem.h>
Quoted 2 lines · Corey Minyard
* Allow multiple handlers to be registered and return if they have handled the NMI or not. oprofile and
nmi_watchdogare modified to use this.
Why not use a notifier call chain as is done with panic & friends instead of implementing the same thing specifically for NMI?
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/ http://members.shaw.ca/golinux/
Quoted 12 lines · Andreas Dilger
Quoted 3 lines · Corey Minyard
* Allow multiple handlers to be registered and return if they have handled the NMI or not. oprofile and
nmi_watchdogare modified to use this.Why not use a notifier call chain as is done with panic & friends instead of implementing the same thing specifically for NMI?
A couple of reasons:
* The "handled" value needs to be passed around so handler know if previous handlers have already handled the NMI.
* It allows the list of registered NMI handlers to be dumped in /proc/interrupts
I thought that the notifiers callout used locks (which would be a no-go), but it doesn't. So I can scratch that one from my list.
Thanks,
-Corey