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@xxxxxxxxx>
> 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!
