perf, x86: fix unknown NMIs on a Pentium4 box

PATCH In review Cyrill Gorcunov 6 participants 18 messages Apr 14, 2011

Subsystems (2)

x86 perf events

lore

Share & tools

lore

Cyrill Gorcunov

author

2011-04-14 14:48 UTC #0 lore raw

PATCH 1/1 perf, x86: fix unknown NMIs on a Pentium4 box

From: Don Zickus <[email protected]>
Subject: [PATCH -tip] perf, x86: fix unknown NMIs on a Pentium4 box

When using perf on a Pentium4 box, lots of unknown NMIs would be generated. This is the result of a P4 quirk that is subtle. The P4 generates an NMI when the counter overflow and unlike other arches where the NMI is a one time event, the P4 continues to assert its NMI until clear by the OS.

As a side effect to this quirk, the NMI on the apic is masked off to prevent a stream of NMIs until the overflow flag is cleared. During the perf re-design, this subtle-ness was overlooked and the apic was unmasked _before_ the overflow flag was cleared. As a result, this generated an extra NMI on the P4 mchines.

The fix is trivial, wait until the NMI is properly handled before un-masking the apic.

Sadly, in the old nmi watchdog there was a note that explained this exact behaviour.

Cyrill Gorcunov: Added a comment into code itself. We should consider
if we need to unmask LVTPC if no oveflow happened at all.

Ingo Molnar: Pointed out that unmasking unconditionally is proven by time
to be correct.

Signed-off-by Don Zickus <[email protected]>

Tested-by Shaun Ruffell <[email protected]>

Tested-by Cyrill Gorcunov <[email protected]>

Acked-by Cyrill Gorcunov <[email protected]>

CC Lin Ming <[email protected]>

CC Maciej Rutecki <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Stephane Eranian <[email protected]>

CC Robert Richter <[email protected]>

---

Ingo, please make sure I've added conform notes about conditional/uconditional unmasking in changelog. Don, I've added a comment in code just to not forget why we need it. Thanks.

 arch/x86/kernel/cpu/perf_event.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/perf_event.c +8 -1

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
=====================================================================
@@ -1370,9 +1370,16 @@ perf_event_nmi_handler(struct notifier_b
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+
+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;

Signature · 1 line

    Cyrill

Ingo Molnar

2011-04-14 15:04 UTC #1 lore raw

Cyrill,

Quoted 2 lines · Cyrill Gorcunov

From: Don Zickus <[email protected]>
Subject: [PATCH -tip] perf, x86: fix unknown NMIs on a Pentium4 box

...

Quoted 1 line

Acked-by Cyrill Gorcunov <[email protected]>

When forwarding patches please always use Signed-off-by, not Acked-by - especially when you also edit the patch slightly.

Is it fine to you if i add your SOB to this patch?

Thanks,

Ingo

Cyrill Gorcunov

author

2011-04-14 15:06 UTC #2 lore raw

Quoted 10 lines · Ingo Molnar

Cyrill,
Quoted 2 lines · Cyrill Gorcunov
From: Don Zickus <[email protected]>
Subject: [PATCH -tip] perf, x86: fix unknown NMIs on a Pentium4 box
...
Quoted 1 line
Acked-by Cyrill Gorcunov <[email protected]>

Quoted 8 lines

When forwarding patches please always use Signed-off-by, not Acked-by - especially when you also edit the patch slightly.
Is it fine to you if i add your SOB to this patch?
Thanks,
Ingo

Of course I'm fine! Sorry Ingo for inconvenience.

Signature · 1 line

    Cyrill

Ingo Molnar

2011-04-14 17:43 UTC #3 lore raw

In reply to Cyrill Gorcunov

Quoted 10 lines · Cyrill Gorcunov

arch/x86/kernel/cpu/perf_event.c +1 -1

@@ -1370,9 +1370,16 @@ perf_event_nmi_handler(struct notifier_b
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1385

Quoted 10 lines

arch/x86/kernel/cpu/perf_event.c +7 -0

+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;

This breaks 'perf top' on Intel Nehalem and probably other CPUs. The NMI gets stuck fast on all CPUs:

NMI: 16 6 3 3 3 3 3 3 3 3 3 3 3 3 4 5 Non-maskable interrupts

Thanks,

Ingo

Ingo Molnar

Tested-by Shaun Ruffell

2011-04-14 17:44 UTC #4 lore raw

below is the commit which needs to be fixed - note that i improved the changelog, please keep these fixes for future submissions.

Thanks,

Ingo

------------------>

Quoted 1 line

From 1b6556e28ea29c2e7fe0eb710b100db035278c32 Mon Sep 17 00:00:00 2001

From: Don Zickus <[email protected]>
Date: Thu, 14 Apr 2011 18:48:48 +0400
Subject: [PATCH] perf, x86: Fix spurious 'unknown NMI' messages on Pentium4 systems

When using perf on a Pentium4 box, lots of unknown NMIs are generated. This is the result of a P4 quirk that is subtle. The P4 generates an NMI when the counter overflows and unlike other models where the NMI is a one time event, the P4 continues to assert its NMI until cleared by the OS.

The fix is trivial: wait until the NMI is properly handled before un-masking the apic.

Tested-by Shaun Ruffell <[email protected]>

Signed-off-by Don Zickus <[email protected]>

Cc Lin Ming <[email protected]>

Cc Maciej Rutecki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Robert Richter <[email protected]>

[ Added a comment into code itself as well. ]

Signed-off-by Cyrill Gorcunov <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

arch/x86/kernel/cpu/perf_event.c +8 -1

index eed3673a..d3a1902 100644
@@ -1370,9 +1370,16 @@ perf_event_nmi_handler(struct notifier_block *self,
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+
+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;

Cyrill Gorcunov

2011-04-14 17:49 UTC #5 lore raw

Quoted 8 lines · Ingo Molnar

below is the commit which needs to be fixed - note that i improved the changelog, please keep these fixes for future submissions.
Thanks,
Ingo

Yes, thanks, i'll ping back as only get nit resolved.

Signature · 1 line

    Cyrill

Shaun Ruffell

2011-04-14 18:13 UTC #6 lore raw

Quoted 6 lines · Cyrill Gorcunov

Quoted 3 lines · Ingo Molnar
below is the commit which needs to be fixed - note that i improved the changelog, please keep these fixes for future submissions.
Yes, thanks, i'll ping back as only get nit resolved.

Cyrill, I had not been running perf when I was testing, but I was able to reproduce what Ingo reported. I'll make sure to run that on any future tests as well. My apologies.

Cyrill Gorcunov

2011-04-14 18:14 UTC #7 lore raw

Quoted 11 lines · Shaun Ruffell

Quoted 6 lines · Cyrill Gorcunov
Quoted 3 lines · Ingo Molnar
below is the commit which needs to be fixed - note that i improved the changelog, please keep these fixes for future submissions.
Yes, thanks, i'll ping back as only get nit resolved.
Cyrill, I had not been running perf when I was testing, but I was able to reproduce what Ingo reported. I'll make sure to run that on any future tests as well. My apologies.

It's mine error, no need for apologies. I suspect this issue might be related to shared MSRs (iirc nehalem have some MSRs shared as well). I'm checking which one might be involved.

Signature · 1 line

    Cyrill

Cyrill Gorcunov

2011-04-14 18:20 UTC #8 lore raw

In reply to Shaun Ruffell

Quoted 11 lines · Shaun Ruffell

Quoted 6 lines · Cyrill Gorcunov
Quoted 3 lines · Ingo Molnar
below is the commit which needs to be fixed - note that i improved the changelog, please keep these fixes for future submissions.
Yes, thanks, i'll ping back as only get nit resolved.
Cyrill, I had not been running perf when I was testing, but I was able to reproduce what Ingo reported. I'll make sure to run that on any future tests as well. My apologies.

Btw, Shaun, perf top wont work on P4 for a while (since it needs nmi-watchdog to be disabled).

Signature · 1 line

    Cyrill

David Ahern

2011-04-14 19:36 UTC #9 lore raw

Quoted 5 lines · Cyrill Gorcunov

On 04/14/2011 10:12 PM, Shaun Ruffell wrote:
Btw, Shaun, perf top wont work on P4 for a while (since it needs nmi-watchdog to be disabled).

perf-top defaults to H/W cycles event. Just need to force it to S/W events (the fallback for systems that do not support cycles event):
perf top -e cpu-clock

David

Cyrill Gorcunov

author

2011-04-14 17:46 UTC #10 lore raw

In reply to Ingo Molnar

On 04/14/2011 09:43 PM, Ingo Molnar wrote:
...

Quoted 17 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1384
Quoted 13 lines
arch/x86/kernel/cpu/perf_event.c +8 -0
 	handled = x86_pmu.handle_irq(args->regs);
+
+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;
This breaks 'perf top' on Intel Nehalem and probably other CPUs. The NMI gets stuck fast on all CPUs:

Quoted 5 lines

NMI: 16 6 3 3 3 3 3 3 3 3 3 3 3 3 4 5 Non-maskable interrupts
Thanks,
Ingo

Hmm. Thanks for info. Investigating...

Signature · 1 line

    Cyrill

Don Zickus

2011-04-14 18:33 UTC #11 lore raw

In reply to Ingo Molnar

Quoted 27 lines · Ingo Molnar

Quoted 10 lines · Cyrill Gorcunov
arch/x86/kernel/cpu/perf_event.c +1 -1
@@ -1370,9 +1370,16 @@ perf_event_nmi_handler(struct notifier_b
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+
arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1385
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +7 -0
+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;
 
This breaks 'perf top' on Intel Nehalem and probably other CPUs. The NMI gets stuck fast on all CPUs:

Quoted 1 line

NMI: 16 6 3 3 3 3 3 3 3 3 3 3 3 3 4 5 Non-maskable interrupts

Damn it, I was working on getting there. First I did P4s, now I was working on acme's core2 issues. Nehalem was next on my list, I swear! :-)))))

So this sucks. I'll grab a Nehalem and see what went wrong. It's probably because of the other 'this seems to work' hacks I put in that handler. I bet if I clean those up, this problem will be fixed.

I will note that using my patch on a core2quad system, lowered the number of back-to-back NMIs I was seeing when running a couple of perf records and a make -j8 (still generates unknown NMIs though :-( ).

Cheers,
Don

Ingo Molnar

2011-04-14 18:46 UTC #12 lore raw

Quoted 33 lines · Don Zickus

Quoted 27 lines · Ingo Molnar
Quoted 10 lines · Cyrill Gorcunov
arch/x86/kernel/cpu/perf_event.c +1 -1
@@ -1370,9 +1370,16 @@ perf_event_nmi_handler(struct notifier_b
 		return NOTIFY_DONE;
 	}
 
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+
arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1385
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +7 -0
+	/*
+	 * Note the unmasking of LVTPC entry must be
+	 * done *after* counter oveflow flag is cleared
+	 * otherwise it might lead to double NMIs generation.
+	 */
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
 	if (!handled)
 		return NOTIFY_DONE;
 
This breaks 'perf top' on Intel Nehalem and probably other CPUs. The NMI gets stuck fast on all CPUs:
Quoted 1 line
NMI: 16 6 3 3 3 3 3 3 3 3 3 3 3 3 4 5 Non-maskable interrupts
Damn it, I was working on getting there. First I did P4s, now I was working on acme's core2 issues. Nehalem was next on my list, I swear! :-)))))

Quoted 7 lines

So this sucks. I'll grab a Nehalem and see what went wrong. It's probably because of the other 'this seems to work' hacks I put in that handler. I bet if I clean those up, this problem will be fixed.
I will note that using my patch on a core2quad system, lowered the number of back-to-back NMIs I was seeing when running a couple of perf records and a make -j8 (still generates unknown NMIs though :-( ).

Here's the cpuinfo:

processor : 15
vendor_id : GenuineIntel
cpu family : 6
model : 26
model name : Intel(R) Xeon(R) CPU X55600 @ 2.80GHz
stepping : 5
cpu MHz : 2794.000
cache size : 8192 KB
physical id : 1
siblings : 8
core id : 3
cpu cores : 4
apicid : 23
initial apicid : 23
fpu : yes
fpu_exception : yes
cpuid level : 11
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm dca sse4_1 sse4_2 popcnt lahf_lm ida dts tpr_shadow vnmi flexpriority ept vpid
bogomips : 5599.19
clflush size : 64
cache_alignment : 64
address sizes : 40 bits physical, 48 bits virtual
power management:

just in case you have trouble reproducing the problem.

Thanks,

Ingo

Ingo Molnar

2011-04-14 18:46 UTC #13 lore raw

In reply to Don Zickus

btw., the bug went away once i removed your patch so it's 100% sure caused by this change.

Thanks,

Ingo

Cyrill Gorcunov

author

2011-04-14 19:43 UTC #14 lore raw

Quoted 7 lines · Ingo Molnar

btw., the bug went away once i removed your patch so it's 100% sure caused by this change.
Thanks,
Ingo

Ingo if you have a chance mind to give this patch a shot please? Seems we might miss unmasking for inflight nmis.

Signature · 20 lines

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event.c
+++ linux-2.6.git/arch/x86/kernel/cpu/perf_event.c
@@ -1365,14 +1365,15 @@ perf_event_nmi_handler(struct notifier_b
 		 * will be empty and daze the CPU. So, we drop it to
 		 * avoid false-positive 'unknown nmi' messages.
 		 */
+		apic_write(APIC_LVTPC, APIC_DM_NMI);
 		return NOTIFY_STOP;
 	default:
 		return NOTIFY_DONE;
 	}

-	apic_write(APIC_LVTPC, APIC_DM_NMI);

 	handled = x86_pmu.handle_irq(args->regs);
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
 	if (!handled)
 		return NOTIFY_DONE;

Don Zickus

2011-04-14 19:58 UTC #15 lore raw

Quoted 11 lines · Cyrill Gorcunov

Quoted 7 lines · Ingo Molnar
btw., the bug went away once i removed your patch so it's 100% sure caused by this change.
Thanks,
Ingo
Ingo if you have a chance mind to give this patch a shot please? Seems we might miss unmasking for inflight nmis.

I don't think this patch will work. It would make sense if the unmasking happened _after_ the "if (!handled)" path, but that is not the path Ingo wanted for v1.

Cheers,
Don

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1381

Quoted 4 lines

arch/x86/kernel/cpu/perf_event.c +1 -1

-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+	apic_write(APIC_LVTPC, APIC_DM_NMI);

^^^^ all handled/unhandled NMIs hit that apic_write

Quoted 3 lines

arch/x86/kernel/cpu/perf_event.c +0 -0
 	if (!handled)
 		return NOTIFY_DONE;

Cyrill Gorcunov

author

2011-04-14 20:05 UTC #16 lore raw

Quoted 8 lines · Don Zickus

Quoted 7 lines · Cyrill Gorcunov
Quoted 6 lines · Ingo Molnar
btw., the bug went away once i removed your patch so it's 100% sure caused by this change.
Thanks,

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Ingo
Ingo if you have a chance mind to give this patch a shot please? Seems we might miss unmasking for inflight nmis.
I don't think this patch will work. It would make sense if the unmasking happened _after_ the "if (!handled)" path, but that is not the path Ingo wanted for v1.

This thing happened if inflight nmi reaches the system and note that inflight NMI comes from perf and masks lvt entry, it has nothing to do with "handled" but rather the _fact_ that NMI reached apic via LVTPC and as result -- masked it. Don, I might be missin something, brain is slowly going to sleep :)

Quoted 4 lines

Cheers,
Don

Quoted 9 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_b · line 1381
Quoted 4 lines
arch/x86/kernel/cpu/perf_event.c +1 -1
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
 	handled = x86_pmu.handle_irq(args->regs);
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
^^^^ all handled/unhandled NMIs hit that apic_write
Quoted 3 lines
arch/x86/kernel/cpu/perf_event.c +0 -0
 	if (!handled)
 		return NOTIFY_DONE;

yeah, Ingo asked to make it this way -- ie like in your former patch, the conditional unmasking is left to be tested in further kernel series.

Signature · 1 line

    Cyrill

Cyrill Gorcunov

2011-04-14 20:18 UTC #17 lore raw

On 04/15/2011 12:05 AM, Cyrill Gorcunov wrote:
...

Quoted 10 lines

Quoted 4 lines
I don't think this patch will work. It would make sense if the unmasking happened _after_ the "if (!handled)" path, but that is not the path Ingo wanted for v1.
This thing happened if inflight nmi reaches the system and note that inflight NMI comes from perf and masks lvt entry, it has nothing to do with "handled" but rather the _fact_ that NMI reached apic via LVTPC and as result -- masked it. Don, I might be missin something, brain is slowly going to sleep :)

Seems I'm wrong in this assumption, otherwise when this mechanism was introduced for first time every in-flight nmi catched would block further perf activity.

Quoted 4 lines

Quoted 4 lines
Cheers,
Don

Signature · 1 line

    Cyrill