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
