linux-kernel - Re: [PATCH V2 06/15] s390/perf: Remove driver-specific throttle support

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: <841b89af-9836-476f-984c-ebe54ccdc0ae@linux.intel.com>
Date: Thu, 15 May 2025 09:56:11 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Thomas Richter <tmricht@...ux.ibm.com>, 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
Cc: eranian@...gle.com, ctshao@...gle.com, linux-s390@...r.kernel.org
Subject: Re: [PATCH V2 06/15] s390/perf: Remove driver-specific throttle
 support



On 2025-05-15 9:15 a.m., Thomas Richter wrote:
> On 5/14/25 17:13, kan.liang@...ux.intel.com wrote:
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> The throttle support has been added in the generic code. Remove
>> the driver-specific throttle support.
>>
>> Besides the throttle, perf_event_overflow may return true because of
>> event_limit. It already does an inatomic event disable. The pmu->stop
>> is not required either.
>>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> Cc: Thomas Richter <tmricht@...ux.ibm.com>
>> Cc: linux-s390@...r.kernel.org
>> ---
>>  arch/s390/kernel/perf_cpum_cf.c | 2 --
>>  arch/s390/kernel/perf_cpum_sf.c | 5 +----
>>  2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum_cf.c
>> index e657fad7e376..6a262e198e35 100644
>> --- a/arch/s390/kernel/perf_cpum_cf.c
>> +++ b/arch/s390/kernel/perf_cpum_cf.c
>> @@ -980,8 +980,6 @@ static int cfdiag_push_sample(struct perf_event *event,
>>  	}
>>  
>>  	overflow = perf_event_overflow(event, &data, &regs);
>> -	if (overflow)
>> -		event->pmu->stop(event, 0);
>>  
>>  	perf_event_update_userpage(event);
>>  	return overflow;
>> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
>> index ad22799d8a7d..91469401f2c9 100644
>> --- a/arch/s390/kernel/perf_cpum_sf.c
>> +++ b/arch/s390/kernel/perf_cpum_sf.c
>> @@ -1072,10 +1072,7 @@ static int perf_push_sample(struct perf_event *event,
>>  	overflow = 0;
>>  	if (perf_event_exclude(event, &regs, sde_regs))
>>  		goto out;
>> -	if (perf_event_overflow(event, &data, &regs)) {
>> -		overflow = 1;
>> -		event->pmu->stop(event, 0);
>> -	}
>> +	overflow = perf_event_overflow(event, &data, &regs);
>>  	perf_event_update_userpage(event);
>>  out:
>>  	return overflow;
> 
> I have installed patch 1 and 6 on top of the linux-next kernel today.
> The results look good, much better than before, but I still do not
> get both counter values in sync on each iteration all the time.
>

For Intel platforms, there is a global control register which can
disable/enable all counters simultaneously. It's invoked in the
pmu_enable/disable pair. It guarantees that the group start/stop/read in
sync.
If there is no such synchronize mechanism in the hardware, the events in
a group usually start/stop one by one. There may be a small gap between
each event.

I'm not familiar with the s390. I guess it may be the cause that you
didn't get both counter values in sync.

Without hardware's help, the patch set cannot completely fix the gap
between counters, but should be able to minimize it.

> Tested-by: Thomas Richter <tmricht@...ux.ibm.com>
Thanks!Kan