[PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Nysal Jan K.A.]

From: "Nysal Jan K.A" <nysal@linux.ibm.com>

After the addition of HOTPLUG_SMT support for PowerPC [1] there was a
regression reported [2] when enabling SMT. On a system with at least
one offline core, when enabling SMT, the expectation is that no CPUs
of offline cores are made online.

On a POWER9 system with 4 cores in SMT4 mode:
$ ppc64_cpu --info
Core   0:    0*    1*    2*    3*
Core   1:    4*    5*    6*    7*
Core   2:    8*    9*   10*   11*
Core   3:   12*   13*   14*   15*

Turn only one core on:
$ ppc64_cpu --cores-on=1
$ ppc64_cpu --info
Core   0:    0*    1*    2*    3*
Core   1:    4     5     6     7
Core   2:    8     9    10    11
Core   3:   12    13    14    15

Change the SMT level to 2:
$ ppc64_cpu --smt=2
$ ppc64_cpu --info
Core   0:    0*    1*    2     3
Core   1:    4     5     6     7
Core   2:    8     9    10    11
Core   3:   12    13    14    15

As expected we see only two CPUs of core 0 are online

Change the SMT level to 4:
$ ppc64_cpu --smt=4
$ ppc64_cpu --info
Core   0:    0*    1*    2*    3*
Core   1:    4*    5*    6*    7*
Core   2:    8*    9*   10*   11*
Core   3:   12*   13*   14*   15*

The CPUs of offline cores are made online. If a core is offline then
enabling SMT should not online CPUs of this core. An arch specific
function topology_is_core_online() is proposed to address this.
Another approach is to check the topology_sibling_cpumask() for any
online siblings. This avoids the need for an arch specific function
but is less efficient and more importantly this introduces a change
in existing behaviour on other architectures.

What is the expected behaviour on x86 when enabling SMT and certain cores
are offline? 

[1] https://lore.kernel.org/lkml/20230705145143.40545-1-ldufour@linux.ibm.com/
[2] https://groups.google.com/g/powerpc-utils-devel/c/wrwVzAAnRlI/m/5KJSoqP4BAAJ

Nysal Jan K.A (2):
  cpu/SMT: Enable SMT only if a core is online
  powerpc/topology: Check if a core is online

 arch/powerpc/include/asm/topology.h | 13 +++++++++++++
 kernel/cpu.c                        | 12 +++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)


base-commit: c760b3725e52403dc1b28644fb09c47a83cacea6
-- 
2.35.3

[PATCH 1/2] cpu/SMT: Enable SMT only if a core is online

[Nysal Jan K.A.]

From: "Nysal Jan K.A" <nysal@linux.ibm.com>

If a core is offline then enabling SMT should not online CPUs of
this core. By enabling SMT, what is intended is either changing the SMT
value from "off" to "on" or setting the SMT level (threads per core) from a
lower to higher value.

On PowerPC the ppc64_cpu utility can be used, among other things, to
perform the following functions:

ppc64_cpu --cores-on                # Get the number of online cores
ppc64_cpu --cores-on=X              # Put exactly X cores online
ppc64_cpu --offline-cores=X[,Y,...] # Put specified cores offline
ppc64_cpu --smt={on|off|value}      # Enable, disable or change SMT level

If the user has decided to offline certain cores, enabling SMT should
not online CPUs in those cores. This patch fixes the issue and changes
the behaviour as described, by introducing an arch specific function
topology_is_core_online(). It is currently implemented only for PowerPC.

Fixes: 73c58e7e1412 ("powerpc: Add HOTPLUG_SMT support")
Reported-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Closes: https://groups.google.com/g/powerpc-utils-devel/c/wrwVzAAnRlI/m/5KJSoqP4BAAJ
Signed-off-by: Nysal Jan K.A <nysal@linux.ibm.com>
---
 kernel/cpu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 563877d6c28b..73c4a6f98c4a 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2676,6 +2676,16 @@ int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
 	return ret;
 }
 
+/**
+ * Check if the core a CPU belongs to is online
+ */
+#if !defined(topology_is_core_online)
+static inline bool topology_is_core_online(unsigned int cpu)
+{
+	return true;
+}
+#endif
+
 int cpuhp_smt_enable(void)
 {
 	int cpu, ret = 0;
@@ -2686,7 +2696,7 @@ int cpuhp_smt_enable(void)
 		/* Skip online CPUs and CPUs on offline nodes */
 		if (cpu_online(cpu) || !node_online(cpu_to_node(cpu)))
 			continue;
-		if (!cpu_smt_thread_allowed(cpu))
+		if (!cpu_smt_thread_allowed(cpu) || !topology_is_core_online(cpu))
 			continue;
 		ret = _cpu_up(cpu, 0, CPUHP_ONLINE);
 		if (ret)
-- 
2.35.3

[PATCH 2/2] powerpc/topology: Check if a core is online

[Nysal Jan K.A.]

From: "Nysal Jan K.A" <nysal@linux.ibm.com>

topology_is_core_online() checks if the core a CPU belongs to
is online. The core is online if at least one of the sibling
CPUs is online. The first CPU of an online core is also online
in the common case, so this should be fairly quick.

Signed-off-by: Nysal Jan K.A <nysal@linux.ibm.com>
---
 arch/powerpc/include/asm/topology.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index f4e6f2dd04b7..16bacfe8c7a2 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -145,6 +145,7 @@ static inline int cpu_to_coregroup_id(int cpu)
 
 #ifdef CONFIG_HOTPLUG_SMT
 #include <linux/cpu_smt.h>
+#include <linux/cpumask.h>
 #include <asm/cputhreads.h>
 
 static inline bool topology_is_primary_thread(unsigned int cpu)
@@ -156,6 +157,18 @@ static inline bool topology_smt_thread_allowed(unsigned int cpu)
 {
 	return cpu_thread_in_core(cpu) < cpu_smt_num_threads;
 }
+
+#define topology_is_core_online topology_is_core_online
+static inline bool topology_is_core_online(unsigned int cpu)
+{
+	int i, first_cpu = cpu_first_thread_sibling(cpu);
+
+	for (i = first_cpu; i < first_cpu + threads_per_core; ++i) {
+		if (cpu_online(i))
+			return true;
+	}
+	return false;
+}
 #endif
 
 #endif /* __KERNEL__ */
-- 
2.35.3

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Michael Ellerman]

"Nysal Jan K.A." <nysal@linux.ibm.com> writes:
> From: "Nysal Jan K.A" <nysal@linux.ibm.com>
>
> After the addition of HOTPLUG_SMT support for PowerPC [1] there was a
> regression reported [2] when enabling SMT.

This implies it was a kernel regression. But it can't be a kernel
regression because previously there was no support at all for the sysfs
interface on powerpc.

IIUIC the regression was in the ppc64_cpu userspace tool, which switched
to using the new kernel interface without taking into account the way it
behaves.

Or are you saying the kernel behaviour changed on x86 after the powerpc
HOTPLUG_SMT was added?

> On a system with at least
> one offline core, when enabling SMT, the expectation is that no CPUs
> of offline cores are made online.
>
> On a POWER9 system with 4 cores in SMT4 mode:
> $ ppc64_cpu --info
> Core   0:    0*    1*    2*    3*
> Core   1:    4*    5*    6*    7*
> Core   2:    8*    9*   10*   11*
> Core   3:   12*   13*   14*   15*
>
> Turn only one core on:
> $ ppc64_cpu --cores-on=1
> $ ppc64_cpu --info
> Core   0:    0*    1*    2*    3*
> Core   1:    4     5     6     7
> Core   2:    8     9    10    11
> Core   3:   12    13    14    15
>
> Change the SMT level to 2:
> $ ppc64_cpu --smt=2
> $ ppc64_cpu --info
> Core   0:    0*    1*    2     3
> Core   1:    4     5     6     7
> Core   2:    8     9    10    11
> Core   3:   12    13    14    15
>
> As expected we see only two CPUs of core 0 are online
>
> Change the SMT level to 4:
> $ ppc64_cpu --smt=4
> $ ppc64_cpu --info
> Core   0:    0*    1*    2*    3*
> Core   1:    4*    5*    6*    7*
> Core   2:    8*    9*   10*   11*
> Core   3:   12*   13*   14*   15*
>
> The CPUs of offline cores are made online. If a core is offline then
> enabling SMT should not online CPUs of this core.

That's the way the ppc64_cpu tool behaves, but it's not necessarily what
other arches want.

> An arch specific
> function topology_is_core_online() is proposed to address this.
> Another approach is to check the topology_sibling_cpumask() for any
> online siblings. This avoids the need for an arch specific function
> but is less efficient and more importantly this introduces a change
> in existing behaviour on other architectures.

It's only x86 and powerpc right?

Having different behaviour on the only two arches that support the
interface does not seem like a good result.

> What is the expected behaviour on x86 when enabling SMT and certain cores
> are offline? 

AFAIK no one really touches SMT on x86 other than to turn it off for
security reasons.

cheers

> [1] https://lore.kernel.org/lkml/20230705145143.40545-1-ldufour@linux.ibm.com/
> [2] https://groups.google.com/g/powerpc-utils-devel/c/wrwVzAAnRlI/m/5KJSoqP4BAAJ
>
> Nysal Jan K.A (2):
>   cpu/SMT: Enable SMT only if a core is online
>   powerpc/topology: Check if a core is online
>
>  arch/powerpc/include/asm/topology.h | 13 +++++++++++++
>  kernel/cpu.c                        | 12 +++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
>
> base-commit: c760b3725e52403dc1b28644fb09c47a83cacea6
> -- 
> 2.35.3

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Michal Suchánek]

On Thu, Jun 13, 2024 at 09:34:10PM +1000, Michael Ellerman wrote:
> "Nysal Jan K.A." <nysal@linux.ibm.com> writes:
> > From: "Nysal Jan K.A" <nysal@linux.ibm.com>
> >
> > After the addition of HOTPLUG_SMT support for PowerPC [1] there was a
> > regression reported [2] when enabling SMT.
> 
> This implies it was a kernel regression. But it can't be a kernel
> regression because previously there was no support at all for the sysfs
> interface on powerpc.
> 
> IIUIC the regression was in the ppc64_cpu userspace tool, which switched
> to using the new kernel interface without taking into account the way it
> behaves.

The reported regression is in the ppc64_cpu tool behavior.

> Or are you saying the kernel behaviour changed on x86 after the powerpc
> HOTPLUG_SMT was added?
> 
> > On a system with at least
> > one offline core, when enabling SMT, the expectation is that no CPUs
> > of offline cores are made online.
> >
> > On a POWER9 system with 4 cores in SMT4 mode:
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4*    5*    6*    7*
> > Core   2:    8*    9*   10*   11*
> > Core   3:   12*   13*   14*   15*
> >
> > Turn only one core on:
> > $ ppc64_cpu --cores-on=1
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4     5     6     7
> > Core   2:    8     9    10    11
> > Core   3:   12    13    14    15
> >
> > Change the SMT level to 2:
> > $ ppc64_cpu --smt=2
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2     3
> > Core   1:    4     5     6     7
> > Core   2:    8     9    10    11
> > Core   3:   12    13    14    15
> >
> > As expected we see only two CPUs of core 0 are online
> >
> > Change the SMT level to 4:
> > $ ppc64_cpu --smt=4
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4*    5*    6*    7*
> > Core   2:    8*    9*   10*   11*
> > Core   3:   12*   13*   14*   15*
> >
> > The CPUs of offline cores are made online. If a core is offline then
> > enabling SMT should not online CPUs of this core.
> 
> That's the way the ppc64_cpu tool behaves, but it's not necessarily what
> other arches want.
> 
> > An arch specific
> > function topology_is_core_online() is proposed to address this.
> > Another approach is to check the topology_sibling_cpumask() for any
> > online siblings. This avoids the need for an arch specific function
> > but is less efficient and more importantly this introduces a change
> > in existing behaviour on other architectures.
> 
> It's only x86 and powerpc right?
> 
> Having different behaviour on the only two arches that support the
> interface does not seem like a good result.

That's unfortunate. At the same time changing the x86 behavior would
potentially lead to similar reports from people relying on the old
behavior.

> > What is the expected behaviour on x86 when enabling SMT and certain cores
> > are offline? 
> 
> AFAIK no one really touches SMT on x86 other than to turn it off for
> security reasons.

In particular I am not aware of x86 suporting those middle partially
enabled states. I don't have a x86 4+ way SMT cpu at hand to test, either.

The more likely excuse is that there is little use case for enabling
previously disabled CPUs (whole cores/thread groups) by changing the SMT
state, even if the SMT code happened to do it in the past.

That is, more technically, that the value of 'off' is 1 - 1 thread of
each core is enabled, and another value representing 'core disabled'
with no thread of the core running is to be treated specially, and not
changed when setting the system-wide SMT value.

These are separate concerns, and should be addressed by separate
interfaces.

Thanks

Michal

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Nysal Jan K.A.]

On Thu, Jun 13, 2024 at 09:34:10PM GMT, Michael Ellerman wrote:
> "Nysal Jan K.A." <nysal@linux.ibm.com> writes:
> > From: "Nysal Jan K.A" <nysal@linux.ibm.com>
> >
> > After the addition of HOTPLUG_SMT support for PowerPC [1] there was a
> > regression reported [2] when enabling SMT.
> 
> This implies it was a kernel regression. But it can't be a kernel
> regression because previously there was no support at all for the sysfs
> interface on powerpc.
> 
> IIUIC the regression was in the ppc64_cpu userspace tool, which switched
> to using the new kernel interface without taking into account the way it
> behaves.
> 
> Or are you saying the kernel behaviour changed on x86 after the powerpc
> HOTPLUG_SMT was added?
> 

The regression is in ppc64_cpu. If we need the older behaviour we will need this
or an equivalent change in the kernel though. Fixing it in userspace in an
efficient way might be difficult.

> > On a system with at least
> > one offline core, when enabling SMT, the expectation is that no CPUs
> > of offline cores are made online.
> >
> > On a POWER9 system with 4 cores in SMT4 mode:
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4*    5*    6*    7*
> > Core   2:    8*    9*   10*   11*
> > Core   3:   12*   13*   14*   15*
> >
> > Turn only one core on:
> > $ ppc64_cpu --cores-on=1
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4     5     6     7
> > Core   2:    8     9    10    11
> > Core   3:   12    13    14    15
> >
> > Change the SMT level to 2:
> > $ ppc64_cpu --smt=2
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2     3
> > Core   1:    4     5     6     7
> > Core   2:    8     9    10    11
> > Core   3:   12    13    14    15
> >
> > As expected we see only two CPUs of core 0 are online
> >
> > Change the SMT level to 4:
> > $ ppc64_cpu --smt=4
> > $ ppc64_cpu --info
> > Core   0:    0*    1*    2*    3*
> > Core   1:    4*    5*    6*    7*
> > Core   2:    8*    9*   10*   11*
> > Core   3:   12*   13*   14*   15*
> >
> > The CPUs of offline cores are made online. If a core is offline then
> > enabling SMT should not online CPUs of this core.
> 
> That's the way the ppc64_cpu tool behaves, but it's not necessarily what
> other arches want.
> 

True, but from a user perspective it seems logical though. I think one can make
a case for either behaviour. 

> > An arch specific
> > function topology_is_core_online() is proposed to address this.
> > Another approach is to check the topology_sibling_cpumask() for any
> > online siblings. This avoids the need for an arch specific function
> > but is less efficient and more importantly this introduces a change
> > in existing behaviour on other architectures.
> 
> It's only x86 and powerpc right?
> 
> Having different behaviour on the only two arches that support the
> interface does not seem like a good result.
> 

Agree, I was originally thinking of sending out a patch changing this for both
architectures, but was unsure if there might be users who now depend on this
behaviour on x86.

> > What is the expected behaviour on x86 when enabling SMT and certain cores
> > are offline? 
> 
> AFAIK no one really touches SMT on x86 other than to turn it off for
> security reasons.
> 
> cheers
> 

Thanks for your comments. It will be good to hear if changing this behaviour
for both x86 and PowerPC might be an acceptable path forward.

Regards
--Nysal

> > [1] https://lore.kernel.org/lkml/20230705145143.40545-1-ldufour@linux.ibm.com/
> > [2] https://groups.google.com/g/powerpc-utils-devel/c/wrwVzAAnRlI/m/5KJSoqP4BAAJ
> >
> > Nysal Jan K.A (2):
> >   cpu/SMT: Enable SMT only if a core is online
> >   powerpc/topology: Check if a core is online
> >
> >  arch/powerpc/include/asm/topology.h | 13 +++++++++++++
> >  kernel/cpu.c                        | 12 +++++++++++-
> >  2 files changed, 24 insertions(+), 1 deletion(-)
> >
> >
> > base-commit: c760b3725e52403dc1b28644fb09c47a83cacea6
> > -- 
> > 2.35.3

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Thomas Gleixner]

Michael!

On Thu, Jun 13 2024 at 21:34, Michael Ellerman wrote:
> IIUIC the regression was in the ppc64_cpu userspace tool, which switched
> to using the new kernel interface without taking into account the way it
> behaves.
>
> Or are you saying the kernel behaviour changed on x86 after the powerpc
> HOTPLUG_SMT was added?

No. The mechanism was always this way. Only offline nodes have been
skipped. x86 never checked for the core being online.

> It's only x86 and powerpc right?
>
> Having different behaviour on the only two arches that support the
> interface does not seem like a good result.
>
>> What is the expected behaviour on x86 when enabling SMT and certain cores
>> are offline? 
>
> AFAIK no one really touches SMT on x86 other than to turn it off for
> security reasons.

Right. So changing it not to online a thread when the full core is
offline should not really break stuff.

OTH, the mechanism to figure that out on x86 is definitely different and
more complicated than on power because the sibling threads are not
having consecutive CPU numbers.

So I'm not sure whether it's worth to make this consistent and I
definitely can live with the proposed patches.

Thanks,

        tglx

Re: [PATCH 2/2] powerpc/topology: Check if a core is online

[Shrikanth Hegde]

On 6/13/24 12:20 AM, Nysal Jan K.A. wrote:
> From: "Nysal Jan K.A" <nysal@linux.ibm.com>
> 
> topology_is_core_online() checks if the core a CPU belongs to
> is online. The core is online if at least one of the sibling
> CPUs is online. The first CPU of an online core is also online
> in the common case, so this should be fairly quick.
> 
> Signed-off-by: Nysal Jan K.A <nysal@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/topology.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index f4e6f2dd04b7..16bacfe8c7a2 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -145,6 +145,7 @@ static inline int cpu_to_coregroup_id(int cpu)
>  
>  #ifdef CONFIG_HOTPLUG_SMT
>  #include <linux/cpu_smt.h>
> +#include <linux/cpumask.h>

Is this header file needed? 
I don't see any reference to cpumask related code. 

>  #include <asm/cputhreads.h>
>  
>  static inline bool topology_is_primary_thread(unsigned int cpu)
> @@ -156,6 +157,18 @@ static inline bool topology_smt_thread_allowed(unsigned int cpu)
>  {
>  	return cpu_thread_in_core(cpu) < cpu_smt_num_threads;
>  }
> +

This is defined only if CONFIG_HOTPLUG_SMT is true. But this could be 
generic function which might be used to check if a core is offline in other cases. 
Would that make sense to keep it out of CONFIG_HOTPLUG_SMT ?

> +#define topology_is_core_online topology_is_core_online
> +static inline bool topology_is_core_online(unsigned int cpu)
> +{
> +	int i, first_cpu = cpu_first_thread_sibling(cpu);
> +
> +	for (i = first_cpu; i < first_cpu + threads_per_core; ++i) {
> +		if (cpu_online(i))
> +			return true;
> +	}
> +	return false;
> +}
>  #endif
>  
>  #endif /* __KERNEL__ */

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Shrikanth Hegde]

On 6/24/24 1:44 AM, Thomas Gleixner wrote:
> Michael!
> 
> On Thu, Jun 13 2024 at 21:34, Michael Ellerman wrote:
>> IIUIC the regression was in the ppc64_cpu userspace tool, which switched
>> to using the new kernel interface without taking into account the way it
>> behaves.
>>
>> Or are you saying the kernel behaviour changed on x86 after the powerpc
>> HOTPLUG_SMT was added?
> 
> No. The mechanism was always this way. Only offline nodes have been
> skipped. x86 never checked for the core being online.
> 
>> It's only x86 and powerpc right?
>>
>> Having different behaviour on the only two arches that support the
>> interface does not seem like a good result.
>>
>>> What is the expected behaviour on x86 when enabling SMT and certain cores
>>> are offline? 
>>
>> AFAIK no one really touches SMT on x86 other than to turn it off for
>> security reasons.
> 
> Right. So changing it not to online a thread when the full core is
> offline should not really break stuff.
> 
> OTH, the mechanism to figure that out on x86 is definitely different and
> more complicated than on power because the sibling threads are not
> having consecutive CPU numbers.

wouldn't topology_sibling_cpumask have this info? 
If the mask is empty does it mean the core is offline? 

> 
> So I'm not sure whether it's worth to make this consistent and I
> definitely can live with the proposed patches.
> 
> Thanks,
> 
>         tglx

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Thomas Gleixner]

On Tue, Jun 25 2024 at 00:41, Shrikanth Hegde wrote:
> On 6/24/24 1:44 AM, Thomas Gleixner wrote:
>> Right. So changing it not to online a thread when the full core is
>> offline should not really break stuff.
>> 
>> OTH, the mechanism to figure that out on x86 is definitely different and
>> more complicated than on power because the sibling threads are not
>> having consecutive CPU numbers.
>
> wouldn't topology_sibling_cpumask have this info? 
> If the mask is empty does it mean the core is offline? 

The mask is not yet available for the to be brought up CPU. That's
established when the CPU boots. It might work because all threads are
brought up during early boot for !~*&^!@% reasons, but then it won't
work under all circumstances, e.g. 'maxcpus=$N'.

We could fix that now with the new topology enumeration code, but that's
a larger scale project.

Thanks,

        tglx

Re: [PATCH 2/2] powerpc/topology: Check if a core is online

[Nysal Jan K.A.]

On Tue, Jun 25, 2024 at 12:36:33AM GMT, Shrikanth Hegde wrote:
> > --- a/arch/powerpc/include/asm/topology.h
> > +++ b/arch/powerpc/include/asm/topology.h
> > @@ -145,6 +145,7 @@ static inline int cpu_to_coregroup_id(int cpu)
> >  
> >  #ifdef CONFIG_HOTPLUG_SMT
> >  #include <linux/cpu_smt.h>
> > +#include <linux/cpumask.h>
> 
> Is this header file needed? 
> I don't see any reference to cpumask related code. 
> 

cpu_online() is defined in that header.

> >  #include <asm/cputhreads.h>
> >  
> >  static inline bool topology_is_primary_thread(unsigned int cpu)
> > @@ -156,6 +157,18 @@ static inline bool topology_smt_thread_allowed(unsigned int cpu)
> >  {
> >  	return cpu_thread_in_core(cpu) < cpu_smt_num_threads;
> >  }
> > +
> 
> This is defined only if CONFIG_HOTPLUG_SMT is true. But this could be 
> generic function which might be used to check if a core is offline in other cases. 
> Would that make sense to keep it out of CONFIG_HOTPLUG_SMT ?
> 

I'm not opposed to the idea, it would also be easy to do that later if there is
another consumer of this function.

Regards
--Nysal

Re: [PATCH 0/2] Skip offline cores when enabling SMT on PowerPC

[Shrikanth Hegde]

On 6/25/24 2:54 AM, Thomas Gleixner wrote:
> On Tue, Jun 25 2024 at 00:41, Shrikanth Hegde wrote:
>> On 6/24/24 1:44 AM, Thomas Gleixner wrote:
>>> Right. So changing it not to online a thread when the full core is
>>> offline should not really break stuff.
>>>
>>> OTH, the mechanism to figure that out on x86 is definitely different and
>>> more complicated than on power because the sibling threads are not
>>> having consecutive CPU numbers.
>>
>> wouldn't topology_sibling_cpumask have this info? 
>> If the mask is empty does it mean the core is offline? 
> 
> The mask is not yet available for the to be brought up CPU. That's
> established when the CPU boots. It might work because all threads are
> brought up during early boot for !~*&^!@% reasons, but then it won't
> work under all circumstances, e.g. 'maxcpus=$N'.
> 
> We could fix that now with the new topology enumeration code, but that's
> a larger scale project.

Ok. 

Can we please document the behavior on different platforms i.e on x86 and PowerPC? 
May be in ABI/testing/sysfs-devices-system-cpu?