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