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 <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Cc: Borislav Petkov <[email protected]>
> Cc: Paul Gortmaker <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Dave Jones <[email protected]>
> Cc: Torsten Kaiser <[email protected]>
> Cc: Jan Beulich <[email protected]>
> Cc: Jan Kiszka <[email protected]>
> Cc: Toshi Kani <[email protected]>
> Cc: Andrew Jones <[email protected]>
> Signed-off-by: Prarit Bhargava <[email protected]>
> ---
> 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)))
On 06/01/2014 05:23 AM, Oren Twaig wrote:
> 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.
I will do a [v2] of the patchset that omits this change
>> - core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
>> + core_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
and then removes the setting of smp_num_siblings in 2/2.
Thanks,
P.