Message-ID: <7353f490-2cac-4b97-9f48-c612b9034561@arm.com>
Date: Wed, 10 Sep 2025 20:29:17 +0100
From: James Morse <james.morse@....com>
To: Dave Martin <Dave.Martin@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org, devicetree@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com,
 Jonathan Cameron <jonathan.cameron@...wei.com>, Rob Herring
 <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
 Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 06/33] ACPI / PPTT: Add a helper to fill a cpumask from a
 cache_id

Hi Dave,

On 09/09/2025 11:14, Dave Martin wrote:
> On Thu, Aug 28, 2025 at 04:58:16PM +0100, James Morse wrote:
>> On 27/08/2025 11:53, Dave Martin wrote:
>>> On Fri, Aug 22, 2025 at 03:29:47PM +0000, James Morse wrote:
>>>> MPAM identifies CPUs by the cache_id in the PPTT cache structure.
>>>>
>>>> The driver needs to know which CPUs are associated with the cache,
>>>> the CPUs may not all be online, so cacheinfo does not have the
>>>> information.

>>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>>> index 660457644a5b..cb93a9a7f9b6 100644
>>>> --- a/drivers/acpi/pptt.c
>>>> +++ b/drivers/acpi/pptt.c
>>>> @@ -971,3 +971,65 @@ int find_acpi_cache_level_from_id(u32 cache_id)
> 
> [...]
> 
>>>> + * acpi_pptt_get_cpumask_from_cache_id() - Get the cpus associated with the
>>>> + *					   specified cache
>>>> + * @cache_id: The id field of the unified cache
>>>> + * @cpus: Where to build the cpumask
>>>> + *
>>>> + * Determine which CPUs are below this cache in the PPTT. This allows the property
>>>> + * to be found even if the CPUs are offline.
>>>> + *
>>>> + * The PPTT table must be rev 3 or later,
>>>> + *
>>>> + * Return: -ENOENT if the PPTT doesn't exist, or the cache cannot be found.
>>>> + * Otherwise returns 0 and sets the cpus in the provided cpumask.
>>>> + */
>>>> +int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id, cpumask_t *cpus)
>>>> +{
> 
> [...]
> 
>>>> +	/*
>>>> +	 * If we found the cache first, we'd still need to walk from each cpu.
>>>> +	 */
>>>> +	for_each_possible_cpu(cpu) {
> 
> [...]
> 
>>> Again, it feels like we are repeating the same walk multiple times to
>>> determine how deep the table is (on which point the table is self-
>>> describing anyway), and then again to derive some static property, and
>>> then we are then doing all of that work multiple times to derive
>>> different static properties, etc.
>>>
>>> Can we not just walk over the tables once and stash the derived
>>> properties somewhere?
>>
>> That is possible - but its a more invasive change to the PPTT parsing code.
>> Before the introduction of the leaf flag, the search for a processor also included a
>> search to check if the discovered node was a leaf.
>>
>> I think this is trading time - walking over the table multiple times, against the memory
>> you'd need to de-serialise the tree to find the necessary properties quickly. I think the
>> reason Jeremy L went this way was because there may never be another request into this
>> code, so being ready with a quick answer was a waste of memory.
>>
>> MPAM doesn't change this - all these things are done up front during driver probing, and
>> the values are cached by the driver.
> 
> I guess that's true.
> 
>>> I'm still getting my head around this parsing code, so I'm not saying
>>> that the approach is incorrect here -- just wondering whether there is
>>> a way to make it simpler.
>>
>> It's walked at boot, and on cpu-hotplug. Neither are particularly performance critical.

> Do we do this only for unknown late secondaries (e.g., that haven't
> previously come online?) 

No, each time a CPU comes online.


> I haven't gone to track this down but, if not,
> this cuts across the assertion that "there may never be another request
> into this code".

CPU hotplug is optional - you don't have to bounce CPUs. It's very common on mobile parts
for power saving. I think its fairly unusual on server parts, once CPUs are online they
stay online.

The cacheinfo code doesn't cache this, it re-reads it every time. That turns out to be
because of PowerPC where some of these properties can be changed while a CPU is offline.
Sure, we could have a Kconfig thing to say ARCH_STATIC_TABLES_ARE_STATIC, but that would
be a different piece of work.
(I've had a couple of stabs at this, but cacheinfo is the shape it needs to be)


> cpu hotlug is slow in practice, but gratuitous cost on this path should
> still be avoided where feasible.
> 
>> I agree that as platforms get bigger, there will be a tipping point ... I don't think
>> anyone has complained yet!
> 
> Ack -- when in ACPI, do as the ACPI folks do, I guess.


Thanks,

James

Powered by blists - more mailing lists