linux-kernel - Re: [PATCH v6 0/4] Add support for AArch64 AMUv1-based arch_freq_get_on

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: <8500d58c-e6c5-04c7-73a0-38d3f77f2cb7@hisilicon.com>
Date: Wed, 14 Aug 2024 16:05:24 +0800
From: Jie Zhan <zhanjie9@...ilicon.com>
To: Beata Michalska <beata.michalska@....com>, Catalin Marinas
	<catalin.marinas@....com>
CC: <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<ionela.voinescu@....com>, <sudeep.holla@....com>, <will@...nel.org>,
	<vincent.guittot@...aro.org>, <vanshikonda@...amperecomputing.com>,
	<sumitg@...dia.com>, <yang@...amperecomputing.com>, <lihuisong@...wei.com>,
	<viresh.kumar@...aro.org>, <rafael@...nel.org>
Subject: Re: [PATCH v6 0/4] Add support for AArch64 AMUv1-based
 arch_freq_get_on_cpu


On 11/07/2024 21:59, Beata Michalska wrote:
> Hi Catalin,
>
> On Mon, Jul 08, 2024 at 06:10:02PM +0100, Catalin Marinas wrote:
>> Hi Beata,
>>
>> On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote:
>>> Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on
>>> existing implementation for FIE and AMUv1 support: the frequency scale
>>> factor, updated on each sched tick, serves as a base for retrieving
>>> the frequency for a given CPU, representing an average frequency
>>> reported between the ticks - thus its accuracy is limited.
>>>
>>> The changes have been rather lightly (due to some limitations) tested on
>>> an FVP model. Note that some small discrepancies have been observed while
>>> testing (on the model) and this is currently being investigated, though it
>>> should not have any significant impact on the overall results.
>> What's the plan with this series? Are you still investigating those
>> discrepancies or is it good to go?
>>
> Overall it should be good to go with small caveat:
> as per discussion [1] we might need to provide new sysfs attribute exposing an
> average frequency instead of plugging new code under existing cpuinfo_cur_freq.
> This is to avoid messing up with other archs and make a clean distinction on
> which attribute provides what information.
> As such, the arch_freq_get_on_cpu implementation provided within this series
> [PATCH v6 3/4] will most probably be shifted to a new function.
>
> Hopefully will be able to send those changes soon.
>
> ---
> [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/
> ---
> BR
> Beata
>
>> -- 
>> Catalin
>>
Hi Beata,

I've recently tested this patchset on a Kunpeng system.
It works as expected when reading scaling_cur_freq.
The frequency samples are much stabler than what cppc_cpufreq returns.

A few minor things.

1. I observed larger errors on idle cpus than busy cpus, though it's 
just up to 1%.
Not sure if this comes from the uncertain time interval between the last 
tick and entering idle.
The shorter averaging interval, the larger error, I supposed.

2. In the current implementation, the resolution of frequency would be:
max_freq_khz / SCHED_CAPACITY_SCALE
This looks a bit unnecessary to me.

It's supposed to get a better resolution if we can do this in 
arch_freq_get_on_cpu():

freq = delta_cycle_cnt * max_freq_khz / delta_const_cnt

which may require caching both current and previous sets of counts in 
the per-cpu struct amu_cntr_sample.

Kind regards,
Jie