[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241023035022.GA2484@ranerica-svr.sc.intel.com>
Date: Tue, 22 Oct 2024 20:50:22 -0700
From: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To: Borislav Petkov <bp@...en8.de>
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>,
Nikolay Borisov <nik.borisov@...e.com>,
Huang Ying <ying.huang@...el.com>,
Ricardo Neri <ricardo.neri@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves
On Tue, Oct 22, 2024 at 03:20:50PM +0200, Borislav Petkov wrote:
> On Fri, Sep 13, 2024 at 01:31:54AM -0700, Ricardo Neri wrote:
> > diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> > index 392d09c936d6..182cacd772b8 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;
> > +}
>
> There already is
>
> #define cache_leaves(cpu) (ci_cacheinfo(cpu)->num_leaves)
>
> And there's also get_cpu_cacheinfo().
>
> And now you're adding more silly wrappers. Yuck.
>
> Can we pls use *one* of those things and work with it everywhere?
I agree. Another wrapper is not needed. I did not use cache_leaves() because
it was internal to drivers/base/cacheinfo.c I can convert it to a function
and expose it in include/linux/cacheinfo.h. I can rename it as
get_cacheinfo_leaves(unsigned int cpu).
Would that make sense?
>
> > @@ -742,19 +753,19 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
> > unsigned int l2_id = 0, l3_id = 0, num_threads_sharing, index_msb;
> >
> > if (c->cpuid_level > 3) {
> > - static int is_initialized;
> > -
> > - if (is_initialized == 0) {
> > - /* Init num_cache_leaves from boot CPU */
> > - num_cache_leaves = find_num_cache_leaves(c);
> > - is_initialized++;
> > - }
> > + /*
> > + * There should be at least one leaf. A non-zero value means
> > + * that the number of leaves has been initialized.
> > + */
> > + if (!get_num_cache_leaves(c->cpu_index))
> > + set_num_cache_leaves(c->cpu_index,
> > + find_num_cache_leaves(c));
>
> Ugly linebreak.
I will make it a single line.
>
> >
> > /*
> > * Whenever possible use cpuid(4), deterministic cache
> > * parameters cpuid leaf to find the cache details
> > */
> > - for (i = 0; i < num_cache_leaves; i++) {
> > + for (i = 0; i < get_num_cache_leaves(c->cpu_index); i++) {
> > struct _cpuid4_info_regs this_leaf = {};
> > int retval;
> >
> > @@ -790,14 +801,14 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
> > * Don't use cpuid2 if cpuid4 is supported. For P4, we use cpuid2 for
> > * trace cache
> > */
> > - if ((num_cache_leaves == 0 || c->x86 == 15) && c->cpuid_level > 1) {
> > + if ((!get_num_cache_leaves(c->cpu_index) || c->x86 == 15) && c->cpuid_level > 1) {
> > /* supports eax=2 call */
> > int j, n;
> > unsigned int regs[4];
> > unsigned char *dp = (unsigned char *)regs;
> > int only_trace = 0;
> >
> > - if (num_cache_leaves != 0 && c->x86 == 15)
> > + if (get_num_cache_leaves(c->cpu_index) && c->x86 == 15)
> > only_trace = 1;
> >
> > /* Number of times to iterate */
> > @@ -993,12 +1004,9 @@ int init_cache_level(unsigned int cpu)
> > {
> > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> >
> > - if (!num_cache_leaves)
> > - return -ENOENT;
>
> Why not
>
> if (!cache_leaves(cpu))
> return -ENOENT;
>
> ?
The only caller of init_cache_level() also checks for !cache_leaves(cpu). I
saw no need to repeat the check here.
Also, I understand that the purpose of the function is to initialize
cpu_cacheinfo::num_levels, which is not used on x86. Moreover,
cpu_cacheinfo::num_levels do not depend on num_leaves.
Having said that, I see other architectures initializing both num_levels
and num_leaves in this function.
Adding this check probably makes the x86 implementation more future-proof
in case callers change their behavior.
Thanks and BR,
Ricardo
Powered by blists - more mailing lists