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

[Beata Michalska]

On Thu, Jun 13, 2024 at 11:27:52AM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 13, 2024 at 10:24 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> >
> > On 07-06-24, 16:21, Rafael J. Wysocki wrote:
> > > On Thu, Jun 6, 2024 at 10:55 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > > 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.
> >
> > Yeah, we would need to do the below part to make it work.
> >
> > > >  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.
> >
> > Right. So this would keep the behavior of the file as is for all platforms and
> > simplify the core.
> >
> > > > - 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.
> >
> > cpuinfo_cur_freq itself ? I thought such changes aren't allowed as they may end
> > up breaking userspace tools.
> 
> cpuinfo_cur_freq is not always present anyway, so user space tools
> need to be able to cope with the lack of it anyway.
>
> > > >   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.
> >
> > Right.
> >
> > > 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.
> >
> > Hmm. If that is the concern (which looks valid), how come it makes sense to do
> > the same on ARM ? Beata, Ionela ?
> >
> > I thought, just like X86, ARM also doesn't have a guaranteed way to know the
> > exact frequency anymore and AMUs are providing a better picture, and so we are
> > moving to the same.
> >
> > If we don't want it for X86, then it can be done with help of a new driver flag
> > CPUFREQ_NO_CPUINFO_SCALING_FREQ, instead of the availability of the get()
> > callback.
> >
> > > 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.
> >
> > Well it is still meaningful to show the return value of the ->get() callback
> > where the hardware provides it.
> 
> But this is a valid point.
> 
> > > 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.
> >
> > Maybe a third file, just for arch_freq_get_on_cpu() is not that bad of an idea
> > :)
> 
> /me thinks so.
I am starting to lean towards that option.
Making both cpuinfo_cur_freq and scaling_cur_freq sane, might create even more
confusion as per which is providing what. We are already in a rather tricky
situation. The interface should be clean, leaving no room for various
interpretations - as much as possible, of course.

---
BR
Beata