Messages in this thread Patch in this message | | | Date | Mon, 18 Mar 2019 10:47:19 +0100 | From | Peter Zijlstra <> | Subject | Re: [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 patches...
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; continue; } @@ -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) inc_irq_stat(apic_perf_irqs);
| |