linux-kernel - Re: [PATCH V2 01/15] perf: Fix the throttle logic for a group

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250516125146.GE412060@e132581.arm.com>
Date: Fri, 16 May 2025 13:51:46 +0100
From: Leo Yan <leo.yan@....com>
To: "Liang, Kan" <kan.liang@...ux.intel.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 Thu, May 15, 2025 at 08:55:05AM -0400, Liang, Kan wrote:

[...]

> >> +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.

Thanks for explaination. In the case above, as you said, all events will
be enabled either in perf_event_unthrottle_group() or in
perf_adjust_freq_unthr_events() with a recalculated period.

Just a minor suggestion. Seems to me, the parameter "start" actually
means "only_enable_sibling". For more readable, the function can be
refine as:

static void perf_event_unthrottle_group(struct perf_event *event,
                                        bool only_enable_sibling)
{
	struct perf_event *sibling, *leader = event->group_leader;

	perf_event_unthrottle(leader,
                only_enable_sibling ? leader != event : true);
        ...
}

Thanks,
Leo