Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683710 > unrolled thread

[GIT pull] smp/hotplug updates for 4.13

Back to article view | Back to linux.kernel

Contents

  [GIT pull] smp/hotplug updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-09 11:10 +0200
    Re: [GIT pull] smp/hotplug updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-09 20:20 +0200
      Re: [GIT pull] smp/hotplug updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 08:10 +0200

#1683710 — [GIT pull] smp/hotplug updates for 4.13

Linus,

please pull the latest smp-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git smp-urgent-for-linus

A single fix for a brown paperbag bug:

  - The unparking of the initial percpu threads of an upcoming CPU happens
    right now on the idle task, but that's wrong as the unpark function
    might sleep. Move it to the control CPU.

Thanks,

	tglx

------------------>
Thomas Gleixner (1):
      smp/hotplug: Move unparking of percpu threads to the control CPU


 kernel/cpu.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index b03a32595cfe..ab860453841d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -271,11 +271,25 @@ void cpu_hotplug_enable(void)
 EXPORT_SYMBOL_GPL(cpu_hotplug_enable);
 #endif	/* CONFIG_HOTPLUG_CPU */
 
+static void __cpuhp_kick_ap_work(struct cpuhp_cpu_state *st);
+
 static int bringup_wait_for_ap(unsigned int cpu)
 {
 	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
 
+	/* Wait for the CPU to reach CPUHP_AP_ONLINE_IDLE */
 	wait_for_completion(&st->done);
+	BUG_ON(!cpu_online(cpu));
+
+	/* Unpark the stopper thread and the hotplug thread of the target cpu */
+	stop_machine_unpark(cpu);
+	kthread_unpark(st->thread);
+
+	/* Should we go further up ? */
+	if (st->target > CPUHP_AP_ONLINE_IDLE) {
+		__cpuhp_kick_ap_work(st);
+		wait_for_completion(&st->done);
+	}
 	return st->result;
 }
 
@@ -296,9 +310,7 @@ static int bringup_cpu(unsigned int cpu)
 	irq_unlock_sparse();
 	if (ret)
 		return ret;
-	ret = bringup_wait_for_ap(cpu);
-	BUG_ON(!cpu_online(cpu));
-	return ret;
+	return bringup_wait_for_ap(cpu);
 }
 
 /*
@@ -767,31 +779,20 @@ void notify_cpu_starting(unsigned int cpu)
 }
 
 /*
- * Called from the idle task. We need to set active here, so we can kick off
- * the stopper thread and unpark the smpboot threads. If the target state is
- * beyond CPUHP_AP_ONLINE_IDLE we kick cpuhp thread and let it bring up the
- * cpu further.
+ * Called from the idle task. Wake up the controlling task which brings the
+ * stopper and the hotplug thread of the upcoming CPU up and then delegates
+ * the rest of the online bringup to the hotplug thread.
  */
 void cpuhp_online_idle(enum cpuhp_state state)
 {
 	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
-	unsigned int cpu = smp_processor_id();
 
 	/* Happens for the boot cpu */
 	if (state != CPUHP_AP_ONLINE_IDLE)
 		return;
 
 	st->state = CPUHP_AP_ONLINE_IDLE;
-
-	/* Unpark the stopper thread and the hotplug thread of this cpu */
-	stop_machine_unpark(cpu);
-	kthread_unpark(st->thread);
-
-	/* Should we go further up ? */
-	if (st->target > CPUHP_AP_ONLINE_IDLE)
-		__cpuhp_kick_ap_work(st);
-	else
-		complete(&st->done);
+	complete(&st->done);
 }
 
 /* Requires cpu_add_remove_lock to be held */

[toc] | [next] | [standalone]

#1683789

On Sun, Jul 9, 2017 at 2:07 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> +       /* Wait for the CPU to reach CPUHP_AP_ONLINE_IDLE */
>         wait_for_completion(&st->done);
> +       BUG_ON(!cpu_online(cpu));

I realize this isn't actually a new BUG_ON(), just a moved one, but
can we *please* just agree to get rid of these machine-killing
idiocies?

The fact that it would be a bug if this triggers doesn't excuse it one
whit. BUG_ON() is simply bad.

Either it's a "this cannot happen", in which case it should be
deleted, or it's a "if this ever happens, we really should notice it",
in which case it should probably be a

   if (WARN_ON)ONCE(..))
      return -EBUSY;

or something.

That routine very much has an existing error path for failure to bring
up a CPU, dammit!

THERE IS NO EXCUSE FOR KILLING THE MACHINE THERE!

If it really was something core where there is no possible way to
continue, and continuing would imply that the end result will kill
your pets and everbody you love, then yes, use BUG_ON().

But if you already have code to handle the failure case, HELL NO!

I've pulled this thing (exactly because it's not a *new* BUG_ON()),
but I really think that people should look at the code they move. And
if it's a BUG_ON() or similar braindamage, they should look very
closely at what alternatives there are.

               Linus

[toc] | [prev] | [next] | [standalone]

#1684789

On Sun, 9 Jul 2017, Linus Torvalds wrote:
> On Sun, Jul 9, 2017 at 2:07 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > +       /* Wait for the CPU to reach CPUHP_AP_ONLINE_IDLE */
> >         wait_for_completion(&st->done);
> > +       BUG_ON(!cpu_online(cpu));
> 
> I realize this isn't actually a new BUG_ON(), just a moved one, but
> can we *please* just agree to get rid of these machine-killing
> idiocies?
> 
> The fact that it would be a bug if this triggers doesn't excuse it one
> whit. BUG_ON() is simply bad.
> 
> Either it's a "this cannot happen", in which case it should be
> deleted, or it's a "if this ever happens, we really should notice it",
> in which case it should probably be a
> 
>    if (WARN_ON)ONCE(..))
>       return -EBUSY;
> 
> or something.
> 
> That routine very much has an existing error path for failure to bring
> up a CPU, dammit!
> 
> THERE IS NO EXCUSE FOR KILLING THE MACHINE THERE!

Agreed. I noticed when I moved the code, but then forgot. Will queue a fix.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web