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

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-05-16 8:51 a.m., Leo Yan wrote:
> 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);
>         ...
> }
> 

It should work for the perf_adjust_freq_unthr_events(), which only start
the leader. But it's possible that the __perf_event_period() update a
sibling, not leader.

I think I can check the name to bool event_has_start.
Is the name OK?

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a270fcda766d..b1cb07fa9c18 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2749,13 +2749,13 @@ static void perf_event_throttle(struct
perf_event *event)
 	perf_log_throttle(event, 0);
 }

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

-	perf_event_unthrottle(leader, leader != event || start);
+	perf_event_unthrottle(leader, event_has_start ? leader != event : true);
 	for_each_sibling_event(sibling, leader)
-		perf_event_unthrottle(sibling, sibling != event || start);
+		perf_event_unthrottle(sibling, event_has_start ? sibling != event :
true);
 }

 static void perf_event_throttle_group(struct perf_event *event)
@@ -4423,7 +4423,7 @@ static void perf_adjust_freq_unthr_events(struct
list_head *event_list)

 		if (hwc->interrupts == MAX_INTERRUPTS) {
 			perf_event_unthrottle_group(event,
-				!event->attr.freq || !event->attr.sample_freq);
+				(event->attr.freq && event->attr.sample_freq));
 		}

 		if (!event->attr.freq || !event->attr.sample_freq)
@@ -6466,7 +6466,7 @@ static void __perf_event_period(struct perf_event
*event,
 		 * while we already re-started the event/group.
 		 */
 		if (event->hw.interrupts == MAX_INTERRUPTS)
-			perf_event_unthrottle_group(event, false);
+			perf_event_unthrottle_group(event, true);
 		perf_pmu_enable(event->pmu);
 	}
 }

Thanks,
Kan