Re: [PATCH v7 1/4] cpu/SMT: Provide a default topology_is_primary_thread()

On Thu, Oct 31 2024 at 20:17, Yicong Yang wrote:

On 2024/10/30 22:55, Thomas Gleixner wrote:

quoted

quoted

+static inline bool topology_is_primary_thread(unsigned int cpu)
+{
+	/*
+	 * On SMT hotplug the primary thread of the SMT won't be disabled.
+	 * Architectures do have a special primary thread (e.g. x86) need
+	 * to override this function. Otherwise just make the first thread
+	 * in the SMT as the primary thread.
+	 */
+	return cpu == cpumask_first(topology_sibling_cpumask(cpu));

How is that supposed to work? Assume both siblings are offline, then the
sibling mask is empty and you can't boot the CPU anymore.

For architectures' using arch_topology, topology_sibling_cpumask() will at least
contain the tested CPU itself. This is initialized in
drivers/base/arch_topology.c:reset_cpu_topology(). So it won't be
empty here.

Fair enough. Can you please expand the comment and say:

     The sibling cpumask of a offline CPU contains always the CPU
     itself.

Besides we don't need to check topology_is_primary_thread() at boot time:
-> cpu_up(cpu)
     cpu_bootable()
       if (cpu_smt_control == CPU_SMT_ENABLED &&
           cpu_smt_thread_allowed(cpu)) // will always return true if !CONFIG_SMT_NUM_THREADS_DYNAMIC
         return true; // we'll always return here and @cpu is always bootable

cpu_smt_control is not guaranteed to have CPU_SMT_ENABLED state, so this
argument is bogus.

Also tested fine in practice.

I've heard that song before.

What matters is not what you tested. What matters is whether the code is
correct _and_ understandable.

Thanks,

        tglx