| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aCz737AGOGzKDVhI@google.com>
Date: Tue, 20 May 2025 15:02:07 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: kan.liang@...ux.intel.com
Cc: peterz@...radead.org, mingo@...hat.com, irogers@...gle.com,
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
Subject: Re: [PATCH V4 01/16] perf: Fix the throttle logic for a group
On Tue, May 20, 2025 at 11:16:29AM -0700, kan.liang@...ux.intel.com wrote:
> From: Kan Liang <kan.liang@...ux.intel.com>
>
> The current throttle logic doesn't work well with a group, e.g., the
> following sampling-read case.
>
> $ perf record -e "{cycles,cycles}:S" ...
>
> $ perf report -D | grep THROTTLE | tail -2
> THROTTLE events: 426 ( 9.0%)
> UNTHROTTLE events: 425 ( 9.0%)
>
> $ perf report -D | grep PERF_RECORD_SAMPLE -a4 | tail -n 5
> 0 1020120874009167 0x74970 [0x68]: PERF_RECORD_SAMPLE(IP, 0x1):
> ... sample_read:
> .... group nr 2
> ..... id 0000000000000327, value 000000000cbb993a, lost 0
> ..... id 0000000000000328, value 00000002211c26df, lost 0
>
> The second cycles event has a much larger value than the first cycles
> event in the same group.
>
> The current throttle logic in the generic code only logs the THROTTLE
> event. It relies on the specific driver implementation to disable
> events. For all ARCHs, the implementation is similar. Only the event is
> disabled, rather than the group.
>
> The logic to disable the group should be generic for all ARCHs. Add the
> logic in the generic code. The following patch will remove the buggy
> driver-specific implementation.
>
> The throttle only happens when an event is overflowed. Stop the entire
> group when any event in the group triggers the throttle.
> The MAX_INTERRUPTS is set to all throttle events.
>
> The unthrottled could happen in 3 places.
> - event/group sched. All events in the group are scheduled one by one.
> All of them will be unthrottled eventually. Nothing needs to be
> changed.
> - The perf_adjust_freq_unthr_events for each tick. Needs to restart the
> group altogether.
> - The __perf_event_period(). The whole group needs to be restarted
> altogether as well.
>
> With the fix,
> $ sudo perf report -D | grep PERF_RECORD_SAMPLE -a4 | tail -n 5
> 0 3573470770332 0x12f5f8 [0x70]: PERF_RECORD_SAMPLE(IP, 0x2):
> ... sample_read:
> .... group nr 2
> ..... id 0000000000000a28, value 00000004fd3dfd8f, lost 0
> ..... id 0000000000000a29, value 00000004fd3dfd8f, lost 0
>
> Suggested-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
Acked-by: Namhyung Kim <namhyung@...nel.org>
Thanks,
Namhyung
> ---
> kernel/events/core.c | 66 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index af78ec118e8f..915698f47682 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2739,6 +2739,39 @@ void perf_event_disable_inatomic(struct perf_event *event)
> static void perf_log_throttle(struct perf_event *event, int enable);
> static void perf_log_itrace_start(struct perf_event *event);
>
> +static void perf_event_unthrottle(struct perf_event *event, bool start)
> +{
> + event->hw.interrupts = 0;
> + if (start)
> + event->pmu->start(event, 0);
> + perf_log_throttle(event, 1);
> +}
> +
> +static void perf_event_throttle(struct perf_event *event)
> +{
> + event->pmu->stop(event, 0);
> + event->hw.interrupts = MAX_INTERRUPTS;
> + perf_log_throttle(event, 0);
> +}
> +
> +static void perf_event_unthrottle_group(struct perf_event *event, bool skip_start_event)
> +{
> + struct perf_event *sibling, *leader = event->group_leader;
> +
> + perf_event_unthrottle(leader, skip_start_event ? leader != event : true);
> + for_each_sibling_event(sibling, leader)
> + perf_event_unthrottle(sibling, skip_start_event ? sibling != event : true);
> +}
> +
> +static void perf_event_throttle_group(struct perf_event *event)
> +{
> + struct perf_event *sibling, *leader = event->group_leader;
> +
> + perf_event_throttle(leader);
> + for_each_sibling_event(sibling, leader)
> + perf_event_throttle(sibling);
> +}
> +
> static int
> event_sched_in(struct perf_event *event, struct perf_event_context *ctx)
> {
> @@ -2767,10 +2800,8 @@ event_sched_in(struct perf_event *event, struct perf_event_context *ctx)
> * ticks already, also for a heavily scheduling task there is little
> * guarantee it'll get a tick in a timely manner.
> */
> - if (unlikely(event->hw.interrupts == MAX_INTERRUPTS)) {
> - perf_log_throttle(event, 1);
> - event->hw.interrupts = 0;
> - }
> + if (unlikely(event->hw.interrupts == MAX_INTERRUPTS))
> + perf_event_unthrottle(event, false);
>
> perf_pmu_disable(event->pmu);
>
> @@ -4393,12 +4424,8 @@ static void perf_adjust_freq_unthr_events(struct list_head *event_list)
>
> hwc = &event->hw;
>
> - if (hwc->interrupts == MAX_INTERRUPTS) {
> - hwc->interrupts = 0;
> - perf_log_throttle(event, 1);
> - if (!is_event_in_freq_mode(event))
> - event->pmu->start(event, 0);
> - }
> + if (hwc->interrupts == MAX_INTERRUPTS)
> + perf_event_unthrottle_group(event, is_event_in_freq_mode(event));
>
> if (!is_event_in_freq_mode(event))
> continue;
> @@ -6426,14 +6453,6 @@ static void __perf_event_period(struct perf_event *event,
> active = (event->state == PERF_EVENT_STATE_ACTIVE);
> if (active) {
> perf_pmu_disable(event->pmu);
> - /*
> - * We could be throttled; unthrottle now to avoid the tick
> - * trying to unthrottle while we already re-started the event.
> - */
> - if (event->hw.interrupts == MAX_INTERRUPTS) {
> - event->hw.interrupts = 0;
> - perf_log_throttle(event, 1);
> - }
> event->pmu->stop(event, PERF_EF_UPDATE);
> }
>
> @@ -6441,6 +6460,14 @@ static void __perf_event_period(struct perf_event *event,
>
> if (active) {
> event->pmu->start(event, PERF_EF_RELOAD);
> + /*
> + * Once the period is force-reset, the event starts immediately.
> + * But the event/group could be throttled. Unthrottle the
> + * event/group now to avoid the next tick trying to unthrottle
> + * while we already re-started the event/group.
> + */
> + if (event->hw.interrupts == MAX_INTERRUPTS)
> + perf_event_unthrottle_group(event, true);
> perf_pmu_enable(event->pmu);
> }
> }
> @@ -10331,8 +10358,7 @@ __perf_event_account_interrupt(struct perf_event *event, int throttle)
> if (unlikely(throttle && hwc->interrupts >= max_samples_per_tick)) {
> __this_cpu_inc(perf_throttled_count);
> tick_dep_set_cpu(smp_processor_id(), TICK_DEP_BIT_PERF_EVENTS);
> - hwc->interrupts = MAX_INTERRUPTS;
> - perf_log_throttle(event, 0);
> + perf_event_throttle_group(event);
> ret = 1;
> }
>
> --
> 2.38.1
>
Powered by blists - more mailing lists