linux-kernel - Re: [PATCH 1/1] cpufreq: Rewire arch specific feedback for cpuinfo/scaling_cur

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240613082358.yq2lui6vc35xi53t@vireshk-i7>
Date: Thu, 13 Jun 2024 13:53:58 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: ionela.voinescu@....com, Beata Michalska <beata.michalska@....com>,
	"Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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 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.

> >   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.

> 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
:)

-- 
viresh