linux-kernel - Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f300d558-9798-40a1-9d11-ec6b6020a1b3@suse.com>
Date: Wed, 28 Aug 2024 15:57:05 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>, x86@...nel.org
Cc: Andreas Herrmann <aherrmann@...e.com>,
 Catalin Marinas <catalin.marinas@....com>, Chen Yu <yu.c.chen@...el.com>,
 Len Brown <len.brown@...el.com>, Radu Rendec <rrendec@...hat.com>,
 Pierre Gondois <Pierre.Gondois@....com>, Pu Wen <puwen@...on.cn>,
 "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
 Sudeep Holla <sudeep.holla@....com>,
 Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
 Will Deacon <will@...nel.org>, Zhang Rui <rui.zhang@...el.com>,
 Huang Ying <ying.huang@...el.com>, Ricardo Neri <ricardo.neri@...el.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves



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@...el.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> ---
> Cc: Andreas Herrmann <aherrmann@...e.com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Chen Yu <yu.c.chen@...el.com>
> Cc: Huang Ying <ying.huang@...el.com>
> Cc: Len Brown <len.brown@...el.com>
> Cc: Nikolay Borisov <nik.borisov@...e.com>
> Cc: Radu Rendec <rrendec@...hat.com>
> Cc: Pierre Gondois <Pierre.Gondois@....com>
> Cc: Pu Wen <puwen@...on.cn>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
> Cc: Sudeep Holla <sudeep.holla@....com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Zhang Rui <rui.zhang@...el.com>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: stable@...r.kernel.org # 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@...e.com>

>   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>