[lkml]   [2019]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [RFC PATCH 2/2] x86/perf/amd: Resolve NMI latency issues when multiple PMCs are active
On Fri, Mar 15, 2019 at 01:03:11PM +0100, Peter Zijlstra wrote:

> Anyway, we already had code to deal with spurious NMIs from AMD; see
> commit:
> 63e6be6d98e1 ("perf, x86: Catch spurious interrupts after disabling counters")
> And that looks to be doing something very much the same. Why then do you
> still need this on top?

And I think I've spotted a bug there; consider the case where only PMC3
has an active event left, then the interrupt would consume the running
state for PMC0-2, not leaving it for the spurious interrupt that might
come after it.

Similarly, if there's nothing running anymore, a single spurious
interrupt will clear the entire state.

It effectively is a single state, not a per-pmc one.

Something like the below would cure that... would that help with
something? Or were we going to get get rid of this entirely with your

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index e2b1447192a8..a8b5535f7888 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1432,6 +1432,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
struct cpu_hw_events *cpuc;
struct perf_event *event;
int idx, handled = 0;
+ int ridx = -1;
u64 val;

cpuc = this_cpu_ptr(&cpu_hw_events);
@@ -1453,8 +1454,8 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
* might still deliver spurious interrupts still
* in flight. Catch them:
- if (__test_and_clear_bit(idx, cpuc->running))
- handled++;
+ if (test_bit(idx, cpuc->running))
+ ridx = idx;

@@ -1477,6 +1478,11 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
x86_pmu_stop(event, 0);

+ if (!handled && ridx >= 0) {
+ __clear_bit(ridx, cpuc->running);
+ handled++;
+ }
if (handled)

 \ /
  Last update: 2019-03-18 10:48    [W:0.071 / U:3.456 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site