Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

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