Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache topo in CPUID[4]

[Zhao Liu <zhao1.liu@linux.intel.com>]

On Mon, Mar 11, 2024 at 05:03:02PM +0800, Xiaoyao Li wrote:
> Date: Mon, 11 Mar 2024 17:03:02 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache
>  topo in CPUID[4]
> 
> On 3/10/2024 9:38 PM, Zhao Liu wrote:
> > Hi Xiaoyao,
> > 
> > > >                case 3: /* L3 cache info */
> > > > -                die_offset = apicid_die_offset(&topo_info);
> > > >                    if (cpu->enable_l3_cache) {
> > > > +                    addressable_threads_width = apicid_die_offset(&topo_info);
> > > 
> > > Please get rid of the local variable @addressable_threads_width.
> > > 
> > > It is truly confusing.
> > 
> > There're several reasons for this:
> > 
> > 1. This commit is trying to use APIC ID topology layout to decode 2
> > cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and
> > CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map
> > to CPUID.04H:EAX[bits 31:26], it's more clear to also map
> > CPUID.04H:EAX[bits 25:14] to another variable.
> 
> I don't dislike using a variable. I dislike the name of that variable since
> it's misleading

Names are hard to choose...

> 
> > 2. All these 2 variables are temporary in this commit, and they will be
> > replaed by 2 helpers in follow-up cleanup of this series.
> 
> you mean patch 20?
> 
> I don't see how removing the local variable @addressable_threads_width
> conflicts with patch 20. As a con, it introduces code churn.

Yes...I prefer to wrap it in variables in advance, then the meaning of
the fields is clearer I think.

> > 3. Similarly, to make it easier to clean up later with the helper and
> > for more people to review, it's neater to explicitly indicate the
> > CPUID.04H:EAX[bits 25:14] with a variable here.
> 
> If you do want keeping the variable. Please add a comment above it to
> explain the meaning.
>

OK, I'll add comments for both 2 variables. Thanks!