A question of perf NMI handler

With nmi_watchdog enabled, perf_event_nmi_handler always return NOTIFY_STOP(active_events > 0), and the notifier call chain will not call further.

If it was not perf NMI, does the perf nmi handler may stop the real NMI handler get called because NOTIFY_STOP is returned??

static int __kprobes perf_event_nmi_handler(struct notifier_block *self,
unsigned long cmd, void *__args)

{
	struct die_args *args = __args;
	struct pt_regs *regs;

	if (!atomic_read(&active_events)) ===> With nmi_watchdog enabled, active_events > 0
		return NOTIFY_DONE;

	switch (cmd) {
	case DIE_NMI:
	case DIE_NMI_IPI:
		break;

	default:
		return NOTIFY_DONE;
	}

	regs = args->regs;

	apic_write(APIC_LVTPC, APIC_DM_NMI);
	/*
	 * Can't rely on the handled return value to say it was our NMI, two
	 * events could trigger 'simultaneously' raising two back-to-back NMIs.

	 *
	 * If the first NMI handles both, the latter will be empty and daze
	 * the CPU.
	 */
	x86_pmu.handle_irq(regs);

	return NOTIFY_STOP;
}

With nmi_watchdog enabled, perf_event_nmi_handler always return NOTIFY_STOP(active_events > 0), and the notifier call chain will not call further.

If it was not perf NMI, does the perf nmi handler may stop the real NMI handler get called because NOTIFY_STOP is returned??

To ensure that all other nmi handlers are
called, the perfctr's nmi handler must have the lowest priority. Then, the handler will be the last in the chain.

Quoted 3 lines · Robert Richter

To ensure that all other nmi handlers are
called, the perfctr's nmi handler must have the lowest priority. Then, the handler will be the last in the chain.

Well, unless another NMI handler has the exact same issue and also starts eating all NMIs, just in case.

There is no general mechanism for recording the NMI source (except if it was external triggered, e.g. by the southbridge). Also, all nmis are mapped to NMI vector 2 and therefore there is no way to find out the reason by using apic mask registers.

Now, if multiple perfctrs trigger an nmi, it may happen that a handler has nothing to do because the counter was already handled by the previous one. Thus, it is valid to have unhandled nmis caused by perfctrs.

So, with counters enabled we always have to return stop for *all* nmis as we cannot detect that it was an perfctr nmi. Otherwise we could trigger an unhandled nmi. To ensure that all other nmi handlers are called, the perfctr's nmi handler must have the lowest priority. Then, the handler will be the last in the chain.

Quoted 4 lines · Robert Richter

There is no general mechanism for recording the NMI source (except if it was external triggered, e.g. by the southbridge). Also, all nmis are mapped to NMI vector 2 and therefore there is no way to find out the reason by using apic mask registers.

This is no different than a shared interrupt, no? All the nmi handlers need to check their own sources to see if they triggered it. You can't expect the generic nmi handler to determine this.

Quoted 5 lines

Now, if multiple perfctrs trigger an nmi, it may happen that a handler has nothing to do because the counter was already handled by the previous one. Thus, it is valid to have unhandled nmis caused by perfctrs.

Quoted 5 lines

So, with counters enabled we always have to return stop for *all* nmis as we cannot detect that it was an perfctr nmi. Otherwise we could trigger an unhandled nmi. To ensure that all other nmi handlers are called, the perfctr's nmi handler must have the lowest priority. Then, the handler will be the last in the chain.

But the cases this break are, external NMI buttons, broken firmware that causes SERRs on the PCI bus, and any other general hardware failures.

So what the perf handler does is really unacceptable. The only reason we are noticing this now is because I put the nmi_watchdog on top of the perf subsystem, so it always has a user and will trigger NOTIFY_STOP. Before, it never had a registerd user so instead returned NOTIFY_DONE and everything worked great.

Quoted 6 lines · Don Zickus

Quoted 4 lines · Robert Richter

There is no general mechanism for recording the NMI source (except if it was external triggered, e.g. by the southbridge). Also, all nmis are mapped to NMI vector 2 and therefore there is no way to find out the reason by using apic mask registers.

Quoted 3 lines

This is no different than a shared interrupt, no? All the nmi handlers need to check their own sources to see if they triggered it. You can't expect the generic nmi handler to determine this.

Sure, but the problem is that the PMU can't reliably do that.

Quoted 12 lines

Quoted 11 lines

Now, if multiple perfctrs trigger an nmi, it may happen that a handler has nothing to do because the counter was already handled by the previous one. Thus, it is valid to have unhandled nmis caused by perfctrs.

So, with counters enabled we always have to return stop for *all* nmis as we cannot detect that it was an perfctr nmi. Otherwise we could trigger an unhandled nmi. To ensure that all other nmi handlers are called, the perfctr's nmi handler must have the lowest priority. Then, the handler will be the last in the chain.

Quoted 2 lines

But the cases this break are, external NMI buttons, broken firmware that causes SERRs on the PCI bus, and any other general hardware failures.

It breaks broken firmware? :-) and you care?

Quoted 6 lines

So what the perf handler does is really unacceptable. The only reason we are noticing this now is because I put the nmi_watchdog on top of the perf subsystem, so it always has a user and will trigger NOTIFY_STOP. Before, it never had a registerd user so instead returned NOTIFY_DONE and everything worked great.

Right so I looked up your thing and while that limits the damage in that at some point it will let NMIs pass, it will still consume too many. Meaning that Yinghai will have to potentially press his NMI button several times before it registers.

Quoted 4 lines

Right so I looked up your thing and while that limits the damage in that at some point it will let NMIs pass, it will still consume too many. Meaning that Yinghai will have to potentially press his NMI button several times before it registers.

Ok. Thanks for reviewing. How does it consume to many? I probably don't understand how perf is being used in the non-simple scenarios.

Quoted 7 lines · Don Zickus

Quoted 4 lines

Right so I looked up your thing and while that limits the damage in that at some point it will let NMIs pass, it will still consume too many. Meaning that Yinghai will have to potentially press his NMI button several times before it registers.

Ok. Thanks for reviewing. How does it consume to many? I probably don't understand how perf is being used in the non-simple scenarios.

Suppose you have 4 counters (AMD, intel-nhm+), when more than 2 overflow the first will raise the PMI, if the other 2+ overflow before we disable the PMU it will try to raise 2+ more PMIs, but because hardware only has a single interrupt pending bit it will at most cause a single extra interrupt after we finish servicing the first one.

So then the first interrupt will see 3+ overflows, return 3+, and will thus eat 2+ NMIs, only one of which will be the pending interrupt, leaving 1+ NMIs from other sources to consume unhandled.

In which case Yinghai will have to press his NMI button 2+ times before it registers.

That said, that might be a better situation than always consuming unknown NMIs..

Quoted 19 lines · Peter Zijlstra

Quoted 7 lines · Don Zickus

Quoted 4 lines

Right so I looked up your thing and while that limits the damage in that at some point it will let NMIs pass, it will still consume too many. Meaning that Yinghai will have to potentially press his NMI button several times before it registers.

Ok. Thanks for reviewing. How does it consume to many? I probably don't understand how perf is being used in the non-simple scenarios.

Suppose you have 4 counters (AMD, intel-nhm+), when more than 2 overflow the first will raise the PMI, if the other 2+ overflow before we disable the PMU it will try to raise 2+ more PMIs, but because hardware only has a single interrupt pending bit it will at most cause a single extra interrupt after we finish servicing the first one.

So then the first interrupt will see 3+ overflows, return 3+, and will thus eat 2+ NMIs, only one of which will be the pending interrupt, leaving 1+ NMIs from other sources to consume unhandled.

Quoted 6 lines

In which case Yinghai will have to press his NMI button 2+ times before it registers.

That said, that might be a better situation than always consuming unknown NMIs..

Well, first I guess having Yinghai CC'ed is a bonus ;)
The second thing is that I don't get why perf handler can't be _last_ call in default_do_nmi, if there were any nmi with reason (serr or parity) I think they should be calling first which of course don't eliminate the former issue but somewhat make it weaken.

Quoted 6 lines

Well, first I guess having Yinghai CC'ed is a bonus ;)
The second thing is that I don't get why perf handler can't be _last_ call in default_do_nmi, if there were any nmi with reason (serr or parity) I think they should be calling first which of course don't eliminate the former issue but somewhat make it weaken.

Because the reason registers are never set. If they were, then the code wouldn't have to walk the notify_chain. :-)

Unknown nmis are unknown nmis, nobody is claiming them. Even worse, there are customers that want to register their nmi handler below the perf handler to claim all the unknown nmis, so they can be logged on the system before being rebooted.

Cheers,
Don

On Wed, Aug 04, 2010 at 11:50:02AM -0400, Don Zickus wrote:
...

Quoted 7 lines

Quoted 6 lines

Well, first I guess having Yinghai CC'ed is a bonus ;)
The second thing is that I don't get why perf handler can't be _last_ call in default_do_nmi, if there were any nmi with reason (serr or parity) I think they should be calling first which of course don't eliminate the former issue but somewhat make it weaken.

Quoted 3 lines

Because the reason registers are never set. If they were, then the code wouldn't have to walk the notify_chain. :-)

maybe we're talking about different things. i meant that if there is nmi with a reason (from 0x61) the handling of such nmi should be done before notify_die I think (if only I not miss something behind).

Quoted 8 lines

Quoted 4 lines

Because the reason registers are never set. If they were, then the code wouldn't have to walk the notify_chain. :-)

maybe we're talking about different things. i meant that if there is nmi with a reason (from 0x61) the handling of such nmi should be done before notify_die I think (if only I not miss something behind).

No we are talking about the same thing. :-) And that code is already

there. The problem is the bits in register 0x61 are not always set correctly in the case of SERRs (well at least in all the cases I have dealt with). So you can easily can a flood of unknown nmis from an SERR and register 0x61 would have the PERR/SERR bits set to 0. Fun, huh?

Cheers,
Don

Quoted 4 lines · Don Zickus

there. The problem is the bits in register 0x61 are not always set correctly in the case of SERRs (well at least in all the cases I have dealt with). So you can easily can a flood of unknown nmis from an SERR and register 0x61 would have the PERR/SERR bits set to 0. Fun, huh?

if there is nothing in nmi_sc the code flows into another branch. And it hits the problem of perf events eating all nmi giving no chance the others. So we take if (!(reason & 0xc0)) case and hit DIE_NMI_IPI (/me scratching the head why it's not under CONFIG_X86_LOCAL_APIC) and drop all code, unpleasant.

Only the upper 2 bits in io_61h indicate the nmi reason, so in case of (!(reason & 0xc0)) the source simply can not be determined and all nmi handlers in the chain must be called (DIE_NMI/DIE_NMI_IPI). The perfctr handler then stops it.

So you can decide to either get an unrecovered nmi panic triggered by a perfctr or losing unknown nmis from other sources. Maybe this can be fixed by implementing handlers for those sources.

(cc'ing Andi)

Quoted 7 lines · Cyrill Gorcunov

Quoted 4 lines · Don Zickus

there. The problem is the bits in register 0x61 are not always set correctly in the case of SERRs (well at least in all the cases I have dealt with). So you can easily can a flood of unknown nmis from an SERR and register 0x61 would have the PERR/SERR bits set to 0. Fun, huh?

Quoted 5 lines

if there is nothing in nmi_sc the code flows into another branch. And it hits the problem of perf events eating all nmi giving no chance the others. So we take if (!(reason & 0xc0)) case and hit DIE_NMI_IPI (/me scratching the head why it's not under CONFIG_X86_LOCAL_APIC) and drop all code, unpleasant.

Only the upper 2 bits in io_61h indicate the nmi reason, so in case of (!(reason & 0xc0)) the source simply can not be determined and all nmi handlers in the chain must be called (DIE_NMI/DIE_NMI_IPI). The perfctr handler then stops it.

So you can decide to either get an unrecovered nmi panic triggered by a perfctr or losing unknown nmis from other sources. Maybe this can be fixed by implementing handlers for those sources.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

yes, that is what I meant by nmi_sc register. I think we need to restucturize current default_do_nmi handler but how to be with perfs I don't know at moment if perf register gets overflowed (ie already has pedning nmi) but we handle it in early nmi cycle this would lead to strange results. Need to think.

Quoted 4 lines

So you can decide to either get an unrecovered nmi panic triggered by a perfctr or losing unknown nmis from other sources. Maybe this can be fixed by implementing handlers for those sources.

Quoted 6 lines · Cyrill Gorcunov

yes, that is what I meant by nmi_sc register. I think we need to restucturize current default_do_nmi handler but how to be with perfs I don't know at moment if perf register gets overflowed (ie already has pedning nmi) but we handle it in early nmi cycle this would lead to strange results. Need to think.

Quoted 1 line

Quoted 3 lines

Quoted 3 lines

So you can decide to either get an unrecovered nmi panic triggered by a perfctr or losing unknown nmis from other sources. Maybe this can be fixed by implementing handlers for those sources.

I was playing around with it yesterday trying to fix this. My idea is to skip an unkown nmi if the privious nmi was a *handled* perfctr

nmi. I will probably post an rfc patch early next week.

Another problem I encountered is that unknown nmis from the chipset are not reenabled, thus when hitting the nmi button I only see one unknown nmi message per boot, if I reenable it, I get an nmi storm firing nmi_watchdog. Uhh....

Quoted 2 lines · Robert Richter

I was playing around with it yesterday trying to fix this. My idea is to skip an unkown nmi if the privious nmi was a *handled* perfctr

You might want to add a little more logic that says *handled* _and_ had more than one perfctr trigger. Most of the time only one perfctr is probably triggering, so you might be eating unknown_nmi's needlessly.

Just a thought.

Quoted 2 lines

nmi. I will probably post an rfc patch early next week.

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001

Quoted 9 lines · Don Zickus

Quoted 2 lines · Robert Richter

I was playing around with it yesterday trying to fix this. My idea is to skip an unkown nmi if the privious nmi was a *handled* perfctr

You might want to add a little more logic that says *handled* _and_ had more than one perfctr trigger. Most of the time only one perfctr is probably triggering, so you might be eating unknown_nmi's needlessly.

Quoted 1 line

Just a thought.

Yes, that's true. It could be implemented on top of the patch below.

Quoted 2 lines

Quoted 2 lines

nmi. I will probably post an rfc patch early next week.

Here it comes:

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

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

index f2da20f..c3cd159 100644
@@ -1200,12 +1200,16 @@ void perf_events_lapic_init(void)
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 }
 
+static DEFINE_PER_CPU(unsigned int, perfctr_handled);
+
 static int __kprobes
 perf_event_nmi_handler(struct notifier_block *self,
 			 unsigned long cmd, void *__args)
 {
 	struct die_args *args = __args;
 	struct pt_regs *regs;
+	unsigned int this_nmi;
+	unsigned int prev_nmi;
 
 	if (!atomic_read(&active_events))
 		return NOTIFY_DONE;

@@ -1214,7 +1218,26 @@ perf_event_nmi_handler(struct notifier_block *self,
 	case DIE_NMI:
 	case DIE_NMI_IPI:
 		break;
-
+	case DIE_NMIUNKNOWN:
+		/*
+		 * This one could be our NMI, two events could trigger
+		 * 'simultaneously' raising two back-to-back NMIs. If
+		 * the first NMI handles both, the latter will be
+		 * empty and daze the CPU.
+		 *
+		 * So, we drop this unknown NMI if the previous NMI
+		 * was handling a perfctr. Otherwise we pass it and
+		 * let the kernel handle the unknown nmi.
+		 *
+		 * Note: this could be improved if we drop unknown
+		 * NMIs only if we handled more than one perfctr in
+		 * the previous NMI.
+		 */
+		this_nmi = percpu_read(irq_stat.__nmi_count);
+		prev_nmi = __get_cpu_var(perfctr_handled);
+		if (this_nmi == prev_nmi + 1)
+			return NOTIFY_STOP;
+		return NOTIFY_DONE;
 	default:
 		return NOTIFY_DONE;
 	}

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1247

Quoted 4 lines

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

@@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
 	regs = args->regs;
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);

If only I'm not missing something this apic_write should go up to "case DIE_NMIUNKNOWN" site, no?

Quoted 7 lines · Cyrill Gorcunov

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1247

Quoted 4 lines

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

@@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
 	regs = args->regs;
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);

If only I'm not missing something this apic_write should go up to "case DIE_NMIUNKNOWN" site, no?

This seems to be code from the non-nmi implementation and can be removed at all, which should be a separate patch. The vector is already set up.

-Robert

Quoted 14 lines · Robert Richter

Quoted 7 lines · Cyrill Gorcunov

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1247

Quoted 4 lines

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

@@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
 	regs = args->regs;
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);

If only I'm not missing something this apic_write should go up to "case DIE_NMIUNKNOWN" site, no?

This seems to be code from the non-nmi implementation and can be removed at all, which should be a separate patch. The vector is already set up.

Quoted 2 lines

-Robert

No, this is just a short way to unmask LVTPC (which is required for cpus). Actually lookig into this snippet I found that in p4 pmu I've made one redundant unmaksing operation. will update as only this area settle down.

Quoted 15 lines · Cyrill Gorcunov

Quoted 14 lines · Robert Richter

Quoted 7 lines · Cyrill Gorcunov

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1247

Quoted 4 lines

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

@@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
 	regs = args->regs;
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);

If only I'm not missing something this apic_write should go up to "case DIE_NMIUNKNOWN" site, no?

This seems to be code from the non-nmi implementation and can be removed at all, which should be a separate patch. The vector is already set up.

Quoted 7 lines

Quoted 2 lines

-Robert

No, this is just a short way to unmask LVTPC (which is required for cpus). Actually lookig into this snippet I found that in p4 pmu I've made one redundant unmaksing operation. will update as only this area settle down.

The vector is setup in hw_perf_enable() and then never masked. The perfctrs nmi is alwayes enabled since then. I still see no reason for unmasking it again with every nmi. Once you handle the nmi it is also enabled.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

It gets masked on NMI arrival, at least for some models (Core Duo, P4, P6 M and I suspect more theh that, that was the reason oprofile has it, also there is a note in SDM V3a page 643).

Quoted 3 lines · Cyrill Gorcunov

It gets masked on NMI arrival, at least for some models (Core Duo, P4, P6 M and I suspect more theh that, that was the reason oprofile has it, also there is a note in SDM V3a page 643).

Yes, that's right, I never noticed that. Maybe it is better to implement the apic write it in model specific code then.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Quoted 6 lines · Robert Richter

Quoted 3 lines · Cyrill Gorcunov

It gets masked on NMI arrival, at least for some models (Core Duo, P4, P6 M and I suspect more theh that, that was the reason oprofile has it, also there is a note in SDM V3a page 643).

Quoted 3 lines

Yes, that's right, I never noticed that. Maybe it is better to implement the apic write it in model specific code then.

Perhaps we can make it simplier I think, ie like it was before -- we just move it under your new DIE_NMIUNKNOWN, in separate patch of course. Though I'm fine with either way.

(actually it's interesting to know wouldn't we leave lvt masked when
we hit 'second delayed nmi has arrived' situation, I guess we didn't hit it before in real yet :-)

Quoted 6 lines

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

-- Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 7 lines · Cyrill Gorcunov

Quoted 6 lines · Robert Richter

Quoted 3 lines · Cyrill Gorcunov

It gets masked on NMI arrival, at least for some models (Core Duo, P4, P6 M and I suspect more theh that, that was the reason oprofile has it, also there is a note in SDM V3a page 643).

Quoted 7 lines

Quoted 3 lines

Yes, that's right, I never noticed that. Maybe it is better to implement the apic write it in model specific code then.

Perhaps we can make it simplier I think, ie like it was before -- we just move it under your new DIE_NMIUNKNOWN, in separate patch of course. Though I'm fine with either way.

I do not understand why you want to put this in the 'unknown' path. Isn't it necessary to unmask the vector with every call of the nmi handler?

-Robert

Quoted 5 lines · Don Zickus

Quoted 2 lines · Robert Richter

I was playing around with it yesterday trying to fix this. My idea is to skip an unkown nmi if the privious nmi was a *handled* perfctr

Quoted 5 lines

You might want to add a little more logic that says *handled* _and_ had more than one perfctr trigger. Most of the time only one perfctr is probably triggering, so you might be eating unknown_nmi's needlessly.

Just a thought.

Yes, that's true. It could be implemented on top of the patch below.

Quoted 3 lines

Quoted 3 lines

nmi. I will probably post an rfc patch early next week.

Here it comes:

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

From 548cf5148f47618854a0eff22b1d55db71b6f8fc Mon Sep 17 00:00:00 2001

Quoted 6 lines · Robert Richter

Quoted 5 lines · Don Zickus

Quoted 2 lines · Robert Richter

I was playing around with it yesterday trying to fix this. My idea is to skip an unkown nmi if the privious nmi was a *handled* perfctr

Quoted 7 lines

Quoted 5 lines

You might want to add a little more logic that says *handled* _and_ had more than one perfctr trigger. Most of the time only one perfctr is probably triggering, so you might be eating unknown_nmi's needlessly.

Just a thought.

Yes, that's true. It could be implemented on top of the patch below.

I did, but the changes basically revert the bulk of your patch.

Quoted 10 lines

Quoted 3 lines

Quoted 3 lines

nmi. I will probably post an rfc patch early next week.

Here it comes:

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

On top of Robert's patch:
(compiled tested only because I don't have a fancy button to trigger unknown nmis)

From 548cf5148f47618854a0eff22b1d55db71b6f8fc Mon Sep 17 00:00:00 2001
From: Don Zickus <[email protected]>
Date: Tue, 10 Aug 2010 16:40:03 -0400
Subject: [PATCH] perf, x86: only skip NMIs when multiple perfctrs trigger

A small optimization on top of Robert's patch that limits the skipping of NMI's to cases where we detect multiple perfctr events have happened.

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

---
 arch/x86/kernel/cpu/perf_event.c |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)

arch/x86/kernel/cpu/perf_event.c +10 -14

index c3cd159..066046d 100644
@@ -1154,7 +1154,7 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
 		/*
 		 * event overflow
 		 */
-		handled		= 1;
+		handled		+= 1;
 		data.period	= event->hw.last_period;
 
 		if (!x86_perf_event_set_period(event))

@@ -1200,7 +1200,7 @@ void perf_events_lapic_init(void)
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 }
 
-static DEFINE_PER_CPU(unsigned int, perfctr_handled);
+static DEFINE_PER_CPU(unsigned int, perfctr_skip);
 
 static int __kprobes
 perf_event_nmi_handler(struct notifier_block *self,

@@ -1208,8 +1208,7 @@ perf_event_nmi_handler(struct notifier_block *self,
 {
 	struct die_args *args = __args;
 	struct pt_regs *regs;
-	unsigned int this_nmi;
-	unsigned int prev_nmi;
+	int handled = 0;
 
 	if (!atomic_read(&active_events))
 		return NOTIFY_DONE;

@@ -1229,14 +1228,11 @@ perf_event_nmi_handler(struct notifier_block *self,
 		 * was handling a perfctr. Otherwise we pass it and
 		 * let the kernel handle the unknown nmi.
 		 *
-		 * Note: this could be improved if we drop unknown
-		 * NMIs only if we handled more than one perfctr in
-		 * the previous NMI.
 		 */
-		this_nmi = percpu_read(irq_stat.__nmi_count);
-		prev_nmi = __get_cpu_var(perfctr_handled);
-		if (this_nmi == prev_nmi + 1)
+		if (__get_cpu_var(perfctr_skip)){
+			__get_cpu_var(perfctr_skip) -=1;
 			return NOTIFY_STOP;
+		}
 		return NOTIFY_DONE;
 	default:
 		return NOTIFY_DONE;

@@ -1246,11 +1242,21 @@ perf_event_nmi_handler(struct notifier_block *self,
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
-	if (!x86_pmu.handle_irq(regs))
+	handled = x86_pmu.handle_irq(regs);
+	if (!handled)
+		/* not our NMI */
 		return NOTIFY_DONE;
-
-	/* handled */
-	__get_cpu_var(perfctr_handled) = percpu_read(irq_stat.__nmi_count);
+	else if (handled > 1)

arch/x86/kernel/cpu/perf_event.c void perf_events_lapic_init(void) · line 1203

Quoted 6 lines · Don Zickus

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

@@ -1200,7 +1200,7 @@ void perf_events_lapic_init(void)
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 }
 
-static DEFINE_PER_CPU(unsigned int, perfctr_handled);
+static DEFINE_PER_CPU(unsigned int, perfctr_skip);

Quoted 33 lines

arch/x86/kernel/cpu/perf_event.c +9 -10

@@ -1229,14 +1228,11 @@ perf_event_nmi_handler(struct notifier_block *self,
 		 * was handling a perfctr. Otherwise we pass it and
 		 * let the kernel handle the unknown nmi.
 		 *
-		 * Note: this could be improved if we drop unknown
-		 * NMIs only if we handled more than one perfctr in
-		 * the previous NMI.
 		 */
-		this_nmi = percpu_read(irq_stat.__nmi_count);
-		prev_nmi = __get_cpu_var(perfctr_handled);
-		if (this_nmi == prev_nmi + 1)
+		if (__get_cpu_var(perfctr_skip)){
+			__get_cpu_var(perfctr_skip) -=1;
 			return NOTIFY_STOP;
+		}
 		return NOTIFY_DONE;
 	default:
 		return NOTIFY_DONE;

@@ -1246,11 +1242,21 @@ perf_event_nmi_handler(struct notifier_block *self,
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
-	if (!x86_pmu.handle_irq(regs))
+	handled = x86_pmu.handle_irq(regs);
+	if (!handled)
+		/* not our NMI */
 		return NOTIFY_DONE;
-
-	/* handled */
-	__get_cpu_var(perfctr_handled) = percpu_read(irq_stat.__nmi_count);
+	else if (handled > 1)
+		/*
+		 * More than one perfctr triggered.  This could have

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1259

Quoted 8 lines

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

+		 * caused a second NMI that we must now skip because
+		 * we have already handled it.  Remember it.
+		 *
+		 * NOTE: We have no way of knowing if a second NMI was
+		 * actually triggered, so we may accidentally skip a valid
+		 * unknown nmi later.
+		 */
+		__get_cpu_var(perfctr_skip) +=1;

May be make it just a pending bit. I mean not something that can go further 1, because you can't have more than 1 pending anyway. I don't know how that could happen you get accidental perctr_skip > 1, may be expected pending NMIs that don't happen somehow, but better be paranoid with that, as it's about trying not to miss hardware errors.

Thanks.

Quoted 4 lines

Quoted 4 lines

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

+		 *
+		 * NOTE: We have no way of knowing if a second NMI was
+		 * actually triggered, so we may accidentally skip a valid
+		 * unknown nmi later.

Quoted 2 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1259

Quoted 2 lines

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

+		 */
+		__get_cpu_var(perfctr_skip) +=1;

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every. Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed. The trick in my patch is that you *know*, which nmi you want to skip.

Quoted 4 lines · Robert Richter

Quoted 4 lines

Quoted 4 lines

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

+		 *
+		 * NOTE: We have no way of knowing if a second NMI was
+		 * actually triggered, so we may accidentally skip a valid
+		 * unknown nmi later.

Quoted 8 lines

Quoted 2 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1259

Quoted 2 lines

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

+		 */
+		__get_cpu_var(perfctr_skip) +=1;

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every. Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed. The trick in my patch is that you *know*, which nmi you want to skip.

I guess I am confused. The way I read your patch was that you assumed the next NMI would be the one you skip and if there was another NMI in between the handled one and the one to skip, you would not skip it (nmi count != prev + 1) and it would produce an accidental unknown nmi.

I tried to change that with my patch by setting the skip flag which would be drained on the next unknown nmi, independent of where it is in the NMI backlog of NMIs.

Did I misread something?

Cheers,
Don

Quoted 14 lines · Don Zickus

Quoted 4 lines · Robert Richter

Quoted 4 lines

Quoted 4 lines

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

+		 *
+		 * NOTE: We have no way of knowing if a second NMI was
+		 * actually triggered, so we may accidentally skip a valid
+		 * unknown nmi later.

Quoted 8 lines

Quoted 2 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1259

Quoted 2 lines

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

+		 */
+		__get_cpu_var(perfctr_skip) +=1;

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every. Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed. The trick in my patch is that you *know*, which nmi you want to skip.

Quoted 4 lines

I guess I am confused. The way I read your patch was that you assumed the next NMI would be the one you skip and if there was another NMI in between the handled one and the one to skip, you would not skip it (nmi count != prev + 1) and it would produce an accidental unknown nmi.

That's how it works.

Quoted 3 lines

I tried to change that with my patch by setting the skip flag which would be drained on the next unknown nmi, independent of where it is in the NMI backlog of NMIs.

"setting the skip flag which would be drained on the next unknown nmi"

That's what is wrong, it drops every unknown nmi, no matter when it is

detected. In between there could be 1000's of valid other nmis handled. You even could have been returned from nmi mode. But still, the next unknown nmi will be dropped. Your patch could accumulate also

Quoted 7 lines · Frederic Weisbecker

arch/x86/kernel/cpu/perf_event.c void perf_events_lapic_init(void) · line 1203

Quoted 6 lines · Don Zickus

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

@@ -1200,7 +1200,7 @@ void perf_events_lapic_init(void)
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 }
 
-static DEFINE_PER_CPU(unsigned int, perfctr_handled);
+static DEFINE_PER_CPU(unsigned int, perfctr_skip);

Yes, using perfctr_skip is better to understand ...

Quoted 37 lines

Quoted 37 lines

arch/x86/kernel/cpu/perf_event.c +13 -10

@@ -1229,14 +1228,11 @@ perf_event_nmi_handler(struct notifier_block *self,
 		 * was handling a perfctr. Otherwise we pass it and
 		 * let the kernel handle the unknown nmi.
 		 *
-		 * Note: this could be improved if we drop unknown
-		 * NMIs only if we handled more than one perfctr in
-		 * the previous NMI.
 		 */
-		this_nmi = percpu_read(irq_stat.__nmi_count);
-		prev_nmi = __get_cpu_var(perfctr_handled);
-		if (this_nmi == prev_nmi + 1)
+		if (__get_cpu_var(perfctr_skip)){
+			__get_cpu_var(perfctr_skip) -=1;
 			return NOTIFY_STOP;
+		}
 		return NOTIFY_DONE;
 	default:
 		return NOTIFY_DONE;

@@ -1246,11 +1242,21 @@ perf_event_nmi_handler(struct notifier_block *self,
 
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 
-	if (!x86_pmu.handle_irq(regs))
+	handled = x86_pmu.handle_irq(regs);
+	if (!handled)
+		/* not our NMI */
 		return NOTIFY_DONE;
-
-	/* handled */
-	__get_cpu_var(perfctr_handled) = percpu_read(irq_stat.__nmi_count);
+	else if (handled > 1)
+		/*
+		 * More than one perfctr triggered.  This could have
+		 * caused a second NMI that we must now skip because
+		 * we have already handled it.  Remember it.
+		 *
+		 * NOTE: We have no way of knowing if a second NMI was

Quoted 4 lines

arch/x86/kernel/cpu/perf_event.c perf_event_nmi_handler(struct notifier_block *self, · line 1259

Quoted 4 lines

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

+		 * actually triggered, so we may accidentally skip a valid
+		 * unknown nmi later.
+		 */
+		__get_cpu_var(perfctr_skip) +=1;

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every.

Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed.

The trick in my patch is that you *know*, which nmi you want to skip.

Quoted 4 lines

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every.

Isn't it what we want? Only the next unknown nmi gets dropped.

Quoted 2 lines

Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed.

I'm not sure what you mean here. Are you thinking about a third NMI source that triggers while we are still handling the first NMI in the back to back sequence?

Quoted 1 line

The trick in my patch is that you *know*, which nmi you want to skip.

Well with the flag you also know which nmi you want to skip.

Quoted 26 lines · Frederic Weisbecker

Quoted 4 lines

... but this will not work. You have to mark the *absolute* nmi number here. If you only raise a flag, the next unknown nmi will be dropped, every.

Isn't it what we want? Only the next unknown nmi gets dropped.

Quoted 2 lines

Because, in between there could have been other nmis that stopped the chain and thus the 'unknown' path is not executed.

I'm not sure what you mean here. Are you thinking about a third NMI source that triggers while we are still handling the first NMI in the back to back sequence?

Quoted 1 line

The trick in my patch is that you *know*, which nmi you want to skip.

Quoted 1 line

Well with the flag you also know which nmi you want to skip.

We cannot assume that all cpus have the same behavior here. Imagine a cpu that handles 2 counters and *does not* trigger a back-to-back nmi. With flags only implemented, the next unknown nmi will be dropped anyway, no matter when it fires. We have to check the nmi sequence.

The next thing you have to be aware is, a registered nmi handler is not called with every nmi. If there was another nmi source and a handler with higher priority was returning a stop, when all other subsequent handlers are not called. Esp. 'unknown nmi' is called only in rare cases. So, a handler might not get noticed of an nmi. This means, if a handler gets called a 2nd time, it must not necessarily the next (2nd) nmi.

Quoted 4 lines

We cannot assume that all cpus have the same behavior here. Imagine a cpu that handles 2 counters and *does not* trigger a back-to-back nmi. With flags only implemented, the next unknown nmi will be dropped anyway, no matter when it fires. We have to check the nmi sequence.

I'd expect it to be an ABI. NMIs can't nest, but if one triggers while servicing another, it should trigger right after (once we "iret", which reenables NMIs).

But I haven't checked intel or amd documentation about that.

Quoted 7 lines

The next thing you have to be aware is, a registered nmi handler is not called with every nmi. If there was another nmi source and a handler with higher priority was returning a stop, when all other subsequent handlers are not called. Esp. 'unknown nmi' is called only in rare cases. So, a handler might not get noticed of an nmi. This means, if a handler gets called a 2nd time, it must not necessarily the next (2nd) nmi.

Yeah, in this case we can just clear the __ger_cpu_var(next_nmi_skip) when another handler service the next NMI.

May be make it just a pending bit. I mean not something that can go further 1, because you can't have more than 1 pending anyway. I don't know how that could happen you get accidental perctr_skip > 1, may be expected pending NMIs that don't happen somehow, but better be paranoid with that, as it's about trying not to miss hardware errors.

Quoted 4 lines

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

On top of Robert's patch:
(compiled tested only because I don't have a fancy button to trigger unknown nmis)

Quoted 5 lines · Don Zickus

Quoted 4 lines

From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

Quoted 3 lines

On top of Robert's patch:
(compiled tested only because I don't have a fancy button to trigger unknown nmis)

You can trigger unknown NMIs via apic->send_IPI_mask(cpu_mask, NMI_VECTOR).

How about the algorithm as follow:

The only thing that might screw this up would be an SMI which takes longer than 1000 but that should be rare and would probably be related to the unknown NMI anyway.

From 8bb831af56d118b85fc38e0ddc2e516f7504b9fb Mon Sep 17 00:00:00 2001

I was debuging this a little more, see version 2 below.

-Robert

Quoted 1 line · Robert Richter

I was debuging this a little more, see version 2 below.

I was testing the patch further, it properly filters perfctr back-to-back nmis. I was able to reliable detect unknown nmis triggered by the nmi button during high load perf sessions with multiple counters, no false positives.

Quoted 4 lines

I was testing the patch further, it properly filters perfctr back-to-back nmis. I was able to reliable detect unknown nmis triggered by the nmi button during high load perf sessions with multiple counters, no false positives.

For my own curiousity, what type of high load perf sessions are you using to test this. I don't know perf well enough to have it generate events across multiple perfctrs.

I was debuging this a little more, see version 2 below.

-Robert

--

From 8bb831af56d118b85fc38e0ddc2e516f7504b9fb Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

When perfctrs are running it is valid to have unhandled nmis, two events could trigger 'simultaneously' raising two back-to-back NMIs. If the first NMI handles both, the latter will be empty and daze the CPU.

The solution to avoid an 'unknown nmi' massage in this case was simply to stop the nmi handler chain when perfctrs are runnning by stating the nmi was handled. This has the drawback that a) we can not detect unknown nmis anymore, and b) subsequent nmi handlers are not called.

This patch addresses this. Now, we check this unknown NMI if it could be a perfctr back-to-back NMI. Otherwise we pass it and let the kernel handle the unknown nmi.

I was debuging this a little more, see version 2 below.

-Robert

--

From 8bb831af56d118b85fc38e0ddc2e516f7504b9fb Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

When perfctrs are running it is valid to have unhandled nmis, two events could trigger 'simultaneously' raising two back-to-back NMIs. If the first NMI handles both, the latter will be empty and daze the CPU.

The solution to avoid an 'unknown nmi' massage in this case was simply to stop the nmi handler chain when perfctrs are runnning by stating the nmi was handled. This has the drawback that a) we can not detect unknown nmis anymore, and b) subsequent nmi handlers are not called.

This patch addresses this. Now, we check this unknown NMI if it could be a perfctr back-to-back NMI. Otherwise we pass it and let the kernel handle the unknown nmi.

This is a debug log:

cpu #6, nmi #32333, skip_nmi #32330, handled = 1, time = 1934364430 cpu #6, nmi #32334, skip_nmi #32330, handled = 1, time = 1934704616 cpu #6, nmi #32335, skip_nmi #32336, handled = 2, time = 1936032320 cpu #6, nmi #32336, skip_nmi #32336, handled = 0, time = 1936034139 cpu #6, nmi #32337, skip_nmi #32336, handled = 1, time = 1936120100 cpu #6, nmi #32338, skip_nmi #32336, handled = 1, time = 1936404607 cpu #6, nmi #32339, skip_nmi #32336, handled = 1, time = 1937983416 cpu #6, nmi #32340, skip_nmi #32341, handled = 2, time = 1938201032 cpu #6, nmi #32341, skip_nmi #32341, handled = 0, time = 1938202830 cpu #6, nmi #32342, skip_nmi #32341, handled = 1, time = 1938443743 cpu #6, nmi #32343, skip_nmi #32341, handled = 1, time = 1939956552 cpu #6, nmi #32344, skip_nmi #32341, handled = 1, time = 1940073224 cpu #6, nmi #32345, skip_nmi #32341, handled = 1, time = 1940485677 cpu #6, nmi #32346, skip_nmi #32347, handled = 2, time = 1941947772 cpu #6, nmi #32347, skip_nmi #32347, handled = 1, time = 1941949818 cpu #6, nmi #32348, skip_nmi #32347, handled = 0, time = 1941951591 Uhhuh. NMI received for unknown reason 00 on CPU 6.
Do you have a strange power saving mode enabled?
Dazed and confused, but trying to continue

Deltas:

nmi #32334 340186
nmi #32335 1327704
nmi #32336 1819 <<<< back-to-back nmi [1]
nmi #32337 85961
nmi #32338 284507
nmi #32339 1578809
nmi #32340 217616
nmi #32341 1798 <<<< back-to-back nmi [2]
nmi #32342 240913
nmi #32343 1512809
nmi #32344 116672
nmi #32345 412453

nmi #32346 1462095   <<<< 1st nmi (standard) handling 2 counters
nmi #32347 2046      <<<< 2nd nmi (back-to-back) handling one counter
nmi #32348 1773      <<<< 3rd nmi (back-to-back) handling no counter! [3]

For back-to-back nmi detection there are the following rules:

The perfctr nmi handler was handling more than one counter and no counter was handled in the subsequent nmi (see [1] and [2] above).

There is another case if there are two subsequent back-to-back nmis [3]. In this case we measure the time between the first and the 2nd. The 2nd is detected as back-to-back because the first handled more than one counter. The time between the 1st and the 2nd is used to calculate a range for which we assume a back-to-back nmi. Now, the 3rd nmi triggers, we measure again the time delta and compare it with the first delta from which we know it was a back-to-back nmi. If the 3rd nmi is within the range, it is also a back-to-back nmi and we drop it.

Signed-off-by Robert Richter <[email protected]>

---

From 8bb831af56d118b85fc38e0ddc2e516f7504b9fb Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Thu, 5 Aug 2010 16:19:59 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

When perfctrs are running it is valid to have unhandled nmis, two events could trigger 'simultaneously' raising two back-to-back NMIs. If the first NMI handles both, the latter will be empty and daze the CPU.

The solution to avoid an 'unknown nmi' massage in this case was simply to stop the nmi handler chain when perfctrs are runnning by stating the nmi was handled. This has the drawback that a) we can not detect unknown nmis anymore, and b) subsequent nmi handlers are not called.

This patch addresses this. Now, we check this unknown NMI if it could be a perfctr back-to-back NMI. Otherwise we pass it and let the kernel handle the unknown nmi.

This is a debug log:

Deltas:

nmi #32346 1462095   <<<< 1st nmi (standard) handling 2 counters
nmi #32347 2046      <<<< 2nd nmi (back-to-back) handling one counter
nmi #32348 1773      <<<< 3rd nmi (back-to-back) handling no counter! [3]

For back-to-back nmi detection there are the following rules:

The perfctr nmi handler was handling more than one counter and no counter was handled in the subsequent nmi (see [1] and [2] above).

There is another case if there are two subsequent back-to-back nmis [3]. In this case we measure the time between the first and the 2nd. The 2nd is detected as back-to-back because the first handled more than one counter. The time between the 1st and the 2nd is used to calculate a range for which we assume a back-to-back nmi. Now, the 3rd nmi triggers, we measure again the time delta and compare it with the first delta from which we know it was a back-to-back nmi. If the 3rd nmi is within the range, it is also a back-to-back nmi and we drop it.

Quoted 8 lines

There is another case if there are two subsequent back-to-back nmis [3]. In this case we measure the time between the first and the 2nd. The 2nd is detected as back-to-back because the first handled more than one counter. The time between the 1st and the 2nd is used to calculate a range for which we assume a back-to-back nmi. Now, the 3rd nmi triggers, we measure again the time delta and compare it with the first delta from which we know it was a back-to-back nmi. If the 3rd nmi is within the range, it is also a back-to-back nmi and we drop it.

I liked the one without funny timestamps in better, the whole timestamps thing just feels too fragile.

Relying on handled > 1 to arm the back-to-back filter seems doable.

(Also, you didn't deal with the TSC going backwards..)

Quoted 3 lines · Peter Zijlstra

I liked the one without funny timestamps in better, the whole timestamps thing just feels too fragile.

Me too, the former Roberts patch (if I'm not missing something) looks good to me.

Quoted 2 lines

Relying on handled > 1 to arm the back-to-back filter seems doable.

It's doable _but_ I think there is nothing we can do, there is no way (at least I known of) to check if there is latched nmi from perf counters. We only can assume that if there multiple counters overflowed most probably the next unknown nmi has the same nature, ie it came from perf.

Yes, we can loose real unknown nmi in this
case but I think this is justified trade off. If an user need a precise counting of unknown nmis he should not arm perf events at all, if there an user with nmi button (guys where did you get this magic buttuns? i need one ;) he better to not arm perf events too otherwise he might have to click twice

(and of course we should keep in mind Andi's proposal but it
is a next step I think).

Quoted 1 line

(Also, you didn't deal with the TSC going backwards..)

Quoted 5 lines · Cyrill Gorcunov

Quoted 3 lines · Peter Zijlstra

I liked the one without funny timestamps in better, the whole timestamps thing just feels too fragile.

Quoted 5 lines

Me too, the former Roberts patch (if I'm not missing something) looks good to me.

Quoted 2 lines

Relying on handled > 1 to arm the back-to-back filter seems doable.

Peter, I will rip out the timestamp code from the -v2 patch. My first patch does not deal with a 2-1-0 sequence, so it has false positives. We do not necessarily need the timestamps if back-to-back nmis are rare. Without using timestamps the statistically lost ratio for unknown nmis will be as the ratio for back-to-back nmis, with timestamps we could catch almost every unknown nmi. So if we encounter problems we could still implement timestamp code on top.

Quoted 5 lines

It's doable _but_ I think there is nothing we can do, there is no way (at least I known of) to check if there is latched nmi from perf counters. We only can assume that if there multiple counters overflowed most probably the next unknown nmi has the same nature, ie it came from perf.

As said, I think with timestamps we could be able to detect 100% of the unknown nmis. I guess we get now more than 90% with mutliple counters, and 100% with a single counter running. So, this is already more than a simple improvement.

Quoted 10 lines

Yes, we can loose real unknown nmi in this
case but I think this is justified trade off. If an user need a precise counting of unknown nmis he should not arm perf events at all, if there an user with nmi button (guys where did you get this magic buttuns? i need one ;) he better to not arm perf events too otherwise he might have to click twice

(and of course we should keep in mind Andi's proposal but it
is a next step I think).

Yes, this patch is the first step, now we can change the nmi handler priority. The perf handler must not have the lowest priority anymore.

Quoted 1 line

Quoted 1 line

(Also, you didn't deal with the TSC going backwards..)

Does this also happen in the case of a back-to-back nmi? I don't know the conditions for a backward running TSC. Maybe, if an nmi is retriggered the TSC wont be adjusted by a negative offset, I don't know...

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

I never heard of backward running tsc, though tsc is a strange beast.

Quoted 2 lines · Cyrill Gorcunov

I never heard of backward running tsc, though tsc is a strange beast.

Its not supposed to happen, but then there's BIOS failure-add that frobs the TSC from SMIs and fun TSC artifacts around CPU frequency changes and people resetting TSC in S-states etc..

In short, never trust the TSC to be even remotely sane.

I suspect Peter was supposed to be in To: field ;)

Robert, I think we still may miss unknown irq, consider the case when unknown nmi is latched while you handle nmi from perf and what is more interesting several counters may be overflowed. So you set delta small enough and second (unknown nmi) will be in range and treated as being perf back-to-back, or I miss something from patch?

Quoted 1 line · Cyrill Gorcunov

I suspect Peter was supposed to be in To: field ;)

It was the attempt to answer 2 mails with one. :)

Quoted 5 lines

Robert, I think we still may miss unknown irq, consider the case when unknown nmi is latched while you handle nmi from perf and what is more interesting several counters may be overflowed. So you set delta small enough and second (unknown nmi) will be in range and treated as being perf back-to-back, or I miss something from patch?

Yes, that's true, but before you have to enable your Infinite Improbability Drive.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

From 71a206394d8a3536b033247ec14ad037fef77236 Mon Sep 17 00:00:00 2001

Peter,

this is version 3 without timestamp code. Compared to -v1 it implements the handling for 2-1-0 nmi sequences.

-Robert

From 71a206394d8a3536b033247ec14ad037fef77236 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
Date: Tue, 17 Aug 2010 16:42:03 +0200
Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs

Signed-off-by Robert Richter <[email protected]>

---

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

+       this_nmi = percpu_read(irq_stat.__nmi_count);
+       if (handled > 1)
+               goto mark_nmi;
+       if ((__get_cpu_var(nmi).marked == this_nmi)
+           && (__get_cpu_var(nmi).handled > 1))
+               /*
+                * We could have two subsequent back-to-back nmis: The
+                * first handles more than one counter, the 2nd
+                * handles only one counter and the 3rd handles no
+                * counter.
+                *
+                * This is the 2nd nmi because the previous was
+                * handling more than one counter. We will mark the
+                * next (3rd) and then drop it if unhandled.
+                */
+               goto mark_nmi;
+

I queued it with that part changed to:

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

+       this_nmi = percpu_read(irq_stat.__nmi_count);
+       if ((handled > 1) ||
+               /* the next nmi could be a back-to-back nmi */
+           ((__get_cpu_var(nmi).marked == this_nmi) &&
+            (__get_cpu_var(nmi).handled > 1))) {
+               /*
+                * We could have two subsequent back-to-back nmis: The
+                * first handles more than one counter, the 2nd
+                * handles only one counter and the 3rd handles no
+                * counter.
+                *

I queued it with that part changed to:

Quoted 2 lines · Peter Zijlstra

I queued it with that part changed to:

I realized the other day this change doesn't cover the nehalem, core and p4 cases which use

intel_pmu_handle_irq
p4_pmu_handle_irq

as their handlers. Though that patch can go on top of Robert's.

Quoted 7 lines · Don Zickus

Quoted 2 lines · Peter Zijlstra

I queued it with that part changed to:

I realized the other day this change doesn't cover the nehalem, core and p4 cases which use

Quoted 4 lines

intel_pmu_handle_irq
p4_pmu_handle_irq

as their handlers. Though that patch can go on top of Robert's.

Something like this?

Review static int p4_pmu_handle_irq(struct pt_r · line 673

Quoted 2 lines

(quoted diff) +0 -0

@@ -673,7 +673,7 @@ static int p4_pmu_handle_irq(struct pt_r
              if (!overflow && (val & (1ULL << (x86_pmu.cntval_bits

- 1))))

arch/x86/kernel/cpu/perf_event_p4.c static int p4_pmu_handle_irq(struct pt_r · line 676

Quoted 4 lines

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

                      continue;
 
-             handled += overflow;
+             handled += !!overflow;

No need to !! here, overflowed returns [0;1] though a small nit and could be updated later :)

1. :-)

Quoted 6 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event_intel.c +1 -0

 		x86_pmu.drain_pebs(regs);
+	}
 
 	for_each_set_bit(bit, (unsigned long *)&status, X86_PMC_IDX_MAX) {
 		struct perf_event *event = cpuc->events[bit];

arch/x86/kernel/cpu/perf_event_intel.c again: · line 758

Quoted 4 lines

arch/x86/kernel/cpu/perf_event_intel.c +2 -0

+		handled++;
+
 		if (!test_bit(bit, cpuc->active_mask))
 			continue;

Sorry I didn't notice this earlier, but I think you want that handled++ after the if(..) continue pieces. Otherwise you will always have a number

Quoted 1 line

1. :-)

Quoted 7 lines · Don Zickus

Quoted 2 lines · Peter Zijlstra

I queued it with that part changed to:

I realized the other day this change doesn't cover the nehalem, core and p4 cases which use

Quoted 4 lines

intel_pmu_handle_irq
p4_pmu_handle_irq

as their handlers. Though that patch can go on top of Robert's.

Something like this?

---
Index: linux-2.6/arch/x86/kernel/cpu/perf_event_intel.c
===================================================================

arch/x86/kernel/cpu/perf_event_intel.c +7 -2

@@ -713,6 +713,7 @@ static int intel_pmu_handle_irq(struct p
 	struct cpu_hw_events *cpuc;
 	int bit, loops;
 	u64 ack, status;
+	int handled = 0;
 
 	perf_sample_data_init(&data, 0);
 

@@ -743,12 +744,16 @@ again:
 	/*
 	 * PEBS overflow sets bit 62 in the global status register
 	 */
-	if (__test_and_clear_bit(62, (unsigned long *)&status))
+	if (__test_and_clear_bit(62, (unsigned long *)&status)) {
+		handled++;
 		x86_pmu.drain_pebs(regs);
+	}
 
 	for_each_set_bit(bit, (unsigned long *)&status, X86_PMC_IDX_MAX) {
 		struct perf_event *event = cpuc->events[bit];
 
+		handled++;
+
 		if (!test_bit(bit, cpuc->active_mask))
 			continue;
 

@@ -772,7 +777,7 @@ again:
 
 done:
 	intel_pmu_enable_all(0);
-	return 1;
+	return handled;
 }
 
 static struct event_constraint *

Index: linux-2.6/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================

arch/x86/kernel/cpu/perf_event_p4.c +2 -2

@@ -673,7 +673,7 @@ static int p4_pmu_handle_irq(struct pt_r
 		if (!overflow && (val & (1ULL << (x86_pmu.cntval_bits - 1))))
 			continue;
 
-		handled += overflow;
+		handled += !!overflow;
 
 		/* event overflow for sure */
 		data.period = event->hw.last_period;

@@ -690,7 +690,7 @@ static int p4_pmu_handle_irq(struct pt_r
 		inc_irq_stat(apic_perf_irqs);
 	}
 
-	return handled > 0;
+	return handled;
 }
 
 /*

Quoted 8 lines · Peter Zijlstra

Quoted 7 lines · Don Zickus

Quoted 2 lines · Peter Zijlstra

I queued it with that part changed to:

I realized the other day this change doesn't cover the nehalem, core and p4 cases which use

Quoted 6 lines

Quoted 4 lines

intel_pmu_handle_irq
p4_pmu_handle_irq

as their handlers. Though that patch can go on top of Robert's.

Something like this?

I tested this patch and Robert's on an AMD box and Nehalem box. Both worked as intended. However I did notice that whenever the AMD box detected handled >1, it was shortly followed by an unknown_nmi that was properly eaten with Robert's logic. Whereas on the Nehalem box I saw a lot of 'handled > 1' messages but very very few of them were followed by an unknown_nmi message (and those messages that did come were properly eaten).

Maybe that is just the differences in the cpu designs.

Of course I had to make the one change I mentioned previously for the perf_event_intel.c file (moving the handled++ logic down a few lines).

I didn't run the test on a P4 box.

Looks great, thanks guys!

Please someone send the final version with a changelog, with all the acks and tested-by's added, so that i can send it Linuswards.

Quoted 5 lines · Ingo Molnar

Please someone send the final version with a changelog, with all the acks and
tested-by's added, so that i can send it Linuswards.

I've got two patches queued, one from Robert (with a slight modification, which removes a goto) and one from myself converting the intel and p4 irq handler.

They're available at:

programming.kicks-ass.net/sekrit/patches.tar.bz2

I did add Tested-by tags, although I suspect its not the exact same bits Don tested..

Quoted 4 lines · Ingo Molnar

Please someone send the final version with a changelog, with all the acks and tested-by's added, so that i can send it Linuswards.

I've got two patches queued, one from Robert (with a slight modification, which removes a goto) and one from myself converting the intel and p4 irq handler.

They're available at:

programming.kicks-ass.net/sekrit/patches.tar.bz2

I did add Tested-by tags, although I suspect its not the exact same bits Don tested..

Quoted 10 lines · Peter Zijlstra

Quoted 4 lines · Ingo Molnar

Please someone send the final version with a changelog, with all the acks and tested-by's added, so that i can send it Linuswards.

I've got two patches queued, one from Robert (with a slight modification, which removes a goto) and one from myself converting the intel and p4 irq handler.

Quoted 6 lines

They're available at:

programming.kicks-ass.net/sekrit/patches.tar.bz2

I did add Tested-by tags, although I suspect its not the exact same bits Don tested..

I can re-test those exact patches later today if needed.

Quoted 13 lines · Don Zickus

Quoted 12 lines · Peter Zijlstra

Quoted 4 lines · Ingo Molnar

Please someone send the final version with a changelog, with all the acks and tested-by's added, so that i can send it Linuswards.

I've got two patches queued, one from Robert (with a slight modification, which removes a goto) and one from myself converting the intel and p4 irq handler.

They're available at:

Quoted 6 lines

Quoted 4 lines

programming.kicks-ass.net/sekrit/patches.tar.bz2

I did add Tested-by tags, although I suspect its not the exact same bits Don tested..

I can re-test those exact patches later today if needed.

Please pick up -tip in an hour or two and holler if something looks wrong.

it's not working so well, i'm getting:

Uhhuh. NMI received for unknown reason 00 on CPU 9. Do you have a strange power saving mode enabled? Dazed and confused, but trying to continue

on a nehalem box, after a perf top and perf stat run.

Thanks,

Ingo

it's not working so well, i'm getting:

Uhhuh. NMI received for unknown reason 00 on CPU 9. Do you have a strange power saving mode enabled? Dazed and confused, but trying to continue

on a nehalem box, after a perf top and perf stat run.

From 198be1044fa603bc9582a5c19134fdf9a433fff0 Mon Sep 17 00:00:00 2001

I tested this patch and Robert's on an AMD box and Nehalem box. Both worked as intended. However I did notice that whenever the AMD box detected handled >1, it was shortly followed by an unknown_nmi that was properly eaten with Robert's logic. Whereas on the Nehalem box I saw a lot of 'handled > 1' messages but very very few of them were followed by an unknown_nmi message (and those messages that did come were properly eaten).

Quoted 9 lines · Cyrill Gorcunov

On Wed, Aug 04, 2010 at 11:50:02AM -0400, Don Zickus wrote:
...

Quoted 7 lines

Quoted 6 lines

Well, first I guess having Yinghai CC'ed is a bonus ;)
The second thing is that I don't get why perf handler can't be _last_ call in default_do_nmi, if there were any nmi with reason (serr or parity) I think they should be calling first which of course don't eliminate the former issue but somewhat make it weaken.

Quoted 7 lines

Quoted 3 lines

Because the reason registers are never set. If they were, then the code wouldn't have to walk the notify_chain. :-)

maybe we're talking about different things. i meant that if there is nmi with a reason (from 0x61) the handling of such nmi should be done before notify_die I think (if only I not miss something behind).

No we are talking about the same thing. :-) And that code is already there. The problem is the bits in register 0x61 are not always set correctly in the case of SERRs (well at least in all the cases I have dealt with). So you can easily can a flood of unknown nmis from an SERR and register 0x61 would have the PERR/SERR bits set to 0. Fun, huh?

So then the first interrupt will see 3+ overflows, return 3+, and will thus eat 2+ NMIs, only one of which will be the pending interrupt, leaving 1+ NMIs from other sources to consume unhandled.

In which case Yinghai will have to press his NMI button 2+ times before it registers.

That said, that might be a better situation than always consuming unknown NMIs..

Suppose you have 4 counters (AMD, intel-nhm+), when more than 2 overflow the first will raise the PMI, if the other 2+ overflow before we disable the PMU it will try to raise 2+ more PMIs, but because hardware only has a single interrupt pending bit it will at most cause a single extra interrupt after we finish servicing the first one.

So then the first interrupt will see 3+ overflows, return 3+, and will thus eat 2+ NMIs, only one of which will be the pending interrupt, leaving 1+ NMIs from other sources to consume unhandled.

In which case Yinghai will have to press his NMI button 2+ times before it registers.

That said, that might be a better situation than always consuming unknown NMIs..

With nmi_watchdog enabled, perf_event_nmi_handler always return NOTIFY_STOP(active_events > 0), and the notifier call chain will not call further.

If it was not perf NMI, does the perf nmi handler may stop the real NMI handler get called because NOTIFY_STOP is returned??