| 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: <aCzf1epMZn20K55q@google.com>
Date: Tue, 20 May 2025 13:02:29 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: "Liang, Kan" <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 V3 02/16] perf: Fix the throttle logic for a group
On Tue, May 20, 2025 at 10:47:21AM -0400, Liang, Kan wrote:
>
>
> On 2025-05-18 3:18 p.m., Namhyung Kim wrote:
> > Hi Kan,
> >
> > On Fri, May 16, 2025 at 11:28:39AM -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
> >
> > Thanks for working on this!
> >
> >>
> >> Suggested-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> >> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> >> ---
> >> kernel/events/core.c | 60 ++++++++++++++++++++++++++++++++------------
> >> 1 file changed, 44 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/kernel/events/core.c b/kernel/events/core.c
> >> index af78ec118e8f..52490c2ce45b 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);
> >
> > This will add more PERF_RECORD_THROTTLE records for sibling events.
>
> Yes
>
> > Maybe we can generate it for the actual target event only?
>
> The current code cannot track the actual target event for unthrottle.
> Because the MAX_INTERRUPTS are set for all events when event_throttle.
Right.
>
> But I think we can only add a PERF_RECORD_THROTTLE record for the leader
> event, which can reduce the number of THROTTLE records.
Sounds good.
>
> The sample right after the THROTTLE record must be generated by the
> actual target event. I think it should be good enough for the perf tool
> to locate the event.
IIRC perf tool doesn't track which event is throttled, but yeah, it'd be
possible to use the next sample to locate it.
>
> I will add the below patch as a separate improvement in V4.
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 52490c2ce45b..cd559501cfbd 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2744,14 +2744,16 @@ 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);
> + if (event == event->group_leader)
> + 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);
> + if (event == event->group_leader)
> + perf_log_throttle(event, 0);
> }
Looks good.
>
>
> >
> > Also the condition for skip_start_event is if it's a freq event.
> > I think we can skip pmu->start() if the sibling is also a freq event.
>
> The skip_start_event is if it will be start later separately. It intends
> to avoid the double start.
>
> In the perf_adjust_freq_unthr_events(), it will only adjust and start
> the leader event, not group. If we skip pmu->start() for a freq sibling
> event, it will not start until the next context switch.
Oh, I missed that it only has leaders in the active list.
Thanks,
Namhyung
Powered by blists - more mailing lists