Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756571AbaFAJYA (ORCPT ); Sun, 1 Jun 2014 05:24:00 -0400 Received: from ar-005-i203.relay.mailchannels.net ([162.253.144.85]:29701 "EHLO relay.mailchannels.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751346AbaFAJX6 (ORCPT ); Sun, 1 Jun 2014 05:23:58 -0400 X-Sender-Id: totalchoicehosting|x-authuser|oren%2bscalemp.com X-Sender-Id: totalchoicehosting|x-authuser|oren%2bscalemp.com X-MC-Relay: Neutral X-MailChannels-SenderId: totalchoicehosting%7Cx-authuser%7Coren%252bscalemp.com X-MailChannels-Auth-Id: totalchoicehosting Message-ID: <538AF11F.8060802@scalemp.com> Date: Sun, 01 Jun 2014 12:23:43 +0300 From: Oren Twaig User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Prarit Bhargava CC: linux-kernel@vger.kernel.org, pbonzini@redhat.com, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Borislav Petkov , Paul Gortmaker , Andrew Morton , Andi Kleen , Dave Jones , Torsten Kaiser , Jan Beulich , Jan Kiszka , Toshi Kani , Andrew Jones , "Shai (Shai@ScaleMP.com)" Subject: Re: [PATCH 1/2] x86, Clean up smp_num_siblings calculation References: <1401450185-23061-1-git-send-email-prarit@redhat.com> <1401450185-23061-2-git-send-email-prarit@redhat.com> In-Reply-To: <1401450185-23061-2-git-send-email-prarit@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-AuthUser: oren+scalemp.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Prarit, See below, On 05/30/2014 02:43 PM, Prarit Bhargava wrote: > I have a system on which I have disabled threading in the BIOS, and I am booting > the kernel with the option "idle=poll". > > The kernel displays > > process: WARNING: polling idle and HT enabled, performance may degrade > > which is incorrect -- I've already disabled HT. > > This warning is issued here: > > void select_idle_routine(const struct cpuinfo_x86 *c) > { > if (boot_option_idle_override == IDLE_POLL && smp_num_siblings > 1) > pr_warn_once("WARNING: polling idle and HT enabled, performance may degrade\n"); > > >From my understanding of the other ares of kernel that use > smp_num_siblings, the value is supposed to be the the number of threads > per core. > > The value of smp_num_siblings is incorrect. In theory, it should be 1 but it > is reported as 2. When I looked into how smp_num_siblings is calculated I > found the following call sequence in the kernel: > > start_kernel -> > check_bugs -> > identify_boot_cpu -> > identify_cpu -> > c_init = init_intel > init_intel -> > detect_extended_topology > (sets value) > > OR > > c_init = init_amd > init_amd -> amd_detect_cmp > -> amd_get_topology > (sets value) > -> detect_ht() > ... (sets value) > detect_ht() > (also sets value) > > ie) it is set three times in some cases and is overwritten by the call > to detect_ht() from identify_cpu() in all cases. > > It should be noted that nothing in the identify_cpu() path or the cpu_up() > path requires smp_num_siblings to be set, prior to the final call to > detect_ht(). > > For x86 boxes, smp_num_siblings is set to a value read in a CPUID call in > detect_ht(). This value is the *factory defined* value in all cases; even > if HT is disabled in BIOS the value still returns 2 if the CPU supports > HT. AMD also reports the factory defined value in all cases. The above is incorrect in case of X-TOPOLOGY mode. I such a case the information about number of siblings comes from the LEVEL_MAX_SIBLINGS() macro and the X86_FEATURE_XTOPOLOGY flag is set to skip detect_ht() work : void detect_ht(struct cpuinfo_x86 *c) ... if (cpu_has(c, X86_FEATURE_XTOPOLOGY)) return; In addition, the information about the number of sibling no longer comes from CPUID(0x1)->ebx but rather from the 0xb leaf of CPUID. I've marked below the problematic code change. Thanks, Oren Twaig > > Other uses of smp_num_siblings involve oprofile (used after boot), and > the perf code which is done well after the initial cpus are brought online. > > This patch removes dead code and moves the assignment of smp_num_siblings > to only the detect_ht() code; it is still always reporting 2. A follow > on patch will fix the calculation. > > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: x86@kernel.org > Cc: Borislav Petkov > Cc: Paul Gortmaker > Cc: Andrew Morton > Cc: Andi Kleen > Cc: Dave Jones > Cc: Torsten Kaiser > Cc: Jan Beulich > Cc: Jan Kiszka > Cc: Toshi Kani > Cc: Andrew Jones > Signed-off-by: Prarit Bhargava > --- > arch/x86/kernel/cpu/amd.c | 1 - > arch/x86/kernel/cpu/common.c | 23 +++++++++++------------ > arch/x86/kernel/cpu/topology.c | 2 +- > arch/x86/kernel/smpboot.c | 5 ++--- > 4 files changed, 14 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index ce8b8ff..6aca2b6 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -304,7 +304,6 @@ static void amd_get_topology(struct cpuinfo_x86 *c) > node_id = ecx & 7; > > /* get compute unit information */ > - smp_num_siblings = ((ebx >> 8) & 3) + 1; > c->compute_unit_id = ebx & 0xff; > cores_per_cu += ((ebx >> 8) & 3); > } else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) { > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index a135239..fc1235c 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -507,42 +507,41 @@ void detect_ht(struct cpuinfo_x86 *c) > u32 eax, ebx, ecx, edx; > int index_msb, core_bits; > static bool printed; > + int threads_per_core; > > if (!cpu_has(c, X86_FEATURE_HT)) > return; > > - if (cpu_has(c, X86_FEATURE_CMP_LEGACY)) > + if (cpu_has(c, X86_FEATURE_CMP_LEGACY)) { > + threads_per_core = 1; > goto out; > + } > > if (cpu_has(c, X86_FEATURE_XTOPOLOGY)) > return; > > cpuid(1, &eax, &ebx, &ecx, &edx); > > - smp_num_siblings = (ebx & 0xff0000) >> 16; > + threads_per_core = smp_num_siblings = (ebx & 0xff0000) >> 16; > > - if (smp_num_siblings == 1) { > - printk_once(KERN_INFO "CPU0: Hyper-Threading is disabled\n"); > + if (threads_per_core <= 1) { > + pr_info_once("CPU: Hyper-Threading is unsupported on this processor.\n"); > goto out; > } > > - if (smp_num_siblings <= 1) > - goto out; > - > - index_msb = get_count_order(smp_num_siblings); > + index_msb = get_count_order(threads_per_core); > c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid, index_msb); > > - smp_num_siblings = smp_num_siblings / c->x86_max_cores; > + threads_per_core = threads_per_core / c->x86_max_cores; > > - index_msb = get_count_order(smp_num_siblings); > + index_msb = get_count_order(threads_per_core); > > core_bits = get_count_order(c->x86_max_cores); > > 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) { > + if (!printed && (c->x86_max_cores * threads_per_core) > 1) { > printk(KERN_INFO "CPU: Physical Processor ID: %d\n", > c->phys_proc_id); > printk(KERN_INFO "CPU: Processor Core ID: %d\n", > diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c > index 4c60eaf..a9b837e 100644 > --- a/arch/x86/kernel/cpu/topology.c > +++ b/arch/x86/kernel/cpu/topology.c > @@ -55,7 +55,7 @@ void detect_extended_topology(struct cpuinfo_x86 *c) > /* > * Populate HT related information from sub-leaf level 0. > */ > - core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx); > + core_level_siblings = LEVEL_MAX_SIBLINGS(ebx); The above is the problem which will make smp_num_sibling to be uninitialised in case of X-TOPOLOGY. > core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax); > > sub_index = 1; > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 3482693..b2ad27c 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -351,8 +351,7 @@ static bool match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) > > void set_cpu_sibling_map(int cpu) > { > - bool has_smt = smp_num_siblings > 1; > - bool has_mp = has_smt || boot_cpu_data.x86_max_cores > 1; > + bool has_mp = boot_cpu_data.x86_max_cores > 1; > struct cpuinfo_x86 *c = &cpu_data(cpu); > struct cpuinfo_x86 *o; > int i; > @@ -370,7 +369,7 @@ void set_cpu_sibling_map(int cpu) > for_each_cpu(i, cpu_sibling_setup_mask) { > o = &cpu_data(i); > > - if ((i == cpu) || (has_smt && match_smt(c, o))) > + if ((i == cpu) || match_smt(c, o)) > link_mask(sibling, cpu, i); > > if ((i == cpu) || (has_mp && match_llc(c, o))) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/