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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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
>