Re: [PATCH 1/1] cpufreq: Rewire arch specific feedback for cpuinfo/scaling_cur_freq

[Viresh Kumar]

On 03-06-24, 15:43, Rafael J. Wysocki wrote:
> On Mon, Jun 3, 2024 at 1:48 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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. 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.

- 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

  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.

  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.

-- 
viresh