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

[Nikolay Borisov]

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>

>   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.
> +	get_cpu_cacheinfo(cpu)->num_leaves = nr_leaves;
> +}
>   
>   /* AMD doesn't have CPUID4. Emulate it here to report the same
>      information to the user.  This makes some assumptions about the machine:


<snip>