From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 12 Jun 2018 10:27:48 -0000 Received: from smtp.eu.citrix.com ([185.25.65.24]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fSgWg-0004mY-Kl for speck@linutronix.de; Tue, 12 Jun 2018 12:27:47 +0200 Subject: [MODERATED] Re: [patch V2 02/12] x86/smp: Provide topology_is_primary_thread() References: <20180606192714.754943543@linutronix.de> <20180606192807.093672737@linutronix.de> <20180611193245.GB25607@char.us.oracle.com> <20180611201557.GB25592@char.us.oracle.com> From: Andrew Cooper Message-ID: <8a1fe85e-d537-8b83-04c2-bdf9033f3a04@citrix.com> Date: Tue, 12 Jun 2018 11:27:37 +0100 MIME-Version: 1.0 In-Reply-To: <20180611201557.GB25592@char.us.oracle.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Content-Language: en-GB To: speck@linutronix.de List-ID: 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 >>> >>> 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 >>> --- >>> 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