Received-SPF: pass (google.com: domain of linux-kernel+bounces-212804-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 13 Jun 2024 13:53:58 +0530
From: Viresh Kumar <viresh.kumar@linaro.org>
To: ionela.voinescu@arm.com, Beata Michalska <beata.michalska@arm.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	len.brown@intel.com, vanshikonda@os.amperecomputing.com,
	sumitg@nvidia.com, vincent.guittot@linaro.org
Subject: Re: [PATCH 1/1] cpufreq: Rewire arch specific feedback for
 cpuinfo/scaling_cur_freq
Message-ID: <20240613082358.yq2lui6vc35xi53t@vireshk-i7>
References: <20240603081331.3829278-1-beata.michalska@arm.com>
 <20240603081331.3829278-2-beata.michalska@arm.com>
 <20240603114811.oio3uemniib5uaa2@vireshk-i7>
 <CAJZ5v0j1bqhmKrJirw+WgEVDdszZ9xQSgmfazVKMVa8H6_5TSw@mail.gmail.com>
 <20240606085513.pptx5dtjcvvg3zo4@vireshk-i7>
 <CAJZ5v0it5vZvzkbErkGOVruPbkxgr8VMTgQzsncpdQmvCOfBng@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0it5vZvzkbErkGOVruPbkxgr8VMTgQzsncpdQmvCOfBng@mail.gmail.com>

On 07-06-24, 16:21, Rafael J. Wysocki wrote:
> On Thu, Jun 6, 2024 at 10:55 AM Viresh Kumar <viresh.kumar@linaro.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