(Was: sched: start stopper early)

Review static int sched_cpu_active(struct notifier_block *nfb, · line 5343

Quoted 12 lines

(quoted diff) +2 -1

 	case CPU_ONLINE:
+		stop_machine_unpark(cpu);
 		/*
 		 * At this point a starting CPU has marked itself as online via
 		 * set_cpu_online(). But it might not yet have marked itself

@@ -5337,7 +5340,7 @@ static int sched_cpu_active(struct notifier_block *nfb,
 		 * Thus, fall-through and help the starting CPU along.
 		 */
 	case CPU_DOWN_FAILED:
-		set_cpu_active((long)hcpu, true);
+		set_cpu_active(cpu, true);

On a second thought, we can't do this (and your initial change has the same problem).

We can not wakeup it before set_cpu_active(). This can lead to the same problem fixed by dd9d3843755da95f6 "sched: Fix cpu_active_mask/ cpu_online_mask race".

I do not understand the cpu_active() check in select_fallback_rq(). x86 doesn't need it, and the recent commit dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race" documents the fact that on any architecture we can ignore !active starting from CPU_ONLINE stage.

But any possible reason why do we need this check in "fallback" must equally apply to select_task_rq().

kernel/sched/core.c +4 -0

+static inline bool cpu_allowed(int cpu)
+{
+	return cpu_online(cpu) && cpu_active(cpu);
+}

Quoted 8 lines · Oleg Nesterov

I do not understand the cpu_active() check in select_fallback_rq(). x86 doesn't need it, and the recent commit dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race" documents the fact that on any architecture we can ignore !active starting from CPU_ONLINE stage.

But any possible reason why do we need this check in "fallback" must equally apply to select_task_rq().

And I still think this is true, select_task_rq() and select_fallback_rq() should use the same check in any case...

Quoted 4 lines

kernel/sched/core.c +4 -0

+static inline bool cpu_allowed(int cpu)
+{
+	return cpu_online(cpu) && cpu_active(cpu);
+}

...

kernel/sched/core.c int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags) · line 1395

Quoted 7 lines

kernel/sched/core.c +1 -1

@@ -1390,7 +1391,7 @@ int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags)
 	 *   not worry about this generic constraint ]
 	 */
 	if (unlikely(!cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
-		     !cpu_online(cpu)))
+		     !cpu_allowed(cpu)))
 		cpu = select_fallback_rq(task_cpu(p), p);

But as Fengguang reports (thanks a lot!) this change is wrong. It leads to another BUG_ON(td->cpu != smp_processor_id()) before ht->park(td->cpu) in smpboot_thread_fn().

I should have realized this. smpboot_park_threads() is called after CPU_DOWN_PREPARE. And this can break other PF_NO_SETAFFINITY threads.

Perhaps I am totally confused, but to me this looks like another indication that select_fallback_rq() should not check cpu_active(), or at least this needs some changes...

I do not understand the cpu_active() check in select_fallback_rq(). x86 doesn't need it, and the recent commit dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race" documents the fact that on any architecture we can ignore !active starting from CPU_ONLINE stage.

But any possible reason why do we need this check in "fallback" must equally apply to select_task_rq().

Quoted 5 lines · Oleg Nesterov

I do not understand the cpu_active() check in select_fallback_rq(). x86 doesn't need it, and the recent commit dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race" documents the fact that on any architecture we can ignore !active starting from CPU_ONLINE stage.

Quoted 2 lines

But any possible reason why do we need this check in "fallback" must equally apply to select_task_rq().

So the reason, from vague memory, is that we want to allow per-cpu threads to start/stop before/after active.

active 'should' really only govern load-balancer bits or so.

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Oleg Nesterov

I do not understand the cpu_active() check in select_fallback_rq(). x86 doesn't need it, and the recent commit dd9d3843755d "sched: Fix cpu_active_mask/cpu_online_mask race" documents the fact that on any architecture we can ignore !active starting from CPU_ONLINE stage.

Quoted 5 lines

Quoted 2 lines

But any possible reason why do we need this check in "fallback" must equally apply to select_task_rq().

So the reason, from vague memory, is that we want to allow per-cpu threads to start/stop before/after active.

I simply can't understand... To me it looks as if we can simply remove the cpu_active() check in select_fallback_rq().

If we race with cpu_down(), cpu_active() is cleared by sched_cpu_inactive() which is CPU_PRI_SCHED_INACTIVE = INT_MIN + 1 priority, so it seems that only cpuset_cpu_inactive() can be called after that and this check is obviously racy anyway.

So why we can't simply remove select_fallback_rq()->cpu_active() ?

Quoted 1 line

active 'should' really only govern load-balancer bits or so.

OK, I don't understand the usage of cpu_active_mask in kernel/sched/, and of course I could easily miss something else. But I doubt very much this check can save us from something bad simply because it is racy.

Yes, we also have synchronize_sched() after CPU_DOWN_PREPARE, but the only thing we do before stop_machine() is smpboot_park_threads(). So this can help (say) set_cpus_allowed_ptr() which uses active_mask, but I don't see how this can connect to ttwu paths.

And again. If there is some reason we can not do this, say, because ipi to non-active CPU can trigger a warning, or something else, then we can hit the same problem because select_task_rq() does not check cpu_active(). The kernel threads like stoppers/workers are probably fine in any case, but userspace can trigger the race with cpu_up/down.

Quoted 4 lines · Oleg Nesterov

I simply can't understand... To me it looks as if we can simply remove the cpu_active() check in select_fallback_rq().

Quoted 4 lines

If we race with cpu_down(), cpu_active() is cleared by sched_cpu_inactive() which is CPU_PRI_SCHED_INACTIVE = INT_MIN + 1 priority, so it seems that only cpuset_cpu_inactive() can be called after that and this check is obviously racy anyway.

Racy yes; however select_task_rq() is called with locks held, therefore preemption disabled. This serializes us against the sync_sched() in cpu_down().

And note that smpboot_park_threads() runs until after the sync_sched(). So you cannot add cpu_active() to select_task_rq() for it must allow per-cpu tasks to remain on the cpu.

Quoted 1 line

So why we can't simply remove select_fallback_rq()->cpu_active() ?

It would allow migration onto a CPU that's known to be going away. That doesn't make sense.

Quoted 4 lines

OK, I don't understand the usage of cpu_active_mask in kernel/sched/, and of course I could easily miss something else. But I doubt very much this check can save us from something bad simply because it is racy.

But safely so; if we observe active, it must stay 'active' until we enable preemption again.

Quoted 4 lines

Yes, we also have synchronize_sched() after CPU_DOWN_PREPARE, but the only thing we do before stop_machine() is smpboot_park_threads(). So this can help (say) set_cpus_allowed_ptr() which uses active_mask, but I don't see how this can connect to ttwu paths.

Say you have a task A running on CPU4, it goes to sleep. We unplug CPU4. We then commence unplugging CPU3, concurrently we wake A. A finds that its old CPU (4) is no longer online and ends up in fallback looking for a new one.

Without the cpu_active() test in fallback, we could place A on CPU3, which is on its way down, just not quite dead.

Even if this is not strictly buggy its a daft thing to do and tempting fate.

Quoted 5 lines

And again. If there is some reason we can not do this, say, because ipi to non-active CPU can trigger a warning, or something else, then we can hit the same problem because select_task_rq() does not check cpu_active(). The kernel threads like stoppers/workers are probably fine in any case, but userspace can trigger the race with cpu_up/down.

So the only 'race' is observing active while we're stuck in sync_sched(),

which is meant to be safe.

It also provides us with a
known spot after which we're sure no new tasks will come onto our dying CPU.

Yes, because that damn cpu_active() check doesn't look strictly necessary ;) Or I misunderstood.