linux-kernel - Re: [PATCH V4 01/16] 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: <ef09af0b-f6d4-4d41-b658-93c9b72485ce@linux.intel.com>
Date: Wed, 28 May 2025 10:51:10 -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, Aishwarya.TCV@....com
Subject: Re: [PATCH V4 01/16] perf: Fix the throttle logic for a group



On 2025-05-28 6:28 a.m., Leo Yan wrote:
> On Tue, May 27, 2025 at 03:30:06PM -0400, Liang, Kan wrote:
> 
> [...]
> 
>> There may be two ways to fix it.
>> - Add a check of MAX_INTERRUPTS in the event_stop. Return immediately if
>> the stop is invoked by the throttle.
>> - Introduce a PMU flag to track the case. Avoid the event_stop in
>> perf_event_throttle() if the flag is detected.
>>
>> The latter looks more generic. It may be used if there are other cases
>> that want to avoid the stop. So the latter is implemented as below.
> 
> Yes.  I agreed the fix below is more general and confirmed it can fix
> the observed issue.
> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 947ad12dfdbe..66f02f46595c 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -303,6 +303,7 @@ struct perf_event_pmu_context;
>>  #define PERF_PMU_CAP_AUX_OUTPUT			0x0080
>>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0100
>>  #define PERF_PMU_CAP_AUX_PAUSE			0x0200
>> +#define PERF_PMU_CAP_NO_THROTTLE_STOP		0x0400
>>
>>  /**
>>   * pmu::scope
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 8327ab0ee641..596597886d96 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -2655,7 +2655,8 @@ static void perf_event_unthrottle(struct
>> perf_event *event, bool start)
>>
>>  static void perf_event_throttle(struct perf_event *event)
>>  {
>> -	event->pmu->stop(event, 0);
>> +	if (!(event->pmu->capabilities & PERF_PMU_CAP_NO_THROTTLE_STOP))
>> +		event->pmu->stop(event, 0);
> 
> A background info is that even a PMU event is not stopped when
> throttling, we still need to re-enable it.  This is why we don't do
> particualy handling for PERF_PMU_CAP_NO_THROTTLE_STOP in
> perf_event_unthrottle().
> 
> Maybe it is deserved add a comment for easier understanding.

Sure. A formal patch has been sent. Please take a look.
https://lore.kernel.org/lkml/20250528144823.2996185-1-kan.liang@linux.intel.com/

Thanks,
Kan>
> Thanks,
> Leo
> 
>>  	event->hw.interrupts = MAX_INTERRUPTS;
>>  	perf_log_throttle(event, 0);
>>  }
>> @@ -11846,7 +11847,8 @@ static int cpu_clock_event_init(struct
>> perf_event *event)
>>  static struct pmu perf_cpu_clock = {
>>  	.task_ctx_nr	= perf_sw_context,
>>
>> -	.capabilities	= PERF_PMU_CAP_NO_NMI,
>> +	.capabilities	= PERF_PMU_CAP_NO_NMI |
>> +			  PERF_PMU_CAP_NO_THROTTLE_STOP,
>>  	.dev		= PMU_NULL_DEV,
>>
>>  	.event_init	= cpu_clock_event_init,
>> @@ -11928,7 +11930,8 @@ static int task_clock_event_init(struct
>> perf_event *event)
>>  static struct pmu perf_task_clock = {
>>  	.task_ctx_nr	= perf_sw_context,
>>
>> -	.capabilities	= PERF_PMU_CAP_NO_NMI,
>> +	.capabilities	= PERF_PMU_CAP_NO_NMI |
>> +			  PERF_PMU_CAP_NO_THROTTLE_STOP,
>>  	.dev		= PMU_NULL_DEV,
>>
>>  	.event_init	= task_clock_event_init,
>>
>>
>> Thanks,
>> Kan
>>
>>
>