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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240829043335.GA4229@ranerica-svr.sc.intel.com>
Date: Wed, 28 Aug 2024 21:33:35 -0700
From: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To: Nikolay Borisov <nik.borisov@...e.com>
Cc: x86@...nel.org, 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 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@...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>

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