x86 processors map cluster to the L2 cache. Add documentation stating
the same, and provide more information on the values and API related to
CPU clusters exposed by the kernel.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
o v1->v2
  - Reworded the definition of cluster on x86 based on Peter's
    suggestion.
  - Fixed double spacing before and after the cluster section.
---
 Documentation/x86/topology.rst | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/x86/topology.rst b/Documentation/x86/topology.rst
index 7f58010ea86a..5dae8a0327d1 100644
--- a/Documentation/x86/topology.rst
+++ b/Documentation/x86/topology.rst
@@ -33,6 +33,7 @@ historical nature and should be cleaned up.
 The topology of a system is described in the units of:
 
     - packages
+    - cluster
     - cores
     - threads
 
@@ -90,6 +91,23 @@ Package-related topology information in the kernel:
         Cache. In general, it is a number identifying an LLC uniquely on the
         system.
 
+Clusters
+========
+A cluster consists of threads of one or more cores sharing the same L2 cache.
+
+Cluster-related topology information in the kernel:
+
+  - 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.
+
 Cores
 =====
 A core consists of 1 or more threads. It does not matter whether the threads
@@ -125,6 +143,11 @@ Thread-related topology information in the kernel:
 
     The number of online threads is also printed in /proc/cpuinfo "siblings."
 
+  - topology_cluster_cpumask():
+
+    The cpumask contains all online threads in the cluster to which a thread
+    belongs.
+
   - topology_sibling_cpumask():
 
     The cpumask contains all online threads in the core to which a thread
@@ -138,6 +161,10 @@ Thread-related topology information in the kernel:
 
     The physical package ID to which a thread belongs.
 
+  - topology_cluster_id();
+
+    The ID of the cluster to which a thread belongs.
+
   - topology_core_id();
 
     The ID of the core to which a thread belongs. It is also printed in /proc/cpuinfo
-- 
2.34.1

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.

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?

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.

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.

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

x86 processors map cluster to the L2 cache. Add documentation stating
the same, and provide more information on the values and API related to
CPU clusters exposed by the kernel.

Suggested-by: Dave Hansen <dave.hansen@intel.com> # cluster_id description
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
o v2->v2.1
  - Reword the cluster_id description based on Dave's suggestions.
o v1->v2
  - Reworded the definition of cluster on x86 based on Peter's
    suggestion.
  - Fixed double spacing before and after the cluster section.
---
 Documentation/x86/topology.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/x86/topology.rst b/Documentation/x86/topology.rst
index 7f58010ea86a..9de14f3f7783 100644
--- a/Documentation/x86/topology.rst
+++ b/Documentation/x86/topology.rst
@@ -33,6 +33,7 @@ historical nature and should be cleaned up.
 The topology of a system is described in the units of:
 
     - packages
+    - cluster
     - cores
     - threads
 
@@ -90,6 +91,22 @@ Package-related topology information in the kernel:
         Cache. In general, it is a number identifying an LLC uniquely on the
         system.
 
+Clusters
+========
+A cluster consists of threads of one or more cores sharing the same L2 cache.
+
+Cluster-related topology information in the kernel:
+
+  - cluster_id:
+
+    A per-CPU variable containing:
+
+      - 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 cluster information is unavailable will show 65535
+        (BAD_APICID) as the cluster_id.
+
 Cores
 =====
 A core consists of 1 or more threads. It does not matter whether the threads
@@ -125,6 +142,11 @@ Thread-related topology information in the kernel:
 
     The number of online threads is also printed in /proc/cpuinfo "siblings."
 
+  - topology_cluster_cpumask():
+
+    The cpumask contains all online threads in the cluster to which a thread
+    belongs.
+
   - topology_sibling_cpumask():
 
     The cpumask contains all online threads in the core to which a thread
@@ -138,6 +160,10 @@ Thread-related topology information in the kernel:
 
     The physical package ID to which a thread belongs.
 
+  - topology_cluster_id();
+
+    The ID of the cluster to which a thread belongs.
+
   - topology_core_id();
 
     The ID of the core to which a thread belongs. It is also printed in /proc/cpuinfo
-- 
2.34.1

On 4/14/23 10:17, K Prateek Nayak wrote:
> +  - cluster_id:
> +
> +    A per-CPU variable containing:
> +
> +      - 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 cluster information is unavailable will show 65535
> +        (BAD_APICID) as the cluster_id.

"... return cluster_id of 65535 (BAD_APICID)."

>      The number of online threads is also printed in /proc/cpuinfo "siblings."
>  
> +  - topology_cluster_cpumask():
> +
> +    The cpumask contains all online threads in the cluster to which a thread
> +    belongs.
> +

Looks OK.

>      The physical package ID to which a thread belongs.
>  
> +  - topology_cluster_id();
> +
> +    The ID of the cluster to which a thread belongs.
> +

Looks OK.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

On 4/14/23 19:24, Bagas Sanjaya wrote:
> On 4/14/23 10:17, K Prateek Nayak wrote:
>> +  - cluster_id:
>> +
>> +    A per-CPU variable containing:
>> +
>> +      - 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 cluster information is unavailable will show 65535
>> +        (BAD_APICID) as the cluster_id.
> "... return cluster_id of 65535 (BAD_APICID)."

Bagas, this is talking about a per-cpu variable.  Variables don't
"return" things, functions do.

I also have a request: I'd really appreciate if you could avoid
reviewing x86-related documentation.  The review comments that I've seen
coming from you have not helped x86 documentation.  They've hurt the
patches more than they have helped.

On 4/17/23 07:23, Dave Hansen wrote:
> On 4/14/23 19:24, Bagas Sanjaya wrote:
>> On 4/14/23 10:17, K Prateek Nayak wrote:
>>> +  - cluster_id:
>>> +
>>> +    A per-CPU variable containing:
>>> +
>>> +      - 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 cluster information is unavailable will show 65535
>>> +        (BAD_APICID) as the cluster_id.
>> "... return cluster_id of 65535 (BAD_APICID)."
> 
> Bagas, this is talking about a per-cpu variable.  Variables don't
> "return" things, functions do.
> 

Oops, I don't see that!

> I also have a request: I'd really appreciate if you could avoid
> reviewing x86-related documentation.  The review comments that I've seen
> coming from you have not helped x86 documentation.  They've hurt the
> patches more than they have helped.
> 

OK, thanks!

-- 
An old man doll... just what I always wanted! - Clara