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
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.
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.
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
