Message-ID: <75454544-f23e-4de9-bc12-64d134fc5027@nvidia.com>
Date: Wed, 29 Jan 2025 16:59:33 +0530
From: Sumit Gupta <sumitg@...dia.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: <yang@...amperecomputing.com>, <vanshikonda@...amperecomputing.com>,
	<lihuisong@...wei.com>, <zhanjie9@...ilicon.com>, Jonathan Corbet
	<corbet@....net>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
	<mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, "H . Peter Anvin" <hpa@...or.com>, Phil Auld
	<pauld@...hat.com>, <x86@...nel.org>, <linux-doc@...r.kernel.org>,
	linux-tegra <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v9 2/5] cpufreq: Introduce an optional cpuinfo_avg_freq
 sysfs entry



On 21/01/25 14:14, 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.
> 
> Note that under the hood, the new attribute relies on the information
> provided by arch_freq_get_on_cpu, which, up to this point, has been
> feeding data for scaling_cur_freq attribute, being the source of
> ambiguity when it comes to interpretation. This has been amended by
> restoring the intended behavior for scaling_cur_freq, with a new
> dedicated config option to maintain status quo for those, who may need
> it.
> 
> CC: Jonathan Corbet <corbet@....net>
> CC: Thomas Gleixner <tglx@...utronix.de>
> CC: Ingo Molnar <mingo@...hat.com>
> CC: Borislav Petkov <bp@...en8.de>
> CC: Dave Hansen <dave.hansen@...ux.intel.com>
> CC: H. Peter Anvin <hpa@...or.com>
> CC: Phil Auld <pauld@...hat.com>
> CC: x86@...nel.org
> CC: linux-doc@...r.kernel.org
> 
> Signed-off-by: Beata Michalska <beata.michalska@....com>
> ---
>   Documentation/admin-guide/pm/cpufreq.rst | 16 ++++++++++++-
>   drivers/cpufreq/Kconfig.x86              | 12 ++++++++++
>   drivers/cpufreq/cpufreq.c                | 30 +++++++++++++++++++++++-
>   3 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
> index a21369eba034..e9969174026c 100644
> --- a/Documentation/admin-guide/pm/cpufreq.rst
> +++ b/Documentation/admin-guide/pm/cpufreq.rst
> @@ -248,6 +248,19 @@ 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.
> +
> +        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.
> +
> +        Note, that failed attempt to retrieve current frequency for a given
> +        CPU(s) will result in an appropriate error.
> +

Minor nit:
Should we also add: Idle CPU's on ARM will return EAGAIN (Resource 
temporarily unavailable) error?

>   ``cpuinfo_max_freq``
>          Maximum possible operating frequency the CPUs belonging to this policy
>          can run at (in kHz).
> @@ -293,7 +306,8 @@ are the following:
>          Some architectures (e.g. ``x86``) may attempt to provide information
>          more precisely reflecting the current CPU frequency through this
>          attribute, but that still may not be the exact current CPU frequency as
> -       seen by the hardware at the moment.
> +       seen by the hardware at the moment. This behavior though, is only
> +       available via c:macro:``CPUFREQ_ARCH_CUR_FREQ`` option.
> 
>   ``scaling_driver``
>          The scaling driver currently in use.
> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> index 97c2d4f15d76..212e1b9afe21 100644
> --- a/drivers/cpufreq/Kconfig.x86
> +++ b/drivers/cpufreq/Kconfig.x86
> @@ -340,3 +340,15 @@ config X86_SPEEDSTEP_RELAXED_CAP_CHECK
>            option lets the probing code bypass some of those checks if the
>            parameter "relaxed_check=1" is passed to the module.
> 
> +config CPUFREQ_ARCH_CUR_FREQ
> +       default y
> +       bool "Current frequency derived from HW provided feedback"
> +       help
> +         This determines whether the scaling_cur_freq sysfs attribute returns
> +         the last requested frequency or a more precise value based on hardware
> +         provided feedback (as architected counters).
> +         Given that a more precise frequency can now be provided via the
> +         cpuinfo_avg_cur_freq attribute, by enabling this option,

s/cpuinfo_avg_cur_freq/cpuinfo_cur_freq/?

Overall looks good to me.

Reviewed-by: Sumit Gupta <sumitg@...dia.com>

Powered by blists - more mailing lists