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

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

On 2025-05-16 10:17 a.m., Leo Yan wrote:
> On Fri, May 16, 2025 at 09:28:07AM -0400, Liang, Kan wrote:
> 
> [...]
> 
>>> 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.
> 
> Should not perf_event_unthrottle_group() always enable sibling events?
>

No. __perf_event_period() can reset the period of a sibling event. I
know it sounds weird, but it's doable.


> The only difference is how the leader event to be enabled.  It can be
> enabled in perf_event_unthrottle_group() in period mode, or in
> frequency mode due to a new period value is generated, the leader
> event is enabled in perf_adjust_freq_unthr_events() or in
> __perf_event_period().
> 
> This is why I suggested to rename the flag to only_enable_sibling:
> 
>   true: only enable sibling events
>   false: enable all events (leader event and sibling events)
> 
> Or, we can rename the flag as "skip_start_event", means to skip
> enabling the event specified in the argument.

The name "skip_start_event" sounds good to me. I will use it in V3.

Thanks,
Kan>
>> I think I can check the name to bool event_has_start.
>> Is the name OK?
> 
> I am still confused for the naming "event_has_start" :)
> 
> What exactly does it mean?
> 
>> 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);
> 
> The logic in the updated code is correct for me.
> 
> Thanks,
> Leo
>