Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Jun 2023 11:38:23 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Eric Lin <eric.lin@sifive.com>
Cc:     Stephane Eranian <eranian@google.com>, mingo@redhat.com,
        acme@kernel.org, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, jolsa@kernel.org,
        namhyung@kernel.org, irogers@google.com, adrian.hunter@intel.com,
        palmer@dabbelt.com, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
        greentime.hu@sifive.com, vincent.chen@sifive.com
Subject: Re: [PATCH] perf/core: Add pmu stop before unthrottling to prevent
 WARNING
Message-ID: <20230627093823.GV83892@hirez.programming.kicks-ass.net>
References: <20230602094841.1225-1-eric.lin@sifive.com>
 <CAPqJEFpamQSbCcD0QJ16u7ctRf1=NYihod6gdZJfxFpE0YUVHQ@mail.gmail.com>
 <CABPqkBRyqsMnNbokBKepkWq1DtzfB0npXySGbKS1T3nQTwmaAw@mail.gmail.com>
 <20230621115811.GD2053369@hirez.programming.kicks-ass.net>
 <CAPqJEFpV8a8D7eA0sspjvThvBxdZhSLPTEbEzN7WiGCAzSnYYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPqJEFpV8a8D7eA0sspjvThvBxdZhSLPTEbEzN7WiGCAzSnYYg@mail.gmail.com>
Precedence: bulk

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?