Message-ID: <CAFivqm+r41cRhsZWjXdOhGhTsQ3nmr50cACk27y-PLee7ZGKeA@mail.gmail.com>
Date: Thu, 28 Aug 2025 14:29:37 -0700
From: Prashant Malani <pmalani@...gle.com>
To: Beata Michalska <beata.michalska@....com>
Cc: Yang Shi <yang@...amperecomputing.com>, open list <linux-kernel@...r.kernel.org>, 
	"open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Viresh Kumar <viresh.kumar@...aro.org>, Catalin Marinas <catalin.marinas@....com>, 
	Ionela Voinescu <Ionela.Voinescu@....com>
Subject: Re: [PATCH] cpufreq: CPPC: Increase delay between perf counter reads

On Aug 28 09:33, Beata Michalska wrote:
> On Mon, Aug 25, 2025 at 08:24:52PM +0000, Prashant Malani wrote:
> > On Aug 25 16:52, Beata Michalska wrote:
> > What you are saying holds when m,n ~ X. But if X >> m,n, the X component
> > dominates. On most platforms, m and n are typically 1-2 us.
> > If X is anything >= 100us, it dominates the m,n component, making both
> > time intervals practically the same, i.e
> >
> > (100 + 1) / (100 + 2) = 101 / 102 = 0.9901 ~ 1.00
> True but that does still influence the error - in this case that's ~1% so
> negligible. But the overall error magnitude does increase when the range
> between min and max of the possible values of m and n gets bigger.
> Question is what's the max error that can be deemed acceptable.
> And I'm pretty sure there are platforms that would require bigger X still.

I think 1-2us is the typical m,n value for most platforms looking at
past measurements in related threads here (it
might be a little less, but it's a little challenging to measure this
accurately with ftrace, since the timestamps have usec precision).

Even a 5% error would be a great improvement from the current state.

>
> >
> > > >
> > > > There have been other observations on this topic [1], that suggest
> > > > that even 100us
> > > > improves the error rate significantly from what it is with 2us.
> > > >
> > > > BR,
> > > Which is exactly why I've mentioned this approach is not really recommended,
> > > being bound to rather specific setup. There have been similar proposals in the
> > > past, all with different values of the delay which should illustrate how fragile
> > > solution (if any) that is.
> >
> > The reports/occurences point to the fact that the current value doesn't work.
> Wasn't claiming it does.
> >
> > Another way of putting it is, why is 2us considered the "right"
> > value?
> Looking at the history, the argument was pretty much the same as yours: was
> considered sufficient for most platforms [1]

Thanks for that link. It provided helpful context.

> >
> > This patch was never meant to be an ideal solution, but it's better than what
> > is there at present. Currently, the `policy->cur` is completely unusable on CPPC,
> > and is cropping up in other locations in the cpufreq driver core [1]
> > while also breaking a userfacing ABI i.e scaling_setspeed.
> >
> > I realize you're working on a solution, so if that is O(weeks) away, it
> > makes sense to wait; otherwise it would seem logical to mitigate the
> > error (it can always be reverted once the "better" solution is in
> > place).
> >
> > Ultimately it's your call, but I'm not convinced with rationale provided
> > thus far.
> Actually it is not up to me, I'm simply sharing my opinion, which is:
> we should fix the problem instead of hiding it.

I think both options can be pursued concurrently :)

> Setting that aside though - this change seems rather harmless.
>
> Aside:
> ./scripts/get_maintainer.pl --m ./drivers/cpufreq/cppc_cpufreq.c

Yes, I use this for all mailing list submissions (including this one),
so I believe the maintainers should be on this thread.

BR,

-Prashant

Powered by blists - more mailing lists