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

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: <aHTOSyhwIAaW_1m1@arm.com>
Date: Mon, 14 Jul 2025 11:30:51 +0200
From: Beata Michalska <beata.michalska@....com>
To: Prashant Malani <pmalani@...gle.com>
Cc: Jie Zhan <zhanjie9@...ilicon.com>,
	Ionela Voinescu <ionela.voinescu@....com>,
	Ben Segall <bsegall@...gle.com>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
	Mel Gorman <mgorman@...e.de>, Peter Zijlstra <peterz@...radead.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Valentin Schneider <vschneid@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	z00813676 <zhenglifeng1@...wei.com>
Subject: Re: [PATCH v2 2/2] cpufreq: CPPC: Dont read counters for idle CPUs

On Wed, Jul 09, 2025 at 03:49:03PM -0700, Prashant Malani wrote:
> On Wed, 9 Jul 2025 at 10:25, Prashant Malani <pmalani@...gle.com> wrote:
> >
> > Hi Beata,
> >
> > Thanks for taking a look.
> >
> > On Mon, 7 Jul 2025 at 01:33, Beata Michalska <beata.michalska@....com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > On Wed, Jul 02, 2025 at 11:38:11AM -0700, Prashant Malani wrote:
> > > > Hi All,
> > > >
> > > > Ionela, Beata, could you kindly review ?
> > > >
> > > > On Fri, 27 Jun 2025 at 10:07, Prashant Malani <pmalani@...gle.com> wrote:
> > > > >
> > > >
> > > > I think it is pertinent to note: the actual act of reading the CPPC counters
> > > > will (at least for ACPI_ADR_SPACE_FIXED_HARDWARE counters)
> > > > wake the CPU up, so even if a CPU *was* idle, the reading of the counters
> > > > calls cpc_read_ffh() [1] which does an IPI on the target CPU [2] thus waking
> > > > it up from WFI.
> > > >
> > > > And that brings us back to the original assertion made in this patch:
> > > > the counter values are quite unreliable when the CPU is in this
> > > > idle (or rather I should correct that to, waking from WFI) state.
> > > >
> > > I'd say that's very platform specific, and as such playing with the delay makes
> > > little sense. I'd need to do more deliberate testing, but I haven't noticed
> > > (yet) any discrepancies in AMU counters on waking up.
> > > Aside, you have mentioned that you've observed the frequency reported to be
> > > above max one (4GHz vs 3.5HZ if I recall correctly) - shouldn't that be clamped
> > > by the driver if the values are outside of supported range ?
> > >
> > > Verifying whether the CPU is idle before poking it just to get a counters
> > > reading to derive current frequency from those does feel rather like an
> > > appealing idea.
> >
> > That's good to hear. What can we do to refine this series further?
So I believe this should be handled in CPUFreq core, if at all.
Would be good to get an input/opinion from the maintainers: Viresh and Rafael.
> >
> > > Narrowing the scope for ACPI_ADR_SPACE_FIXED_HARDWARE cases
> > > could be solved by providing a query for the address space. Though I am not an
> > > expert here so would be good to get some input from someone who is
> > > (on both).
> >
> > Who would be the expert here (are they on this mailing list)?
Probably as above + Sudeep Holla
> >
> > Could you point me to an example for the query for the address space? Then
> > I can respin this series to use that query and narrow the scope.
>
Actually was suggesting adding one.
> Actually, if the idea of this optimization (the idle_cpu check) sounds
> good to you,
> I don't see why we should limit it to ACPI_ADR_SPACE_FIXED_HARDWARE.
> IOW, the patch can remain in its current form.
> 
Right, that does need though verifying against all users.

In the meantime ....
It seems that the issue of getting counters on a CPU that is idle is not
in the counters themselves, but in the way how they are being read - at least
from what I can observe.
The first read experience longer delay between reading core and const counters,
and as const one is read as a second one, it misses some increments (within
calculated delta). So, what we could do within the driver is either:
- Add a way to request reading both counters in a single cpc_read (preferable
  I guess, though I would have to have a closer look at that)
- Add some logic that would make sure the reads are not far apart

Would you be able to verify that on your end?

---
BR
Beata
> Best regards,
> 
> -- 
> -Prashant