nmi_watchdog fixes for -rt

PATCH In review Hiroshi Shimamoto 2 participants 8 messages Apr 28, 2008

Subsystems (2)

x86 Scheduler

lore

Share & tools

lore

Hiroshi Shimamoto

author

2008-04-28 18:10 UTC #0 lore raw

cover nmi_watchdog fixes for -rt

Hi,

Here is a patchset of nmi_watchdog fixes for -rt kernel. These patches are against 2.6.24.4-rt4.
Could you please review?

thanks,
Hiroshi Shimamoto

Hiroshi Shimamoto

author

2008-04-28 18:14 UTC #1 lore raw

PATCH 1/4 x86_64: send NMI after nmi_show_regs on

From: Hiroshi Shimamoto <[email protected]>

The flags nmi_show_regs should be set before send NMI.

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
 arch/x86/kernel/nmi_64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/nmi_64.c +0 -1

index d187ab9..69cc737 100644
@@ -327,11 +327,11 @@ void nmi_show_all_regs(void)
 	if (system_state == SYSTEM_BOOTING)
 		return;
 
-	smp_send_nmi_allbutself();

I know this wasn't your code to begin with but, how does this function exit? I mean, we set an array where each index per online cpu is set to one, then do an "nmi_allbutself", and then wait on those indexes to turn zero, one at a time. If we are CPU 0 here, we set that index to 1, then enter the loop, and will block forever on this "while" loop below.

Am I missing something?

Thanks,

-- Steve

Quoted 8 lines

arch/x86/kernel/nmi_64.c +2 -1

+	smp_send_nmi_allbutself();
+
 	for_each_online_cpu(i) {
 		while (nmi_show_regs[i] == 1)
 			barrier();
--

1.5.4.1

Hiroshi Shimamoto

author

2008-04-28 21:34 UTC #3 lore raw

Re: [PATCH -rt 1/4] x86_64: send NMI after nmi_show_regs on

Quoted 17 lines · Steven Rostedt

arch/x86/kernel/nmi_64.c void nmi_show_all_regs(void) · line 331
Quoted 12 lines · Hiroshi Shimamoto
arch/x86/kernel/nmi_64.c +0 -2
index d187ab9..69cc737 100644
@@ -327,11 +327,11 @@ void nmi_show_all_regs(void)
 	if (system_state == SYSTEM_BOOTING)
 		return;
 
-	smp_send_nmi_allbutself();
-
 	for_each_online_cpu(i)
 		nmi_show_regs[i] = 1;
Hi Hiroshi,

Quoted 5 lines

I know this wasn't your code to begin with but, how does this function exit? I mean, we set an array where each index per online cpu is set to one, then do an "nmi_allbutself", and then wait on those indexes to turn zero, one at a time. If we are CPU 0 here, we set that index to 1, then enter the loop, and will block forever on this "while" loop below.

Hm, I'm not quit sure when NMI disabled.
If NMI is working issuing CPU will receive NMI and the flag turns off in NMI handler.
I'll look into it again and will work if needed.

thanks,
Hiroshi Shimamoto

Hiroshi Shimamoto

author

2008-04-28 18:16 UTC #4 lore raw

PATCH 2/4 x86: return true for NMI handled

In reply to Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>

NMI for show_regs causes unknown NMI when nmi_watchdog is local APIC mode. Because lapic_wd_event() will fail due to still running perfctr. If NMI is for show_regs, nmi_watchdog_tick() should return 1.

On x86_32, call irq_show_regs_callback() is moved to top of the nmi_watchdog_tick() same as x86_64.

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
 arch/x86/kernel/nmi_32.c |   10 +++++-----
 arch/x86/kernel/nmi_64.c |    9 +++++----
 include/linux/sched.h    |    2 +-
 3 files changed, 11 insertions(+), 10 deletions(-)

arch/x86/kernel/nmi_32.c +5 -5

index da9deb3..d1f92ca 100644
@@ -350,10 +350,10 @@ void nmi_show_all_regs(void)
 
 static DEFINE_RAW_SPINLOCK(nmi_print_lock);
 
-notrace void irq_show_regs_callback(int cpu, struct pt_regs *regs)
+notrace int irq_show_regs_callback(int cpu, struct pt_regs *regs)
 {
 	if (!nmi_show_regs[cpu])
-		return;
+		return 0;
 
 	nmi_show_regs[cpu] = 0;
 	spin_lock(&nmi_print_lock);

@@ -362,6 +362,7 @@ notrace void irq_show_regs_callback(int cpu, struct pt_regs *regs)
 		per_cpu(irq_stat, cpu).apic_timer_irqs);
 	show_regs(regs);
 	spin_unlock(&nmi_print_lock);
+	return 1;
 }
 
 notrace __kprobes int

@@ -376,8 +377,9 @@ nmi_watchdog_tick(struct pt_regs *regs, unsigned reason)
 	unsigned int sum;
 	int touched = 0;
 	int cpu = smp_processor_id();
-	int rc=0;
+	int rc;
 
+	rc = irq_show_regs_callback(cpu, regs);
 	__profile_tick(CPU_PROFILING, regs);
 
 	/* check for other users first */

@@ -404,8 +406,6 @@ nmi_watchdog_tick(struct pt_regs *regs, unsigned reason)
 	sum = per_cpu(irq_stat, cpu).apic_timer_irqs +
 		per_cpu(irq_stat, cpu).irq0_irqs;
 
-	irq_show_regs_callback(cpu, regs);
-
 	/* if the apic timer isn't firing, this cpu isn't doing much */
 	/* if the none of the timers isn't firing, this cpu isn't doing much */
 	if (!touched && last_irq_sums[cpu] == sum) {

arch/x86/kernel/nmi_64.c +5 -4

index 5d3073c..afc0317 100644
@@ -340,10 +340,10 @@ void nmi_show_all_regs(void)
 
 static DEFINE_RAW_SPINLOCK(nmi_print_lock);
 
-notrace void irq_show_regs_callback(int cpu, struct pt_regs *regs)
+notrace int irq_show_regs_callback(int cpu, struct pt_regs *regs)
 {
 	if (!nmi_show_regs[cpu])
-		return;
+		return 0;
 
 	nmi_show_regs[cpu] = 0;
 	spin_lock(&nmi_print_lock);

@@ -351,6 +351,7 @@ notrace void irq_show_regs_callback(int cpu, struct pt_regs *regs)
 	printk(KERN_WARNING "apic_timer_irqs: %d\n", read_pda(apic_timer_irqs));
 	show_regs(regs);
 	spin_unlock(&nmi_print_lock);
+	return 1;
 }
 
 notrace int __kprobes

@@ -359,9 +360,9 @@ nmi_watchdog_tick(struct pt_regs * regs, unsigned reason)
 	int sum;
 	int touched = 0;
 	int cpu = smp_processor_id();
-	int rc = 0;
+	int rc;
 
-	irq_show_regs_callback(cpu, regs);
+	rc = irq_show_regs_callback(cpu, regs);
 	__profile_tick(CPU_PROFILING, regs);
 
 	/* check for other users first */

include/linux/sched.h +1 -1

index 4176f87..a37200a 100644
@@ -292,7 +292,7 @@ static inline void show_state(void)
 }
 
 extern void show_regs(struct pt_regs *);
-extern void irq_show_regs_callback(int cpu, struct pt_regs *regs);
+extern int irq_show_regs_callback(int cpu, struct pt_regs *regs);
 
 /*
  * TASK is a pointer to the task whose backtrace we want to see (or NULL for current

Signature · 1 line

1.5.4.1

Hiroshi Shimamoto

author

2008-04-28 18:17 UTC #5 lore raw

PATCH 3/4 x86: nmi_watchdog NMI needed for irq_show_regs_callback()

In reply to Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>

The -rt kernel doesn't panic immediately when NMI lockup detected. Because the kernel waits show_regs on all cpus, but NMI is not come so frequently.

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
 arch/x86/kernel/nmi_32.c |    7 +++++++
 arch/x86/kernel/nmi_64.c |    8 +++++++-
 2 files changed, 14 insertions(+), 1 deletions(-)

arch/x86/kernel/nmi_32.c +7 -0

index f55f05b..da9deb3 100644
@@ -428,6 +428,13 @@ nmi_watchdog_tick(struct pt_regs *regs, unsigned reason)
 				if (i == cpu)
 					continue;
 				nmi_show_regs[i] = 1;
+			}
+
+			smp_send_nmi_allbutself();
+
+			for_each_online_cpu(i) {
+				if (i == cpu)
+					continue;
 				while (nmi_show_regs[i] == 1)
 					cpu_relax();
 			}

arch/x86/kernel/nmi_64.c +7 -0

index 69cc737..5d3073c 100644
@@ -412,10 +412,16 @@ nmi_watchdog_tick(struct pt_regs * regs, unsigned reason)
 				if (i == cpu)
 					continue;
 				nmi_show_regs[i] = 1;
+			}
+
+			smp_send_nmi_allbutself();
+
+			for_each_online_cpu(i) {
+				if (i == cpu)
+					continue;
 				while (nmi_show_regs[i] == 1)
 					cpu_relax();
 			}

Hiroshi Shimamoto

author

2008-04-28 18:19 UTC #6 lore raw

PATCH 4/4 wait for finish show_regs() before panic

In reply to Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>

It might cause kdump failure that the kernel doesn't wait for finish show_regs(). The nmi_show_regs variable for show_regs() flag is cleared before show_regs() is really called. This flag should be cleared after show_regs().
kdump stops all CPUs other than crashing CPU by NMI handler, but if show_regs() takes a bit time, kdump cannot wait and will continue process. It means that the 2nd kernel and the old kernel run simultaneously and it might cause unexpected behavior, such as randomly reboot.

Signed-off-by Hiroshi Shimamoto <[email protected]>

Signed-off-by Maxim Uvarov <[email protected]>

---
 arch/x86/kernel/nmi_32.c |    2 +-
 arch/x86/kernel/nmi_64.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/nmi_32.c +1 -1

index d1f92ca..f46bb6e 100644
@@ -355,13 +355,13 @@ notrace int irq_show_regs_callback(int cpu, struct pt_regs *regs)
 	if (!nmi_show_regs[cpu])
 		return 0;
 
-	nmi_show_regs[cpu] = 0;
 	spin_lock(&nmi_print_lock);
 	printk(KERN_WARNING "NMI show regs on CPU#%d:\n", cpu);
 	printk(KERN_WARNING "apic_timer_irqs: %d\n",
 		per_cpu(irq_stat, cpu).apic_timer_irqs);
 	show_regs(regs);
 	spin_unlock(&nmi_print_lock);
+	nmi_show_regs[cpu] = 0;
 	return 1;
 }

arch/x86/kernel/nmi_64.c +1 -1

index afc0317..8bc2328 100644
@@ -345,12 +345,12 @@ notrace int irq_show_regs_callback(int cpu, struct pt_regs *regs)
 	if (!nmi_show_regs[cpu])
 		return 0;
 
-	nmi_show_regs[cpu] = 0;
 	spin_lock(&nmi_print_lock);
 	printk(KERN_WARNING "NMI show regs on CPU#%d:\n", cpu);
 	printk(KERN_WARNING "apic_timer_irqs: %d\n", read_pda(apic_timer_irqs));
 	show_regs(regs);
 	spin_unlock(&nmi_print_lock);
+	nmi_show_regs[cpu] = 0;
 	return 1;
 }

Signature · 1 line

1.5.4.1

Steven Rostedt

reviewer

2008-04-28 19:03 UTC #7 lore raw

In reply to Hiroshi Shimamoto

Quoted 5 lines · Hiroshi Shimamoto

Hi,
Here is a patchset of nmi_watchdog fixes for -rt kernel. These patches are against 2.6.24.4-rt4.
Could you please review?

Patches look good!

I'll queue them up for the next release.

Thanks,

-- Steve