| 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: <4a44b8cb-7c73-4926-8b9f-1f63929ea48f@linux.intel.com>
Date: Thu, 15 May 2025 08:55:05 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Leo Yan <leo.yan@....com>
Cc: peterz@...radead.org, mingo@...hat.com, namhyung@...nel.org,
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
Subject: Re: [PATCH V2 01/15] perf: Fix the throttle logic for a group
On 2025-05-15 5:43 a.m., Leo Yan wrote:
> On Wed, May 14, 2025 at 08:13:47AM -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>
>> ---
>>
>> Changes since V1:
>> - Apply the suggested throttle/unthrottle functions from Peter.
>> The MAX_INTERRUPTS and throttle logs are applied to all events.
>> - Update the description and comments accordingly
>>
>> kernel/events/core.c | 58 +++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 44 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index a84abc2b7f20..a270fcda766d 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -2734,6 +2734,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 start)
>> +{
>> + struct perf_event *sibling, *leader = event->group_leader;
>> +
>> + perf_event_unthrottle(leader, leader != event || start);
>> + for_each_sibling_event(sibling, leader)
>> + perf_event_unthrottle(sibling, sibling != event || start);
>
> Seems to me that the condition "leader != event || start" is bit tricky
> (similarly for the check "sibling != event || start").
>
> If a session sets the frequency (with option -F in perf tool), the
> following flow is triggered:
>
> perf_adjust_freq_unthr_events()
> `> perf_event_unthrottle_group(event, false);
>
> The argument "start" is false, so all sibling events will be enabled,
> but the event pointed by the "event" argument remains disabled.
Right. Because the following code will adjust the period of the event
and start it.
The PMU is disabled at the moment. There is no difference in starting
the leader first or the member first.
> Though
> the __perf_event_period() function will enables all events with adjusted
> period, but it is still risky for counting discrepancy caused by the
> flow described above.
The __perf_event_period() is similar. The event in both cases has to
adjust the period before re-start the event, which has to be done
outside of the perf_event_unthrottle_group().
Thanks,
Kan>
> Thanks,
> Leo
>
>> +}
>> +
>> +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)
>> {
>> @@ -4389,10 +4422,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 (!event->attr.freq || !event->attr.sample_freq)
>> - event->pmu->start(event, 0);
>> + perf_event_unthrottle_group(event,
>> + !event->attr.freq || !event->attr.sample_freq);
>> }
>>
>> if (!event->attr.freq || !event->attr.sample_freq)
>> @@ -6421,14 +6452,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);
>> }
>>
>> @@ -6436,6 +6459,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, false);
>> perf_pmu_enable(event->pmu);
>> }
>> }
>> @@ -10326,8 +10357,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