Skip to contentSkip to conversation

regression fix: "smpboot: do_boot_cpu failed(-1) to wakeup CPU#1"

PATCH Applied Len Brown 4 participants 10 messages Oct 16, 2015 – Nov 6, 2015 x86

lore
Share & tools
lore

Len Brown

author
#0 lore raw

cover regression fix: "smpboot: do_boot_cpu failed(-1) to wakeup CPU#1"

Shane, Donald,
Please test this pair of patches.

If the default kernel still has a problem booting CPU #1 on yoru Core2 system, "cpu_init_udelay=10000" should get you back to the legacy delays.

thanks,
Len Brown, Intel Open Source Technology Center

author
#1 lore raw

PATCH 1/2 x86 smpboot: fix cpu_init_udelay=10000

From: Len Brown <[email protected]>

For legacy machines cpu_init_udelay defaults to 10,000. For modern machines it is set to 0.

The user should be able to set cpu_init_udelay to any value on the cmdline, including 10,000.
Before this patch, that was seen as "unchanged from default" and thus on a modern machine, the user request was ignored and the delay was set to 0.

---
 arch/x86/kernel/smpboot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
arch/x86/kernel/smpboot.c +5 -2
index e0c198e..32267cc 100644
@@ -509,7 +509,7 @@ void __inquire_remote_apic(int apicid)
  */
 #define UDELAY_10MS_DEFAULT 10000
 
-static unsigned int init_udelay = UDELAY_10MS_DEFAULT;
+static unsigned int init_udelay = INT_MAX;
 
 static int __init cpu_init_udelay(char *str)
 {
@@ -522,13 +522,16 @@ early_param("cpu_init_udelay", cpu_init_udelay);
 static void __init smp_quirk_init_udelay(void)
 {
 	/* if cmdline changed it from default, leave it alone */
-	if (init_udelay != UDELAY_10MS_DEFAULT)
+	if (init_udelay != INT_MAX)
 		return;
 
 	/* if modern processor, use no delay */
 	if (((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) && (boot_cpu_data.x86 == 6)) ||
 	    ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && (boot_cpu_data.x86 >= 0xF)))
 		init_udelay = 0;
+
+	/* else, use legacy delay */
+	init_udelay = UDELAY_10MS_DEFAULT;
 }
 
 /*
Signature · 1 line
2.6.1.145.gb27dacc
#2 lore raw

Re: [PATCH 1/2] x86 smpboot: fix cpu_init_udelay=10000

Quoted 34 lines

From: Len Brown <[email protected]>

For legacy machines cpu_init_udelay defaults to 10,000. For modern machines it is set to 0.

The user should be able to set cpu_init_udelay to any value on the cmdline, including 10,000.
Before this patch, that was seen as "unchanged from default" and thus on a modern machine, the user request was ignored and the delay was set to 0.

---
 arch/x86/kernel/smpboot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
arch/x86/kernel/smpboot.c +1 -2
index e0c198e..32267cc 100644
@@ -509,7 +509,7 @@ void __inquire_remote_apic(int apicid)
  */
 #define UDELAY_10MS_DEFAULT 10000
 
-static unsigned int init_udelay = UDELAY_10MS_DEFAULT;
+static unsigned int init_udelay = INT_MAX;
 
 static int __init cpu_init_udelay(char *str)
 {
@@ -522,13 +522,16 @@ early_param("cpu_init_udelay", cpu_init_udelay);
 static void __init smp_quirk_init_udelay(void)
 {
      /* if cmdline changed it from default, leave it alone */
-     if (init_udelay != UDELAY_10MS_DEFAULT)

arch/x86/kernel/smpboot.c early_param("cpu_init_udelay", cpu_init_udelay); · line 534

Quoted 10 lines
arch/x86/kernel/smpboot.c +4 -0
+     if (init_udelay != INT_MAX)
           return;
 
      /* if modern processor, use no delay */
      if (((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) && (boot_cpu_data.x86 == 6)) ||
          ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && (boot_cpu_data.x86 >= 0xF)))
           init_udelay = 0;
+
+     /* else, use legacy delay */
+     init_udelay = UDELAY_10MS_DEFAULT;

Hi Len,

Is this really what you intended? The else is commented out so if init_udelay is quirked to be 0 it will always be reset to UDELAY_10MS_DEFAULT. Also init_udelay is unsigned, so would UINT_MAX be a better choice?

Shane

author
#3 lore raw

Re: [PATCH 1/2] x86 smpboot: fix cpu_init_udelay=10000

Quoted 3 lines

Is this really what you intended? The else is commented out so if init_udelay is quirked to be 0 it will always be reset to UDELAY_10MS_DEFAULT. Also init_udelay is unsigned, so would UINT_MAX be a better choice?

Hi Shane,
Thanks for pointing out this flaw.
Seems it will make 4.3 10ms slower than 4.2 on these boxes, by default. We'll send an incremental patch for 4.4.

Len Brown, Intel Open Source Technology Center

#4 lore raw

Re: [PATCH 1/2] x86 smpboot: fix cpu_init_udelay=10000

Quoted 8 lines
Quoted 3 lines

Is this really what you intended? The else is commented out so if init_udelay is quirked to be 0 it will always be reset to UDELAY_10MS_DEFAULT. Also init_udelay is unsigned, so would UINT_MAX be a better choice?

Hi Shane,
Thanks for pointing out this flaw.
Seems it will make 4.3 10ms slower than 4.2 on these boxes, by default. We'll send an incremental patch for 4.4.

Sounds good. Thanks Len.

#5 lore raw

[tip:x86/urgent] x86/smpboot: Fix cpu_init_udelay= 10000 corner case boot parameter misbehavior

In reply to Len Brown
x86/smpboot: Fix cpu_init_udelay=10000 corner case boot parameter misbehavior
Commit-ID:  f1ccd249319efca4ee4faf1d904f5a362cac7c81
Gitweb:     http://git.kernel.org/tip/f1ccd249319efca4ee4faf1d904f5a362cac7c81
Author:     Len Brown <[email protected]>
AuthorDate: Fri, 16 Oct 2015 00:14:28 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 19 Oct 2015 09:14:41 +0200

x86/smpboot: Fix cpu_init_udelay=10000 corner case boot parameter misbehavior

For legacy machines cpu_init_udelay defaults to 10,000. For modern machines it is set to 0.

The user should be able to set cpu_init_udelay to
any value on the cmdline, including 10,000.

Before this patch, that was seen as "unchanged from default" and thus on a modern machine, the user request was ignored and the delay was set to 0.

---
 arch/x86/kernel/smpboot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
arch/x86/kernel/smpboot.c +5 -2
index e0c198e..32267cc 100644
@@ -509,7 +509,7 @@ void __inquire_remote_apic(int apicid)
  */
 #define UDELAY_10MS_DEFAULT 10000
 
-static unsigned int init_udelay = UDELAY_10MS_DEFAULT;
+static unsigned int init_udelay = INT_MAX;
 
 static int __init cpu_init_udelay(char *str)
 {
@@ -522,13 +522,16 @@ early_param("cpu_init_udelay", cpu_init_udelay);
 static void __init smp_quirk_init_udelay(void)
 {
 	/* if cmdline changed it from default, leave it alone */
-	if (init_udelay != UDELAY_10MS_DEFAULT)
+	if (init_udelay != INT_MAX)
 		return;
 
 	/* if modern processor, use no delay */
 	if (((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) && (boot_cpu_data.x86 == 6)) ||
 	    ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && (boot_cpu_data.x86 >= 0xF)))
 		init_udelay = 0;
+
+	/* else, use legacy delay */
+	init_udelay = UDELAY_10MS_DEFAULT;
 }
 
 /*
author
#6 lore raw

PATCH 2/2 x86 smpboot: fix CPU #1 boot timeout

In reply to Len Brown

From: Len Brown <[email protected]>

Commit a9bcaa02a5104ace6a9d9e4a9cd9192a9e7744d6
("x86/smpboot: Remove SIPI delays from cpu_up()")

Caused some Intel Core2 processors to time-out when bringing up CPU #1.

That patch reduced the SIPI delays from udelay() 300, 200 to udelay() 0, 0 on modern processors.

Several Intel(R) Core(TM)2 systems
failed to bring up CPU #1 10/10 times after that change.

Increasing either of the SIPI delays to udelay(1) results in success. So here we increase both to udelay(10). While this may be 20x slower than the absolute minimum, it is still 20x to 30x faster than the original code.

---
 arch/x86/kernel/smpboot.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/x86/kernel/smpboot.c +6 -2
index 32267cc..892ee2e5 100644
@@ -660,7 +660,9 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
 		/*
 		 * Give the other CPU some time to accept the IPI.
 		 */
-		if (init_udelay)
+		if (init_udelay == 0)
+			udelay(10);
+		else
 			udelay(300);
 
 		pr_debug("Startup point 1\n");
@@ -671,7 +673,9 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
 		/*
 		 * Give the other CPU some time to accept the IPI.
 		 */
-		if (init_udelay)
+		if (init_udelay == 0)
+			udelay(10);
+		else
 			udelay(200);
 
 		if (maxlvt > 3)		/* Due to the Pentium erratum 3AP.  */
Signature · 1 line
2.6.1.145.gb27dacc
#7 lore raw

[tip:x86/urgent] x86/smpboot: Fix CPU #1 boot timeout

x86/smpboot: Fix CPU #1 boot timeout
Commit-ID:  fcafddec4e78a7776db4b6685db6b2902d4300fc
Gitweb:     http://git.kernel.org/tip/fcafddec4e78a7776db4b6685db6b2902d4300fc
Author:     Len Brown <[email protected]>
AuthorDate: Fri, 16 Oct 2015 00:14:29 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 19 Oct 2015 09:14:41 +0200

x86/smpboot: Fix CPU #1 boot timeout

The following commit:

a9bcaa02a5104ac ("x86/smpboot: Remove SIPI delays from cpu_up()")

Caused some Intel Core2 processors to time-out when bringing up CPU #1, resulting in the missing of that CPU after bootup.

That patch reduced the SIPI delays from udelay() 300, 200 to udelay() 0, 0 on modern processors.

Several Intel(R) Core(TM)2 systems failed to bring up CPU #1 10/10 times after that change.

Increasing either of the SIPI delays to udelay(1) results in success. So here we increase both to udelay(10). While this may be 20x slower than the absolute minimum, it is still 20x to 30x faster than the original code.

---
 arch/x86/kernel/smpboot.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/x86/kernel/smpboot.c +6 -2
index 32267cc..892ee2e5 100644
@@ -660,7 +660,9 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
 		/*
 		 * Give the other CPU some time to accept the IPI.
 		 */
-		if (init_udelay)
+		if (init_udelay == 0)
+			udelay(10);
+		else
 			udelay(300);
 
 		pr_debug("Startup point 1\n");
@@ -671,7 +673,9 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
 		/*
 		 * Give the other CPU some time to accept the IPI.
 		 */
-		if (init_udelay)
+		if (init_udelay == 0)
+			udelay(10);
+		else
 			udelay(200);
 
 		if (maxlvt > 3)		/* Due to the Pentium erratum 3AP.  */
#8 lore raw
In reply to Len Brown
Quoted 5 lines · Len Brown

Shane, Donald,
Please test this pair of patches.

If the default kernel still has a problem booting CPU #1 on your Core2 system, "cpu_init_udelay=10000" should get you back to the legacy delays.

Applied patches to Linus' 4.3-rc4 (with none of our recently discussed changes.)

Result missing CPU is back!

I also tried the "cpu_init_udelay=1" with rc5 and that also gave me back the missing CPU? Previously I had tried "cpu_init_udelay=10000" and it did not get missing CPU back (with rc3 or rc4).

I can do more testing if you need it.

Thanks,
Don

#9 lore raw
In reply to Len Brown
Quoted 5 lines

Shane, Donald,
Please test this pair of patches.

If the default kernel still has a problem booting CPU #1 on yoru Core2 system, "cpu_init_udelay=10000" should get you back to the legacy delays.

I tried with both patches and it worked fine without cpu_init_udelay.

Thanks,

Shane