Messages in this thread | | | Date | Tue, 27 Jun 2023 11:38:23 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] perf/core: Add pmu stop before unthrottling to prevent WARNING |
| |
On Tue, Jun 27, 2023 at 05:08:07PM +0800, Eric Lin wrote:
> > Yeah, Changelog fails to explain how we got to the faulty state -- and > > without that we can't judge if the proposed solution actually fixes the > > problem or not. > > > > Hi Stephane, Peter, > > Most of the pmu driver will call *_pmu_stop(event,0) in the > *_pmu_handle_irq() function and update the hwc->state with > PERF_HES_STOPPED flag as below: > > arch/alpha/kernel/perf_event.c:856: if > (perf_event_overflow(event, &data, regs)) { > arch/alpha/kernel/perf_event.c-857- /* Interrupts > coming too quickly; "throttle" the > arch/alpha/kernel/perf_event.c-858- * counter, > i.e., disable it for a little while. > arch/alpha/kernel/perf_event.c-859- */ > arch/alpha/kernel/perf_event.c-860- > alpha_pmu_stop(event, 0); > arch/alpha/kernel/perf_event.c-861- } > ----- > arch/arc/kernel/perf_event.c:603: if > (perf_event_overflow(event, &data, regs)) > arch/arc/kernel/perf_event.c-604- > arc_pmu_stop(event, 0); > arch/arc/kernel/perf_event.c-605- } > ----- > arch/x86/events/amd/core.c:935: if (perf_event_overflow(event, > &data, regs)) > arch/x86/events/amd/core.c-936- x86_pmu_stop(event, 0); > arch/x86/events/amd/core.c-937- } > ----- > > However, some of the pmu drivers stop the event in the > *_pmu_handle_irq() without updating the hwc->state with > PERF_HES_STOPPED flag as below: > > arch/arm/kernel/perf_event_v7.c:994: if > (perf_event_overflow(event, &data, regs)) > arch/arm/kernel/perf_event_v7.c-995- > cpu_pmu->disable(event); // <== not update with PERF_HES_STOPPED > arch/arm/kernel/perf_event_v7.c-996- } > ------ > arch/csky/kernel/perf_event.c:1142: if > (perf_event_overflow(event, &data, regs)) > arch/csky/kernel/perf_event.c-1143- > csky_pmu_stop_event(event); // <== not update with PERF_HES_STOPPED > arch/csky/kernel/perf_event.c-1144- } > ------- > arch/loongarch/kernel/perf_event.c:492: if (perf_event_overflow(event, > data, regs)) > arch/loongarch/kernel/perf_event.c-493- > loongarch_pmu_disable_event(idx); // <== not update with > PERF_HES_STOPPED > arch/loongarch/kernel/perf_event.c-494-} > ------- > arch/mips/kernel/perf_event_mipsxx.c:794: if > (perf_event_overflow(event, data, regs)) > arch/mips/kernel/perf_event_mipsxx.c-795- > mipsxx_pmu_disable_event(idx); // <== not update with PERF_HES_STOPPED > arch/mips/kernel/perf_event_mipsxx.c-796-} > .... > > Furthermore, these drivers did not add event->hw.state checking in > *_pmu_start() before starting the event like x86 does: > > 1497 static void x86_pmu_start(struct perf_event *event, int flags) > 1498 { > 1499 struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > 1500 int idx = event->hw.idx; > 1501 > 1502 if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED))) > 1503 return; > 1504 > > As a result, these drivers won't trigger the WARN_ON_ONCE warning as > shown in this patch. > > However, if a pmu driver like RISC-V pmu which didn't call > *_pmu_stop(event,0) without updating the hwc->state with > PERF_HES_STOPPED flag in the *_pmu_handle_irq() function > but has event->hw.state checking in *_pmu_start(), it could trigger > the WARN_ON_ONCE warning as shown in this patch. > > Therefore, I think we need to call pmu->stop() before unthrottling the > event to prevent this warning.
How is that not a pmu driver problem ? I'd think we should be fixing those drivers. Mark, do you have have any memories of how the ARM driver came to be this way?
| |