Message-ID: <50ed4910-381a-40cf-bdc5-e008a5b28efb@linux.intel.com>
Date: Tue, 3 Jun 2025 16:22:47 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: peterz@...radead.org, mingo@...hat.com, namhyung@...nel.org,
 mark.rutland@....com, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org, eranian@...gle.com, ctshao@...gle.com,
 tmricht@...ux.ibm.com, leo.yan@....com, Aishwarya TCV <aishwarya.tcv@....com>
Subject: Re: [PATCH V2] perf: Fix the throttle error of some clock events



On 2025-06-03 12:35 p.m., Ian Rogers wrote:
> On Wed, May 28, 2025 at 10:59 AM <kan.liang@...ux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> The Arm CI reports RCU stall, which can be reproduced by the below perf
>> command.
>>   perf record -a -e cpu-clock -- sleep 2
>>
>> The cpu-clock and task_clock are two special SW events, which rely on
>> the hrtimer. Instead of invoking the stop(), the HRTIMER_NORESTART is
>> returned to stop the timer. Because the hrtimer interrupt handler cannot
>> cancel itself, which causes infinite loop.
> 
> I'm having a hard time understanding this. Currently in the code we
> have two PMUs for cpu_clock and task_clock:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/kernel/events/core.c#n10729
> ```
> static struct pmu perf_cpu_clock; /* fwd declaration */
> static struct pmu perf_task_clock;
> ```
> This is weird as both of them are programmed using type
> PERF_TYPE_SOFTWARE (1) and there is a corresponding sysfs PMU at
> `/sys/bus/event_source/devices/software/type` so wouldn't it make
> sense for there to be 1 PMU? 

No, they don't use the type PERF_TYPE_SOFTWARE.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/events/core.c#n14729

They need different operators from the regular software events. I guess
that's why they have a dedicated PMU.

> Anyway, the stop functions are similar:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/kernel/events/core.c#n11804
> ```
> static void cpu_clock_event_stop(struct perf_event *event, int flags)
> {
>         perf_swevent_cancel_hrtimer(event);
>         cpu_clock_event_update(event);
> }
> ...
> static void task_clock_event_stop(struct perf_event *event, int flags)
> {
>         perf_swevent_cancel_hrtimer(event);
>         task_clock_event_update(event, event->ctx->time);
> }
> ```
> and I think you are talking about perf_swevent_cancel_hrtimer:
> ```
> static void perf_swevent_cancel_hrtimer(struct perf_event *event)
> {
>         struct hw_perf_event *hwc = &event->hw;
> 
>         if (is_sampling_event(event)) {
>                 ktime_t remaining = hrtimer_get_remaining(&hwc->hrtimer);
>                 local64_set(&hwc->period_left, ktime_to_ns(remaining));
> 
>                 hrtimer_cancel(&hwc->hrtimer);
>         }
> }
> ```
> but I'm not seeing where HRTIMER_NORESTART comes into this.

That's an issue introduced by the generic throttle patch set, which
unconditionally invoke the event_stop() when throttle is triggered.

It's wrong to call the hrtimer_cancel() to stop the timer in the timer
handler. Because the hrtimer_cancel() waits for the handler to finish,
which is a deadlock.

The correct way (or original way) is to set a HRTIMER_NORESTART.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/events/core.c#n11717

So we need a method to specially handle the cpu_clock and task_clock.

> 
>> There may be two ways to fix it.
>> - Add a check of MAX_INTERRUPTS in the event_stop. Return immediately if
>> the stop is invoked by the throttle.
>> - Introduce a PMU flag to track the case. Avoid the event_stop in
>> perf_event_throttle() if the flag is detected.
> 
> I'm not sure what the issue is with the hrtimer_cancel from the
> explanation up to here. If we can't cancel the timer can we have the
> timer callback perf_swevent_hrtimer drop the event?

A callback is not required. Just ignore the
perf_swevent_cancel_hrtimer() when it's invoked from
perf_swevent_hrtimer(). It's similar to the first method (way) above.

> 
> I can see how the patch is working around the problem but from an API
> point-of-view having PERF_PMU_CAP_NO_THROTTLE_STOP seems to be adding
> more possible complexity to PMUs. Possibly if the software event stop
> worked better it would help other future users too.
>

The PERF_PMU_CAP_NO_THROTTLE_STOP should only be useful if there are
some future special PMUs which doesn't want to stop by the generic code.
To be honest, I have no idea if there will be such PMUs.
If no such PMUs, we should not need the flag.
> Sorry for my ignorance in this and I'm likely missing the reason this
> is the best solution. I think improving the commit message would help
> other future people and not just me.I will send a V3 to drop the flag and only apply the fix for the two
clock events. We can have a comparison then.

Thanks,
Kan>
> Thanks,
> Ian
> 
>> The latter looks more generic. It may be used if there are more other
>> cases that want to avoid the stop later. The latter is implemented.
>>
>> Reported-by: Leo Yan <leo.yan@....com>
>> Reported-by: Aishwarya TCV <aishwarya.tcv@....com>
>> Closes: https://lore.kernel.org/lkml/20250527161656.GJ2566836@e132581.arm.com/
>> Tested-by: Leo Yan <leo.yan@....com>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>>
>> Changes since V1:
>> - Rebase on top of the latest tip.git
>> - Add Tested-by from Leo
>>
>>  include/linux/perf_event.h |  1 +
>>  kernel/events/core.c       | 23 ++++++++++++++++++++---
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 52dc7cfab0e0..97a747a97a50 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -305,6 +305,7 @@ struct perf_event_pmu_context;
>>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE  0x0100
>>  #define PERF_PMU_CAP_AUX_PAUSE         0x0200
>>  #define PERF_PMU_CAP_AUX_PREFER_LARGE  0x0400
>> +#define PERF_PMU_CAP_NO_THROTTLE_STOP  0x0800
>>
>>  /**
>>   * pmu::scope
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index f34c99f8ce8f..abd19bb571e3 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -2656,7 +2656,22 @@ static void perf_event_unthrottle(struct perf_event *event, bool start)
>>
>>  static void perf_event_throttle(struct perf_event *event)
>>  {
>> -       event->pmu->stop(event, 0);
>> +       /*
>> +        * Some PMUs, e.g., cpu-clock and task_clock, may rely on
>> +        * a special mechanism (hrtimer) to manipulate counters.
>> +        * The regular stop doesn't work, since the hrtimer interrupt
>> +        * handler cannot cancel itself.
>> +        *
>> +        * The stop should be avoided for such cases. Let the
>> +        * driver-specific code handle it.
>> +        *
>> +        * The counters will eventually be disabled in the driver-specific
>> +        * code. In unthrottle, they still need to be re-enabled.
>> +        * There is no handling for PERF_PMU_CAP_NO_THROTTLE_STOP in
>> +        * the perf_event_unthrottle().
>> +        */
>> +       if (!(event->pmu->capabilities & PERF_PMU_CAP_NO_THROTTLE_STOP))
>> +               event->pmu->stop(event, 0);
>>         event->hw.interrupts = MAX_INTERRUPTS;
>>         if (event == event->group_leader)
>>                 perf_log_throttle(event, 0);
>> @@ -11848,7 +11863,8 @@ static int cpu_clock_event_init(struct perf_event *event)
>>  static struct pmu perf_cpu_clock = {
>>         .task_ctx_nr    = perf_sw_context,
>>
>> -       .capabilities   = PERF_PMU_CAP_NO_NMI,
>> +       .capabilities   = PERF_PMU_CAP_NO_NMI |
>> +                         PERF_PMU_CAP_NO_THROTTLE_STOP,
>>         .dev            = PMU_NULL_DEV,
>>
>>         .event_init     = cpu_clock_event_init,
>> @@ -11930,7 +11946,8 @@ static int task_clock_event_init(struct perf_event *event)
>>  static struct pmu perf_task_clock = {
>>         .task_ctx_nr    = perf_sw_context,
>>
>> -       .capabilities   = PERF_PMU_CAP_NO_NMI,
>> +       .capabilities   = PERF_PMU_CAP_NO_NMI |
>> +                         PERF_PMU_CAP_NO_THROTTLE_STOP,
>>         .dev            = PMU_NULL_DEV,
>>
>>         .event_init     = task_clock_event_init,
>> --
>> 2.38.1
>>
> 

Powered by blists - more mailing lists