forked kernel task and mm structures imbalanced on NUMA

Another problem I found when testing this patch is that the scheduler has some issues of its own when balancing. This is improved by traversing the sd groups starting from a different spot each time, so processes get sprinkled around the nodes a bit better.

kernel/sched.c +0 -4

@@ -2550,14 +2561,16 @@ void wake_up_new_task(struct task_struct
         * We set TASK_WAKING so that select_task_rq() can drop rq->lock
         * without people poking at ->cpus_allowed.
         */
-       cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
-       set_task_cpu(p, cpu);
-
-       p->state = TASK_RUNNING;

Quoted 4 lines · Nick Piggin

Another problem I found when testing this patch is that the scheduler has some issues of its own when balancing. This is improved by traversing the sd groups starting from a different spot each time, so processes get sprinkled around the nodes a bit better.

Right, makes sense. And I think we could merge that group iteration without much problems.

Your alternative placement for sched_exec() seems to make sense too, the earlier we do that the better the memory allocations will be.

Your changes to sched_fork() and wake_up_new_task() made my head hurt a bit -- but that's not your fault. I'm not quite sure why you're changing that though.

The addition of sched_fork_suggest_cpu() to select a target node seems to make sense, but since you then call fork balancing a second time we have a chance of ending up on a totally different node all together.

So I think it would make sense to rework the fork balancing muck to be called only once and stick with its decision.

One thing that would make the whole fork path much easier is fully ripping out that child_runs_first mess for CONFIG_SMP, I think its been disabled by default for long enough, and its always been broken in the face of fork balancing anyway.

So basically we have to move fork balancing back to sched_fork(), I'd have to again look at wth happens to ->cpus_allowed, but I guess it should be fixable, and I don't think we should care overly much about cpu-hotplug.

A specific code comment:

Quoted 13 lines

kernel/sched.c +4 -5

@@ -2550,14 +2561,16 @@ void wake_up_new_task(struct task_struct
         * We set TASK_WAKING so that select_task_rq() can drop rq->lock
         * without people poking at ->cpus_allowed.
         */
-       cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
-       set_task_cpu(p, cpu);
-
-       p->state = TASK_RUNNING;
-       task_rq_unlock(rq, &flags);
+       if (!cpumask_test_cpu(cpu, &p->cpus_allowed)) {
+               p->state = TASK_WAKING;
+               cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
+               set_task_cpu(p, cpu);

kernel/sched.c void wake_up_new_task(struct task_struct · line 2572

Quoted 5 lines

kernel/sched.c +4 -0

+               p->state = TASK_RUNNING;
+               task_rq_unlock(rq, &flags);
+               rq = task_rq_lock(p, &flags);
+       }
 #endif

That's iffy because p->cpus_allowed isn't stable when we're not holding the task's current rq->lock or p->state is not TASK_WAKING.

Quoted 2 lines

So I think it would make sense to rework the fork balancing muck to be called only once and stick with its decision.

Just need to close that race somehow. AFAIKS we can't use TASK_WAKING because that must not be preempted?

Quoted 4 lines

One thing that would make the whole fork path much easier is fully ripping out that child_runs_first mess for CONFIG_SMP, I think its been disabled by default for long enough, and its always been broken in the face of fork balancing anyway.

Interesting problem. vfork is nice for fork+exec, but it's a bit restrictive.

Quoted 4 lines

So basically we have to move fork balancing back to sched_fork(), I'd have to again look at wth happens to ->cpus_allowed, but I guess it should be fixable, and I don't think we should care overly much about cpu-hotplug.

No more than simply getting it right. Simply calling into the balancer again seems to be the simplest way to do it.

Quoted 15 lines

A specific code comment:

Quoted 13 lines

kernel/sched.c +4 -5

@@ -2550,14 +2561,16 @@ void wake_up_new_task(struct task_struct
         * We set TASK_WAKING so that select_task_rq() can drop rq->lock
         * without people poking at ->cpus_allowed.
         */
-       cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
-       set_task_cpu(p, cpu);
-
-       p->state = TASK_RUNNING;
-       task_rq_unlock(rq, &flags);
+       if (!cpumask_test_cpu(cpu, &p->cpus_allowed)) {
+               p->state = TASK_WAKING;
+               cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
+               set_task_cpu(p, cpu);

Quoted 9 lines

kernel/sched.c void wake_up_new_task(struct task_struct · line 2572

Quoted 5 lines

kernel/sched.c +4 -0

+               p->state = TASK_RUNNING;
+               task_rq_unlock(rq, &flags);
+               rq = task_rq_lock(p, &flags);
+       }
 #endif

That's iffy because p->cpus_allowed isn't stable when we're not holding the task's current rq->lock or p->state is not TASK_WAKING.

Oop, yeah missed that. Half hearted attempt to avoid more rq locks.

I'd have to again look at wth happens to ->cpus_allowed, but I guess it should be fixable

This isn't really a new problem, and I don't know how important it is, but I recently came across it again when doing some aim7 testing with huge numbers of tasks.

Quoted 3 lines · Nick Piggin

This isn't really a new problem, and I don't know how important it is, but I recently came across it again when doing some aim7 testing with huge numbers of tasks.

Seems reasonable. Of course you need to at least save/restore the old CPU policy, and use a subset of it.

Another approach would be to migrate this on touch, but that is probably slightly more difficult. The advantage would be that on multiple migrations it would follow. And it would be a bit slower for the initial case.

Quoted 6 lines · Andi Kleen

Quoted 3 lines · Nick Piggin

This isn't really a new problem, and I don't know how important it is, but I recently came across it again when doing some aim7 testing with huge numbers of tasks.

Quoted 2 lines

Seems reasonable. Of course you need to at least save/restore the old CPU policy, and use a subset of it.

The mpolicy? My patch does that (mpol_prefer_cpu_start/end). The real problem is that it can actually violate the parent's mempolicy. For example MPOL_BIND and cpus_allowed set on a node outside the mempolicy.

Quoted 3 lines

slightly more difficult. The advantage would be that on multiple migrations it would follow. And it would be a bit slower for the initial case.

Migrate what on touch? Talking mainly about kernel memory structures, task_struct, mm, vmas, page tables, kernel stack, etc.

Quoted 7 lines · Nick Piggin

Quoted 6 lines · Andi Kleen

Quoted 3 lines · Nick Piggin

This isn't really a new problem, and I don't know how important it is, but I recently came across it again when doing some aim7 testing with huge numbers of tasks.

Quoted 6 lines

Quoted 2 lines

Seems reasonable. Of course you need to at least save/restore the old CPU policy, and use a subset of it.

The mpolicy? My patch does that (mpol_prefer_cpu_start/end). The real problem is that it can actually violate the parent's mempolicy. For example MPOL_BIND and cpus_allowed set on a node outside the mempolicy.

I don't see where you store 'old', but maybe I missed it.

Quoted 6 lines

Quoted 3 lines

slightly more difficult. The advantage would be that on multiple migrations it would follow. And it would be a bit slower for the initial case.

Migrate what on touch? Talking mainly about kernel memory structures, task_struct, mm, vmas, page tables, kernel stack, etc.

Migrate task_struct, mm, vmas, page tables, kernel stack on reasonable touch. As long as they are not shared it shouldn't be too difficult.

+ sd->iter = iter->next;

Quoted 1 line · Nick Piggin

+ sd->iter = iter->next;

BTW, the (utter lack of) synchronization of sd->iter probably wants a comment somewhere ;-)