Re: [PATCH v2 2/2] cpufreq: CPPC: Dont read counters for idle CPUs

[Prashant Malani <pmalani@google.com>]

On Thu, 14 Aug 2025 at 22:13, Prashant Malani <pmalani@google.com> wrote:
>
> Thanks a lot for taking a look at this, Rafael.
>
> On Aug 14 13:48, Rafael J. Wysocki wrote:
> >
> > First off, AFAICS, using idle_cpu() for reliable detection of CPU
> > idleness in a sysfs attribute code path would be at least
> > questionable, if not outright invalid.  By the time you have got a
> > result from it, there's nothing to prevent the CPU in question from
> > going idle or waking up from idle.
>
> This is a heuristic-based optimization. The observation is that when
> the CPU is idle (or near-idle/lightly loaded, since FFH actually wakes
> up an idle CPU), the AMU counters as read from the kernel are unreliable.
> It is fine if the CPU wakes up from idle immediately after the check.
> In that case, we'd return the desired frequency (via PCC reg read), which
> is what the frequency would be anyway (if the AMU measurement was
> actually taken).
>
> In a sense, the assumption here is no worse than what is there at
> present; currently the samples are taken across 2us, and (theoretically)
> if the difference between them is 0, we take the fallback path. There is
> nothing to prevent the CPU from waking up immediately after that 2us
> sample period.
>
> > Moreover, the fact that the given
> > CPU is idle from the scheduler perspective doesn't actually mean that
> > it is in an idle state and so it has no bearing on whether or not its
> > performance counters can be accessed etc.
>
> The idle check isn't meant to guard against accessing the counters.
> AFAICT it is perfectly valid to access the counters even when the CPU is
> actually idle.
>
> >
> > The way x86 deals with this problem is to snapshot the counters in
> > question periodically (actually, in scheduler ticks) and fall back to
> > cpu_khz if the interval between the two consecutive updates is too
> > large (see https://elixir.bootlin.com/linux/v6.16/source/arch/x86/kernel/cpu/aperfmperf.c#L502).
> > I think that this is the only reliable way to handle it, but I may be
> > mistaken.
>
> This is interesting. I think it may not work for the CPPC case, since
> the registers in question are in some cases accessed through PCC reads
> which require semaphores. I think it would be untenable to do that in
> the tick handler (but I may be mistaken here). It's easier on x86
> since those are always just MSRs.
> We could probably do it for the FFH case, but then we're bifurcating
> the computation method and IMO that's not worth the hassle.

I looked around a bit more and it turns out arm64 is already doing something
similar to what you propose.
It takes a snapshot of the counters every tick, and falls back to
arch_scale_freq_capacity() for another non-idle CPU (checked using idle_cpu())
in the same frequency domain if the interval between snapshots is too long [1].

However, this function only works some of the time; if the CPU hasn't had
a tick in a while *and* there is no other non-idle CPU in the frequency domain,
it returns an error [2].

Moreover, cpuinfo_cur_freq doesn't use this (it reads the frequency directly
from the CPPC driver).

(I'll abandon this series if you still believe this is an invalid
approach; just thought
I'd get this bit of clarification in, for my own understanding).

BR,

-Prashant

[1] https://elixir.bootlin.com/linux/v6.16-rc7/source/arch/arm64/kernel/topology.c#L285
[2] https://elixir.bootlin.com/linux/v6.16-rc7/source/arch/arm64/kernel/topology.c#L336