Re: [PATCH v7 3/4] arm64: topology: Support SMT control on ACPI based system

On 2024/11/8 1:20, Pierre Gondois wrote:

On 10/30/24 13:54, Yicong Yang wrote:

quoted

From: Yicong Yang <redacted>

For ACPI we'll build the topology from PPTT and we cannot directly
get the SMT number of each core. Instead using a temporary xarray
to record the heterogeneous information (from ACPI_PPTT_ACPI_IDENTICAL)
and SMT information of the first core in its heterogeneous CPU cluster
when building the topology. Then we can know the largest SMT number
in the system. If a homogeneous system's using ACPI 6.2 or later,
all the CPUs should be under the root node of PPTT. There'll be
only one entry in the xarray and all the CPUs in the system will
be assumed identical.

The core's SMT control provides two interface to the users [1]:
1) enable/disable SMT by writing on/off
2) enable/disable SMT by writing thread number 1/max_thread_number

If a system have more than one SMT thread number the 2) may
not handle it well, since there're multiple thread numbers in the
system and 2) only accept 1/max_thread_number. So issue a warning
to notify the users if such system detected.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-devices-system-cpu#n542
Signed-off-by: Yicong Yang <redacted>
---
  arch/arm64/kernel/topology.c | 57 ++++++++++++++++++++++++++++++++++++
  1 file changed, 57 insertions(+)

[...]

quoted

@@ -67,6 +106,24 @@ int __init parse_acpi_topology(void)

          cpu_topology[cpu].package_id = topology_id;
      }
  +    /*
+     * This should be a short loop depending on the number of heterogeneous
+     * CPU clusters. Typically on a homogeneous system there's only one
+     * entry in the XArray.
+     */
+    xa_for_each(&hetero_cpu, hetero_id, entry) {
+        if (entry->thread_num != max_smt_thread_num && max_smt_thread_num)
+            pr_warn_once("Heterogeneous SMT topology is partly supported by SMT control\n");
+
+        if (entry->thread_num > max_smt_thread_num)
+            max_smt_thread_num = entry->thread_num;
+
+        xa_erase(&hetero_cpu, hetero_id);
+        kfree(entry);
+    }

I think you need to check that max_smt_thread_num !=0,
like in the DT path. Otherwise on a platform with no SMT,
max_smt_thread_num = 0 and I hit:

kernel/cpu::cpu_smt_set_num_threads()
WARN_ON(!num_threads || (num_threads > max_threads));


->
if (max_smt_thread_num)
    cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num);

sorry for this silly mistake. yes we should do the same like in the DT path.
I'll get this fixed. thanks for testing.

quoted

+
+    cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num);
+    xa_destroy(&hetero_cpu);
      return 0;
  }
  #endif

.