Hi Xiaoyao,
Did the following reason convince you? Could I take your r/b tag with
current code? ;-)
Thanks,
Zhao
On Sun, Mar 10, 2024 at 09:38:19PM +0800, Zhao Liu wrote:
> Date: Sun, 10 Mar 2024 21:38:19 +0800
> From: Zhao Liu <zhao1.liu@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache
> topo in CPUID[4]
>
> 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.
>
> 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.
>
> 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.
>
> 4. I call this field as addressable_threads_width since it's "Maximum
> number of addressable IDs for logical processors sharing this cache".
>
> Its own name is long, but given the length, only individual words could
> be selected as names.
>
> TBH, "addressable" deserves more emphasis than "sharing". The former
> emphasizes the fact that the number of threads chosen here is based on
> the APIC ID layout and does not necessarily correspond to actual threads.
>
> Therefore, this variable is needed here.
>
> Thanks,
> Zhao
>
