linux-kernel - Re: [PATCH v7 1/4] cpufreq: Introduce an optional cpuinfo_avg

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] [day] [month] [year] [list]

Message-ID: <09887c82-2813-59c3-2ff2-0b7223b37b9e@hisilicon.com>
Date: Wed, 25 Sep 2024 16:58:36 +0800
From: Jie Zhan <zhanjie9@...ilicon.com>
To: Beata Michalska <beata.michalska@....com>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-pm@...r.kernel.org>,
	<ionela.voinescu@....com>, <sudeep.holla@....com>, <will@...nel.org>,
	<catalin.marinas@....com>, <rafael@...nel.org>, <viresh.kumar@...aro.org>
CC: <sumitg@...dia.com>, <yang@...amperecomputing.com>,
	<vanshikonda@...amperecomputing.com>, <lihuisong@...wei.com>
Subject: Re: [PATCH v7 1/4] cpufreq: Introduce an optional cpuinfo_avg_freq
 sysfs entry

Hi Beata,

Great thanks for the update.

On 13/09/2024 21:29, Beata Michalska wrote:
> Currently the CPUFreq core exposes two sysfs attributes that can be used
> to query current frequency of a given CPU(s): namely cpuinfo_cur_freq
> and scaling_cur_freq. Both provide slightly different view on the
> subject and they do come with their own drawbacks.
> 
> cpuinfo_cur_freq provides higher precision though at a cost of being
> rather expensive. Moreover, the information retrieved via this attribute
> is somewhat short lived as frequency can change at any point of time
> making it difficult to reason from.
> 
> scaling_cur_freq, on the other hand, tends to be less accurate but then
> the actual level of precision (and source of information) varies between
> architectures making it a bit ambiguous.
> 
> The new attribute, cpuinfo_avg_freq, is intended to provide more stable,
> distinct interface, exposing an average frequency of a given CPU(s), as
> reported by the hardware, over a time frame spanning no more than a few
> milliseconds. As it requires appropriate hardware support, this
> interface is optional.
> 
> Signed-off-by: Beata Michalska <beata.michalska@....com>
> ---
>  Documentation/admin-guide/pm/cpufreq.rst | 10 ++++++++
>  drivers/cpufreq/cpufreq.c                | 31 ++++++++++++++++++++++++
>  include/linux/cpufreq.h                  |  1 +
>  3 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
> index fe1be4ad88cb..2204d6132c05 100644
> --- a/Documentation/admin-guide/pm/cpufreq.rst
> +++ b/Documentation/admin-guide/pm/cpufreq.rst
> @@ -248,6 +248,16 @@ are the following:
>  	If that frequency cannot be determined, this attribute should not
>  	be present.
>  
> +``cpuinfo_avg_freq``
> +        An average frequency (in KHz) of all CPUs belonging to a given policy,
> +        derived from a hardware provided feedback and reported on a time frame
> +        spanning at most few milliseconds.

I don't think it's necessary to put the 'at most few milliseconds'
limitation on.

It's supposed to be fine for other platforms to implement the interface
with a longer time period, e.g. a few seconds, in the future.  Otherwise,
this would probably force the implementation of 'cpuinfo_avg_freq' to be
binded with the 'scale freq tick' stuff.

> +
> +        This is expected to be based on the frequency the hardware actually runs
> +        at and, as such, might require specialised hardware support (such as AMU
> +        extension on ARM). If one cannot be determined, this attribute should
> +        not be present.
> +
>  ``cpuinfo_max_freq``
>  	Maximum possible operating frequency the CPUs belonging to this policy
>  	can run at (in kHz).

...

> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d4d2f4d1d7cb..48262073707e 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -1195,6 +1195,7 @@ static inline int of_perf_domain_get_sharing_cpumask(int pcpu, const char *list_
>  #endif
>  
>  extern unsigned int arch_freq_get_on_cpu(int cpu);
> +extern int arch_freq_avg_get_on_cpu(int cpu);

It's werid to have two different functions with mostly the same behaviour,
i.e. arch_freq_get_on_cpu() and arch_freq_avg_get_on_cpu().

Appreciated that there would be some capatibility work with x86 at the
moment if merging them, e.g. return type, default implementation, impact on
some userspace tools, etc.

Anyhow, are they supposed to be merged in the near future?


Thanks,
Jie
>  
>  #ifndef arch_set_freq_scale
>  static __always_inline