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

Groups > linux.kernel > #1196161 > unrolled thread

Re: [PATCH] sched: fix cpu_active_mask/cpu_online_mask race

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] sched: fix cpu_active_mask/cpu_online_mask race Peter Zijlstra <peterz@infradead.org> - 2015-07-30 18:10 +0200
    [PATCH v2] sched: fix cpu_active_mask/cpu_online_mask race Jan H. Schönherr <jschoenh@amazon.de> - 2015-07-30 21:20 +0200

#1196161 — Re: [PATCH] sched: fix cpu_active_mask/cpu_online_mask race

On Mon, Jul 27, 2015 at 08:21:28PM +0200, Jan H. Schönherr wrote:
> From: Jan H. Schönherr <jschoenh@amazon.de>
> 
> Together, these give us the following non-working solutions:
> - secondary CPU sets active before online, because active is assumed to
>   be a subset of online;
> - secondary CPU sets online before active, because the primary CPU
>   assumes that an online CPU is also active;
> - secondary CPU sets online and waits for primary CPU to set active,
>   because it might deadlock.
> 
> Commit 875ebe940d77 ("powerpc/smp: Wait until secondaries are active &
> online") introduces an arch-specific solution to this arch-independent
> problem.
> 
> Now, go for a more general solution without explicit waiting and simply
> set active twice: once on the secondary CPU after online was set and
> once on the primary CPU after online was seen.

Very vile indeed ;-)

> Fixes: 6acbfb96976f ("sched: Fix hotplug vs. set_cpus_allowed_ptr()")
> Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/sched/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 78b4bad10..155bb4a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5433,6 +5433,7 @@ static int sched_cpu_active(struct notifier_block *nfb,
>  	case CPU_STARTING:
>  		set_cpu_rq_start_time();
>  		return NOTIFY_OK;
> +	case CPU_ONLINE:

Not without a big honking comment right there :-)

>  	case CPU_DOWN_FAILED:
>  		set_cpu_active((long)hcpu, true);
>  		return NOTIFY_OK;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1196383 — [PATCH v2] sched: fix cpu_active_mask/cpu_online_mask race

From: Jan H. Schönherr <jschoenh@amazon.de>

There is a race condition in SMP bootup code, which may result in

    WARNING: CPU: 0 PID: 1 at kernel/workqueue.c:4418
    workqueue_cpu_up_callback()
or
    kernel BUG at kernel/smpboot.c:135!

It can be triggered with a bit of luck in Linux guests running on
busy hosts.

CPU0                        CPUn
====                        ====

_cpu_up()
  __cpu_up()
                            start_secondary()
                              set_cpu_online()
                                cpumask_set_cpu(cpu,
                                           to_cpumask(cpu_online_bits));
  cpu_notify(CPU_ONLINE)
    <do stuff, see below>
                                cpumask_set_cpu(cpu,
                                           to_cpumask(cpu_active_bits));

During the various CPU_ONLINE callbacks CPUn is online but not active.
Several things can go wrong at that point, depending on the scheduling
of tasks on CPU0.

Variant 1:

  cpu_notify(CPU_ONLINE)
    workqueue_cpu_up_callback()
      rebind_workers()
        set_cpus_allowed_ptr()

  This call fails because it requires an active CPU; rebind_workers()
  ends with a warning:

    WARNING: CPU: 0 PID: 1 at kernel/workqueue.c:4418
    workqueue_cpu_up_callback()

Variant 2:

  cpu_notify(CPU_ONLINE)
    smpboot_thread_call()
      smpboot_unpark_threads()
       ..
        __kthread_unpark()
          __kthread_bind()
          wake_up_state()
           ..
            select_task_rq()
              select_fallback_rq()

  The ->wake_cpu of the unparked thread is not allowed, making a call
  to select_fallback_rq() necessary. Then, select_fallback_rq() cannot
  find an allowed, active CPU and promptly resets the allowed CPUs, so
  that the task in question ends up on CPU0.

  When those unparked tasks are eventually executed, they run
  immediately into a BUG:

    kernel BUG at kernel/smpboot.c:135!

Just changing the order in which the online/active bits are set (and
adding some memory barriers), would solve the two issues above.
However, it would change the order of operations back to the one
before commit 6acbfb96976f ("sched: Fix hotplug vs.
set_cpus_allowed_ptr()"), thus, reintroducing that particular problem.

Going further back into history, we have at least the following commits
touching this topic:
- commit 2baab4e90495 ("sched: Fix select_fallback_rq() vs
  cpu_active/cpu_online")
- commit 5fbd036b552f ("sched: Cleanup cpu_active madness")

Together, these give us the following non-working solutions:
- secondary CPU sets active before online, because active is assumed to
  be a subset of online;
- secondary CPU sets online before active, because the primary CPU
  assumes that an online CPU is also active;
- secondary CPU sets online and waits for primary CPU to set active,
  because it might deadlock.

Commit 875ebe940d77 ("powerpc/smp: Wait until secondaries are active &
online") introduces an arch-specific solution to this arch-independent
problem.

Now, go for a more general solution without explicit waiting and simply
set active twice: once on the secondary CPU after online was set and
once on the primary CPU after online was seen.

Fixes: 6acbfb96976f ("sched: Fix hotplug vs. set_cpus_allowed_ptr()")
Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>

---

v2:
- Big honking comment in the CPU_ONLINE path for Peter. :)

 kernel/sched/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 78b4bad10..e967343 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5433,6 +5433,14 @@ static int sched_cpu_active(struct notifier_block *nfb,
 	case CPU_STARTING:
 		set_cpu_rq_start_time();
 		return NOTIFY_OK;
+	case CPU_ONLINE:
+		/*
+		 * At this point a starting CPU has marked itself as online via
+		 * set_cpu_online(). But it might not yet have marked itself
+		 * as active, which is essential from here on.
+		 *
+		 * Thus, fall-through and help the starting CPU along.
+		 */
 	case CPU_DOWN_FAILED:
 		set_cpu_active((long)hcpu, true);
 		return NOTIFY_OK;
-- 
2.4.6.1.gea4e83c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web