Re: [PATCH] perf/core: Add pmu stop before unthrottling to prevent WARNING

[Peter Zijlstra <peterz@infradead.org>]

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?