[PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

It turns out that the NMI latency workaround from commit 6d3edaae16c6
("x86/perf/amd: Resolve NMI latency issues for active PMCs") ends up
being too conservative and results in the perf NMI handler claiming NMIs
to easily on AMD hardware when the NMI watchdog is active.

This has an impact, for example, on the hpwdt (HPE watchdog timer) module.
This module can produce an NMI that is used to reset the system. It
registers an NMI handler for the NMI_UNKNOWN type and relies on the fact
that nothing has claimed an NMI so that its handler will be invoked when
the watchdog device produces an NMI. After the referenced commit, the
hpwdt module is unable to process its generated NMI if the NMI watchdog is
active, because the current NMI latency mitigation results in the NMI
being claimed by the perf NMI handler.

Update the AMD perf NMI latency mitigation workaround to, instead, use a
window of time. Whenever a PMC is handled in the perf NMI handler, set a
timestamp which will act as a perf NMI window. Any NMIs arriving within
that window will be claimed by perf. Anything outside that window will
not be claimed by perf. The value for the NMI window is set to 100 msecs.
This is a conservative value that easily covers any NMI latency in the
hardware. While this still results in a window in which the hpwdt module
will not receive its NMI, the window is now much, much smaller.

Cc: Jerry Hoemann <jerry.hoemann@hpe.com>
Fixes: 6d3edaae16c6 ("x86/perf/amd: Resolve NMI latency issues for active PMCs")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/events/amd/core.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index e7d35f60d53f..64c3e70b0556 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -5,12 +5,14 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/jiffies.h>
 #include <asm/apicdef.h>
 #include <asm/nmi.h>
 
 #include "../perf_event.h"
 
-static DEFINE_PER_CPU(unsigned int, perf_nmi_counter);
+static DEFINE_PER_CPU(unsigned long, perf_nmi_tstamp);
+static unsigned long perf_nmi_window;
 
 static __initconst const u64 amd_hw_cache_event_ids
 				[PERF_COUNT_HW_CACHE_MAX]
@@ -641,11 +643,12 @@ static void amd_pmu_disable_event(struct perf_event *event)
  * handler when multiple PMCs are active or PMC overflow while handling some
  * other source of an NMI.
  *
- * Attempt to mitigate this by using the number of active PMCs to determine
- * whether to return NMI_HANDLED if the perf NMI handler did not handle/reset
- * any PMCs. The per-CPU perf_nmi_counter variable is set to a minimum of the
- * number of active PMCs or 2. The value of 2 is used in case an NMI does not
- * arrive at the LAPIC in time to be collapsed into an already pending NMI.
+ * Attempt to mitigate this by creating an NMI window in which un-handled NMIs
+ * received during this window will be claimed. This prevents extending the
+ * window past when it is possible that latent NMIs should be received. The
+ * per-CPU perf_nmi_tstamp will be set to the window end time whenever perf has
+ * handled a counter. When an un-handled NMI is received, it will be claimed
+ * only if arriving within that window.
  */
 static int amd_pmu_handle_irq(struct pt_regs *regs)
 {
@@ -663,21 +666,19 @@ static int amd_pmu_handle_irq(struct pt_regs *regs)
 	handled = x86_pmu_handle_irq(regs);
 
 	/*
-	 * If a counter was handled, record the number of possible remaining
-	 * NMIs that can occur.
+	 * If a counter was handled, record a timestamp such that un-handled
+	 * NMIs will be claimed if arriving within that window.
 	 */
 	if (handled) {
-		this_cpu_write(perf_nmi_counter,
-			       min_t(unsigned int, 2, active));
+		this_cpu_write(perf_nmi_tstamp,
+			       jiffies + perf_nmi_window);
 
 		return handled;
 	}
 
-	if (!this_cpu_read(perf_nmi_counter))
+	if (time_after(jiffies, this_cpu_read(perf_nmi_tstamp)))
 		return NMI_DONE;
 
-	this_cpu_dec(perf_nmi_counter);
-
 	return NMI_HANDLED;
 }
 
@@ -909,6 +910,9 @@ static int __init amd_core_pmu_init(void)
 	if (!boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
 		return 0;
 
+	/* Avoid calulating the value each time in the NMI handler */
+	perf_nmi_window = msecs_to_jiffies(100);
+
 	switch (boot_cpu_data.x86) {
 	case 0x15:
 		pr_cont("Fam15h ");
-- 
2.17.1

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 06:57:41PM +0000, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> It turns out that the NMI latency workaround from commit 6d3edaae16c6
> ("x86/perf/amd: Resolve NMI latency issues for active PMCs") ends up
> being too conservative and results in the perf NMI handler claiming NMIs
> to easily on AMD hardware when the NMI watchdog is active.
> 
> This has an impact, for example, on the hpwdt (HPE watchdog timer) module.
> This module can produce an NMI that is used to reset the system. It
> registers an NMI handler for the NMI_UNKNOWN type and relies on the fact
> that nothing has claimed an NMI so that its handler will be invoked when
> the watchdog device produces an NMI. After the referenced commit, the
> hpwdt module is unable to process its generated NMI if the NMI watchdog is
> active, because the current NMI latency mitigation results in the NMI
> being claimed by the perf NMI handler.
> 
> Update the AMD perf NMI latency mitigation workaround to, instead, use a
> window of time. Whenever a PMC is handled in the perf NMI handler, set a
> timestamp which will act as a perf NMI window. Any NMIs arriving within
> that window will be claimed by perf. Anything outside that window will
> not be claimed by perf. The value for the NMI window is set to 100 msecs.
> This is a conservative value that easily covers any NMI latency in the
> hardware. While this still results in a window in which the hpwdt module
> will not receive its NMI, the window is now much, much smaller.

Blergh, I so hate all this. The proposed patch is basically duct tape.

The horribly retarded x86 NMI infrastructure strikes again :/

Tom; do you have any idea how expensive it is to twiddle CR8 and play
games with interrupt priorities instead of piling world + dog on this
one NMI line? (as compared to CLI/STI)

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Lendacky, Thomas]

On 8/1/19 4:16 PM, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 06:57:41PM +0000, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> It turns out that the NMI latency workaround from commit 6d3edaae16c6
>> ("x86/perf/amd: Resolve NMI latency issues for active PMCs") ends up
>> being too conservative and results in the perf NMI handler claiming NMIs
>> to easily on AMD hardware when the NMI watchdog is active.
>>
>> This has an impact, for example, on the hpwdt (HPE watchdog timer) module.
>> This module can produce an NMI that is used to reset the system. It
>> registers an NMI handler for the NMI_UNKNOWN type and relies on the fact
>> that nothing has claimed an NMI so that its handler will be invoked when
>> the watchdog device produces an NMI. After the referenced commit, the
>> hpwdt module is unable to process its generated NMI if the NMI watchdog is
>> active, because the current NMI latency mitigation results in the NMI
>> being claimed by the perf NMI handler.
>>
>> Update the AMD perf NMI latency mitigation workaround to, instead, use a
>> window of time. Whenever a PMC is handled in the perf NMI handler, set a
>> timestamp which will act as a perf NMI window. Any NMIs arriving within
>> that window will be claimed by perf. Anything outside that window will
>> not be claimed by perf. The value for the NMI window is set to 100 msecs.
>> This is a conservative value that easily covers any NMI latency in the
>> hardware. While this still results in a window in which the hpwdt module
>> will not receive its NMI, the window is now much, much smaller.
> 
> Blergh, I so hate all this. The proposed patch is basically duct tape.

Yeah, I'm not a fan either.

> 
> The horribly retarded x86 NMI infrastructure strikes again :/
> 
> Tom; do you have any idea how expensive it is to twiddle CR8 and play
> games with interrupt priorities instead of piling world + dog on this
> one NMI line? (as compared to CLI/STI)

I can check on that.  What are you thinking?

Thanks,
Tom

> 

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Thomas Gleixner]

On Thu, 1 Aug 2019, Lendacky, Thomas wrote:
> On 8/1/19 4:16 PM, Peter Zijlstra wrote:
> > On Thu, Aug 01, 2019 at 06:57:41PM +0000, Lendacky, Thomas wrote:
> >> From: Tom Lendacky <thomas.lendacky@amd.com>
> >>
> >> It turns out that the NMI latency workaround from commit 6d3edaae16c6
> >> ("x86/perf/amd: Resolve NMI latency issues for active PMCs") ends up
> >> being too conservative and results in the perf NMI handler claiming NMIs
> >> to easily on AMD hardware when the NMI watchdog is active.
> >>
> >> This has an impact, for example, on the hpwdt (HPE watchdog timer) module.
> >> This module can produce an NMI that is used to reset the system. It
> >> registers an NMI handler for the NMI_UNKNOWN type and relies on the fact
> >> that nothing has claimed an NMI so that its handler will be invoked when
> >> the watchdog device produces an NMI. After the referenced commit, the
> >> hpwdt module is unable to process its generated NMI if the NMI watchdog is
> >> active, because the current NMI latency mitigation results in the NMI
> >> being claimed by the perf NMI handler.
> >>
> >> Update the AMD perf NMI latency mitigation workaround to, instead, use a
> >> window of time. Whenever a PMC is handled in the perf NMI handler, set a
> >> timestamp which will act as a perf NMI window. Any NMIs arriving within
> >> that window will be claimed by perf. Anything outside that window will
> >> not be claimed by perf. The value for the NMI window is set to 100 msecs.
> >> This is a conservative value that easily covers any NMI latency in the
> >> hardware. While this still results in a window in which the hpwdt module
> >> will not receive its NMI, the window is now much, much smaller.
> > 
> > Blergh, I so hate all this. The proposed patch is basically duct tape.
> 
> Yeah, I'm not a fan either.
> 
> > 
> > The horribly retarded x86 NMI infrastructure strikes again :/
> > 
> > Tom; do you have any idea how expensive it is to twiddle CR8 and play
> > games with interrupt priorities instead of piling world + dog on this
> > one NMI line? (as compared to CLI/STI)
> 
> I can check on that.  What are you thinking?

Avoid the whole NMI mess, make the PMC interrupt a proper vector in the
highest prio bucket and instead of using CLI/STI use CR8. That would have
the additional advantage that we could prevent perf "NMI" then occsionally :)

Thanks,

	tglx

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 11:34:23PM +0200, Thomas Gleixner wrote:
> On Thu, 1 Aug 2019, Lendacky, Thomas wrote:
> > On 8/1/19 4:16 PM, Peter Zijlstra wrote:
> > > On Thu, Aug 01, 2019 at 06:57:41PM +0000, Lendacky, Thomas wrote:
> > >> From: Tom Lendacky <thomas.lendacky@amd.com>
> > >>
> > >> It turns out that the NMI latency workaround from commit 6d3edaae16c6
> > >> ("x86/perf/amd: Resolve NMI latency issues for active PMCs") ends up
> > >> being too conservative and results in the perf NMI handler claiming NMIs
> > >> to easily on AMD hardware when the NMI watchdog is active.
> > >>
> > >> This has an impact, for example, on the hpwdt (HPE watchdog timer) module.
> > >> This module can produce an NMI that is used to reset the system. It
> > >> registers an NMI handler for the NMI_UNKNOWN type and relies on the fact
> > >> that nothing has claimed an NMI so that its handler will be invoked when
> > >> the watchdog device produces an NMI. After the referenced commit, the
> > >> hpwdt module is unable to process its generated NMI if the NMI watchdog is
> > >> active, because the current NMI latency mitigation results in the NMI
> > >> being claimed by the perf NMI handler.
> > >>
> > >> Update the AMD perf NMI latency mitigation workaround to, instead, use a
> > >> window of time. Whenever a PMC is handled in the perf NMI handler, set a
> > >> timestamp which will act as a perf NMI window. Any NMIs arriving within
> > >> that window will be claimed by perf. Anything outside that window will
> > >> not be claimed by perf. The value for the NMI window is set to 100 msecs.
> > >> This is a conservative value that easily covers any NMI latency in the
> > >> hardware. While this still results in a window in which the hpwdt module
> > >> will not receive its NMI, the window is now much, much smaller.
> > > 
> > > Blergh, I so hate all this. The proposed patch is basically duct tape.
> > 
> > Yeah, I'm not a fan either.
> > 
> > > 
> > > The horribly retarded x86 NMI infrastructure strikes again :/
> > > 
> > > Tom; do you have any idea how expensive it is to twiddle CR8 and play
> > > games with interrupt priorities instead of piling world + dog on this
> > > one NMI line? (as compared to CLI/STI)
> > 
> > I can check on that.  What are you thinking?
> 
> Avoid the whole NMI mess, make the PMC interrupt a proper vector in the
> highest prio bucket and instead of using CLI/STI use CR8. That would have
> the additional advantage that we could prevent perf "NMI" then occsionally :)

Exactly, and not only the PMC, we can basically start giving out actual
vectors on register_nmi_handler() and do away with all that shared line
crap.

And then the actual NMI line will be mostly empty again, and it can read
its stupid slow reason port again.

One complication though; IRET et al only do EFLAGS, not CR8, so that's
going to be massive fun :-(

Did I say I hates the x86 interrupt scheme?

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Thomas Gleixner]

On Thu, 1 Aug 2019, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 11:34:23PM +0200, Thomas Gleixner wrote:
> > Avoid the whole NMI mess, make the PMC interrupt a proper vector in the
> > highest prio bucket and instead of using CLI/STI use CR8. That would have
> > the additional advantage that we could prevent perf "NMI" then occsionally :)
> 
> Exactly, and not only the PMC, we can basically start giving out actual
> vectors on register_nmi_handler() and do away with all that shared line
> crap.
> 
> And then the actual NMI line will be mostly empty again, and it can read
> its stupid slow reason port again.
> 
> One complication though; IRET et al only do EFLAGS, not CR8, so that's
> going to be massive fun :-(
> 
> Did I say I hates the x86 interrupt scheme?

You're not alone.

That stuff definitely violates article 3 of the Convention for the
Protection of Human Rights and Fundamental Freedoms.

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Lendacky, Thomas]

On 8/1/19 4:59 PM, Thomas Gleixner wrote:
> On Thu, 1 Aug 2019, Peter Zijlstra wrote:
>> On Thu, Aug 01, 2019 at 11:34:23PM +0200, Thomas Gleixner wrote:
>>> Avoid the whole NMI mess, make the PMC interrupt a proper vector in the
>>> highest prio bucket and instead of using CLI/STI use CR8. That would have
>>> the additional advantage that we could prevent perf "NMI" then occsionally :)
>>
>> Exactly, and not only the PMC, we can basically start giving out actual
>> vectors on register_nmi_handler() and do away with all that shared line
>> crap.
>>
>> And then the actual NMI line will be mostly empty again, and it can read
>> its stupid slow reason port again.
>>
>> One complication though; IRET et al only do EFLAGS, not CR8, so that's
>> going to be massive fun :-(

Talking to the hardware folks, they say setting CR8 is a serializing
instruction and has to communicate out to the APIC, so it's better to
use CLI/STI.

Thanks,
Tom

>>
>> Did I say I hates the x86 interrupt scheme?
> 
> You're not alone.
> 
> That stuff definitely violates article 3 of the Convention for the
> Protection of Human Rights and Fundamental Freedoms.
> 
> 

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Peter Zijlstra]

On Fri, Aug 02, 2019 at 02:33:41PM +0000, Lendacky, Thomas wrote:
> On 8/1/19 4:59 PM, Thomas Gleixner wrote:
> > On Thu, 1 Aug 2019, Peter Zijlstra wrote:
> >> On Thu, Aug 01, 2019 at 11:34:23PM +0200, Thomas Gleixner wrote:
> >>> Avoid the whole NMI mess, make the PMC interrupt a proper vector in the
> >>> highest prio bucket and instead of using CLI/STI use CR8. That would have
> >>> the additional advantage that we could prevent perf "NMI" then occsionally :)
> >>
> >> Exactly, and not only the PMC, we can basically start giving out actual
> >> vectors on register_nmi_handler() and do away with all that shared line
> >> crap.
> >>
> >> And then the actual NMI line will be mostly empty again, and it can read
> >> its stupid slow reason port again.
> >>
> >> One complication though; IRET et al only do EFLAGS, not CR8, so that's
> >> going to be massive fun :-(
> 
> Talking to the hardware folks, they say setting CR8 is a serializing
> instruction and has to communicate out to the APIC, so it's better to
> use CLI/STI.

Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8,
are serializing instructions", which had given me a little hope.

At the same time, all these chips still have the APIC TPR field too, so
much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8
I suppose :-(

I'll still finish the patches I started, just to see what it would look
like.

Thomas mentioned combining this with software IRQ disable (like Power),
but at that point maybe full software priorities aren't too bad either.
We'll see.

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Peter Zijlstra]

On Fri, Aug 02, 2019 at 06:20:15PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 02, 2019 at 02:33:41PM +0000, Lendacky, Thomas wrote:

> > Talking to the hardware folks, they say setting CR8 is a serializing
> > instruction and has to communicate out to the APIC, so it's better to
> > use CLI/STI.
> 
> Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8,
> are serializing instructions", which had given me a little hope.
> 
> At the same time, all these chips still have the APIC TPR field too, so
> much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8
> I suppose :-(
> 
> I'll still finish the patches I started, just to see what it would look
> like.

Another 'fun' issue I ran into while doing these patches; STI has a 1
instruction shadow, which we rely on, MOV CR8 does not. So things like:

native_safe_halt:
	sti
	hlt

turn into:

native_safe_halt:
	cli
	movl $0, %rax
	movq %rax, %cr8
	sti
	hlt

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Jerry Hoemann]

On Fri, Aug 02, 2019 at 06:33:28PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 02, 2019 at 06:20:15PM +0200, Peter Zijlstra wrote:
> > On Fri, Aug 02, 2019 at 02:33:41PM +0000, Lendacky, Thomas wrote:
> 
> > > Talking to the hardware folks, they say setting CR8 is a serializing
> > > instruction and has to communicate out to the APIC, so it's better to
> > > use CLI/STI.
> > 
> > Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8,
> > are serializing instructions", which had given me a little hope.
> > 
> > At the same time, all these chips still have the APIC TPR field too, so
> > much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8
> > I suppose :-(
> > 
> > I'll still finish the patches I started, just to see what it would look
> > like.
> 
> Another 'fun' issue I ran into while doing these patches; STI has a 1
> instruction shadow, which we rely on, MOV CR8 does not. So things like:
> 
> native_safe_halt:
> 	sti
> 	hlt
> 
> turn into:
> 
> native_safe_halt:
> 	cli
> 	movl $0, %rax
> 	movq %rax, %cr8
> 	sti
> 	hlt
> 

Hi Peter,

What is our the next step here?

Are you still looking to make this change?

Do we want to pick up Tom Lendacky's patch on an interim basis while
you're working on the bigger change?  (I can say we tested Tom's
patch and it does address the issue we were seeing.)

Thanks

Jerry

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp

[Jerry Hoemann]

On Thu, Aug 08, 2019 at 02:33:24PM -0600, Jerry Hoemann wrote:
> On Fri, Aug 02, 2019 at 06:33:28PM +0200, Peter Zijlstra wrote:
> > On Fri, Aug 02, 2019 at 06:20:15PM +0200, Peter Zijlstra wrote:
> > > On Fri, Aug 02, 2019 at 02:33:41PM +0000, Lendacky, Thomas wrote:
> > 
> > > > Talking to the hardware folks, they say setting CR8 is a serializing
> > > > instruction and has to communicate out to the APIC, so it's better to
> > > > use CLI/STI.
> > > 
> > > Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8,
> > > are serializing instructions", which had given me a little hope.
> > > 
> > > At the same time, all these chips still have the APIC TPR field too, so
> > > much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8
> > > I suppose :-(
> > > 
> > > I'll still finish the patches I started, just to see what it would look
> > > like.
> > 
> > Another 'fun' issue I ran into while doing these patches; STI has a 1
> > instruction shadow, which we rely on, MOV CR8 does not. So things like:
> > 
> > native_safe_halt:
> > 	sti
> > 	hlt
> > 
> > turn into:
> > 
> > native_safe_halt:
> > 	cli
> > 	movl $0, %rax
> > 	movq %rax, %cr8
> > 	sti
> > 	hlt
> > 
> 
> Hi Peter,
> 
> What is our the next step here?
> 
> Are you still looking to make this change?
> 
> Do we want to pick up Tom Lendacky's patch on an interim basis while
> you're working on the bigger change?  (I can say we tested Tom's
> patch and it does address the issue we were seeing.)
> 
> Thanks
> 
> Jerry
> 

Hi Peter,

Have you gotten a chance to look into this more?

Thanks

Jerry


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------