[PATCH v1] x86/smpboot: broken calibration path during cpu bringup

* [PATCH v1] x86/smpboot: broken calibration path during cpu bringup
@ 2017-10-28  0:11 Pavel Tatashin
  2017-11-07 12:19 ` [tip:x86/urgent] x86/smpboot: Make optimization of delay calibration work correctly tip-bot for Pavel Tatashin
  2017-11-07 15:09 ` tip-bot for Pavel Tatashin
  0 siblings, 2 replies; 3+ messages in thread
From: Pavel Tatashin @ 2017-10-28  0:11 UTC (permalink / raw)
  To: steven.sistare, daniel.m.jordan, bob.picco, tglx, peterz,
	linux-kernel, x86, hpa

While studying why it takes 0.06s to bring up every cpu, which accounts to
15.36s on 256 cpu system, I determined that it is all because of
calibrate_delay() call.

After, studying code further I found that there are bugs in the current
code:

If tsc is enabled, and cpu has TSC_CONSTANT feature, and a cpu is in the
same core has already been calibrated, we do not need to calibrate again:

This check is done here:

calibrate_delay()
	calibrate_delay_is_known()

But, calibrate_delay() is called before topology for new cpu is updated,
so we never actually take the optimized path.

The second bug, is that inside calibrate_delay_is_known() there is branch
like this:

if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
	return 0;

But the logic is broken, it should be:

if (tsc_disabled || !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
	return 0;

Fixes: c25323c07345 ("x86/tsc: Use topology functions")

Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
---
 arch/x86/kernel/smpboot.c | 13 ++++++++-----
 arch/x86/kernel/tsc.c     |  6 ++----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ad59edd84de7..e7a3bab6818b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -193,6 +193,14 @@ static void smp_callin(void)
 	 */
 	smp_store_cpu_info(cpuid);
 
+	/*
+	 * This must be done before setting cpu_online_mask
+	 * or calling notify_cpu_starting.
+	 * And also before calibrate_delay(), as the information about topology
+	 * is used to determine if calibration is needed.
+	 */
+	set_cpu_sibling_map(raw_smp_processor_id());
+
 	/*
 	 * Get our bogomips.
 	 * Update loops_per_jiffy in cpu_data. Previous call to
@@ -203,11 +211,6 @@ static void smp_callin(void)
 	cpu_data(cpuid).loops_per_jiffy = loops_per_jiffy;
 	pr_debug("Stack at about %p\n", &cpuid);
 
-	/*
-	 * This must be done before setting cpu_online_mask
-	 * or calling notify_cpu_starting.
-	 */
-	set_cpu_sibling_map(raw_smp_processor_id());
 	wmb();
 
 	notify_cpu_starting(cpuid);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 796d96bb0821..a99cde96201f 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1346,12 +1346,10 @@ void __init tsc_init(void)
 unsigned long calibrate_delay_is_known(void)
 {
 	int sibling, cpu = smp_processor_id();
+	int constant_tsc = cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC);
 	struct cpumask *mask = topology_core_cpumask(cpu);
 
-	if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
-		return 0;
-
-	if (!mask)
+	if (tsc_disabled || !constant_tsc || !mask)
 		return 0;
 
 	sibling = cpumask_any_but(mask, cpu);
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread