Re: [PATCH v2 2/2] x86/Documentation: Add documentation about cluster

[K Prateek Nayak]

Hello Dave,

Thank you for taking a look at the series.

On 4/13/2023 11:27 PM, Dave Hansen wrote:
> On 4/13/23 10:29, K Prateek Nayak wrote:
>> +  - cluster_id:
>> +
>> +    A per-CPU variable containing:
>> +
>> +      - On Intel, the common upper bits of APIC ID of the list of CPUs sharing
>> +        the L2 Cache with lower bits set to 0.
>> +
>> +      - On AMD and Hygon, with Topology Extension, the common upper bits of the
>> +        Extended APIC ID of the list of CPUs sharing the L2 Cache, left shifted
>> +        to remove trailing 0s.
> 
> I think this is too much detail for Documentation.  We have the code if
> anyone cares _this_ much.

Yes, I agree. I'll reword this as you suggested.

> 
> Also, I'm perplexed by the "left shifted" comment.  I don't see a lot of
> left shifting in the patch.  Am I just missing it?

In Patch1, cacheinfo_topoext_init_l2c_id() sets l2c_id as follows for AMD
and Hygon processors:

  bits = get_count_order(num_sharing_cache);
  per_cpu(cpu_l2c_id, cpu) = c->apicid >> bits;

For Intel, in init_intel_cacheinfo(), l2c_id is set as follows:

  index_msb = get_count_order(num_threads_sharing);
  l2_id = c->apicid & ~((1 << index_msb) - 1);
  ...
  per_cpu(cpu_l2c_id, cpu) = l2_id;

In the former, only the upper bits that are same for all the threads in a
cluster are retained, shifting out the lower bits, whereas in the latter
the lower bits are set to 0s keeping the upper bits, common to all the
threads on the cluster, as is. Let me know if I'm missing something.

> 
> Further, this makes it sound like all Intel CPUs have the cluster_id
> populated.  I'm also not sure that folks reading this will have any
> worldly idea what "Topology Extension" is.

I agree, it becomes too technical.

> 
> Why don't we just say that some CPUs don't have this info?  That way we
> don't need to spell out AMD vs. Intel or expect our users to go figuring
> out of their CPU has "Topology Extension" or leaf 3 or wherever this
> info is on Intel.
> 
> How about:
> 
> A per-CPU variable containing:
> 
>    - Some upper bits extracted from the APIC ID.  CPUs which have the
>      same value in these bits share an L2 and have the same cluster_id.
> 
>      CPUs for which L2 cache information is unavailable will show 65535
>      as the cluster_id.

I'll reword the description based on your suggestion in the next version.

--
Thanks and Regards,
Prateek