x86/smpboot: Unbreak CPU0 hotplug

PATCH RESEND In review Vitaly Kuznetsov 4 participants 6 messages Aug 3, 2017 – Aug 14, 2017 x86

Share & tools

Vitaly Kuznetsov

author

2017-08-03 10:58 UTC #0 lore raw

PATCH RESEND 1/1 x86/smpboot: Unbreak CPU0 hotplug

A hang on CPU0 onlining after a preceding offlining is observed. Trace shows that CPU0 is stuck in check_tsc_sync_target() waiting for source CPU to run check_tsc_sync_source() but this never happens. Source CPU, in its turn, is stuck on synchronize_sched() which is called from native_cpu_up() -> do_boot_cpu() -> unregister_nmi_handler().

Fix the issue by moving unregister_nmi_handler() from do_boot_cpu() to native_cpu_up() after cpu onlining is done.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
It's been awile since my v1 submission, no comments so far. Resending.

---
 arch/x86/kernel/smpboot.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

arch/x86/kernel/smpboot.c +17 ~~-13~~

index b474c8de7fba..54b9e89d4d6b 100644
@@ -971,7 +971,8 @@ void common_cpu_up(unsigned int cpu, struct task_struct *idle)
  * Returns zero if CPU booted OK, else error code from
  * ->wakeup_secondary_cpu.
  */
-static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
+static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
+		       int *cpu0_nmi_registered)
 {
 	volatile u32 *trampoline_status =
 		(volatile u32 *) __va(real_mode_header->trampoline_status);

@@ -979,7 +980,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 	unsigned long start_ip = real_mode_header->trampoline_start;
 
 	unsigned long boot_error = 0;
-	int cpu0_nmi_registered = 0;
 	unsigned long timeout;
 
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);

@@ -1035,7 +1035,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 		boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
 	else
 		boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
-						     &cpu0_nmi_registered);
+						     cpu0_nmi_registered);
 
 	if (!boot_error) {
 		/*

@@ -1080,12 +1080,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 		 */
 		smpboot_restore_warm_reset_vector();
 	}
-	/*
-	 * Clean up the nmi handler. Do this after the callin and callout sync
-	 * to avoid impact of possible long unregister time.
-	 */
-	if (cpu0_nmi_registered)
-		unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
 
 	return boot_error;
 }

@@ -1093,8 +1087,9 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int apicid = apic->cpu_present_to_apicid(cpu);
+	int cpu0_nmi_registered = 0;
 	unsigned long flags;
-	int err;
+	int err, ret = 0;
 
 	WARN_ON(irqs_disabled());

@@ -1131,10 +1126,11 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 
 	common_cpu_up(cpu, tidle);
 
-	err = do_boot_cpu(apicid, cpu, tidle);
+	err = do_boot_cpu(apicid, cpu, tidle, &cpu0_nmi_registered);
 	if (err) {
 		pr_err("do_boot_cpu failed(%d) to wakeup CPU#%u\n", err, cpu);
-		return -EIO;
+		ret = -EIO;
+		goto unreg_nmi;
 	}
 
 	/*

@@ -1150,7 +1146,15 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 		touch_nmi_watchdog();
 	}
 
-	return 0;
+unreg_nmi:
+	/*
+	 * Clean up the nmi handler. Do this after the callin and callout sync
+	 * to avoid impact of possible long unregister time.
+	 */
+	if (cpu0_nmi_registered)
+		unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
+
+	return ret;
 }
 
 /**

Signature · 1 line

2.13.3

Vitaly Kuznetsov

author

2017-08-10 12:01 UTC #1 lore raw

Quoted 6 lines · Vitaly Kuznetsov

A hang on CPU0 onlining after a preceding offlining is observed. Trace shows that CPU0 is stuck in check_tsc_sync_target() waiting for source CPU to run check_tsc_sync_source() but this never happens. Source CPU, in its turn, is stuck on synchronize_sched() which is called from native_cpu_up() -> do_boot_cpu() -> unregister_nmi_handler().

Quoted 6 lines

Fix the issue by moving unregister_nmi_handler() from do_boot_cpu() to native_cpu_up() after cpu onlining is done.
Signed-off-by Vitaly Kuznetsov <[email protected]>
---
It's been awile since my v1 submission, no comments so far. Resending.

Sorry, but

ping?

I haven't received a single comment on this since the initial submission on June, 26 - is it so bad? :-)

Signature · 1 line

  Vitaly

Ingo Molnar

2017-08-10 14:23 UTC #2 lore raw

Quoted 10 lines · Vitaly Kuznetsov

Quoted 8 lines · Vitaly Kuznetsov
A hang on CPU0 onlining after a preceding offlining is observed. Trace shows that CPU0 is stuck in check_tsc_sync_target() waiting for source CPU to run check_tsc_sync_source() but this never happens. Source CPU, in its turn, is stuck on synchronize_sched() which is called from native_cpu_up() -> do_boot_cpu() -> unregister_nmi_handler().
Fix the issue by moving unregister_nmi_handler() from do_boot_cpu() to native_cpu_up() after cpu onlining is done.

Looks like a classic ABBA deadlock, due to the use of synchronize_sched() in unregister_nmi_handler(), right?

Quoted 11 lines

Quoted 4 lines
Signed-off-by Vitaly Kuznetsov <[email protected]>
---
It's been awile since my v1 submission, no comments so far. Resending.
Sorry, but
ping?
I haven't received a single comment on this since the initial submission on June, 26 - is it so bad? :-)

So the fix looks good to me at first sight, but wanted to wait for Thomas to ack it - once he gets back from vacation.

Thanks,

Ingo

Vitaly Kuznetsov

author

2017-08-10 14:56 UTC #3 lore raw

Quoted 10 lines · Ingo Molnar

Quoted 8 lines · Vitaly Kuznetsov
Quoted 6 lines · Vitaly Kuznetsov
A hang on CPU0 onlining after a preceding offlining is observed. Trace shows that CPU0 is stuck in check_tsc_sync_target() waiting for source CPU to run check_tsc_sync_source() but this never happens. Source CPU, in its turn, is stuck on synchronize_sched() which is called from native_cpu_up() -> do_boot_cpu() -> unregister_nmi_handler().

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
Fix the issue by moving unregister_nmi_handler() from do_boot_cpu() to native_cpu_up() after cpu onlining is done.
Looks like a classic ABBA deadlock, due to the use of synchronize_sched() in unregister_nmi_handler(), right?

Exactly.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Signed-off-by Vitaly Kuznetsov <[email protected]>
---
It's been awile since my v1 submission, no comments so far. Resending.
Sorry, but

Quoted 8 lines

Quoted 4 lines
ping?
I haven't received a single comment on this since the initial submission on June, 26 - is it so bad? :-)
So the fix looks good to me at first sight, but wanted to wait for Thomas to ack it - once he gets back from vacation.

Thanks!

Signature · 1 line

  Vitaly

Thomas Gleixner

Acked-by

2017-08-14 07:09 UTC #4 lore raw

In reply to Ingo Molnar

Quoted 3 lines · Ingo Molnar

Quoted 2 lines · Vitaly Kuznetsov
I haven't received a single comment on this since the initial submission on June, 26 - is it so bad? :-)

Sorry slipped through the cracks.

Quoted 2 lines

So the fix looks good to me at first sight, but wanted to wait for Thomas to ack it - once he gets back from vacation.

Acked-by Thomas Gleixner <[email protected]>

tip-bot for Vitaly Kuznetsov

merge bot

2017-08-10 16:43 UTC #5 lore raw

In reply to Vitaly Kuznetsov

x86/smpboot: Unbreak CPU0 hotplug

Commit-ID:  10e66760fa8ee11f254a69433fc132d04758a5fc
Gitweb:     http://git.kernel.org/tip/10e66760fa8ee11f254a69433fc132d04758a5fc
Author:     Vitaly Kuznetsov <[email protected]>
AuthorDate: Thu, 3 Aug 2017 12:58:18 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 10 Aug 2017 17:02:47 +0200

x86/smpboot: Unbreak CPU0 hotplug

So it's a classic ABBA deadlock, due to the use of synchronize_sched() in unregister_nmi_handler().

Fix the bug by moving unregister_nmi_handler() from do_boot_cpu() to native_cpu_up() after cpu onlining is done.

Signed-off-by Vitaly Kuznetsov <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/smpboot.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

arch/x86/kernel/smpboot.c +17 ~~-13~~

index b474c8d..54b9e89 100644
@@ -971,7 +971,8 @@ void common_cpu_up(unsigned int cpu, struct task_struct *idle)
  * Returns zero if CPU booted OK, else error code from
  * ->wakeup_secondary_cpu.
  */
-static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
+static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
+		       int *cpu0_nmi_registered)
 {
 	volatile u32 *trampoline_status =
 		(volatile u32 *) __va(real_mode_header->trampoline_status);

@@ -979,7 +980,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 	unsigned long start_ip = real_mode_header->trampoline_start;
 
 	unsigned long boot_error = 0;
-	int cpu0_nmi_registered = 0;
 	unsigned long timeout;
 
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);

@@ -1035,7 +1035,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 		boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
 	else
 		boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
-						     &cpu0_nmi_registered);
+						     cpu0_nmi_registered);
 
 	if (!boot_error) {
 		/*

@@ -1080,12 +1080,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 		 */
 		smpboot_restore_warm_reset_vector();
 	}
-	/*
-	 * Clean up the nmi handler. Do this after the callin and callout sync
-	 * to avoid impact of possible long unregister time.
-	 */
-	if (cpu0_nmi_registered)
-		unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
 
 	return boot_error;
 }

@@ -1093,8 +1087,9 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int apicid = apic->cpu_present_to_apicid(cpu);
+	int cpu0_nmi_registered = 0;
 	unsigned long flags;
-	int err;
+	int err, ret = 0;
 
 	WARN_ON(irqs_disabled());

@@ -1131,10 +1126,11 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 
 	common_cpu_up(cpu, tidle);
 
-	err = do_boot_cpu(apicid, cpu, tidle);
+	err = do_boot_cpu(apicid, cpu, tidle, &cpu0_nmi_registered);
 	if (err) {
 		pr_err("do_boot_cpu failed(%d) to wakeup CPU#%u\n", err, cpu);
-		return -EIO;
+		ret = -EIO;
+		goto unreg_nmi;
 	}
 
 	/*

@@ -1150,7 +1146,15 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 		touch_nmi_watchdog();
 	}
 
-	return 0;
+unreg_nmi:
+	/*
+	 * Clean up the nmi handler. Do this after the callin and callout sync
+	 * to avoid impact of possible long unregister time.
+	 */
+	if (cpu0_nmi_registered)
+		unregister_nmi_handler(NMI_LOCAL, "wake_cpu0");
+
+	return ret;
 }
 
 /**