Re: [PATCH] x86/cacheinfo: Don't use cpu_llc_shared_map for !CONFIG_SMP

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
To: Borislav Petkov <bp@alien8.de>
Cc: ssengar@microsoft.com, mikelley@microsoft.com,
	tglx@linutronix.de, mingo@redhat.com,
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
	peterz@infradead.org, tim.c.chen@linux.intel.com,
	will@kernel.org, song.bao.hua@hisilicon.com,
	suravee.suthikulpanit@amd.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/cacheinfo: Don't use cpu_llc_shared_map for !CONFIG_SMP
Date: Wed, 17 Aug 2022 21:52:25 -0700	[thread overview]
Message-ID: <20220818045225.GA9054@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <Yv1ELaWHbRfvdt/p@zn.tnic>

Thanks for review, please find my comments inline.

On Wed, Aug 17, 2022 at 09:40:29PM +0200, Borislav Petkov wrote:
> On Wed, Aug 10, 2022 at 09:15:15AM -0700, Saurabh Sengar wrote:
> > cpu_llc_shared_map is always declared and defined, but populated in
> > arch/x86/kernel/smpboot.c which only builds for CONFIG_SMP=y. For
> > UniProcessor builds this mask is never populated and hence invalid.
> > Current code doesn't handle the case of AMD UniProcessor correctly,
> 
> What is "AMD UniProcessor"?
Shall I mention here "Non-SMP AMD processor" instead ?

> 
> > which results "shared_cpu_map" and "shared_cpu_list" files missing from
> > sysfs entries for l3 cache. This patch fixes this issue.
> 
> What issue exactly?
> 
> What is the real life use case for this?

We observed that lscpu version 2.34 was causing segfault with latest kernel,
which motivate us to debug this issue. Root cause being shared_cpu_map file
missing. There could be more usecases where this file is getting queried for
L3 cache information but at the moment I am aware of only lscpu.
Do we need to add this info in commit.

> 
> > This code used to work because of a another bug in 'cpumask_next',
> > where in the CONFIG_SMP=n case the cpumask_next() ignores empty mask
> > that results as if CPU 0 was set. This bug in 'cpumask_next' was fixed by
> > following commit, which exposes the cpu_llc_shared_map bug.
> > 
> > b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")
> > 
> > Fixes: 2b83809a5e ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")
> 
> Add
> 
> ---
> [core]
>         abbrev = 12
> 
> [alias]
>         one = show -s --pretty='format:%h (\"%s\")'
> ---
> 
> to your git .config so that when you do
> 
> $ git one <commit id>
> 
> you can get the proper formatting and abbreviated sha1 for commits.
Thanks for suggestion, will fix this in V2.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette