Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

On Wed, Aug 28, 2024 at 03:57:05PM +0300, Nikolay Borisov wrote:
> 
> 
> On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> > Linux remembers cpu_cachinfo::num_leaves per CPU, but x86 initializes all
> > CPUs from the same global "num_cache_leaves".
> > 
> > This is erroneous on systems such as Meteor Lake, where each CPU has a
> > distinct num_leaves value. Delete the global "num_cache_leaves" and
> > initialize num_leaves on each CPU.
> > 
> > Reviewed-by: Len Brown <len.brown@xxxxxxxxx>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> > ---
> > Cc: Andreas Herrmann <aherrmann@xxxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Chen Yu <yu.c.chen@xxxxxxxxx>
> > Cc: Huang Ying <ying.huang@xxxxxxxxx>
> > Cc: Len Brown <len.brown@xxxxxxxxx>
> > Cc: Nikolay Borisov <nik.borisov@xxxxxxxx>
> > Cc: Radu Rendec <rrendec@xxxxxxxxxx>
> > Cc: Pierre Gondois <Pierre.Gondois@xxxxxxx>
> > Cc: Pu Wen <puwen@xxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx # 6.3+
> > ---
> > After this change, all CPUs will traverse CPUID leaf 0x4 when booted for
> > the first time. On systems with symmetric cache topologies this is
> > useless work.
> > 
> > Creating a list of processor models that have asymmetric cache topologies
> > was considered. The burden of maintaining such list would outweigh the
> > performance benefit of skipping this extra step.
> > ---
> > Changes since v4:
> >   * None
> > 
> > Changes since v3:
> >   * Rebased on v6.7-rc5.
> > 
> > Changes since v2:
> >   * None
> > 
> > Changes since v1:
> >   * Do not make num_cache_leaves a per-CPU variable. Instead, reuse the
> >     existing per-CPU ci_cpu_cacheinfo variable. (Dave Hansen)
> > ---
> 
> 
> Overall LGTM, one minor nit below which is not a deal breaker.
> 
> 
> Reviewed-by: Nikolay Borisov <nik.borisov@xxxxxxxx>

Thank you!

> 
> >   arch/x86/kernel/cpu/cacheinfo.c | 44 +++++++++++++++++++--------------
> >   1 file changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> > index 392d09c936d6..b5e216677a46 100644
> > --- a/arch/x86/kernel/cpu/cacheinfo.c
> > +++ b/arch/x86/kernel/cpu/cacheinfo.c
> > @@ -178,7 +178,16 @@ struct _cpuid4_info_regs {
> >   	struct amd_northbridge *nb;
> >   };
> > -static unsigned short num_cache_leaves;
> > +static inline unsigned int get_num_cache_leaves(unsigned int cpu)
> > +{
> > +	return get_cpu_cacheinfo(cpu)->num_leaves;
> > +}
> > +
> > +static inline void
> > +set_num_cache_leaves(unsigned int nr_leaves, unsigned int cpu)
> > +{
> 
> nit: I think it's more natural to have the cpu parameter come first.

Sure! I will wait a few days for more feedback. Then I will post an
updated version with this change.

Best,
Ricardo