Re: [RFC PATCH 2/2] x86/perf/amd: Resolve NMI latency issues when multiple PMCs are active

[Peter Zijlstra]

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);