Message-ID: <CAJZ5v0it5vZvzkbErkGOVruPbkxgr8VMTgQzsncpdQmvCOfBng@mail.gmail.com>
Date: Fri, 7 Jun 2024 16:21:51 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Beata Michalska <beata.michalska@....com>, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, len.brown@...el.com, 
	ionela.voinescu@....com, vanshikonda@...amperecomputing.com, 
	sumitg@...dia.com, vincent.guittot@...aro.org
Subject: Re: [PATCH 1/1] cpufreq: Rewire arch specific feedback for cpuinfo/scaling_cur_freq

On Thu, Jun 6, 2024 at 10:55 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 03-06-24, 15:43, Rafael J. Wysocki wrote:
> > On Mon, Jun 3, 2024 at 1:48 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > > Rafael,
> > >
> > > We probably need to decide on a policy for these two files, it is
> > > getting a bit confusing.
> > >
> > > cpuinfo_cur_freq:
> > >
> > > The purpose of this file is abundantly clear. This returns the best
> > > possible guess of the current hardware frequency. It should rely on
> > > arch_freq_get_on_cpu() or ->get() to get the value.
> >
> > Let me quote the documentation:
> >
> > "This is expected to be the frequency the hardware actually runs at.
> > If that frequency cannot be determined, this attribute should not be
> > present."
> >
> > In my reading, this has nothing to do with arch_freq_get_on_cpu(), at
> > least on x86.
> >
> > > Perhaps we can
> > > make this available all the time, instead of conditionally on ->get()
> > > callback (which isn't present for intel-pstate for example).
> >
> > We could, but then on x86 there is no expectation that this file will
> > be present and changing this may introduce significant confusion
> > because of the way it is documented (which would need to be changed,
> > but people might be forgiven for failing to notice the change of
> > interpretation of this file).
>
> > > scaling_cur_freq:
> > >
> > > This should better reflect the last requested frequency, but since a
> > > significant time now it is trying to show what cpuinfo_cur_freq shows.
> >
> > Well, not really.
> >
> > > commit c034b02e213d ("cpufreq: expose scaling_cur_freq sysfs file for set_policy() drivers")
> > > commit f8475cef9008 ("x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF")
> >
> > "In the majority of cases, this is the frequency of the last P-state
> > requested by the scaling driver from the hardware using the scaling
> > interface provided by it, which may or may not reflect the frequency
> > the CPU is actually running at (due to hardware design and other
> > limitations).
> >
> > 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."
>
> Right, with time the documentation is updated and now it has mixed
> the purpose of both these files IMO.
>
> > So the problem is that on Intel x86 with HWP and intel_pstate in the
> > active mode, say, "the frequency of the last P-state requested by the
> > scaling driver from the hardware" is actually never known, so exposing
> > it via scaling_cur_freq is not possible.
> >
> > Moreover, because cpuinfo_cur_freq is not present at all in that case,
> > scaling_cur_freq is the only way to allow user space to get an idea
> > about the CPU current frequency.  I don't think it can be changed now
> > without confusing users.
>
> Yes, this is a valid concern. The changes in documentation have been
> there for many years and changing the behavior now is not going to be
> an easy / right thing to do.
>
> > > What should we do ? I wonder if we will break some userspace tools
> > > (which may have started relying on these changes).
> >
> > We will.
> >
> > IIUC, it is desirable to expose "the frequency of the last P-state
> > requested by the scaling driver from the hardware" via
> > scaling_cur_freq on ARM, but it is also desirable to expose an
> > approximation of the actual current CPU frequency, so the only way to
> > do that without confusing the heck out of everybody downstream would
> > be to introduce a new attribute for this purpose and document it
> > precisely.
>
> Hmm, having 3 files would confuse people even more I guess. I wanted
> to get this sorted to have the same behavior for all platforms, but it
> seems somewhat difficult to achieve now.
>
> What about this, hopefully this doesn't break any existing platforms
> and fix the problems for ARM (and others):
>
> - scaling_cur_freq:
>
>   Returns the frequency of the last P-state requested by the scaling
>   driver from the hardware.

This would change the behavior for intel_pstate in the passive mode AFAICS.

ATM it calls arch_freq_get_on_cpu(), after the change it would return
policy->cur which would not be the same value most of the time.  And
in the ->adjust_perf() case policy->cur is not updated by it even.

>  For set_policy() drivers, use the ->get()
>   callback to get a value that can provide the best estimate to user.
>
>   To make this work, we can add get() callback to intel and amd pstate
>   drivers, and use arch_freq_get_on_cpu().
>
>   This will keep the current behavior intact for such drivers.

Well, the passive mode thing would need to be addressed then.

> - cpuinfo_cur_freq:
>
>   Currently this file is available only if the get() callback is
>   available. Maybe we can keep this behavior as is, and expose this
>   now for both the pstate drivers (once above change is added). We
>   will be left with only one driver that doesn't provide the get()
>   callback: pasemi-cpufreq.c

I would rather get rid of it completely.

>   Coming back to the implementation of the file read operation, I
>   think the whole purpose of arch_freq_get_on_cpu() was to get a
>   better estimate (which may not be perfect) of the frequency the
>   hardware is really running at (in the last window) and if a platform
>   provides this, then it can be given priority over the ->get()
>   callback in order to show the value to userspace.

There was a reason to add it and it was related to policy->cur being
meaningless on x86 in general (even in the acpi-cpufreq case), but
let's not go there.

Hooking this up to cpuinfo_cur_freq on x86 wouldn't make much sense
IMV because at times it is not even close to the frequency the
hardware is running at.  It comes from the previous tick period,
basically, and the hardware can adjust the frequency with a resolution
that is orders of magnitude higher than the tick rate.

>   And so, if the platform provides, we can use arch_freq_get_on_cpu()
>   first and then the get() callback.
>
> That would leave us to this change for the core, and yes a get()
> callback for intel-pstate and amd-pstate:
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7c6879efe9ef..e265f8450160 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -756,12 +756,8 @@ __weak unsigned int arch_freq_get_on_cpu(int cpu)
>  static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>  {
>         ssize_t ret;
> -       unsigned int freq;
>
> -       freq = arch_freq_get_on_cpu(policy->cpu);
> -       if (freq)
> -               ret = sprintf(buf, "%u\n", freq);
> -       else if (cpufreq_driver->setpolicy && cpufreq_driver->get)
> +       if (cpufreq_driver->setpolicy && cpufreq_driver->get)
>                 ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu));
>         else
>                 ret = sprintf(buf, "%u\n", policy->cur);
> @@ -795,7 +791,10 @@ store_one(scaling_max_freq, max);
>  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
>                                         char *buf)
>  {
> -       unsigned int cur_freq = __cpufreq_get(policy);
> +       unsigned int cur_freq = arch_freq_get_on_cpu(policy->cpu);
> +
> +       if (!cur_freq)
> +               cur_freq = __cpufreq_get(policy);
>
>         if (cur_freq)
>                 return sprintf(buf, "%u\n", cur_freq);
>
>
> I think this will also make more sense from documentation's
> perspective, which says that:
>
> "In the majority of cases, this is the frequency of the last P-state
> requested by the scaling driver from the hardware using the scaling
> interface provided by it, which may or may not reflect the frequency
> the CPU is actually running at (due to hardware design and other
> limitations)."
>
> -- we do this at the core level.
>
> "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."
>
> -- and this at driver level, as a special case.

Well, this sounds nice, but the changes are a bit problematic.

If you don't want 3 files, I'd drop cpuinfo_cur_freq and introduce
something else to replace it which will expose the
arch_freq_get_on_cpu() return value and will be documented
accordingly.

Then scaling_cur_freq can be (over time) switched over to returning
policy->cur in the cases when it is meaningful and -ENODATA otherwise.

This would at least allow us to stop making up stuff.

Powered by blists - more mailing lists