Re: [RFC PATCH 1/2] cpu/hotplug: Skip disabled CPUs in cpuhp_smt_enable

[Thomas Gleixner]

On Tue, Sep 29 2026 at 19:41, salil.mehta@xxxxxxxxxx wrote:
> From: Salil Mehta <salil.mehta@xxxxxxxxxx>
>
> The CPU enabled mask describes whether a present CPU may currently be
> brought online. cpuhp_smt_enable() is an online operation, but currently
> walks all present CPUs and only filters CPUs that are already online or
> belong to offline NUMA nodes.
>
> This can make it attempt _cpu_up() for a present CPU which firmware has
> not enabled and which has not yet been registered as a CPU device.
>
> Use the enabled mask for the policy decision it was introduced to
> represent. This keeps present-but-disabled CPUs out of the SMT bring-up
> path while still allowing registered offline SMT threads to be brought
> back online.
>
> Present and enabled are separate generic CPU states, so callers which
> intend to bring CPUs online should not assume that every present CPU is
> enabled.

The 'enabled mask' was introduced two years ago to deal with the ARM64
oddity of present, but disabled CPUs, which nobody else needs. So yes,
it's generic in some sense, but that's not the argument to be made here.

The argument is that present CPUs might not be set in the enabled mask
and therefore cannot be brought online.

> Signed-off-by: Salil Mehta <salil.mehta@xxxxxxxxxx>
> ---
>  kernel/cpu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index b3c8553d7bd6..988b7a1e8298 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2706,8 +2706,9 @@ int cpuhp_smt_enable(void)
>  	cpu_maps_update_begin();
>  	cpu_smt_control = CPU_SMT_ENABLED;
>  	for_each_present_cpu(cpu) {
> -		/* Skip online CPUs and CPUs on offline nodes */
> -		if (cpu_online(cpu) || !node_online(cpu_to_node(cpu)))
> +		/* Skip online/disabled CPUs and CPUs on offline nodes */
> +		if (cpu_online(cpu) || !cpu_enabled(cpu) ||
> +		    !node_online(cpu_to_node(cpu)))

Seriously?

What's wrong with the obvious one-liner:

-  	for_each_present_cpu(cpu) {
+  	for_each_enabled_cpu(cpu) {

That would be too self explaining, right?

But that change is only a small subset of the overall story because now
the hotplug core looks at enabled mask in one place while all the other
places still utilize present mask.

With your second patch, that will then try to bring up present, but
disabled CPUs in bringup_nonboot_cpus(), which might be a silent fail or
not. The other instances which use present mask are probably silently
skipped due to the hotplug state.

Just because you did not observe a warning due to that does not mean
it's correct.

Thanks,

        tglx