Hi,
On Thu, Aug 28, 2025 at 04:58:16PM +0100, James Morse wrote:
> Hi Dave,
>
> 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.
> >
> > Nit: cacheinfo lacking the information is not a consequence of the
> > driver needing it.
> >
> > Maybe split the sentence:
> >
> > -> "[...] associated with the cache. The CPUs may not [...]"
>
> Sure,
OK
> >> 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?) 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 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.
Cheers
---Dave
