[patch V2 00/12] cpu/hotplug: SMT control

[patch V2 00/12] cpu/hotplug: SMT control

[Thomas Gleixner]

The following series is a reworked version of the initial proof of concept
patch. The main changes are:

  - The primary sibling evaluation has been changed to use APIC ID, so the
    hacky stuff is gone.

  - The control has now 3 states: on, off, forceoff

    forceoff is a irreversible operation and if given on the command line
    via 'nosmt=force' it makes the processor/APIC enumeration code discard
    the non primary siblings. That affects also the number of possible CPUs
    and is more or less equivalent to disabling SMT in the BIOS.

    If 'forceoff' is written to the sysfs file, then the non primamry
    siblings are offlined as with 'off', but the operation cannot be
    undone. That has obviously no effect on num_possible_cpus as that has
    been evaluated in the early boot process.

  - Command line and sysfs interface are documented

Survived testing on various Intel and AMD machines and in VMs of different
flavours and topology configurations.

Applies on top of Linus tree.

Thanks,

	tglx

---
 Documentation/ABI/testing/sysfs-devices-system-cpu |   16 ++
 Documentation/admin-guide/kernel-parameters.txt    |    4 
 arch/Kconfig                                       |    3 
 arch/x86/include/asm/apic.h                        |    8 +
 arch/x86/include/asm/topology.h                    |    4 
 arch/x86/kernel/acpi/boot.c                        |    3 
 arch/x86/kernel/apic/apic.c                        |   34 ++++
 arch/x86/kernel/cpu/amd.c                          |   18 +-
 arch/x86/kernel/cpu/common.c                       |   36 ++--
 arch/x86/kernel/cpu/cpu.h                          |    2 
 arch/x86/kernel/cpu/intel.c                        |    7 
 arch/x86/kernel/cpu/topology.c                     |   41 +++--
 arch/x86/kernel/smpboot.c                          |    9 +
 b/arch/x86/Kconfig                                 |    1 
 include/linux/cpu.h                                |   12 +
 kernel/cpu.c                                       |  152 +++++++++++++++++++--
 kernel/sched/core.c                                |   32 +---
 kernel/sched/fair.c                                |    1 
 18 files changed, 308 insertions(+), 75 deletions(-)

[patch V2 01/12] sched/smt: Update sched_smt_present at runtime

[Thomas Gleixner]

Subject: [patch V2 01/12] sched/smt: Update sched_smt_present at runtime
From: Peter Zijlstra <peterz@infradead.org>

The static key sched_smt_present is only updated at boot time when SMT
siblings have been deteced. Booting with maxcpus=1 and bringing the
siblings online after boot rebuilds the scheduling domains correctly but
does not update the static key, so the SMT code is not enabled.

Let the key update in the scheduler CPU hotplug code to fix this.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c |   32 ++++++++++++++------------------
 kernel/sched/fair.c |    1 +
 2 files changed, 15 insertions(+), 18 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5771,6 +5771,20 @@ int sched_cpu_activate(unsigned int cpu)
 	struct rq *rq = cpu_rq(cpu);
 	struct rq_flags rf;
 
+#ifdef CONFIG_SCHED_SMT
+	/*
+	 * We can't rely on the sched domains themselves to reliably inform us
+	 * of SMT, but if we ever see a CPU with siblings pass by, enable the
+	 * SMT code.
+	 *
+	 * The failure case for domains is when we boot with SMT disabled, then
+	 * create partitions that split all cores and then hotplug the
+	 * siblings. In that case we'll never build an SMT domain.
+	 */
+	if (cpumask_weight(cpu_smt_mask(cpu)) > 1)
+		static_branch_enable_cpuslocked(&sched_smt_present);
+#endif
+
 	set_cpu_active(cpu, true);
 
 	if (sched_smp_initialized) {
@@ -5868,22 +5882,6 @@ int sched_cpu_dying(unsigned int cpu)
 }
 #endif
 
-#ifdef CONFIG_SCHED_SMT
-DEFINE_STATIC_KEY_FALSE(sched_smt_present);
-
-static void sched_init_smt(void)
-{
-	/*
-	 * We've enumerated all CPUs and will assume that if any CPU
-	 * has SMT siblings, CPU0 will too.
-	 */
-	if (cpumask_weight(cpu_smt_mask(0)) > 1)
-		static_branch_enable(&sched_smt_present);
-}
-#else
-static inline void sched_init_smt(void) { }
-#endif
-
 void __init sched_init_smp(void)
 {
 	sched_init_numa();
@@ -5905,8 +5903,6 @@ void __init sched_init_smp(void)
 	init_sched_rt_class();
 	init_sched_dl_class();
 
-	sched_init_smt();
-
 	sched_smp_initialized = true;
 }
 
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6238,6 +6238,7 @@ static inline int find_idlest_cpu(struct
 }
 
 #ifdef CONFIG_SCHED_SMT
+DEFINE_STATIC_KEY_FALSE(sched_smt_present);
 
 static inline void set_idle_cores(int cpu, int val)
 {

[MODERATED] Re: [patch V2 01/12] sched/smt: Update sched_smt_present at runtime

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:15PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 01/12] sched/smt: Update sched_smt_present at runtime
> From: Peter Zijlstra <peterz@infradead.org>
> 
> The static key sched_smt_present is only updated at boot time when SMT
> siblings have been deteced. Booting with maxcpus=1 and bringing the
> siblings online after boot rebuilds the scheduling domains correctly but
> does not update the static key, so the SMT code is not enabled.
> 
> Let the key update in the scheduler CPU hotplug code to fix this.
> 
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

with one question below (not that matters much, but):
> ---
>  kernel/sched/core.c |   32 ++++++++++++++------------------
>  kernel/sched/fair.c |    1 +
>  2 files changed, 15 insertions(+), 18 deletions(-)
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5771,6 +5771,20 @@ int sched_cpu_activate(unsigned int cpu)
>  	struct rq *rq = cpu_rq(cpu);
>  	struct rq_flags rf;
>  
> +#ifdef CONFIG_SCHED_SMT
> +	/*
> +	 * We can't rely on the sched domains themselves to reliably inform us
> +	 * of SMT, but if we ever see a CPU with siblings pass by, enable the
> +	 * SMT code.
> +	 *
> +	 * The failure case for domains is when we boot with SMT disabled, then
> +	 * create partitions that split all cores and then hotplug the

Don't you nee to first hotplug the siblings, then create the partitions?

Or did you mean 'hotplug' == smp_callin?

> +	 * siblings. In that case we'll never build an SMT domain.
> +	 */
> +	if (cpumask_weight(cpu_smt_mask(cpu)) > 1)
> +		static_branch_enable_cpuslocked(&sched_smt_present);
> +#endif
> +
>  	set_cpu_active(cpu, true);
>  
>  	if (sched_smp_initialized) {
> @@ -5868,22 +5882,6 @@ int sched_cpu_dying(unsigned int cpu)
>  }
>  #endif
>  
> -#ifdef CONFIG_SCHED_SMT
> -DEFINE_STATIC_KEY_FALSE(sched_smt_present);
> -
> -static void sched_init_smt(void)
> -{
> -	/*
> -	 * We've enumerated all CPUs and will assume that if any CPU
> -	 * has SMT siblings, CPU0 will too.
> -	 */
> -	if (cpumask_weight(cpu_smt_mask(0)) > 1)
> -		static_branch_enable(&sched_smt_present);
> -}
> -#else
> -static inline void sched_init_smt(void) { }
> -#endif
> -
>  void __init sched_init_smp(void)
>  {
>  	sched_init_numa();
> @@ -5905,8 +5903,6 @@ void __init sched_init_smp(void)
>  	init_sched_rt_class();
>  	init_sched_dl_class();
>  
> -	sched_init_smt();
> -
>  	sched_smp_initialized = true;
>  }
>  
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6238,6 +6238,7 @@ static inline int find_idlest_cpu(struct
>  }
>  
>  #ifdef CONFIG_SCHED_SMT
> +DEFINE_STATIC_KEY_FALSE(sched_smt_present);
>  
>  static inline void set_idle_cores(int cpu, int val)
>  {
> 

Re: [patch V2 01/12] sched/smt: Update sched_smt_present at runtime

[Thomas Gleixner]

On Mon, 11 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> > +#ifdef CONFIG_SCHED_SMT
> > +	/*
> > +	 * We can't rely on the sched domains themselves to reliably inform us
> > +	 * of SMT, but if we ever see a CPU with siblings pass by, enable the
> > +	 * SMT code.
> > +	 *
> > +	 * The failure case for domains is when we boot with SMT disabled, then
> > +	 * create partitions that split all cores and then hotplug the
> 
> Don't you nee to first hotplug the siblings, then create the partitions?
> 
> Or did you mean 'hotplug' == smp_callin?

The sched domains are rebuilt when a CPU is plugged. It's done in the
scheduler hotplug function which is called on the upcoming CPU.

The current code has the following issue:

When you boot a machine with maxcpus=1, then the scheduler init code will
not see a sibling and not enable the static key.

When you plug a sibling CPU later on, then the domain rebuild will create
the proper SMT aware domains, but the static key stays false. As a
consequence SMT scheduling wont work correctly.

That's why the scheduler hotplug function needs to fiddle with the static
key as well.

Thanks,

	tglx

[MODERATED] Re: [patch V2 01/12] sched/smt: Update sched_smt_present at runtime

[Peter Zijlstra]

On Mon, Jun 11, 2018 at 02:35:08PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> > +#ifdef CONFIG_SCHED_SMT
> > +	/*
> > +	 * We can't rely on the sched domains themselves to reliably inform us
> > +	 * of SMT, but if we ever see a CPU with siblings pass by, enable the
> > +	 * SMT code.
> > +	 *
> > +	 * The failure case for domains is when we boot with SMT disabled, then
> > +	 * create partitions that split all cores and then hotplug the
> 
> Don't you nee to first hotplug the siblings, then create the partitions?

Not with things like isolcpus. Suppose 0-9 are SMT0 and 10-19 are SMT1
and boot with isolcpus=10-19.

The actual sched domains that result from that will not contain a
complete core and thus would not enable the key.

I'm not entirely sure if the scenario from the comment is possible;
that would require cpusets to be created for offline CPUs.

But the basic point remains the same, it is possible to construct
domains such that no single core is 'complete' and it will thus not
observe SMT (through the domains).

[patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Thomas Gleixner]

Subject: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()
From: Thomas Gleixner <tglx@linutronix.de>

If the CPU is supporting SMT then the primary thread can be found by
checking the lower APIC ID bits for zero. smp_num_siblings is used to build
the mask for the APIC ID bits which need to be taken into account.

This uses the MPTABLE or ACPI/MADT supplied APIC ID, which can be different
than the initial APIC ID in CPUID. But according to AMD the lower bits have
to be consistent. Intel gave a tentative confirmation as well.

Preparatory patch to support disabling SMT at boot/runtime.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/apic.h     |    6 ++++++
 arch/x86/include/asm/topology.h |    4 +++-
 arch/x86/kernel/apic/apic.c     |   15 +++++++++++++++
 arch/x86/kernel/smpboot.c       |    9 +++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -500,6 +500,12 @@ extern int default_check_phys_apicid_pre
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+#ifdef CONFIG_SMP
+bool apic_id_is_primary_thread(unsigned int id);
+#else
+static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
+#endif
+
 extern void irq_enter(void);
 extern void irq_exit(void);
 
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -123,13 +123,15 @@ static inline int topology_max_smt_threa
 }
 
 int topology_update_package_map(unsigned int apicid, unsigned int cpu);
-extern int topology_phys_to_logical_pkg(unsigned int pkg);
+int topology_phys_to_logical_pkg(unsigned int pkg);
+bool topology_is_primary_thread(unsigned int cpu);
 #else
 #define topology_max_packages()			(1)
 static inline int
 topology_update_package_map(unsigned int apicid, unsigned int cpu) { return 0; }
 static inline int topology_phys_to_logical_pkg(unsigned int pkg) { return 0; }
 static inline int topology_max_smt_threads(void) { return 1; }
+static inline bool topology_is_primary_thread(unsigned int cpu) { return true; ]
 #endif
 
 static inline void arch_fix_phys_package_id(int num, u32 slot)
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2189,6 +2189,21 @@ static int cpuid_to_apicid[] = {
 	[0 ... NR_CPUS - 1] = -1,
 };
 
+/**
+ * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread
+ * @id:	APIC ID to check
+ */
+bool apic_id_is_primary_thread(unsigned int apicid)
+{
+	u32 mask;
+
+	if (smp_num_siblings == 1)
+		return true;
+	/* Isolate the SMT bit(s) in the APICID and check for 0 */
+	mask = (1U << fls(smp_num_siblings) - 1) - 1;
+	return !(apicid & mask);
+}
+
 /*
  * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
  * and cpuid_to_apicid[] synchronized.
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -266,6 +266,15 @@ static void notrace start_secondary(void
 }
 
 /**
+ * topology_is_primary_thread - Check whether CPU is the primary SMT thread
+ * @cpu:	CPU to check
+ */
+bool topology_is_primary_thread(unsigned int cpu)
+{
+	return apic_id_is_primary_thread(per_cpu(x86_cpu_to_apicid, cpu));
+}
+
+/**
  * topology_phys_to_logical_pkg - Map a physical package id to a logical
  *
  * Returns logical package id or -1 if not found

[MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> If the CPU is supporting SMT then the primary thread can be found by
> checking the lower APIC ID bits for zero. smp_num_siblings is used to build
> the mask for the APIC ID bits which need to be taken into account.
> 
> This uses the MPTABLE or ACPI/MADT supplied APIC ID, which can be different
> than the initial APIC ID in CPUID. But according to AMD the lower bits have
> to be consistent. Intel gave a tentative confirmation as well.
> 
> Preparatory patch to support disabling SMT at boot/runtime.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/apic.h     |    6 ++++++
>  arch/x86/include/asm/topology.h |    4 +++-
>  arch/x86/kernel/apic/apic.c     |   15 +++++++++++++++
>  arch/x86/kernel/smpboot.c       |    9 +++++++++
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -500,6 +500,12 @@ extern int default_check_phys_apicid_pre
>  
>  #endif /* CONFIG_X86_LOCAL_APIC */
>  
> +#ifdef CONFIG_SMP
> +bool apic_id_is_primary_thread(unsigned int id);
> +#else
> +static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
> +#endif
> +
>  extern void irq_enter(void);
>  extern void irq_exit(void);
>  
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -123,13 +123,15 @@ static inline int topology_max_smt_threa
>  }
>  
>  int topology_update_package_map(unsigned int apicid, unsigned int cpu);
> -extern int topology_phys_to_logical_pkg(unsigned int pkg);
> +int topology_phys_to_logical_pkg(unsigned int pkg);
> +bool topology_is_primary_thread(unsigned int cpu);
>  #else
>  #define topology_max_packages()			(1)
>  static inline int
>  topology_update_package_map(unsigned int apicid, unsigned int cpu) { return 0; }
>  static inline int topology_phys_to_logical_pkg(unsigned int pkg) { return 0; }
>  static inline int topology_max_smt_threads(void) { return 1; }
> +static inline bool topology_is_primary_thread(unsigned int cpu) { return true; ]
>  #endif
>  
>  static inline void arch_fix_phys_package_id(int num, u32 slot)
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2189,6 +2189,21 @@ static int cpuid_to_apicid[] = {
>  	[0 ... NR_CPUS - 1] = -1,
>  };
>  
> +/**
> + * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread
> + * @id:	APIC ID to check
> + */
> +bool apic_id_is_primary_thread(unsigned int apicid)
> +{
> +	u32 mask;
> +
> +	if (smp_num_siblings == 1)
> +		return true;
> +	/* Isolate the SMT bit(s) in the APICID and check for 0 */
> +	mask = (1U << fls(smp_num_siblings) - 1) - 1;

So I was thinking - this looks right with 2 siblings (or 1), but
what if there are four siblings (and here please stop me - b/c there
are probably other assumptions made in the code base where things
would break if you have say 3, 5, or 4 siblings).

Anyhow if we did have four siblings, the mask would end up with 11b
(see below):

smp_num_siblings	fls	-1	1<<	 -1
	1 (001b)	1	0	1	0
	2 (010b)	2	1	2	01b
	4 (100b)	3	2	4	11b


> +	return !(apicid & mask);

So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:

	0x76	01110110b & 01 -> !0 -> return 1
	0x77	01110111b & 01 -> !1 -> return 0

that looks good.

If this is a 4 thread. APICID 0x4, 0x5, and 0x6

	0x4	0100 & 11b -> 00 -> !0 -> return 1
	0x5	0101 & 11b -> 01 -> !1 -> return 0 
	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]

That is the 3rd sibling would be incorrectly identified as primary thread.

I think? I would welcome somebody checking my logic over, but then
perhaps this is non-issue-b/c-we-don-do more than 2 so go away.

> +}
> +
>  /*
>   * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
>   * and cpuid_to_apicid[] synchronized.
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -266,6 +266,15 @@ static void notrace start_secondary(void
>  }
>  
>  /**
> + * topology_is_primary_thread - Check whether CPU is the primary SMT thread
> + * @cpu:	CPU to check
> + */
> +bool topology_is_primary_thread(unsigned int cpu)
> +{
> +	return apic_id_is_primary_thread(per_cpu(x86_cpu_to_apicid, cpu));
> +}
> +
> +/**
>   * topology_phys_to_logical_pkg - Map a physical package id to a logical
>   *
>   * Returns logical package id or -1 if not found
> 

[MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Konrad Rzeszutek Wilk]

On Mon, Jun 11, 2018 at 03:32:45PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
> > Subject: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > If the CPU is supporting SMT then the primary thread can be found by
> > checking the lower APIC ID bits for zero. smp_num_siblings is used to build
> > the mask for the APIC ID bits which need to be taken into account.
> > 
> > This uses the MPTABLE or ACPI/MADT supplied APIC ID, which can be different
> > than the initial APIC ID in CPUID. But according to AMD the lower bits have
> > to be consistent. Intel gave a tentative confirmation as well.
> > 
> > Preparatory patch to support disabling SMT at boot/runtime.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/include/asm/apic.h     |    6 ++++++
> >  arch/x86/include/asm/topology.h |    4 +++-
> >  arch/x86/kernel/apic/apic.c     |   15 +++++++++++++++
> >  arch/x86/kernel/smpboot.c       |    9 +++++++++
> >  4 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > --- a/arch/x86/include/asm/apic.h
> > +++ b/arch/x86/include/asm/apic.h
> > @@ -500,6 +500,12 @@ extern int default_check_phys_apicid_pre
> >  
> >  #endif /* CONFIG_X86_LOCAL_APIC */
> >  
> > +#ifdef CONFIG_SMP
> > +bool apic_id_is_primary_thread(unsigned int id);
> > +#else
> > +static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
> > +#endif
> > +
> >  extern void irq_enter(void);
> >  extern void irq_exit(void);
> >  
> > --- a/arch/x86/include/asm/topology.h
> > +++ b/arch/x86/include/asm/topology.h
> > @@ -123,13 +123,15 @@ static inline int topology_max_smt_threa
> >  }
> >  
> >  int topology_update_package_map(unsigned int apicid, unsigned int cpu);
> > -extern int topology_phys_to_logical_pkg(unsigned int pkg);
> > +int topology_phys_to_logical_pkg(unsigned int pkg);
> > +bool topology_is_primary_thread(unsigned int cpu);
> >  #else
> >  #define topology_max_packages()			(1)
> >  static inline int
> >  topology_update_package_map(unsigned int apicid, unsigned int cpu) { return 0; }
> >  static inline int topology_phys_to_logical_pkg(unsigned int pkg) { return 0; }
> >  static inline int topology_max_smt_threads(void) { return 1; }
> > +static inline bool topology_is_primary_thread(unsigned int cpu) { return true; ]
> >  #endif
> >  
> >  static inline void arch_fix_phys_package_id(int num, u32 slot)
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -2189,6 +2189,21 @@ static int cpuid_to_apicid[] = {
> >  	[0 ... NR_CPUS - 1] = -1,
> >  };
> >  
> > +/**
> > + * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread
> > + * @id:	APIC ID to check
> > + */
> > +bool apic_id_is_primary_thread(unsigned int apicid)
> > +{
> > +	u32 mask;
> > +
> > +	if (smp_num_siblings == 1)
> > +		return true;
> > +	/* Isolate the SMT bit(s) in the APICID and check for 0 */
> > +	mask = (1U << fls(smp_num_siblings) - 1) - 1;
> 
> So I was thinking - this looks right with 2 siblings (or 1), but
> what if there are four siblings (and here please stop me - b/c there
> are probably other assumptions made in the code base where things
> would break if you have say 3, 5, or 4 siblings).
> 
> Anyhow if we did have four siblings, the mask would end up with 11b
> (see below):
> 
> smp_num_siblings	fls	-1	1<<	 -1
> 	1 (001b)	1	0	1	0
> 	2 (010b)	2	1	2	01b
> 	4 (100b)	3	2	4	11b
> 
> 
> > +	return !(apicid & mask);
> 
> So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:
> 
> 	0x76	01110110b & 01 -> !0 -> return 1
> 	0x77	01110111b & 01 -> !1 -> return 0
> 
> that looks good.
> 
> If this is a 4 thread. APICID 0x4, 0x5, and 0x6
> 
> 	0x4	0100 & 11b -> 00 -> !0 -> return 1
> 	0x5	0101 & 11b -> 01 -> !1 -> return 0 
> 	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]
> 
> That is the 3rd sibling would be incorrectly identified as primary thread.
> 
> I think? I would welcome somebody checking my logic over, but then
> perhaps this is non-issue-b/c-we-don-do more than 2 so go away.

Or just add:

	BUG_ON(smp_num_siblings > 2);

And all is good and we can touch this when when this goes up.
> 
> > +}
> > +
> >  /*
> >   * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
> >   * and cpuid_to_apicid[] synchronized.
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -266,6 +266,15 @@ static void notrace start_secondary(void
> >  }
> >  
> >  /**
> > + * topology_is_primary_thread - Check whether CPU is the primary SMT thread
> > + * @cpu:	CPU to check
> > + */
> > +bool topology_is_primary_thread(unsigned int cpu)
> > +{
> > +	return apic_id_is_primary_thread(per_cpu(x86_cpu_to_apicid, cpu));
> > +}
> > +
> > +/**
> >   * topology_phys_to_logical_pkg - Map a physical package id to a logical
> >   *
> >   * Returns logical package id or -1 if not found
> > 

[MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Andrew Cooper]

On 11/06/18 21:15, speck for Konrad Rzeszutek Wilk wrote:
> On Mon, Jun 11, 2018 at 03:32:45PM -0400, speck for Konrad Rzeszutek Wilk wrote:
>> On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
>>> Subject: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()
>>> From: Thomas Gleixner <tglx@linutronix.de>
>>>
>>> If the CPU is supporting SMT then the primary thread can be found by
>>> checking the lower APIC ID bits for zero. smp_num_siblings is used to build
>>> the mask for the APIC ID bits which need to be taken into account.
>>>
>>> This uses the MPTABLE or ACPI/MADT supplied APIC ID, which can be different
>>> than the initial APIC ID in CPUID. But according to AMD the lower bits have
>>> to be consistent. Intel gave a tentative confirmation as well.
>>>
>>> Preparatory patch to support disabling SMT at boot/runtime.
>>>
>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>>  arch/x86/include/asm/apic.h     |    6 ++++++
>>>  arch/x86/include/asm/topology.h |    4 +++-
>>>  arch/x86/kernel/apic/apic.c     |   15 +++++++++++++++
>>>  arch/x86/kernel/smpboot.c       |    9 +++++++++
>>>  4 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> --- a/arch/x86/include/asm/apic.h
>>> +++ b/arch/x86/include/asm/apic.h
>>> @@ -500,6 +500,12 @@ extern int default_check_phys_apicid_pre
>>>  
>>>  #endif /* CONFIG_X86_LOCAL_APIC */
>>>  
>>> +#ifdef CONFIG_SMP
>>> +bool apic_id_is_primary_thread(unsigned int id);
>>> +#else
>>> +static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
>>> +#endif
>>> +
>>>  extern void irq_enter(void);
>>>  extern void irq_exit(void);
>>>  
>>> --- a/arch/x86/include/asm/topology.h
>>> +++ b/arch/x86/include/asm/topology.h
>>> @@ -123,13 +123,15 @@ static inline int topology_max_smt_threa
>>>  }
>>>  
>>>  int topology_update_package_map(unsigned int apicid, unsigned int cpu);
>>> -extern int topology_phys_to_logical_pkg(unsigned int pkg);
>>> +int topology_phys_to_logical_pkg(unsigned int pkg);
>>> +bool topology_is_primary_thread(unsigned int cpu);
>>>  #else
>>>  #define topology_max_packages()			(1)
>>>  static inline int
>>>  topology_update_package_map(unsigned int apicid, unsigned int cpu) { return 0; }
>>>  static inline int topology_phys_to_logical_pkg(unsigned int pkg) { return 0; }
>>>  static inline int topology_max_smt_threads(void) { return 1; }
>>> +static inline bool topology_is_primary_thread(unsigned int cpu) { return true; ]
>>>  #endif
>>>  
>>>  static inline void arch_fix_phys_package_id(int num, u32 slot)
>>> --- a/arch/x86/kernel/apic/apic.c
>>> +++ b/arch/x86/kernel/apic/apic.c
>>> @@ -2189,6 +2189,21 @@ static int cpuid_to_apicid[] = {
>>>  	[0 ... NR_CPUS - 1] = -1,
>>>  };
>>>  
>>> +/**
>>> + * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread
>>> + * @id:	APIC ID to check
>>> + */
>>> +bool apic_id_is_primary_thread(unsigned int apicid)
>>> +{
>>> +	u32 mask;
>>> +
>>> +	if (smp_num_siblings == 1)
>>> +		return true;
>>> +	/* Isolate the SMT bit(s) in the APICID and check for 0 */
>>> +	mask = (1U << fls(smp_num_siblings) - 1) - 1;
>> So I was thinking - this looks right with 2 siblings (or 1), but
>> what if there are four siblings (and here please stop me - b/c there
>> are probably other assumptions made in the code base where things
>> would break if you have say 3, 5, or 4 siblings).
>>
>> Anyhow if we did have four siblings, the mask would end up with 11b
>> (see below):
>>
>> smp_num_siblings	fls	-1	1<<	 -1
>> 	1 (001b)	1	0	1	0
>> 	2 (010b)	2	1	2	01b
>> 	4 (100b)	3	2	4	11b
>>
>>
>>> +	return !(apicid & mask);
>> So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:
>>
>> 	0x76	01110110b & 01 -> !0 -> return 1
>> 	0x77	01110111b & 01 -> !1 -> return 0
>>
>> that looks good.
>>
>> If this is a 4 thread. APICID 0x4, 0x5, and 0x6
>>
>> 	0x4	0100 & 11b -> 00 -> !0 -> return 1
>> 	0x5	0101 & 11b -> 01 -> !1 -> return 0 
>> 	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]
>>
>> That is the 3rd sibling would be incorrectly identified as primary thread.
>>
>> I think? I would welcome somebody checking my logic over, but then
>> perhaps this is non-issue-b/c-we-don-do more than 2 so go away.
> Or just add:
>
> 	BUG_ON(smp_num_siblings > 2);
>
> And all is good and we can touch this when when this goes up.

Knights processors already have 4 threads per core.

~Andrew

Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Thomas Gleixner]

On Mon, 11 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
> Anyhow if we did have four siblings, the mask would end up with 11b
> (see below):
> 
> smp_num_siblings	fls	-1	1<<	 -1
> 	1 (001b)	1	0	1	0
> 	2 (010b)	2	1	2	01b
> 	4 (100b)	3	2	4	11b

That's what I intended :)

> 
> > +	return !(apicid & mask);
> 
> So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:
> 
> 	0x76	01110110b & 01 -> !0 -> return 1
> 	0x77	01110111b & 01 -> !1 -> return 0
> 
> that looks good.
> 
> If this is a 4 thread. APICID 0x4, 0x5, and 0x6
> 
> 	0x4	0100 & 11b -> 00 -> !0 -> return 1
> 	0x5	0101 & 11b -> 01 -> !1 -> return 0 
> 	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]
> 
> That is the 3rd sibling would be incorrectly identified as primary thread.
> 
> I think? I would welcome somebody checking my logic over, but then
> perhaps this is non-issue-b/c-we-don-do more than 2 so go away.

You are somehow confusing '!' and '~'

Thanks,

	tglx

[MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Andrew Cooper]

On 12/06/18 09:05, speck for Thomas Gleixner wrote:
> On Mon, 11 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
>> On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
>> Anyhow if we did have four siblings, the mask would end up with 11b
>> (see below):
>>
>> smp_num_siblings	fls	-1	1<<	 -1
>> 	1 (001b)	1	0	1	0
>> 	2 (010b)	2	1	2	01b
>> 	4 (100b)	3	2	4	11b
> That's what I intended :)
>
>>> +	return !(apicid & mask);
>> So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:
>>
>> 	0x76	01110110b & 01 -> !0 -> return 1
>> 	0x77	01110111b & 01 -> !1 -> return 0
>>
>> that looks good.
>>
>> If this is a 4 thread. APICID 0x4, 0x5, and 0x6
>>
>> 	0x4	0100 & 11b -> 00 -> !0 -> return 1
>> 	0x5	0101 & 11b -> 01 -> !1 -> return 0 
>> 	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]
>>
>> That is the 3rd sibling would be incorrectly identified as primary thread.
>>
>> I think? I would welcome somebody checking my logic over, but then
>> perhaps this is non-issue-b/c-we-don-do more than 2 so go away.
> You are somehow confusing '!' and '~'

The original logic looks correct to me, and matches my equivalent in Xen.

~Andrew

[MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread()

[Konrad Rzeszutek Wilk]

On Tue, Jun 12, 2018 at 11:31:50AM +0100, speck for Andrew Cooper wrote:
> On 12/06/18 09:05, speck for Thomas Gleixner wrote:
> > On Mon, 11 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> >> On Wed, Jun 06, 2018 at 09:27:16PM +0200, speck for Thomas Gleixner wrote:
> >> Anyhow if we did have four siblings, the mask would end up with 11b
> >> (see below):
> >>
> >> smp_num_siblings	fls	-1	1<<	 -1
> >> 	1 (001b)	1	0	1	0
> >> 	2 (010b)	2	1	2	01b
> >> 	4 (100b)	3	2	4	11b
> > That's what I intended :)
> >
> >>> +	return !(apicid & mask);
> >> So with an normal 2-sibling, say APICID 0x76 and 0x77 with mask 1b:
> >>
> >> 	0x76	01110110b & 01 -> !0 -> return 1
> >> 	0x77	01110111b & 01 -> !1 -> return 0
> >>
> >> that looks good.
> >>
> >> If this is a 4 thread. APICID 0x4, 0x5, and 0x6
> >>
> >> 	0x4	0100 & 11b -> 00 -> !0 -> return 1
> >> 	0x5	0101 & 11b -> 01 -> !1 -> return 0 
> >> 	0x6	0110 & 11b -> 10 -> !1111..1101 -> return 0xffff..which cast to bool is 'true' [BAD]
> >>
> >> That is the 3rd sibling would be incorrectly identified as primary thread.
> >>
> >> I think? I would welcome somebody checking my logic over, but then
> >> perhaps this is non-issue-b/c-we-don-do more than 2 so go away.
> > You are somehow confusing '!' and '~'

<blushes>

Um, moving right along..

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> The original logic looks correct to me, and matches my equivalent in Xen.
> 
> ~Andrew

[patch V2 03/12] cpu/hotplug: Make bringup/teardown of smp threads symmetric

[Thomas Gleixner]

Subject: [patch V2 03/12] cpu/hotplug: Make bringup/teardown of smp threads symmetric
From: Thomas Gleixner <tglx@linutronix.de>

The asymetry caused a warning to trigger if the bootup was stopped in state
CPUHP_AP_ONLINE_IDLE. The warning no longer triggers as kthread_park() can
now be invoked on already or still parked threads. But there is still no
reason to have this asymetric.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/cpu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -754,7 +754,6 @@ static int takedown_cpu(unsigned int cpu
 
 	/* Park the smpboot threads */
 	kthread_park(per_cpu_ptr(&cpuhp_state, cpu)->thread);
-	smpboot_park_threads(cpu);
 
 	/*
 	 * Prevent irq alloc/free while the dying cpu reorganizes the
@@ -1332,7 +1331,7 @@ static struct cpuhp_step cpuhp_hp_states
 	[CPUHP_AP_SMPBOOT_THREADS] = {
 		.name			= "smpboot/threads:online",
 		.startup.single		= smpboot_unpark_threads,
-		.teardown.single	= NULL,
+		.teardown.single	= smpboot_park_threads,
 	},
 	[CPUHP_AP_IRQ_AFFINITY_ONLINE] = {
 		.name			= "irq/affinity:online",

[MODERATED] Re: [patch V2 03/12] cpu/hotplug: Make bringup/teardown of smp threads symmetric

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:17PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 03/12] cpu/hotplug: Make bringup/teardown of smp threads symmetric
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The asymetry caused a warning to trigger if the bootup was stopped in state
         ^- two 'm's 


> CPUHP_AP_ONLINE_IDLE. The warning no longer triggers as kthread_park() can
> now be invoked on already or still parked threads. But there is still no
> reason to have this asymetric.

s/asymetric/be asymmetric/
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/cpu.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -754,7 +754,6 @@ static int takedown_cpu(unsigned int cpu
>  
>  	/* Park the smpboot threads */
>  	kthread_park(per_cpu_ptr(&cpuhp_state, cpu)->thread);
> -	smpboot_park_threads(cpu);
>  
>  	/*
>  	 * Prevent irq alloc/free while the dying cpu reorganizes the
> @@ -1332,7 +1331,7 @@ static struct cpuhp_step cpuhp_hp_states
>  	[CPUHP_AP_SMPBOOT_THREADS] = {
>  		.name			= "smpboot/threads:online",
>  		.startup.single		= smpboot_unpark_threads,
> -		.teardown.single	= NULL,
> +		.teardown.single	= smpboot_park_threads,
>  	},
>  	[CPUHP_AP_IRQ_AFFINITY_ONLINE] = {
>  		.name			= "irq/affinity:online",
> 

[patch V2 04/12] cpu/hotplug: Split do_cpu_down()

[Thomas Gleixner]

Subject: [patch V2 04/12] cpu/hotplug: Split do_cpu_down()
From: Thomas Gleixner <tglx@linutronix.de>

Split out the inner workings of do_cpu_down() to allow reuse of that
function for the upcoming SMT disabling mechanism.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/cpu.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -906,20 +906,19 @@ static int __ref _cpu_down(unsigned int
 	return ret;
 }
 
+static int cpu_down_maps_locked(unsigned int cpu, enum cpuhp_state target)
+{
+	if (cpu_hotplug_disabled)
+		return -EBUSY;
+	return _cpu_down(cpu, 0, target);
+}
+
 static int do_cpu_down(unsigned int cpu, enum cpuhp_state target)
 {
 	int err;
 
 	cpu_maps_update_begin();
-
-	if (cpu_hotplug_disabled) {
-		err = -EBUSY;
-		goto out;
-	}
-
-	err = _cpu_down(cpu, 0, target);
-
-out:
+	err = cpu_down_maps_locked(cpu, target);
 	cpu_maps_update_done();
 	return err;
 }

[MODERATED] Re: [patch V2 04/12] cpu/hotplug: Split do_cpu_down()

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:18PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 04/12] cpu/hotplug: Split do_cpu_down()
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Split out the inner workings of do_cpu_down() to allow reuse of that
> function for the upcoming SMT disabling mechanism.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

[patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Thomas Gleixner]

Subject: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT
From: Thomas Gleixner <tglx@linutronix.de>

Proide a command line and a sysfs knob to control SMT.

Switching SMT control off, offlines all online CPUs which are secondary
threads of a physical core and prevents onlining of secondary threads after
that point.

Switching it back on lifts the restrictions again, but does not online the
offlined secondary siblings.

It also can be set to force off, which is an irreversible operation and
later changes will add support for x86 to discard secondary threads in the
MP table and ACPI/MADT.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 Documentation/ABI/testing/sysfs-devices-system-cpu |   16 ++
 Documentation/admin-guide/kernel-parameters.txt    |    4 
 arch/Kconfig                                       |    3 
 arch/x86/Kconfig                                   |    1 
 include/linux/cpu.h                                |   12 +
 kernel/cpu.c                                       |  132 +++++++++++++++++++++
 6 files changed, 168 insertions(+)

--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -490,3 +490,19 @@ Description:	Information about CPU vulne
 		"Not affected"	  CPU is not affected by the vulnerability
 		"Vulnerable"	  CPU is affected and no mitigation in effect
 		"Mitigation: $M"  CPU is affected and mitigation $M is in effect
+
+What:		/sys/devices/system/cpu/smt
+		/sys/devices/system/cpu/smt/active
+		/sys/devices/system/cpu/smt/control
+Date:		June 2018
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:	Control Symetric Multi Threading (SMT)
+
+		active:  Tells whether SMT is active (enabled and available)
+
+		control: Read/write interface to control SMT. Possible
+			 values:
+
+			 "on"		SMT is enabled
+			 "off"		SMT is disabled
+			 "forceoff"	SMT is force disabled. Cannot be changed.
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2687,6 +2687,10 @@
 	nosmt		[KNL,S390] Disable symmetric multithreading (SMT).
 			Equivalent to smt=1.
 
+			[KNL,x86] Disable symmetric multithreading (SMT).
+			nosmt=force: Force disable SMT, similar to disabling
+				     it in the BIOS.
+
 	nospectre_v2	[X86] Disable all mitigations for the Spectre variant 2
 			(indirect branch prediction) vulnerability. System may
 			allow data leaks with this option, which is equivalent
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -13,6 +13,9 @@ config KEXEC_CORE
 config HAVE_IMA_KEXEC
 	bool
 
+config HOTPLUG_SMT
+	bool
+
 config OPROFILE
 	tristate "OProfile system profiling"
 	depends on PROFILING
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -185,6 +185,7 @@ config X86
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_USER_RETURN_NOTIFIER
+	select HOTPLUG_SMT			if SMP
 	select IRQ_FORCED_THREADING
 	select NEED_SG_DMA_LENGTH
 	select PCI_LOCKLESS_CONFIG
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -166,4 +166,16 @@ void cpuhp_report_idle_dead(void);
 static inline void cpuhp_report_idle_dead(void) { }
 #endif /* #ifdef CONFIG_HOTPLUG_CPU */
 
+enum {
+	CPU_SMT_ENABLED,
+	CPU_SMT_DISABLED,
+	CPU_SMT_FORCE_DISABLED,
+};
+
+#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
+extern int cpu_smt_control;
+#else
+# define cpu_smt_control		(CPU_SMT_ENABLED)
+#endif
+
 #endif /* _LINUX_CPU_H_ */
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -933,6 +933,29 @@ EXPORT_SYMBOL(cpu_down);
 #define takedown_cpu		NULL
 #endif /*CONFIG_HOTPLUG_CPU*/
 
+#ifdef CONFIG_HOTPLUG_SMT
+int cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
+
+static int __init smt_cmdline_disable(char *str)
+{
+	cpu_smt_control = CPU_SMT_DISABLED;
+	if (str && !strcmp(str, "force")) {
+		pr_info("SMT: Force disabled\n");
+		cpu_smt_control = CPU_SMT_FORCE_DISABLED;
+	}
+	return 0;
+}
+early_param("nosmt", smt_cmdline_disable);
+
+static inline bool cpu_smt_blocked(unsigned int cpu)
+{
+	return cpu_smt_control != CPU_SMT_ENABLED &&
+		!topology_is_primary_thread(cpu);
+}
+#else
+static inline bool cpu_smt_blocked(unsigned int cpu) { return false; }
+#endif
+
 /**
  * notify_cpu_starting(cpu) - Invoke the callbacks on the starting CPU
  * @cpu: cpu that just started
@@ -1056,6 +1079,10 @@ static int do_cpu_up(unsigned int cpu, e
 		err = -EBUSY;
 		goto out;
 	}
+	if (cpu_smt_blocked(cpu)) {
+		err = -EPERM;
+		goto out;
+	}
 
 	err = _cpu_up(cpu, 0, target);
 out:
@@ -1904,10 +1931,115 @@ static const struct attribute_group cpuh
 	NULL
 };
 
+#ifdef CONFIG_HOTPLUG_SMT
+
+static const char *smt_states[] = {
+	"on",
+	"off",
+	"forceoff",
+};
+
+static ssize_t
+show_smt_control(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE - 2, "%s\n", smt_states[cpu_smt_control]);
+}
+
+static int cpuhp_smt_disable(int val)
+{
+	int cpu, ret = 0;
+
+	cpu_maps_update_begin();
+	for_each_online_cpu(cpu) {
+		if (topology_is_primary_thread(cpu))
+			continue;
+		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
+		if (ret)
+			break;
+	}
+	if (!ret)
+		cpu_smt_control = val;
+	cpu_maps_update_done();
+	return ret;
+}
+
+static int cpuhp_smt_enable(void)
+{
+	cpu_maps_update_begin();
+	cpu_smt_control = CPU_SMT_ENABLED;
+	cpu_maps_update_done();
+	return 0;
+}
+
+static ssize_t
+store_smt_control(struct device *dev, struct device_attribute *attr,
+		  const char *buf, size_t count)
+{
+	int val, ret;
+
+	if (sysfs_streq(buf, "on"))
+		val = CPU_SMT_ENABLED;
+	else if (sysfs_streq(buf, "off"))
+		val = CPU_SMT_DISABLED;
+	else if (sysfs_streq(buf, "forceoff"))
+		val = CPU_SMT_FORCE_DISABLED;
+	else
+		return -EINVAL;
+
+	if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
+		return -EPERM;
+
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	if (val != cpu_smt_control)
+		ret = val ? cpuhp_smt_disable(val) : cpuhp_smt_enable();
+
+	unlock_device_hotplug();
+	return ret ? ret : count;
+}
+static DEVICE_ATTR(control, 0644, show_smt_control, store_smt_control);
+
+static ssize_t
+show_smt_active(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	bool active = topology_max_smt_threads() > 1;
+
+	return snprintf(buf, PAGE_SIZE - 2, "%d\n", active);
+}
+static DEVICE_ATTR(active, 0444, show_smt_active, NULL);
+
+static struct attribute *cpuhp_smt_attrs[] = {
+	&dev_attr_control.attr,
+	&dev_attr_active.attr,
+	NULL
+};
+
+static const struct attribute_group cpuhp_smt_attr_group = {
+	.attrs = cpuhp_smt_attrs,
+	.name = "smt",
+	NULL
+};
+
+static int __init cpu_smt_state_init(void)
+{
+	return sysfs_create_group(&cpu_subsys.dev_root->kobj,
+				  &cpuhp_smt_attr_group);
+}
+
+#else
+static inline int cpu_smt_state_init(void) { return 0; }
+#endif
+
 static int __init cpuhp_sysfs_init(void)
 {
 	int cpu, ret;
 
+	ret = cpu_smt_state_init();
+	if (ret)
+		return ret;
+
 	ret = sysfs_create_group(&cpu_subsys.dev_root->kobj,
 				 &cpuhp_cpu_root_attr_group);
 	if (ret)

[MODERATED] Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:19PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Proide a command line and a sysfs knob to control SMT.

s/Proide/Provide/
> 
> Switching SMT control off, offlines all online CPUs which are secondary
> threads of a physical core and prevents onlining of secondary threads after
> that point.
> 
> Switching it back on lifts the restrictions again, but does not online the

I am not sure what you mean by 'Switching it back on lifts the restrictions again'
as you said in earlier 'prevents onlining of secondary'. So that sounds like
you can't switch the CPU back on as the restriction is in place?

Oh you mean offlining a _core_ CPU and then onlining it!

Perhaps

"When offlining and onlining a core CPU the restrictions are still in effect -
that is you can't online secondary siblings' ?

[But then I am looking at the patch and you do allow it?!]


> offlined secondary siblings.
> 
> It also can be set to force off, which is an irreversible operation and

s/force off/force off (aka, never allow secondary siblings to be onlined)/?

> later changes will add support for x86 to discard secondary threads in the

is it worth to say 'later changes' ? Perhaps just enumerate the name of the patches?

..snip..
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -166,4 +166,16 @@ void cpuhp_report_idle_dead(void);
>  static inline void cpuhp_report_idle_dead(void) { }
>  #endif /* #ifdef CONFIG_HOTPLUG_CPU */
>  
> +enum {
> +	CPU_SMT_ENABLED,
> +	CPU_SMT_DISABLED,
> +	CPU_SMT_FORCE_DISABLED,
> +};
> +
> +#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
> +extern int cpu_smt_control;

Could this be an named enum? We had this disaster with the ssb_something_cmd and
ssb_something mismatch - and if we ever do something similar this can at least
be nicely caught?

> +#else
> +# define cpu_smt_control		(CPU_SMT_ENABLED)
> +#endif
> +
>  #endif /* _LINUX_CPU_H_ */
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -933,6 +933,29 @@ EXPORT_SYMBOL(cpu_down);
>  #define takedown_cpu		NULL
>  #endif /*CONFIG_HOTPLUG_CPU*/
>  
> +#ifdef CONFIG_HOTPLUG_SMT
> +int cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
> +
> +static int __init smt_cmdline_disable(char *str)
> +{
> +	cpu_smt_control = CPU_SMT_DISABLED;
> +	if (str && !strcmp(str, "force")) {
> +		pr_info("SMT: Force disabled\n");
> +		cpu_smt_control = CPU_SMT_FORCE_DISABLED;
> +	}
> +	return 0;
> +}
> +early_param("nosmt", smt_cmdline_disable);
> +
> +static inline bool cpu_smt_blocked(unsigned int cpu)

May I suggest:

 cpu_smt_prohibited ?
 cpu_smt_allowed ?

blocked sounds like an poll or something that returns -EBUSY whiile
this will most certainly never do that.
> +{
> +	return cpu_smt_control != CPU_SMT_ENABLED &&
> +		!topology_is_primary_thread(cpu);

Heck, this could even be'
 cpu_smt_check

and return -EPERM?
> +}
> +#else
> +static inline bool cpu_smt_blocked(unsigned int cpu) { return false; }
> +#endif
> +
>  /**
>   * notify_cpu_starting(cpu) - Invoke the callbacks on the starting CPU
>   * @cpu: cpu that just started
> @@ -1056,6 +1079,10 @@ static int do_cpu_up(unsigned int cpu, e
>  		err = -EBUSY;
>  		goto out;
>  	}
> +	if (cpu_smt_blocked(cpu)) {
> +		err = -EPERM;

.. which allow us to remove those '{}' but <shrugs> maybe I am overthinking it.

> +		goto out;
> +	}
>  
>  	err = _cpu_up(cpu, 0, target);
>  out:
> @@ -1904,10 +1931,115 @@ static const struct attribute_group cpuh
>  	NULL
>  };
>  
> +#ifdef CONFIG_HOTPLUG_SMT
> +
> +static const char *smt_states[] = {
> +	"on",
> +	"off",
> +	"forceoff",
> +};

May I suggest we follow the same pattern as in bugs.c? That is

static const char *smt_stats[] = { 
	[SMT_ENABLED] = "on",

... and so on?

> +

..snip..
> +static ssize_t
> +store_smt_control(struct device *dev, struct device_attribute *attr,
> +		  const char *buf, size_t count)
> +{
> +	int val, ret;
> +
> +	if (sysfs_streq(buf, "on"))
> +		val = CPU_SMT_ENABLED;
> +	else if (sysfs_streq(buf, "off"))
> +		val = CPU_SMT_DISABLED;
> +	else if (sysfs_streq(buf, "forceoff"))
> +		val = CPU_SMT_FORCE_DISABLED;
> +	else
> +		return -EINVAL;
> +
> +	if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
> +		return -EPERM;
> +
> +	ret = lock_device_hotplug_sysfs();
> +	if (ret)
> +		return ret;
> +
> +	if (val != cpu_smt_control)
> +		ret = val ? cpuhp_smt_disable(val) : cpuhp_smt_enable();

Just in case somebody messed up the enums would it make sense to add

BUILD_BUG_ON(CPU_SMT_ENABLED != 0);

so that this logic can still work properly? Or alterntively switch to an 'switch'
statement?

[MODERATED] Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On 06/06/2018 12:27 PM, speck for Thomas Gleixner wrote:
> +What:		/sys/devices/system/cpu/smt
> +		/sys/devices/system/cpu/smt/active
> +		/sys/devices/system/cpu/smt/control
> +Date:		June 2018
> +Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
> +Description:	Control Symetric Multi Threading (SMT)
> +
> +		active:  Tells whether SMT is active (enabled and available)

Just to be clear: "enabled and available" does not necessarily mean that
we have any HT threads *online*, it just means that we *might* have one
come online.

Right?

Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Thomas Gleixner]

On Wed, 20 Jun 2018, speck for Dave Hansen wrote:

> On 06/06/2018 12:27 PM, speck for Thomas Gleixner wrote:
> > +What:		/sys/devices/system/cpu/smt
> > +		/sys/devices/system/cpu/smt/active
> > +		/sys/devices/system/cpu/smt/control
> > +Date:		June 2018
> > +Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
> > +Description:	Control Symetric Multi Threading (SMT)
> > +
> > +		active:  Tells whether SMT is active (enabled and available)
> 
> Just to be clear: "enabled and available" does not necessarily mean that
> we have any HT threads *online*, it just means that we *might* have one
> come online.
> 
> Right?

No. That should actuaally be 'online' because recompute_smt_state() updates
__max_smt_threads and the sysfs file checks topology_max_smt_threads().

I'll fix that right now.

Thanks,

	tglx

[MODERATED] Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On 06/06/2018 12:27 PM, speck for Thomas Gleixner wrote:
> +enum {
> +	CPU_SMT_ENABLED,
> +	CPU_SMT_DISABLED,
> +	CPU_SMT_FORCE_DISABLED,
> +};
> +
> +#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
> +extern int cpu_smt_control;
> +#else
> +# define cpu_smt_control		(CPU_SMT_ENABLED)
> +#endif

Shouldn't this be:

# define cpu_smt_control		(CPU_SMT_FORCE_DISABLED)

Otherwise, /sysfs looks like SMT is enabled even though it's off at
compile-time.

Also, do we have a spot that does

	cpu_smt_control = CPU_SMT_FORCE_DISABLED;

at runtime for the cases where we *know* there is no SMT support in the
processor?  I only see that via the sysfs controls but not the normal
boot process.

Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Thomas Gleixner]

On Wed, 20 Jun 2018, speck for Dave Hansen wrote:

> On 06/06/2018 12:27 PM, speck for Thomas Gleixner wrote:
> > +enum {
> > +	CPU_SMT_ENABLED,
> > +	CPU_SMT_DISABLED,
> > +	CPU_SMT_FORCE_DISABLED,
> > +};
> > +
> > +#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
> > +extern int cpu_smt_control;
> > +#else
> > +# define cpu_smt_control		(CPU_SMT_ENABLED)
> > +#endif
> 
> Shouldn't this be:
> 
> # define cpu_smt_control		(CPU_SMT_FORCE_DISABLED)
> 
> Otherwise, /sysfs looks like SMT is enabled even though it's off at
> compile-time.

Makes sense.

> Also, do we have a spot that does
> 
> 	cpu_smt_control = CPU_SMT_FORCE_DISABLED;
> 
> at runtime for the cases where we *know* there is no SMT support in the
> processor?  I only see that via the sysfs controls but not the normal
> boot process.

No, but that should be doable. Though maybe we should add a
'SMT_NOT_SUPPORTED' mode to it. Let me have a look.

Thanks,

	tglx

[patch V2 06/12] x86/cpu: Remove the pointless CPU printout

[Thomas Gleixner]

Subject: [patch V2 06/12] x86/cpu: Remove the pointless CPU printout
From: Thomas Gleixner <tglx@linutronix.de>

The value of this printout is dubious at best and there is no point in
having it in two different places along with convoluted ways to reach it.

Remove it completely.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/common.c   |   19 +++++--------------
 arch/x86/kernel/cpu/topology.c |   10 ----------
 2 files changed, 5 insertions(+), 24 deletions(-)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -669,7 +669,7 @@ void detect_ht(struct cpuinfo_x86 *c)
 		return;
 
 	if (cpu_has(c, X86_FEATURE_CMP_LEGACY))
-		goto out;
+		return;
 
 	if (cpu_has(c, X86_FEATURE_XTOPOLOGY))
 		return;
@@ -678,14 +678,14 @@ void detect_ht(struct cpuinfo_x86 *c)
 
 	smp_num_siblings = (ebx & 0xff0000) >> 16;
 
+	if (!smp_num_siblings)
+		smp_num_siblings = 1;
+
 	if (smp_num_siblings == 1) {
 		pr_info_once("CPU0: Hyper-Threading is disabled\n");
-		goto out;
+		return;
 	}
 
-	if (smp_num_siblings <= 1)
-		goto out;
-
 	index_msb = get_count_order(smp_num_siblings);
 	c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid, index_msb);
 
@@ -697,15 +697,6 @@ void detect_ht(struct cpuinfo_x86 *c)
 
 	c->cpu_core_id = apic->phys_pkg_id(c->initial_apicid, index_msb) &
 				       ((1 << core_bits) - 1);
-
-out:
-	if (!printed && (c->x86_max_cores * smp_num_siblings) > 1) {
-		pr_info("CPU: Physical Processor ID: %d\n",
-			c->phys_proc_id);
-		pr_info("CPU: Processor Core ID: %d\n",
-			c->cpu_core_id);
-		printed = 1;
-	}
 #endif
 }
 
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -33,7 +33,6 @@ int detect_extended_topology(struct cpui
 	unsigned int eax, ebx, ecx, edx, sub_index;
 	unsigned int ht_mask_width, core_plus_mask_width;
 	unsigned int core_select_mask, core_level_siblings;
-	static bool printed;
 
 	if (c->cpuid_level < 0xb)
 		return -1;
@@ -86,15 +85,6 @@ int detect_extended_topology(struct cpui
 	c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
 
 	c->x86_max_cores = (core_level_siblings / smp_num_siblings);
-
-	if (!printed) {
-		pr_info("CPU: Physical Processor ID: %d\n",
-		       c->phys_proc_id);
-		if (c->x86_max_cores > 1)
-			pr_info("CPU: Processor Core ID: %d\n",
-			       c->cpu_core_id);
-		printed = 1;
-	}
 #endif
 	return 0;
 }

[MODERATED] Re: [patch V2 06/12] x86/cpu: Remove the pointless CPU printout

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:20PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 06/12] x86/cpu: Remove the pointless CPU printout
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The value of this printout is dubious at best and there is no point in
> having it in two different places along with convoluted ways to reach it.
> 
> Remove it completely.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Heh, when I was looking at patch #1 I stumbled on this and thought
"oh man this looks like it could use some flossing, will cobble up a patch
after reviewing this full patchset" and sure enough you did for me :-)

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> ---
>  arch/x86/kernel/cpu/common.c   |   19 +++++--------------
>  arch/x86/kernel/cpu/topology.c |   10 ----------
>  2 files changed, 5 insertions(+), 24 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -669,7 +669,7 @@ void detect_ht(struct cpuinfo_x86 *c)
>  		return;
>  
>  	if (cpu_has(c, X86_FEATURE_CMP_LEGACY))
> -		goto out;
> +		return;
>  
>  	if (cpu_has(c, X86_FEATURE_XTOPOLOGY))
>  		return;
> @@ -678,14 +678,14 @@ void detect_ht(struct cpuinfo_x86 *c)
>  
>  	smp_num_siblings = (ebx & 0xff0000) >> 16;
>  
> +	if (!smp_num_siblings)
> +		smp_num_siblings = 1;
> +
>  	if (smp_num_siblings == 1) {
>  		pr_info_once("CPU0: Hyper-Threading is disabled\n");
> -		goto out;
> +		return;
>  	}
>  
> -	if (smp_num_siblings <= 1)
> -		goto out;
> -
>  	index_msb = get_count_order(smp_num_siblings);
>  	c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid, index_msb);
>  
> @@ -697,15 +697,6 @@ void detect_ht(struct cpuinfo_x86 *c)
>  
>  	c->cpu_core_id = apic->phys_pkg_id(c->initial_apicid, index_msb) &
>  				       ((1 << core_bits) - 1);
> -
> -out:
> -	if (!printed && (c->x86_max_cores * smp_num_siblings) > 1) {
> -		pr_info("CPU: Physical Processor ID: %d\n",
> -			c->phys_proc_id);
> -		pr_info("CPU: Processor Core ID: %d\n",
> -			c->cpu_core_id);
> -		printed = 1;
> -	}
>  #endif
>  }
>  
> --- a/arch/x86/kernel/cpu/topology.c
> +++ b/arch/x86/kernel/cpu/topology.c
> @@ -33,7 +33,6 @@ int detect_extended_topology(struct cpui
>  	unsigned int eax, ebx, ecx, edx, sub_index;
>  	unsigned int ht_mask_width, core_plus_mask_width;
>  	unsigned int core_select_mask, core_level_siblings;
> -	static bool printed;
>  
>  	if (c->cpuid_level < 0xb)
>  		return -1;
> @@ -86,15 +85,6 @@ int detect_extended_topology(struct cpui
>  	c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
>  
>  	c->x86_max_cores = (core_level_siblings / smp_num_siblings);
> -
> -	if (!printed) {
> -		pr_info("CPU: Physical Processor ID: %d\n",
> -		       c->phys_proc_id);
> -		if (c->x86_max_cores > 1)
> -			pr_info("CPU: Processor Core ID: %d\n",
> -			       c->cpu_core_id);
> -		printed = 1;
> -	}
>  #endif
>  	return 0;
>  }
> 

[patch V2 07/12] x86/cpu/AMD: Remove the pointless detect_ht() call

[Thomas Gleixner]

Subject: [patch V2 07/12] x86/cpu/AMD: Remove the pointless detect_ht() call
From: Thomas Gleixner <tglx@linutronix.de>

Real 32bit AMD CPUs do not have SMT and the only value of the call was to
reach the magic printout which got removed.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/amd.c |    4 ----
 1 file changed, 4 deletions(-)

--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -855,10 +855,6 @@ static void init_amd(struct cpuinfo_x86
 		srat_detect_node(c);
 	}
 
-#ifdef CONFIG_X86_32
-	detect_ht(c);
-#endif
-
 	init_amd_cacheinfo(c);
 
 	if (c->x86 >= 0xf)

[MODERATED] Re: [patch V2 07/12] x86/cpu/AMD: Remove the pointless detect_ht() call

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:21PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 07/12] x86/cpu/AMD: Remove the pointless detect_ht() call
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Real 32bit AMD CPUs do not have SMT and the only value of the call was to
> reach the magic printout which got removed.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> ---
>  arch/x86/kernel/cpu/amd.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -855,10 +855,6 @@ static void init_amd(struct cpuinfo_x86
>  		srat_detect_node(c);
>  	}
>  
> -#ifdef CONFIG_X86_32
> -	detect_ht(c);
> -#endif
> -
>  	init_amd_cacheinfo(c);
>  
>  	if (c->x86 >= 0xf)
> 

[patch V2 08/12] x86/cpu/common: Provide detect_ht_early()

[Thomas Gleixner]

Subject: [patch V2 08/12] x86/cpu/common: Provide detect_ht_early()
From: Thomas Gleixner <tglx@linutronix.de>

To support force disabling of SMT it's required to know the number of
thread siblings early. detect_ht() cannot be called before the APIC driver
is selected, so split out the part which initializes smp_num_siblings.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/common.c |   25 ++++++++++++++-----------
 arch/x86/kernel/cpu/cpu.h    |    1 +
 2 files changed, 15 insertions(+), 11 deletions(-)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -658,33 +658,36 @@ static void cpu_detect_tlb(struct cpuinf
 		tlb_lld_4m[ENTRIES], tlb_lld_1g[ENTRIES]);
 }
 
-void detect_ht(struct cpuinfo_x86 *c)
+int detect_ht_early(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	u32 eax, ebx, ecx, edx;
-	int index_msb, core_bits;
-	static bool printed;
 
 	if (!cpu_has(c, X86_FEATURE_HT))
-		return;
+		return -1;
 
 	if (cpu_has(c, X86_FEATURE_CMP_LEGACY))
-		return;
+		return -1;
 
 	if (cpu_has(c, X86_FEATURE_XTOPOLOGY))
-		return;
+		return -1;
 
 	cpuid(1, &eax, &ebx, &ecx, &edx);
 
 	smp_num_siblings = (ebx & 0xff0000) >> 16;
+	if (smp_num_siblings == 1)
+		pr_info_once("CPU0: Hyper-Threading is disabled\n");
+#endif
+	return 0;
+}
 
-	if (!smp_num_siblings)
-		smp_num_siblings = 1;
+void detect_ht(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+	int index_msb, core_bits;
 
-	if (smp_num_siblings == 1) {
-		pr_info_once("CPU0: Hyper-Threading is disabled\n");
+	if (detect_ht_early(c) < 0)
 		return;
-	}
 
 	index_msb = get_count_order(smp_num_siblings);
 	c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid, index_msb);
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -56,6 +56,7 @@ extern void init_amd_cacheinfo(struct cp
 
 extern void detect_num_cpu_cores(struct cpuinfo_x86 *c);
 extern int detect_extended_topology(struct cpuinfo_x86 *c);
+extern int detect_ht_early(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
 
 unsigned int aperfmperf_get_khz(int cpu);

[MODERATED] Re: [patch V2 08/12] x86/cpu/common: Provide detect_ht_early()

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:22PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 08/12] x86/cpu/common: Provide detect_ht_early()
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To support force disabling of SMT it's required to know the number of
> thread siblings early. detect_ht() cannot be called before the APIC driver
> is selected, so split out the part which initializes smp_num_siblings.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> ---
>  arch/x86/kernel/cpu/common.c |   25 ++++++++++++++-----------
>  arch/x86/kernel/cpu/cpu.h    |    1 +
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -658,33 +658,36 @@ static void cpu_detect_tlb(struct cpuinf
>  		tlb_lld_4m[ENTRIES], tlb_lld_1g[ENTRIES]);
>  }
>  
> -void detect_ht(struct cpuinfo_x86 *c)
> +int detect_ht_early(struct cpuinfo_x86 *c)
>  {
>  #ifdef CONFIG_SMP
>  	u32 eax, ebx, ecx, edx;
> -	int index_msb, core_bits;
> -	static bool printed;
>  
>  	if (!cpu_has(c, X86_FEATURE_HT))
> -		return;
> +		return -1;
>  
>  	if (cpu_has(c, X86_FEATURE_CMP_LEGACY))
> -		return;
> +		return -1;
>  
>  	if (cpu_has(c, X86_FEATURE_XTOPOLOGY))
> -		return;
> +		return -1;
>  
>  	cpuid(1, &eax, &ebx, &ecx, &edx);
>  
>  	smp_num_siblings = (ebx & 0xff0000) >> 16;
> +	if (smp_num_siblings == 1)
> +		pr_info_once("CPU0: Hyper-Threading is disabled\n");
> +#endif
> +	return 0;
> +}
>  
> -	if (!smp_num_siblings)
> -		smp_num_siblings = 1;
> +void detect_ht(struct cpuinfo_x86 *c)
> +{
> +#ifdef CONFIG_SMP
> +	int index_msb, core_bits;
>  
> -	if (smp_num_siblings == 1) {
> -		pr_info_once("CPU0: Hyper-Threading is disabled\n");
> +	if (detect_ht_early(c) < 0)
>  		return;
> -	}
>  
>  	index_msb = get_count_order(smp_num_siblings);
>  	c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid, index_msb);
> --- a/arch/x86/kernel/cpu/cpu.h
> +++ b/arch/x86/kernel/cpu/cpu.h
> @@ -56,6 +56,7 @@ extern void init_amd_cacheinfo(struct cp
>  
>  extern void detect_num_cpu_cores(struct cpuinfo_x86 *c);
>  extern int detect_extended_topology(struct cpuinfo_x86 *c);
> +extern int detect_ht_early(struct cpuinfo_x86 *c);
>  extern void detect_ht(struct cpuinfo_x86 *c);
>  
>  unsigned int aperfmperf_get_khz(int cpu);
> 

[patch V2 09/12] x86/cpu/topology: Provide detect_extended_topology_early()

[Thomas Gleixner]

Subject: [patch V2 09/12] x86/cpu/topology: Provide detect_extended_topology_early()
From: Thomas Gleixner <tglx@linutronix.de>

To support force disabling of SMT it's required to know the number of
thread siblings early. detect_extended_topology() cannot be called before
the APIC driver is selected, so split out the part which initializes
smp_num_siblings.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/cpu.h      |    1 +
 arch/x86/kernel/cpu/topology.c |   31 ++++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -55,6 +55,7 @@ extern void init_intel_cacheinfo(struct
 extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
 
 extern void detect_num_cpu_cores(struct cpuinfo_x86 *c);
+extern int detect_extended_topology_early(struct cpuinfo_x86 *c);
 extern int detect_extended_topology(struct cpuinfo_x86 *c);
 extern int detect_ht_early(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -22,17 +22,10 @@
 #define BITS_SHIFT_NEXT_LEVEL(eax)	((eax) & 0x1f)
 #define LEVEL_MAX_SIBLINGS(ebx)		((ebx) & 0xffff)
 
-/*
- * Check for extended topology enumeration cpuid leaf 0xb and if it
- * exists, use it for populating initial_apicid and cpu topology
- * detection.
- */
-int detect_extended_topology(struct cpuinfo_x86 *c)
+int detect_extended_topology_early(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
-	unsigned int eax, ebx, ecx, edx, sub_index;
-	unsigned int ht_mask_width, core_plus_mask_width;
-	unsigned int core_select_mask, core_level_siblings;
+	unsigned int eax, ebx, ecx, edx;
 
 	if (c->cpuid_level < 0xb)
 		return -1;
@@ -51,10 +44,30 @@ int detect_extended_topology(struct cpui
 	 * initial apic id, which also represents 32-bit extended x2apic id.
 	 */
 	c->initial_apicid = edx;
+	smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
+#endif
+	return 0;
+}
+
+/*
+ * Check for extended topology enumeration cpuid leaf 0xb and if it
+ * exists, use it for populating initial_apicid and cpu topology
+ * detection.
+ */
+int detect_extended_topology(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+	unsigned int eax, ebx, ecx, edx, sub_index;
+	unsigned int ht_mask_width, core_plus_mask_width;
+	unsigned int core_select_mask, core_level_siblings;
+
+	if (detect_extended_topology_early(c) < 0)
+		return -1;
 
 	/*
 	 * Populate HT related information from sub-leaf level 0.
 	 */
+	cpuid_count(0xb, SMT_LEVEL, &eax, &ebx, &ecx, &edx);
 	core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
 	core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 

[MODERATED] Re: [patch V2 09/12] x86/cpu/topology: Provide detect_extended_topology_early()

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:23PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 09/12] x86/cpu/topology: Provide detect_extended_topology_early()
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To support force disabling of SMT it's required to know the number of
> thread siblings early. detect_extended_topology() cannot be called before
> the APIC driver is selected, so split out the part which initializes
> smp_num_siblings.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

[patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early

[Thomas Gleixner]

Subject: [patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early
From: Thomas Gleixner <tglx@linutronix.de>

Make use of the new early detection function to initialize smp_num_siblings
on the boot cpu before the MP-Table or ACPI/MADT scan happens. That's
required for force disabling SMT.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/intel.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -301,6 +301,13 @@ static void early_init_intel(struct cpui
 	}
 
 	check_mpx_erratum(c);
+
+	/*
+	 * Get the number of SMT siblings early from the extended topology
+	 * leaf, if available
+	 */
+	if (detect_extended_topology_early(c) < 0 && IS_ENABLED(CONFIG_X86_32))
+		detect_ht_early(c);
 }
 
 #ifdef CONFIG_X86_32

[MODERATED] Re: [patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:24PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Make use of the new early detection function to initialize smp_num_siblings
> on the boot cpu before the MP-Table or ACPI/MADT scan happens. That's
> required for force disabling SMT.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/cpu/intel.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -301,6 +301,13 @@ static void early_init_intel(struct cpui
>  	}
>  
>  	check_mpx_erratum(c);
> +
> +	/*
> +	 * Get the number of SMT siblings early from the extended topology
> +	 * leaf, if available
> +	 */
> +	if (detect_extended_topology_early(c) < 0 && IS_ENABLED(CONFIG_X86_32))

Shouldn't this be || ?

That is you could run on a virtualized env where there is no XTOPOLOGY
and you need to fallback to the earlier code-base (detect_ht_early) to
figure out the SMT topology? (This being an X86_64 build)?

> +		detect_ht_early(c);
>  }
>  
>  #ifdef CONFIG_X86_32
> 

Re: [patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early

[Thomas Gleixner]

On Tue, 12 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 06, 2018 at 09:27:24PM +0200, speck for Thomas Gleixner wrote:
> > Subject: [patch V2 10/12] x86/cpu/intel: Evaluate smp_num_siblings early
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > Make use of the new early detection function to initialize smp_num_siblings
> > on the boot cpu before the MP-Table or ACPI/MADT scan happens. That's
> > required for force disabling SMT.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/kernel/cpu/intel.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -301,6 +301,13 @@ static void early_init_intel(struct cpui
> >  	}
> >  
> >  	check_mpx_erratum(c);
> > +
> > +	/*
> > +	 * Get the number of SMT siblings early from the extended topology
> > +	 * leaf, if available
> > +	 */
> > +	if (detect_extended_topology_early(c) < 0 && IS_ENABLED(CONFIG_X86_32))
> 
> Shouldn't this be || ?
> 
> That is you could run on a virtualized env where there is no XTOPOLOGY
> and you need to fallback to the earlier code-base (detect_ht_early) to
> figure out the SMT topology? (This being an X86_64 build)?

Duh. I have no idea why I put IS_ENABLED(CONFIG_X86_32) there at all. If
detect_extended_topology_early() returns an error, then it should always
fall back to detect_ht_early(). Thanks for spotting it.

Thanks,

	tglx

[patch V2 11/12] x86/cpu/AMD: Evaluate smp_num_siblings early

[Thomas Gleixner]

Subject: [patch V2 11/12] x86/cpu/AMD: Evaluate smp_num_siblings early
From: Thomas Gleixner <tglx@linutronix.de>

To support force disabling of SMT it's required to know the number of
thread siblings early. amd_get_topology() cannot be called before the APIC
driver is selected, so split out the part which initializes
smp_num_siblings and invoke it from amd_early_init().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/amd.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -315,6 +315,18 @@ static void legacy_fixup_core_id(struct
 	c->cpu_core_id %= cus_per_node;
 }
 
+
+static void amd_get_topology_early(struct cpuinfo_x86 *c)
+{
+	if (boot_cpu_has(X86_FEATURE_TOPOEXT) &&
+	    c->extended_cpuid_level >= 0x80000008) {
+		u32 eax, ebx, ecx, edx;
+
+		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
+		smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
+	}
+}
+
 /*
  * Fixup core topology information for
  * (1) AMD multi-node processors
@@ -681,6 +693,8 @@ static void early_init_amd(struct cpuinf
 		set_cpu_bug(c, X86_BUG_AMD_E400);
 
 	early_detect_mem_encrypt(c);
+
+	amd_get_topology_early(c);
 }
 
 static void init_amd_k8(struct cpuinfo_x86 *c)

[patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force

[Thomas Gleixner]

Subject: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force
From: Thomas Gleixner <tglx@linutronix.de>

nosmt on the kernel command line merily prevents the onlining of the
secondary SMT siblings.

nosmt=force makes the APIC detection code ignore the secondary SMT siblings
completely, so they even do not show up as possible CPUs. This is more or
less equivalent to disabling SMT in the BIOS.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/apic.h |    2 ++
 arch/x86/kernel/acpi/boot.c |    3 ++-
 arch/x86/kernel/apic/apic.c |   21 ++++++++++++++++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -502,8 +502,10 @@ extern int default_check_phys_apicid_pre
 
 #ifdef CONFIG_SMP
 bool apic_id_is_primary_thread(unsigned int id);
+bool apic_id_disabled(unsigned int id);
 #else
 static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
+static inline bool apic_id_disabled(unsigned int id) { return false; }
 #endif
 
 extern void irq_enter(void);
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -181,7 +181,8 @@ static int acpi_register_lapic(int id, u
 	}
 
 	if (!enabled) {
-		++disabled_cpus;
+		if (!apic_id_disabled(id))
+			++disabled_cpus;
 		return -EINVAL;
 	}
 
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2200,10 +2200,20 @@ bool apic_id_is_primary_thread(unsigned
 	if (smp_num_siblings == 1)
 		return true;
 	/* Isolate the SMT bit(s) in the APICID and check for 0 */
-	mask = (1U << fls(smp_num_siblings) - 1) - 1;
+	mask = (1U << (fls(smp_num_siblings) - 1)) - 1;
 	return !(apicid & mask);
 }
 
+/**
+ * apic_id_disabled - Check whether APIC ID is disabled via SMT control
+ * @id:	APIC ID to check
+ */
+bool apic_id_disabled(unsigned int id)
+{
+	return (cpu_smt_control == CPU_SMT_FORCE_DISABLED &&
+		!apic_id_is_primary_thread(id));
+}
+
 /*
  * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
  * and cpuid_to_apicid[] synchronized.
@@ -2299,6 +2309,15 @@ int generic_processor_info(int apicid, i
 		return -EINVAL;
 	}
 
+	/*
+	 * If SMT is force disabled and the APIC ID belongs to
+	 * a secondary thread, ignore it.
+	 */
+	if (apic_id_disabled(apicid)) {
+		pr_info_once("Ignoring secondary SMT threads\n");
+		return -EINVAL;
+	}
+
 	if (apicid == boot_cpu_physical_apicid) {
 		/*
 		 * x86_bios_cpu_apicid is required to have processors listed

[MODERATED] Re: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force

[Linus Torvalds]

On Wed, 6 Jun 2018, speck for Thomas Gleixner wrote:
> 
> nosmt=force makes the APIC detection code ignore the secondary SMT siblings
> completely, so they even do not show up as possible CPUs. This is more or
> less equivalent to disabling SMT in the BIOS.

So if we go down this route, I do think it should be very clearly 
documented that the BIOS disable can (and does) end up having resource 
allocation advantages that we cannot do in software later on.

The intel optimization manual is not very clear on what the partitioning 
rules are.

I find:

  "In general, the buffers for staging instructions between major pipe 
   stages  are partitioned. These buffers include µop queues after the  
   execution trace cache, the queues after the register rename stage, the
   reorder buffer which stages instructions for retirement, and the load 
   and store buffers.

   In the case of load and store buffers, partitioning also provided an 
   easier implementation to maintain memory ordering for each logical 
   processor and detect memory ordering violations"

but some of that partitioning may be relaxed if the HT thread is "not 
active":

  "In Intel microarchitecture code name Sandy Bridge, the micro-op queue 
   is statically partitioned to provide 28 entries for each logical 
   processor,  irrespective of software executing in single thread or 
   multiple threads. If one logical processor is not active in Intel 
   microarchitecture code name Ivy Bridge, then a single thread executing 
   on that processor  core can use the 56 entries in the micro-op queue"

but I do not know what "not active" means, and how dynamic it is. Some of 
that partitioning may be entirely static and depend on the early BIOS 
disabling of HT, and even if we park the cores, the resources will just be 
wasted.

From the above, it does sound like even if some of the instruction queues 
may be relaxed when one thread is idle (if that is what "not active") 
means, load and store buffers may be entirely statically partitioned. 

So I would really wanmt to have a big note along with the

  "This is more or less equivalent to disabling SMT in the BIOS"

because on some loads it might be a case of "less equivalent" than not.

               Linus

Re: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force

[Thomas Gleixner]

On Wed, 6 Jun 2018, speck for Linus Torvalds wrote:
> So I would really wanmt to have a big note along with the
> 
>   "This is more or less equivalent to disabling SMT in the BIOS"
> 
> because on some loads it might be a case of "less equivalent" than not.

Fair enough. I surely meant it in the sense of the kernels POV, not from
the performance perspective. I'll amend the changelog in case we come to
the conclusion that this whole thing is worthwhile.

I personaly like it for a completely non speculative reason. I often enough
cursed when I had to fiddle in the BIOS settings to test SMT vs. non SMT
stuff. Admittedly virtual machines have made that way simpler, but it's
still easier to just tweak the kernel command line :)

Thanks,

	tglx

[MODERATED] Re: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:26PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> nosmt on the kernel command line merily prevents the onlining of the
> secondary SMT siblings.
> 
> nosmt=force makes the APIC detection code ignore the secondary SMT siblings
> completely, so they even do not show up as possible CPUs. This is more or
> less equivalent to disabling SMT in the BIOS.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/apic.h |    2 ++
>  arch/x86/kernel/acpi/boot.c |    3 ++-
>  arch/x86/kernel/apic/apic.c |   21 ++++++++++++++++++++-
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -502,8 +502,10 @@ extern int default_check_phys_apicid_pre
>  
>  #ifdef CONFIG_SMP
>  bool apic_id_is_primary_thread(unsigned int id);
> +bool apic_id_disabled(unsigned int id);
>  #else
>  static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
> +static inline bool apic_id_disabled(unsigned int id) { return false; }
>  #endif
>  
>  extern void irq_enter(void);
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -181,7 +181,8 @@ static int acpi_register_lapic(int id, u
>  	}
>  
>  	if (!enabled) {
> -		++disabled_cpus;
> +		if (!apic_id_disabled(id))
> +			++disabled_cpus;
>  		return -EINVAL;
>  	}
>  
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2200,10 +2200,20 @@ bool apic_id_is_primary_thread(unsigned
>  	if (smp_num_siblings == 1)
>  		return true;
>  	/* Isolate the SMT bit(s) in the APICID and check for 0 */
> -	mask = (1U << fls(smp_num_siblings) - 1) - 1;
> +	mask = (1U << (fls(smp_num_siblings) - 1)) - 1;

Should this be squashed in patch "8a9d907a102d x86/smp: Provide topology_is_primary_thread()" ?

>  	return !(apicid & mask);
>  }
>  
> +/**
> + * apic_id_disabled - Check whether APIC ID is disabled via SMT control
> + * @id:	APIC ID to check
> + */
> +bool apic_id_disabled(unsigned int id)
> +{
> +	return (cpu_smt_control == CPU_SMT_FORCE_DISABLED &&
> +		!apic_id_is_primary_thread(id));
> +}
> +
>  /*
>   * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
>   * and cpuid_to_apicid[] synchronized.
> @@ -2299,6 +2309,15 @@ int generic_processor_info(int apicid, i
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * If SMT is force disabled and the APIC ID belongs to
> +	 * a secondary thread, ignore it.
> +	 */
> +	if (apic_id_disabled(apicid)) {
> +		pr_info_once("Ignoring secondary SMT threads\n");
> +		return -EINVAL;
> +	}
> +

Ouch.

Besides the above comment (squash):


Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

>  	if (apicid == boot_cpu_physical_apicid) {
>  		/*
>  		 * x86_bios_cpu_apicid is required to have processors listed
> 

Re: [patch V2 12/12] x86/apic: Ignore secondary threads if nosmt=force

[Thomas Gleixner]

On Tue, 12 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> >  	/* Isolate the SMT bit(s) in the APICID and check for 0 */
> > -	mask = (1U << fls(smp_num_siblings) - 1) - 1;
> > +	mask = (1U << (fls(smp_num_siblings) - 1)) - 1;
> 
> Should this be squashed in patch "8a9d907a102d x86/smp: Provide topology_is_primary_thread()" ?

Yes. I'm sure I wanted to do that ....

> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks,

	tglx

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Andi Kleen]

On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> The following series is a reworked version of the initial proof of concept
> patch. The main changes are:

I still think it's pointless to add so much kernel code for something
that can be done with a straight forward user script at run time
using existing APIs.

We need strong justifications to add new kernel APIs.

https://raw.githubusercontent.com/andikleen/pmu-tools/master/cputop.py

SMT-off:

./cputop.py 'thread == 1' offline | sh

SMT-on:

/cputop.py offline online | sh

-Andi

Re: [patch V2 00/12] cpu/hotplug: SMT control

[Thomas Gleixner]

On Wed, 6 Jun 2018, speck for Andi Kleen wrote:
> On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> > The following series is a reworked version of the initial proof of concept
> > patch. The main changes are:
> 
> I still think it's pointless to add so much kernel code for something
> that can be done with a straight forward user script at run time
> using existing APIs.
> 
> We need strong justifications to add new kernel APIs.
> 
> https://raw.githubusercontent.com/andikleen/pmu-tools/master/cputop.py

It can be done with 3 lines of bash as well.

But it does neither work from the kernel command line nor does it provide
protections against re-online nor full enforcement. 

Sure it adds the massive amount of 225 lines including comments and
Documentation, but it's straight forward and works and has it's merits as a
conveniance/testing mechanism as well.

Thanks,

	tglx

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Jiri Kosina]

On Thu, 7 Jun 2018, speck for Thomas Gleixner wrote:

> > I still think it's pointless to add so much kernel code for something
> > that can be done with a straight forward user script at run time
> > using existing APIs.
> > 
> > We need strong justifications to add new kernel APIs.
> > 
> > https://raw.githubusercontent.com/andikleen/pmu-tools/master/cputop.py
> 
> It can be done with 3 lines of bash as well.
> 
> But it does neither work from the kernel command line nor does it provide
> protections against re-online nor full enforcement. 
> 
> Sure it adds the massive amount of 225 lines including comments and
> Documentation, but it's straight forward and works and has it's merits as a
> conveniance/testing mechanism as well.

In addition to that, form a purely distro-centric POV: how exactly would 
you envision this script to be shipped to the world (as we'll have to have 
way to distribute it reliably to all the 'untrusted VM' providers at 
least). Creating a separate 'smt-disable' package just for this seems like 
an overkill, OTOH everybody has a process in place how to tweak kernel 
cmdline / sysfs parameters.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Andi Kleen]

On Thu, Jun 07, 2018 at 09:42:35AM +0200, speck for Jiri Kosina wrote:
> On Thu, 7 Jun 2018, speck for Thomas Gleixner wrote:
> 
> > > I still think it's pointless to add so much kernel code for something
> > > that can be done with a straight forward user script at run time
> > > using existing APIs.
> > > 
> > > We need strong justifications to add new kernel APIs.
> > > 
> > > https://raw.githubusercontent.com/andikleen/pmu-tools/master/cputop.py
> > 
> > It can be done with 3 lines of bash as well.
> > 
> > But it does neither work from the kernel command line nor does it provide
> > protections against re-online nor full enforcement. 
> > 
> > Sure it adds the massive amount of 225 lines including comments and
> > Documentation, but it's straight forward and works and has it's merits as a
> > conveniance/testing mechanism as well.
> 
> In addition to that, form a purely distro-centric POV: how exactly would 
> you envision this script to be shipped to the world (as we'll have to have 
> way to distribute it reliably to all the 'untrusted VM' providers at 
> least). Creating a separate 'smt-disable' package just for this seems like 
> an overkill, OTOH everybody has a process in place how to tweak kernel 
> cmdline / sysfs parameters.

I would add the script to tools/*

It could be added to one of the existing packages that package software
from there, and then you recommend those who actually run untrusted
VMs to use it. Most likely it's more complicated than just tweaking
the command line anyways, because likely you only want it on a subset
of the cores

I spent some time now rewriting my script to C as a "smtctl" and it could be
easily packaged in tools/*

-Andi

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Andi Kleen]

On Thu, Jun 07, 2018 at 08:50:14AM +0200, speck for Thomas Gleixner wrote:
> On Wed, 6 Jun 2018, speck for Andi Kleen wrote:
> > On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> > > The following series is a reworked version of the initial proof of concept
> > > patch. The main changes are:
> > 
> > I still think it's pointless to add so much kernel code for something
> > that can be done with a straight forward user script at run time
> > using existing APIs.
> > 
> > We need strong justifications to add new kernel APIs.
> > 
> > https://raw.githubusercontent.com/andikleen/pmu-tools/master/cputop.py
> 
> It can be done with 3 lines of bash as well.

I don't think so because you need to read the topology. There are systems
which have a different order of logical CPUs.

> But it does neither work from the kernel command line nor does it provide
> protections against re-online 

And why is that needed? Who onlines random CPUs? 

> nor full enforcement. 

What's the point of full enforcement? It only makes sense if you 
run untrusted guest. I cannot think of a credible scenario where
it makes sense.


> Sure it adds the massive amount of 225 lines including comments and
> Documentation, but it's straight forward and works and has it's merits as a
> conveniance/testing mechanism as well.

IMHO a tool is far more convenient than anything in sysfs. Most people
have wrappers anyways because the regist^wsysfs is so complicated
these days (at least I always have to grep for the exact paths)

Given a command line option is also fairly simple, but IMHO it's not really
an use model we should encourage. As Linus said it only really
makes sense if you actually run an untrusted guest. So you should
only do it when actually do that. Making that decision at boot time
is the wrong time. You will just penalize a lot of stuff totally unecessarily.

The best usage for the guests is likely core isolation anyways, and I don't see
how you can do that from the kernel command line. 

-Andi

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Konrad Rzeszutek Wilk]

On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> The following series is a reworked version of the initial proof of concept
> patch. The main changes are:
> 
>   - The primary sibling evaluation has been changed to use APIC ID, so the
>     hacky stuff is gone.
> 
>   - The control has now 3 states: on, off, forceoff
> 
>     forceoff is a irreversible operation and if given on the command line
>     via 'nosmt=force' it makes the processor/APIC enumeration code discard
>     the non primary siblings. That affects also the number of possible CPUs
>     and is more or less equivalent to disabling SMT in the BIOS.
> 
>     If 'forceoff' is written to the sysfs file, then the non primamry
>     siblings are offlined as with 'off', but the operation cannot be
>     undone. That has obviously no effect on num_possible_cpus as that has
>     been evaluated in the early boot process.
> 
>   - Command line and sysfs interface are documented
> 
> Survived testing on various Intel and AMD machines and in VMs of different
> flavours and topology configurations.
> 
> Applies on top of Linus tree.

Any chance you could provide an git bundle for those lazy folks who like
to compile and test it out?

Re: [patch V2 00/12] cpu/hotplug: SMT control

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

On Thu, 7 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> > The following series is a reworked version of the initial proof of concept
> > patch. The main changes are:
> > 
> >   - The primary sibling evaluation has been changed to use APIC ID, so the
> >     hacky stuff is gone.
> > 
> >   - The control has now 3 states: on, off, forceoff
> > 
> >     forceoff is a irreversible operation and if given on the command line
> >     via 'nosmt=force' it makes the processor/APIC enumeration code discard
> >     the non primary siblings. That affects also the number of possible CPUs
> >     and is more or less equivalent to disabling SMT in the BIOS.
> > 
> >     If 'forceoff' is written to the sysfs file, then the non primamry
> >     siblings are offlined as with 'off', but the operation cannot be
> >     undone. That has obviously no effect on num_possible_cpus as that has
> >     been evaluated in the early boot process.
> > 
> >   - Command line and sysfs interface are documented
> > 
> > Survived testing on various Intel and AMD machines and in VMs of different
> > flavours and topology configurations.
> > 
> > Applies on top of Linus tree.
> 
> Any chance you could provide an git bundle for those lazy folks who like
> to compile and test it out?

Sure. Attached.

Thanks,

	tglx

[-- Attachment #2: Type: application/octet-stream, Size: 12586 bytes --]

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Josh Poimboeuf]

On Wed, Jun 06, 2018 at 09:27:14PM +0200, speck for Thomas Gleixner wrote:
> The following series is a reworked version of the initial proof of concept
> patch. The main changes are:
> 
>   - The primary sibling evaluation has been changed to use APIC ID, so the
>     hacky stuff is gone.
> 
>   - The control has now 3 states: on, off, forceoff
> 
>     forceoff is a irreversible operation and if given on the command line
>     via 'nosmt=force' it makes the processor/APIC enumeration code discard
>     the non primary siblings. That affects also the number of possible CPUs
>     and is more or less equivalent to disabling SMT in the BIOS.
> 
>     If 'forceoff' is written to the sysfs file, then the non primamry
>     siblings are offlined as with 'off', but the operation cannot be
>     undone. That has obviously no effect on num_possible_cpus as that has
>     been evaluated in the early boot process.
> 
>   - Command line and sysfs interface are documented
> 
> Survived testing on various Intel and AMD machines and in VMs of different
> flavours and topology configurations.
> 
> Applies on top of Linus tree.

If smt is disabled, should it be reported in the 'l1tf' vulnerabilities
file?

-- 
Josh

[MODERATED] Re: [patch V2 00/12] cpu/hotplug: SMT control

[Jiri Kosina]

On Wed, 6 Jun 2018, speck for Thomas Gleixner wrote:

> The following series is a reworked version of the initial proof of concept
> patch. The main changes are:

I've been torturing it for a few hours today in various ways, and no issue 
popped up, so FWIW

	Tested-by: Jiri Kosina <jkosina@suse.cz>

Thanks,

-- 
Jiri Kosina
SUSE Labs