Date: Thu, 13 Jun 2024 11:27:52 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: ionela.voinescu@....com, Beata Michalska <beata.michalska@....com>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	len.brown@...el.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 13, 2024 at 10:24 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 07-06-24, 16:21, Rafael J. Wysocki wrote:
> > On Thu, Jun 6, 2024 at 10:55 AM Viresh Kumar <viresh.kumar@...aro.org> 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.

Powered by blists - more mailing lists