Skip to contentSkip to conversation

- Move migrate_all_tasks to CPU_DEAD handling

PATCH In review Srivatsa Vaddagiri 4 participants 21 messages Apr 5, 2004 – Apr 12, 2004

Subsystems (4)
MM core Scheduler exec and binfmt CPU hotplug
lore
Share & tools
lore

Srivatsa Vaddagiri

author
#0 lore raw

PATCH 1/1 - Move migrate_all_tasks to CPU_DEAD handling

Hi Rusty,
migrate_all_tasks is currently run with rest of the machine stopped.
It iterates thr' the complete task table, turning off cpu affinity of any task that it finds affine to the dying cpu. Depending on the task table size this can take considerable time. All this time machine is stopped, doing nothing.

I think Nick was working on reducing this time spent in migrating tasks by concentrating only on the tasks in the runqueue and catch up with sleeping tasks as and when they wake up (in try_to_wake_up). But this still can be considerable time spent depending on the number of tasks in the dying CPU's runqueue.

Other solutions that we thought of were put off by either changes required in hot path (schedule for ex) or the complexity of implementation.

I was tempted to think of an alternate _simple_ solution that :

a. avoids doing migrate tasks with machine stopped b. avoids any changes in core code (especially hot path)

Point a) leads to a more scalable solution where amout of time machine is stopped is _independent_ of runqueue length or task table length.

The solution that I came up with (which can be shot down if you think its not correct/good :-) which meets both the above goals was to have idle task put to the _front_ of the dying CPU's runqueue at the highest priority possible. This cause idle thread to run _immediately_ after kstopmachine thread yields. Idle thread notices that its cpu is offline and dies quickly. Task migration can then be done at leisure in CPU_DEAD notification, when rest of the CPUs are running.

Some advantages I see with this approach are:

- More scalable. Predicatable amout of time that machine is stopped.
- No changes to core code. We are just exploiting scheduler rules which runs the next high-priority task on the runqueue. Also since I put idle task to the _front_ of the runqueue, there are no races when a equally high priority task is woken up and added to the runqueue. It gets in at the back of the runqueue, _after_ idle task!
- No changes in hot path/core code (schedule/try_to_wake_up)
- cpu_is_offline check that is presenty required in load_balance can be removed, thus speeding it up a bit
- Probably the cpu_is_offline check that is present in try_to_wake_up (sync case) can also be removed ..I dont think that check is required even w/o this patch?

Patch below is (stress) tested on 4-way PPC64 box against 2.6.5-ames. To test it, you probably need to also apply the "fix __cpu_up race" patch that I posted earlier.

Pls let me know if this topic is worth pursuing!

---

 linux-2.6.5-ames-vatsa/include/linux/sched.h |    5 -
 linux-2.6.5-ames-vatsa/kernel/cpu.c          |   28 +++++-
 linux-2.6.5-ames-vatsa/kernel/sched.c        |  109 ++++++++++++++++++++-------
 3 files changed, 109 insertions(+), 33 deletions(-)
include/linux/sched.h renamed from include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD +3 -2
diff -puN include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD include/linux/sched.h
@@ -549,8 +549,9 @@ extern void node_nr_running_init(void);
 #define node_nr_running_init() {}
 #endif
 
-/* Move tasks off this (offline) CPU onto another. */
-extern void migrate_all_tasks(void);
+/* Move tasks off dead CPU onto another. */
+extern void migrate_all_tasks(int cpu);
+extern void sched_idle_next(void);
 extern void set_user_nice(task_t *p, long nice);
 extern int task_prio(task_t *p);
 extern int task_nice(task_t *p);
kernel/sched.c renamed from kernel/sched.c~migrate_all_tasks_in_CPU_DEAD +84 -25

195 lines not on this page · raw message

kernel/cpu.c renamed from kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD +22 -6
diff -puN kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD kernel/cpu.c
@@ -43,13 +43,13 @@ void unregister_cpu_notifier(struct noti
 EXPORT_SYMBOL(unregister_cpu_notifier);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static inline void check_for_tasks(int cpu, struct task_struct *k)
+static inline void check_for_tasks(int cpu)
 {
 	struct task_struct *p;
 
 	write_lock_irq(&tasklist_lock);
 	for_each_process(p) {
-		if (task_cpu(p) == cpu && p != k)
+		if (task_cpu(p) == cpu)
 			printk(KERN_WARNING "Task %s is on cpu %d\n",
 				p->comm, cpu);
 	}

@@ -96,8 +96,14 @@ static int take_cpu_down(void *unused)
 	if (err < 0)
 		cpu_set(smp_processor_id(), cpu_online_map);
 	else
-		/* Everyone else gets kicked off. */
-		migrate_all_tasks();
+		/* Force scheduler to switch to idle task when we yield.
+		 * We expect idle task to _immediately_ notice that it's cpu
+		 * is offline and die quickly.
+		 *
+		 * This allows us to defer calling mirate_all_tasks until
+		 * CPU_DEAD notification time.
+		 */
+		sched_idle_next();
 
 	return err;
 }

@@ -106,6 +112,7 @@ int cpu_down(unsigned int cpu)
 {
 	int err;
 	struct task_struct *p;
+	cpumask_t old_allowed, tmp;
 
 	if ((err = lock_cpu_hotplug_interruptible()) != 0)
 		return err;

@@ -120,16 +127,21 @@ int cpu_down(unsigned int cpu)
 		goto out;
 	}
 
+	/* Ensure that we are not runnable on dying cpu */
+	old_allowed = current->cpus_allowed;
+	tmp = CPU_MASK_ALL;
+	cpu_clear(cpu, tmp);
+	set_cpus_allowed(current, tmp);
+
 	p = __stop_machine_run(take_cpu_down, NULL, cpu);
 	if (IS_ERR(p)) {
 		err = PTR_ERR(p);
-		goto out;
+		goto out_allowed;
 	}
 
 	if (cpu_online(cpu))
 		goto out_thread;
 
-	check_for_tasks(cpu, p);
 
 	/* Wait for it to sleep (leaving idle task). */
 	while (!idle_cpu(cpu))

@@ -146,10 +158,14 @@ int cpu_down(unsigned int cpu)
 	    == NOTIFY_BAD)
 		BUG();
 
+	check_for_tasks(cpu);
+
 	cpu_run_sbin_hotplug(cpu, "offline");
 
 out_thread:
 	err = kthread_stop(p);
+out_allowed:
+	set_cpus_allowed(current, old_allowed);
 out:
 	unlock_cpu_hotplug();
 	return err;

_

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#1 lore raw
Quoted 8 lines · Srivatsa Vaddagiri

Hi Rusty,
migrate_all_tasks is currently run with rest of the machine stopped.
It iterates thr' the complete task table, turning off cpu affinity of any task that it finds affine to the dying cpu. Depending on the task table size this can take considerable time. All this time machine is stopped, doing nothing.

I think Nick was working on reducing this time spent in migrating tasks
by concentrating only on the tasks in the runqueue and catch up with sleeping
tasks as and when they wake up (in try_to_wake_up). But this still can be considerable time spent depending on the number of tasks in the dying CPU's runqueue.

Hi Srivatsa,
First of all, if you're proposing this stuff for inclusion, you should port it to the -mm tree, because I don't think Andrew will want any other scheduler work going in just now. It wouldn't be too hard.

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

author
#2 lore raw
Quoted 4 lines · Nick Piggin

First of all, if you're proposing this stuff for inclusion, you should port it to the -mm tree, because I don't think Andrew will want any other scheduler work going in just now. It wouldn't be too hard.

Will send out today a patch against latest -mm tree!

Quoted 4 lines

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

Is there some place where I can download your patch (or is it in -mm tree)?

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#3 lore raw
Quoted 11 lines · Srivatsa Vaddagiri
Quoted 4 lines · Nick Piggin

First of all, if you're proposing this stuff for inclusion, you should port it to the -mm tree, because I don't think Andrew will want any other scheduler work going in just now. It wouldn't be too hard.

Will send out today a patch against latest -mm tree!

Quoted 9 lines
Quoted 4 lines

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

Is there some place where I can download your patch (or is it in -mm tree)?

I have attached it (against 2.6.5-mm1). I haven't actually tested it yet because I haven't got around to finding and using the i386 test code yet.

It also contains a copule of cleanups:
rename double_lock_balance to second_rq_lock, and make migrate_all_tasks static, and have the hotplug code call sched_cpu_stop.

Comments would be welcome.

Nick

 linux-2.6-npiggin/include/linux/sched.h |    4 
 linux-2.6-npiggin/kernel/cpu.c          |    9 +
 linux-2.6-npiggin/kernel/sched.c        |  178 ++++++++++++++++++++------------
 3 files changed, 121 insertions(+), 70 deletions(-)
kernel/sched.c renamed from kernel/sched.c~hotplugcpu-lazy-migrate +113 -65
diff -puN kernel/sched.c~hotplugcpu-lazy-migrate kernel/sched.c
@@ -41,6 +41,7 @@
 #include <linux/cpu.h>
 #include <linux/percpu.h>
 #include <linux/kthread.h>
+#include <linux/list.h>
 
 #ifdef CONFIG_NUMA
 #define cpu_to_node_mask(cpu) node_to_cpumask(cpu_to_node(cpu))

@@ -710,6 +711,53 @@ static inline int wake_idle(int cpu, tas
 }
 #endif
 
+#ifdef CONFIG_HOTPLUG_CPU
+/*
+ * go_away: choose a new CPU for tsk if the one it is on has gone
+ * offline. Updates cpus_allowed affinity if it absolutely has to.
+ * Returns chosen destination CPU.
+ */
+static int go_away(struct task_struct *tsk)
+{
+	cpumask_t mask;
+	int cpu, node, dest_cpu;
+
+	/*
+	 * watch out for per node tasks, let's stay on this node.
+	 * TODO turn this into a sched_domain flag - np
+	 */
+	cpu = task_cpu(tsk);
+	node = cpu_to_node(cpu);
+	mask = node_to_cpumask(node);
+
+	/*
+	 * Figure out where this task should go (attempt to keep it on-node),
+	 * and check if it can be migrated as-is.  NOTE that kernel threads
+	 * bound to more than one online cpu will be migrated.
+	 */
+	cpus_and(mask, mask, tsk->cpus_allowed);
+	dest_cpu = any_online_cpu(mask);
+	if (dest_cpu == NR_CPUS)
+		dest_cpu = any_online_cpu(tsk->cpus_allowed);
+	if (dest_cpu == NR_CPUS) {
+		cpus_clear(tsk->cpus_allowed);
+		cpus_complement(tsk->cpus_allowed);
+		dest_cpu = any_online_cpu(tsk->cpus_allowed);
+
+		/*
+		 * Don't tell them about moving exiting tasks or kernel
+		 * threads (both mm NULL), since they never leave kernel.
+		 */
+		if (tsk->mm && printk_ratelimit()) {
+			printk(KERN_INFO "process %d (%s) no "
+				       "longer affine to cpu%d\n",
+				       tsk->pid, tsk->comm, task_cpu(tsk));
+		}
+	}
+	return dest_cpu;
+}
+#endif
+
 /***
  * try_to_wake_up - wake up a thread
  * @p: the to-be-woken-up thread

@@ -748,11 +796,18 @@ static int try_to_wake_up(task_t * p, un
 	this_cpu = smp_processor_id();
 
 #ifdef CONFIG_SMP
-	if (unlikely(task_running(rq, p) || cpu_is_offline(this_cpu)))
+	if (unlikely(task_running(rq, p))
 		goto out_activate;
 
 	new_cpu = cpu;
 
+#ifdef CONFIG_HOTPLUG_CPU
+	if (unlikely(cpu_is_offline(cpu))) {
+		/* Must lazy-migrate off this CPU */
+		goto out_set_cpu;
+	}
+#endif
+
 	if (cpu == this_cpu || unlikely(!cpu_isset(this_cpu, p->cpus_allowed)))
 		goto out_set_cpu;
 

@@ -1257,17 +1312,17 @@ out:
 #endif /* CONFIG_NUMA */
 
 /*
- * double_lock_balance - lock the busiest runqueue, this_rq is locked already.
+ * second_rq_lock - lock the busiest runqueue, this_rq is locked already.
  */
-static void double_lock_balance(runqueue_t *this_rq, runqueue_t *busiest)
+static void second_rq_lock(runqueue_t *locked, runqueue_t *to_lock)
 {
-	if (unlikely(!spin_trylock(&busiest->lock))) {
-		if (busiest < this_rq) {
-			spin_unlock(&this_rq->lock);
-			spin_lock(&busiest->lock);
-			spin_lock(&this_rq->lock);
+	if (unlikely(!spin_trylock(&to_lock->lock))) {
+		if (to_lock < locked) {
+			spin_unlock(&locked->lock);
+			spin_lock(&to_lock->lock);
+			spin_lock(&locked->lock);
 		} else
-			spin_lock(&busiest->lock);
+			spin_lock(&to_lock->lock);
 	}
 }
 

@@ -1592,7 +1647,7 @@ static int load_balance(int this_cpu, ru
 	}
 
 	/* Attempt to move tasks */
-	double_lock_balance(this_rq, busiest);
+	second_rq_lock(this_rq, busiest);
 
 	nr_moved = move_tasks(this_rq, this_cpu, busiest, imbalance, sd, idle);
 	spin_unlock(&this_rq->lock);

@@ -1662,7 +1717,7 @@ static int load_balance_newidle(int this
 		goto out;
 
 	/* Attempt to move tasks */
-	double_lock_balance(this_rq, busiest);
+	second_rq_lock(this_rq, busiest);
 
 	nr_moved = move_tasks(this_rq, this_cpu, busiest,
 					imbalance, sd, NEWLY_IDLE);

@@ -1744,7 +1799,7 @@ static void active_load_balance(runqueue
  		}
 
 		rq = cpu_rq(push_cpu);
-		double_lock_balance(busiest, rq);
+		second_rq_lock(busiest, rq);
 		move_tasks(rq, push_cpu, busiest, 1, sd, IDLE);
 		spin_unlock(&rq->lock);
 next_group:

@@ -3221,6 +3276,8 @@ EXPORT_SYMBOL_GPL(set_cpus_allowed);
  *
  * So we race with normal scheduler movements, but that's OK, as long
  * as the task is no longer on this CPU.
+ *
+ * Called with this_rq locked
  */
 static void __migrate_task(struct task_struct *p, int dest_cpu)
 {

@@ -3228,7 +3285,7 @@ static void __migrate_task(struct task_s
 
 	rq_dest = cpu_rq(dest_cpu);
 
-	double_rq_lock(this_rq(), rq_dest);
+	second_rq_lock(this_rq(), rq_dest);
 	/* Already moved. */
 	if (task_cpu(p) != smp_processor_id())
 		goto out;

@@ -3246,7 +3303,7 @@ static void __migrate_task(struct task_s
 	p->timestamp = rq_dest->timestamp_last_tick;
 
 out:
-	double_rq_unlock(this_rq(), rq_dest);
+	spin_unlock(&rq_dest->lock);
 }
 
 /*

@@ -3286,8 +3343,6 @@ static int migration_thread(void * data)
 		req = list_entry(head->next, migration_req_t, list);
 		list_del_init(head->next);
 
-		spin_unlock(&rq->lock);
-
 		if (req->type == REQ_MOVE_TASK) {
 			__migrate_task(req->task, req->dest_cpu);
 		} else if (req->type == REQ_SET_DOMAIN) {

@@ -3296,7 +3351,7 @@ static int migration_thread(void * data)
 			WARN_ON(1);
 		}
 
-		local_irq_enable();
+		spin_unlock_irq(&rq->lock);
 
 		complete(&req->done);
 	}

@@ -3304,60 +3359,53 @@ static int migration_thread(void * data)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-/* migrate_all_tasks - function to migrate all the tasks from the
- * current cpu caller must have already scheduled this to the target
- * cpu via set_cpus_allowed.  Machine is stopped.  */
-void migrate_all_tasks(void)
-{
-	struct task_struct *tsk, *t;
-	int dest_cpu, src_cpu;
-	unsigned int node;
-
-	/* We're nailed to this CPU. */
-	src_cpu = smp_processor_id();
-
-	/* Not required, but here for neatness. */
-	write_lock(&tasklist_lock);
-
-	/* watch out for per node tasks, let's stay on this node */
-	node = cpu_to_node(src_cpu);
-
-	do_each_thread(t, tsk) {
-		cpumask_t mask;
-		if (tsk == current)
-			continue;
+/*
+ * migrate_all_tasks - function to migrate all the tasks from the
+ * current CPU. Current CPU must be marked offline.
+ */
+static void migrate_all_tasks(void)
+{
+	runqueue_t *rq;
+	int i, j;
+	int dest_cpu;
 
-		if (task_cpu(tsk) != src_cpu)
-			continue;
+	rq = this_rq_lock();
 
-		/* Figure out where this task should go (attempting to
-		 * keep it on-node), and check if it can be migrated
-		 * as-is.  NOTE that kernel threads bound to more than
-		 * one online cpu will be migrated. */
-		mask = node_to_cpumask(node);
-		cpus_and(mask, mask, tsk->cpus_allowed);
-		dest_cpu = any_online_cpu(mask);
-		if (dest_cpu == NR_CPUS)
-			dest_cpu = any_online_cpu(tsk->cpus_allowed);
-		if (dest_cpu == NR_CPUS) {
-			cpus_clear(tsk->cpus_allowed);
-			cpus_complement(tsk->cpus_allowed);
-			dest_cpu = any_online_cpu(tsk->cpus_allowed);
-
-			/* Don't tell them about moving exiting tasks
-			   or kernel threads (both mm NULL), since
-			   they never leave kernel. */
-			if (tsk->mm && printk_ratelimit())
-				printk(KERN_INFO "process %d (%s) no "
-				       "longer affine to cpu%d\n",
-				       tsk->pid, tsk->comm, src_cpu);
+again:
+	/* Traverse the runqueue */
+	for (i = 0; i < 2; i++) {
+		for (j = 0; j < MAX_PRIO; j++) {
+			struct task_struct *tsk, *tmp;
+			list_for_each_entry_safe(tsk, tmp,
+					&rq->arrays[i].queue[j], run_list) {
+				if (tsk == current)
+					continue;
+
+				dest_cpu = go_away(tsk);
+				__migrate_task(tsk, dest_cpu);
+			}
 		}
+	}
 
-		__migrate_task(tsk, dest_cpu);
-	} while_each_thread(t, tsk);
+	/* __migrate_task can drop the lock (via second_rq_lock).
+	 * Recheck and go again if we're not the only ones left. */
+	if (rq->nr_running > 1)
+		goto again;
 
-	write_unlock(&tasklist_lock);
+	rq_unlock(rq);
 }
+
+/*
+ * sched_cpu_stop is called by CPU hotplug code when it intends to take
+ * the current CPU down. It must be called after the CPU has been marked
+ * offline.
+ */
+void sched_cpu_stop(void)
+{
+	/* At the moment all we need to do is migrate tasks off */
+	migrate_all_tasks();
+}
+
 #endif /* CONFIG_HOTPLUG_CPU */
 
 /*
kernel/cpu.c renamed from kernel/cpu.c~hotplugcpu-lazy-migrate +6 -3
diff -puN kernel/cpu.c~hotplugcpu-lazy-migrate kernel/cpu.c
@@ -91,13 +91,16 @@ static int take_cpu_down(void *unused)
 	/* Take offline: makes arch_cpu_down somewhat easier. */
 	cpu_clear(smp_processor_id(), cpu_online_map);
 
+	/* Ensure all other CPUs see that we're offline */
+	wmb();
+
+	/* Everyone else gets kicked off. */
+	sched_cpu_stop();
+
 	/* Ensure this CPU doesn't handle any more interrupts. */
 	err = __cpu_disable();
 	if (err < 0)
 		cpu_set(smp_processor_id(), cpu_online_map);
-	else
-		/* Everyone else gets kicked off. */
-		migrate_all_tasks();
 
 	return err;
 }
include/linux/sched.h renamed from include/linux/sched.h~hotplugcpu-lazy-migrate +2 -2
diff -puN include/linux/sched.h~hotplugcpu-lazy-migrate include/linux/sched.h
@@ -664,8 +664,8 @@ extern void sched_balance_exec(void);
 #define sched_balance_exec()   {}
 #endif
 
-/* Move tasks off this (offline) CPU onto another. */
-extern void migrate_all_tasks(void);
+/* sched_cpu_stop must be called after the CPU is marked offline */
+extern void sched_cpu_stop(void);
 extern void set_user_nice(task_t *p, long nice);
 extern int task_prio(task_t *p);
 extern int task_nice(task_t *p);

_

#4 lore raw
Quoted 30 lines · Nick Piggin
Quoted 12 lines · Srivatsa Vaddagiri
Quoted 4 lines · Nick Piggin

First of all, if you're proposing this stuff for inclusion, you should port it to the -mm tree, because I don't think Andrew will want any other scheduler work going in just now. It wouldn't be too hard.

Will send out today a patch against latest -mm tree!

Quoted 10 lines
Quoted 4 lines

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

Is there some place where I can download your patch (or is it in -mm tree)?

I have attached it (against 2.6.5-mm1). I haven't actually tested it
#ifdef CONFIG_SMP

kernel/sched.c renamed from kernel/sched.c~hotplugcpu-lazy-migrate +1 -1
-	if (unlikely(task_running(rq, p) || cpu_is_offline(this_cpu)))
+	if (unlikely(task_running(rq, p))
 		goto out_activate;

kernel/sched.c static int try_to_wake_up(task_t * p, un · line 810

Quoted 10 lines
kernel/sched.c renamed from kernel/sched.c~hotplugcpu-lazy-migrate +7 -0
 
 	new_cpu = cpu;
 
+#ifdef CONFIG_HOTPLUG_CPU
+	if (unlikely(cpu_is_offline(cpu))) {
+		/* Must lazy-migrate off this CPU */
+		goto out_set_cpu;
+	}
+#endif
+

Err, that should be:

#ifdef CONFIG_HOTPULG_CPU
	if (unlikely(cpu_is_offline(cpu))) {
		/* Must lazy-migrate off this CPU */
		new_cpu = go_away(p);
		goto out_set_cpu;
	}

#endif

author
#5 lore raw
In reply to Srivatsa Vaddagiri
Quoted 1 line · Srivatsa Vaddagiri

Will send out today a patch against latest -mm tree!

And here's the patch against 2.6.5-mm1 (did some minimal testing on 4-way Pentium Box - Will run stress tests tomorrow and update the patch if necessary!)

---

 linux-2.6.5-mm1-vatsa/include/linux/sched.h |    3 
 linux-2.6.5-mm1-vatsa/kernel/cpu.c          |   29 ++++++--
 linux-2.6.5-mm1-vatsa/kernel/sched.c        |   92 +++++++++++++++++++++-------
 3 files changed, 92 insertions(+), 32 deletions(-)
include/linux/sched.h renamed from include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD +1 -2
diff -puN include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD include/linux/sched.h
@@ -664,8 +664,7 @@ extern void sched_balance_exec(void);
 #define sched_balance_exec()   {}
 #endif
 
-/* Move tasks off this (offline) CPU onto another. */
-extern void migrate_all_tasks(void);
+extern void sched_idle_next(void);
 extern void set_user_nice(task_t *p, long nice);
 extern int task_prio(task_t *p);
 extern int task_nice(task_t *p);
kernel/sched.c renamed from kernel/sched.c~migrate_all_tasks_in_CPU_DEAD +69 -23

176 lines not on this page · raw message

kernel/cpu.c renamed from kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD +22 -7
diff -puN kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD kernel/cpu.c
@@ -43,13 +43,13 @@ void unregister_cpu_notifier(struct noti
 EXPORT_SYMBOL(unregister_cpu_notifier);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static inline void check_for_tasks(int cpu, struct task_struct *k)
+static inline void check_for_tasks(int cpu)
 {
 	struct task_struct *p;
 
 	write_lock_irq(&tasklist_lock);
 	for_each_process(p) {
-		if (task_cpu(p) == cpu && p != k)
+		if (task_cpu(p) == cpu)
 			printk(KERN_WARNING "Task %s is on cpu %d\n",
 				p->comm, cpu);
 	}

@@ -96,8 +96,14 @@ static int take_cpu_down(void *unused)
 	if (err < 0)
 		cpu_set(smp_processor_id(), cpu_online_map);
 	else
-		/* Everyone else gets kicked off. */
-		migrate_all_tasks();
+		/* Force scheduler to switch to idle task when we yield.
+		 * We expect idle task to _immediately_ notice that it's cpu
+		 * is offline and die quickly.
+		 *
+		 * This allows us to defer calling mirate_all_tasks until
+		 * CPU_DEAD notification time.
+		 */
+		sched_idle_next();
 
 	return err;
 }

@@ -106,6 +112,7 @@ int cpu_down(unsigned int cpu)
 {
 	int err;
 	struct task_struct *p;
+	cpumask_t old_allowed, tmp;
 
 	if ((err = lock_cpu_hotplug_interruptible()) != 0)
 		return err;

@@ -120,17 +127,21 @@ int cpu_down(unsigned int cpu)
 		goto out;
 	}
 
+	/* Ensure that we are not runnable on dying cpu */
+	old_allowed = current->cpus_allowed;
+	tmp = CPU_MASK_ALL;
+	cpu_clear(cpu, tmp);
+	set_cpus_allowed(current, tmp);
+
 	p = __stop_machine_run(take_cpu_down, NULL, cpu);
 	if (IS_ERR(p)) {
 		err = PTR_ERR(p);
-		goto out;
+		goto out_allowed;
 	}
 
 	if (cpu_online(cpu))
 		goto out_thread;
 
-	check_for_tasks(cpu, p);
-
 	/* Wait for it to sleep (leaving idle task). */
 	while (!idle_cpu(cpu))
 		yield();

@@ -146,10 +157,14 @@ int cpu_down(unsigned int cpu)
 	    == NOTIFY_BAD)
 		BUG();
 
+	check_for_tasks(cpu);
+
 	cpu_run_sbin_hotplug(cpu, "offline");
 
 out_thread:
 	err = kthread_stop(p);
+out_allowed:
+	set_cpus_allowed(current, old_allowed);
 out:
 	unlock_cpu_hotplug();
 	return err;

_

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
author
#6 lore raw
In reply to Nick Piggin
Quoted 4 lines · Nick Piggin

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

Hi Nick,
I went thr' your patch and have some comments:

1. The benefit I see in your patch (over the solution present today) is you migrate immediately only tasks in the runqueue and don't bother abt sleeping tasks. You catch up with them as and when they wake up.

However by doing so, are we not adding an overhead in the wake-up path? CPU offline should be a (very) rare event and to support that we have to check a cpu's offline status _every_ wakeup call.

IMHO it is best if we migrate _all_ tasks in one shot during the rare offline event and thus avoid the necessity of cpu_is_offline check during the (more) hotter wake_up path.

2. Also note that, migrate_all_tasks is being currently run with rest of the machine frozen. So holding/not-holding tasklist lock during that period does not make a difference!

My patch avoids having to migrate _immediately_ even the tasks present in the runqueue. So the amout of time machine is frozen is greatly reduced.

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#7 lore raw
Quoted 11 lines · Srivatsa Vaddagiri
Quoted 4 lines · Nick Piggin

I think my stuff is a bit orthogonal to what you're attempting. And they should probably work well together. My "lazy migrate" patch means the tasklist lock does not need to be held at all, only the dying runqueue's lock.

Hi Nick,
I went thr' your patch and have some comments:

Hi, thanks for the comments.

Quoted 4 lines

1. The benefit I see in your patch (over the solution present today) is you migrate immediately only tasks in the runqueue and don't bother abt sleeping tasks. You catch up with them as and when they wake up.

That is correct, yes.

Quoted 4 lines

However by doing so, are we not adding an overhead in the wake-up path? CPU offline should be a (very) rare event and to support that we have to check a cpu's offline status _every_ wakeup call.

Note there was already that check in the wakeup path, although I suspect it was someone being overly cautious and isn't required.

Also in my patch, the offline check should probably be done below the check for if (cpu == this_cpu... because that should be a common route.

Quoted 4 lines

IMHO it is best if we migrate _all_ tasks in one shot during the rare offline event and thus avoid the necessity of cpu_is_offline check during the (more) hotter wake_up path.

OK, whatever you like. At least you have my alternative if you ever run into a problem here.

Quoted 4 lines

2. Also note that, migrate_all_tasks is being currently run with rest of the machine frozen. So holding/not-holding tasklist lock during that period does not make a difference!

Yeah, so Rusty pointed out. It still potentially moves a lot fewer tasks though, but I guess it was really waiting for a patch like yours ;)

Quoted 4 lines

My patch avoids having to migrate _immediately_ even the tasks present in the runqueue. So the amout of time machine is frozen is greatly reduced.

I really don't have much interest in the code so it is up to you. If you ever have some realtime system that does cpu hotplugging then give me a call!

Nick

author
#8 lore raw
Quoted 3 lines · Nick Piggin

Also in my patch, the offline check should probably be done below the check for if (cpu == this_cpu... because that should be a common route.

Will this be true for wakeups which are triggered from
expiring timers also? The timers on the dead CPU are migrated to other CPUs. When they fire, the timer fn runs on a different CPU and can try to wake up a task 'n add it to dead cpu! So we probably need a unconditional cpu_is_offline check in try_to_wake_up?

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#9 lore raw
Quoted 7 lines · Srivatsa Vaddagiri
Quoted 3 lines · Nick Piggin

Also in my patch, the offline check should probably be done below the check for if (cpu == this_cpu... because that should be a common route.

Quoted 5 lines

Will this be true for wakeups which are triggered from
expiring timers also? The timers on the dead CPU are migrated to other CPUs. When they fire, the timer fn runs on a different CPU and can try to wake up a task 'n add it to dead cpu! So we probably need a unconditional cpu_is_offline
check in try_to_wake_up?

AFAIKS, no.

If this happens before migrate_all_tasks, there shouldn't be a problem because migrate_all_tasks will move the woken task anyway.

It can't happen after migrate_all_tasks, because there is nothing on the offline CPU to be woken up.

If you do need the check there, then my lazy migrate method is unquestionably better, because this is the only thing it would otherwise have to add to a fastpath. Right?

author
#10 lore raw
Quoted 7 lines · Nick Piggin

AFAIKS, no.

If this happens before migrate_all_tasks, there shouldn't be a problem because migrate_all_tasks will move the woken task anyway.

It can't happen after migrate_all_tasks, because there is nothing on the offline CPU to be woken up.

Hmm ..I was thinking of this scenario ..Lets say task A uses schedule_timeout on CPU3 :

	schedule_timeout(10ms);

A timer is added in CPU3 meant to fire after (max) 10 ms. The task is then put to sleep.

During this sleep duration, CPU3 can go down. migrate_all_tasks not finding A in the runqueue won't bother abt it.

As pary of CPU_DEAD processing, migrate_timers will move the timer that was added in CPU3 to CPU2 (say).

After 10 ms, when the timer fires on CPU2, it will do a wakeup on Task A. At that point, won't Task A still be affine to CPU3? Won't try_to_wake_up attempt adding it to CPU3? At that point 'this_cpu' is 2 and 'cpu' is 3 (in try_to_wake_up)?

Quoted 3 lines

If you do need the check there, then my lazy migrate method is unquestionably better, because this is the only thing it would otherwise have to add to a fastpath. Right?

I don't think we strictly need the cpu_is_offline check in try_to_wake_up if we were to migrate _all_ (running 'n sleeping) tasks in one shot (with tasklist lock held) when a CPU goes down :-)

Sorry I did not mean to compare our patches like this, just trying to work out which will be the right thing to do!!

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#11 lore raw
In reply to Nick Piggin
Quoted 2 lines · Nick Piggin

Note there was already that check in the wakeup path, although I suspect it was someone being overly cautious and isn't required.

No, it really is required.

The stop_machine thread runs on the cpu, then kicks everyone else off, then does a complete() (in stop_machine.c:do_stop()). Without this check, the complete() drags the sleeping process (which called stop_machine) onto the dying CPU.

Hmm, maybe we could explicitly exclude downed cpu from calling task's mask. Kind of hacky: theoretically you should never modify a task's affinity unless the user actually asked for it (since anyone can ask for another tasks affinity).

Cheers!
Rusty.

Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
#12 lore raw
Quoted 13 lines · Rusty Russell
Quoted 2 lines · Nick Piggin

Note there was already that check in the wakeup path, although I suspect it was someone being overly cautious and isn't required.

No, it really is required.

The stop_machine thread runs on the cpu, then kicks everyone else off, then does a complete() (in stop_machine.c:do_stop()). Without this check, the complete() drags the sleeping process (which called stop_machine) onto the dying CPU.

Quoted 5 lines

Hmm, maybe we could explicitly exclude downed cpu from calling task's mask. Kind of hacky: theoretically you should never modify a task's affinity unless the user actually asked for it (since anyone can ask for another tasks affinity).

Ahh yeah ok. Well it seems this isn't required with Srivatsa's patch though, although it would be required again if my lazy migrate patch were to go on top of that.

In that case, I'm happy with moving migrate_all_tasks to CPU_DEAD handling, and keeping the lazy migrate patch out of the tree.

author
#13 lore raw
In reply to Rusty Russell
Quoted 6 lines · Rusty Russell

No, it really is required.

The stop_machine thread runs on the cpu, then kicks everyone else off, then does a complete() (in stop_machine.c:do_stop()). Without this check, the complete() drags the sleeping process (which called stop_machine) onto the dying CPU.

Precisely. That's why I ended up adding this in cpu_down!!

+ /* Ensure that we are not runnable on dying cpu */
+ old_allowed = current->cpus_allowed;
+ tmp = CPU_MASK_ALL;
+ cpu_clear(cpu, tmp);
+ set_cpus_allowed(current, tmp);

I restore the mask though (under covers of lock_cpu_hotplug) before returning from cpu_down. Task should never see this violated affinity.

Rusty,
What do you think abt the whole patch? It has withstood
my stress-test harness :-)

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#14 lore raw
Quoted 2 lines · Srivatsa Vaddagiri

I restore the mask though (under covers of lock_cpu_hotplug) before returning from cpu_down. Task should never see this violated affinity.

But other tasks can do a getaffinity() on it and see the wrong affinity. Probably not a big issue.

Quoted 3 lines

Rusty,
What do you think abt the whole patch? It has withstood
my stress-test harness :-)

I agree with Ingo: it's clever, well done. Minor nitpicks:

+void migrate_all_tasks(int cpu)

 {
        struct task_struct *tsk, *t;
        int dest_cpu, src_cpu;
        unsigned int node;
-       /* We're nailed to this CPU. */
-       src_cpu = smp_processor_id();
+       src_cpu = cpu;

Just make the parameter name "src_cpu"?

+ /* Take idle task off runqueue and restore it's
+ * policy/priority
+ */
+ rq = task_rq_lock(rq->idle, &flags);
+
+ /* Call init_idle instead ?? init_idle doesn't restore
the
+ * policy though for us ..
+ */
+ deactivate_task(rq->idle, rq);
+ __setscheduler(rq->idle, SCHED_NORMAL, MAX_PRIO);
+
+ task_rq_unlock(rq, &flags);

One-line comments and compact code good:

		/* Idle task back to normal (off runqueue, low prio) */
		rq = task_rq_lock(rq->idle, &flags);
		deactivate_task(rq->idle, rq);
		__setscheduler(rq->idle, SCHED_NORMAL, MAX_PRIO);
		task_rq_unlock(rq, &flags);

+ /* Force scheduler to switch to idle task when we yield.
+ * We expect idle task to _immediately_ notice that it's
cpu

+                * is offline and die quickly.
+                *
+                * This allows us to defer calling mirate_all_tasks

until
+ * CPU_DEAD notification time.
+ */
+ sched_idle_next();

This comment's very big. They don't need to know all the things we don't do. I'd prefer:

		/* Force idle task to run as soon as we yield: it should
		   immediately notice cpu is offline and die quickly. */

I'm happy for this to go in..
Rusty

Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
author
#15 lore raw
Quoted 2 lines · Rusty Russell

But other tasks can do a getaffinity() on it and see the wrong affinity. Probably not a big issue.

hmm .. the fact that getaffinity reads the cpus_allowed mask w/o doing lock_cpu_hotplug makes it already racy wrt setaffinity?

Maybe it needs to take CPU hotplug sem before it reads the mask?

Quoted 13 lines

I agree with Ingo: it's clever, well done. Minor nitpicks:

(quoted diff) +2 -2
+void migrate_all_tasks(int cpu)
 {
        struct task_struct *tsk, *t;
        int dest_cpu, src_cpu;
        unsigned int node;
 
-       /* We're nailed to this CPU. */
-       src_cpu = smp_processor_id();
+       src_cpu = cpu;

Just make the parameter name "src_cpu"?

[snip]

Quoted 5 lines

This comment's very big. They don't need to know all the things we don't do. I'd prefer:

		/* Force idle task to run as soon as we yield: it should
		   immediately notice cpu is offline and die quickly. */

Sure, I will change as per your comments.

I would like to run my stress tests for longer time before I send it for inclusion (i would be on vacation till next tuesday ..so maybe i will send in the patch after that!)

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#16 lore raw
Quoted 6 lines · Srivatsa Vaddagiri
Quoted 2 lines · Rusty Russell

But other tasks can do a getaffinity() on it and see the wrong affinity. Probably not a big issue.

hmm .. the fact that getaffinity reads the cpus_allowed mask w/o doing lock_cpu_hotplug makes it already racy wrt setaffinity?

But that's OK: that's a user race. It's like reading a file at the same time as writing it.

Quoted 1 line

Maybe it needs to take CPU hotplug sem before it reads the mask?

Yes, taking it in both syscalls would work, too.

Quoted 3 lines

I would like to run my stress tests for longer time before I send it for inclusion (i would be on vacation till next tuesday ..so maybe i will send in the patch after that!)

Thanks. BTW, can you take a look at the FIXME: in xics.c and change it to be dynamic. Joel is having trouble proving it's a problem, and yet Anton assures me that ioremap is needed, so CPUs not present at boot which are brought in should be failing...

Thanks!
Rusty.

Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
author
#17 lore raw
In reply to Srivatsa Vaddagiri
Quoted 2 lines · Srivatsa Vaddagiri

I would like to run my stress tests for longer time before I send it for inclusion

I had kept my stress tests running over the weekend and here's an updated patch. Changes since last time:

- Register scheduler's callback at highest priority. Task migration needs to happen before anything else. (I have also been running with timer/softirq callbacks at lowest prio of -10).

- Do write_lock_irq on tasklist_lock (migrate_all_tasks) instead of just write_lock. Protection against any code (signal handling?) that can attempt to take a read lock in interrupt context.

- In preempption case, there is a narrow window where check_all_tasks will warn of a task still bound to dead cpu. That task happens to be a newly created child. copy_process created the new task structure for the child and initialized it, but before it could be added in the task list it got preempted. migrate_all_tasks won't find it. However check_all_tasks _can_ find it and warn if the parent has not done wake_up_forked_process yet on the child. This is a false warning and hence added some logic not to warn in this special case (although I don't think the logic is 100% correct - comments to fix this wellcome!)

- Analyzing the above lead to another task "leak", where a task can be woken up on a dead CPU. If CLONE_STOPPED is set, then do_fork won't wake up the child. It will leave it in a stopped state. In such a case, the newly created task can be affine to dead CPU and migrate_all_tasks may not migrate it (since it didn't find it in the task table - see copy_process preemption race explained above). When the stopped task is continued later, it can be added to dead cpu's runqueue (?). Fixed this special case in do_fork.

Note: I think the above task leak would have been true in the old scenario as well where migrate_all_tasks was run with rest of m/c frozen.

Patch against both 2.6.5-mm4 and 2.6.5-ames follows. Rusty, pls consider for inclusion.

Name : Defer migrate_all_tasks to CPU_DEAD handling
Author : Srivatsa Vaddagiri ([email protected])
Status : Tested on 2.6.5-mm4 on a 4-way Pentium box

---

 linux-2.6.5-mm4-vatsa/include/linux/sched.h |    3 
 linux-2.6.5-mm4-vatsa/kernel/cpu.c          |   29 +++++---
 linux-2.6.5-mm4-vatsa/kernel/fork.c         |    6 +
 linux-2.6.5-mm4-vatsa/kernel/sched.c        |   94 +++++++++++++++++++---------
 4 files changed, 92 insertions(+), 40 deletions(-)
include/linux/sched.h renamed from include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD +1 -2
diff -puN include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD include/linux/sched.h
@@ -668,8 +668,7 @@ extern void sched_balance_exec(void);
 #define sched_balance_exec()   {}
 #endif
 
-/* Move tasks off this (offline) CPU onto another. */
-extern void migrate_all_tasks(void);
+extern void sched_idle_next(void);
 extern void set_user_nice(task_t *p, long nice);
 extern int task_prio(task_t *p);
 extern int task_nice(task_t *p);
kernel/sched.c renamed from kernel/sched.c~migrate_all_tasks_in_CPU_DEAD +66 -28

181 lines not on this page · raw message

kernel/cpu.c renamed from kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD +20 -9
diff -puN kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD kernel/cpu.c
@@ -43,15 +43,16 @@ void unregister_cpu_notifier(struct noti
 EXPORT_SYMBOL(unregister_cpu_notifier);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static inline void check_for_tasks(int cpu, struct task_struct *k)
+static inline void check_for_tasks(int cpu)
 {
 	struct task_struct *p;
 
 	write_lock_irq(&tasklist_lock);
 	for_each_process(p) {
-		if (task_cpu(p) == cpu && p != k)
-			printk(KERN_WARNING "Task %s is on cpu %d\n",
-				p->comm, cpu);
+		if (task_cpu(p) == cpu && (p->utime != 0 || p->stime != 0))
+			printk(KERN_WARNING "Task %s (pid = %d) is on cpu %d\
+				(state = %ld, flags = %lx) \n",
+				 p->comm, p->pid, cpu, p->state, p->flags);
 	}
 	write_unlock_irq(&tasklist_lock);
 }

@@ -96,8 +97,9 @@ static int take_cpu_down(void *unused)
 	if (err < 0)
 		cpu_set(smp_processor_id(), cpu_online_map);
 	else
-		/* Everyone else gets kicked off. */
-		migrate_all_tasks();
+		/* Force idle task to run as soon as we yield: it should
+		   immediately notice cpu is offline and die quickly. */
+		sched_idle_next();
 
 	return err;
 }

@@ -106,6 +108,7 @@ int cpu_down(unsigned int cpu)
 {
 	int err;
 	struct task_struct *p;
+	cpumask_t old_allowed, tmp;
 
 	if ((err = lock_cpu_hotplug_interruptible()) != 0)
 		return err;

@@ -120,17 +123,21 @@ int cpu_down(unsigned int cpu)
 		goto out;
 	}
 
+	/* Ensure that we are not runnable on dying cpu */
+	old_allowed = current->cpus_allowed;
+	tmp = CPU_MASK_ALL;
+	cpu_clear(cpu, tmp);
+	set_cpus_allowed(current, tmp);
+
 	p = __stop_machine_run(take_cpu_down, NULL, cpu);
 	if (IS_ERR(p)) {
 		err = PTR_ERR(p);
-		goto out;
+		goto out_allowed;
 	}
 
 	if (cpu_online(cpu))
 		goto out_thread;
 
-	check_for_tasks(cpu, p);
-
 	/* Wait for it to sleep (leaving idle task). */
 	while (!idle_cpu(cpu))
 		yield();

@@ -146,10 +153,14 @@ int cpu_down(unsigned int cpu)
 	    == NOTIFY_BAD)
 		BUG();
 
+	check_for_tasks(cpu);
+
 	cpu_run_sbin_hotplug(cpu, "offline");
 
 out_thread:
 	err = kthread_stop(p);
+out_allowed:
+	set_cpus_allowed(current, old_allowed);
 out:
 	unlock_cpu_hotplug();
 	return err;
kernel/fork.c renamed from kernel/fork.c~migrate_all_tasks_in_CPU_DEAD +5 -1
diff -puN kernel/fork.c~migrate_all_tasks_in_CPU_DEAD kernel/fork.c
@@ -33,6 +33,7 @@
 #include <linux/ptrace.h>
 #include <linux/mount.h>
 #include <linux/audit.h>
+#include <linux/cpu.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -1198,8 +1199,11 @@ long do_fork(unsigned long clone_flags,
 
 		if (!(clone_flags & CLONE_STOPPED))
 			wake_up_forked_process(p);	/* do this last */
-		else
+		else {
 			p->state = TASK_STOPPED;
+			if (unlikely(cpu_is_offline(task_cpu(p))))
+				set_task_cpu(p, smp_processor_id());
+		}
 		++total_forks;
 
 		if (unlikely (trace)) {

Name : Defer migrate_all_tasks to CPU_DEAD handling
Author : Srivatsa Vaddagiri ([email protected])
Status : Tested on 2.6.5-ames on a 4-way PPC64 box (p630)

---

 ameslab-vatsa/include/linux/sched.h |    3 -
 ameslab-vatsa/kernel/cpu.c          |   29 +++++++---
 ameslab-vatsa/kernel/fork.c         |    6 +-
 ameslab-vatsa/kernel/sched.c        |  101 ++++++++++++++++++++++++++----------
 4 files changed, 101 insertions(+), 38 deletions(-)
include/linux/sched.h renamed from include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD +1 -2
diff -puN include/linux/sched.h~migrate_all_tasks_in_CPU_DEAD include/linux/sched.h
@@ -549,8 +549,7 @@ extern void node_nr_running_init(void);
 #define node_nr_running_init() {}
 #endif
 
-/* Move tasks off this (offline) CPU onto another. */
-extern void migrate_all_tasks(void);
+extern void sched_idle_next(void);
 extern void set_user_nice(task_t *p, long nice);
 extern int task_prio(task_t *p);
 extern int task_nice(task_t *p);
kernel/sched.c renamed from kernel/sched.c~migrate_all_tasks_in_CPU_DEAD +75 -26

185 lines not on this page · raw message

kernel/fork.c renamed from kernel/fork.c~migrate_all_tasks_in_CPU_DEAD +5 -1
diff -puN kernel/fork.c~migrate_all_tasks_in_CPU_DEAD kernel/fork.c
@@ -31,6 +31,7 @@
 #include <linux/futex.h>
 #include <linux/ptrace.h>
 #include <linux/mount.h>
+#include <linux/cpu.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -1168,8 +1169,11 @@ long do_fork(unsigned long clone_flags,
 
 		if (!(clone_flags & CLONE_STOPPED))
 			wake_up_forked_process(p);	/* do this last */
-		else
+		else {
 			p->state = TASK_STOPPED;
+			if (unlikely(cpu_is_offline(task_cpu(p))))
+				set_task_cpu(p, smp_processor_id());
+		}
 		++total_forks;
 
 		if (unlikely (trace)) {
kernel/cpu.c renamed from kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD +20 -9
diff -puN kernel/cpu.c~migrate_all_tasks_in_CPU_DEAD kernel/cpu.c
@@ -43,15 +43,16 @@ void unregister_cpu_notifier(struct noti
 EXPORT_SYMBOL(unregister_cpu_notifier);
 
 #ifdef CONFIG_HOTPLUG_CPU
-static inline void check_for_tasks(int cpu, struct task_struct *k)
+static inline void check_for_tasks(int cpu)
 {
 	struct task_struct *p;
 
 	write_lock_irq(&tasklist_lock);
 	for_each_process(p) {
-		if (task_cpu(p) == cpu && p != k)
-			printk(KERN_WARNING "Task %s is on cpu %d\n",
-				p->comm, cpu);
+		if (task_cpu(p) == cpu && (p->utime != 0 || p->stime != 0))
+			printk(KERN_WARNING "Task %s (pid = %d) is on cpu %d\
+				(state = %ld, flags = %lx) \n",
+				 p->comm, p->pid, cpu, p->state, p->flags);
 	}
 	write_unlock_irq(&tasklist_lock);
 }

@@ -96,8 +97,9 @@ static int take_cpu_down(void *unused)
 	if (err < 0)
 		cpu_set(smp_processor_id(), cpu_online_map);
 	else
-		/* Everyone else gets kicked off. */
-		migrate_all_tasks();
+		/* Force idle task to run as soon as we yield: it should
+		   immediately notice cpu is offline and die quickly. */
+		sched_idle_next();
 
 	return err;
 }

@@ -106,6 +108,7 @@ int cpu_down(unsigned int cpu)
 {
 	int err;
 	struct task_struct *p;
+	cpumask_t old_allowed, tmp;
 
 	if ((err = lock_cpu_hotplug_interruptible()) != 0)
 		return err;

@@ -120,17 +123,21 @@ int cpu_down(unsigned int cpu)
 		goto out;
 	}
 
+	/* Ensure that we are not runnable on dying cpu */
+	old_allowed = current->cpus_allowed;
+	tmp = CPU_MASK_ALL;
+	cpu_clear(cpu, tmp);
+	set_cpus_allowed(current, tmp);
+
 	p = __stop_machine_run(take_cpu_down, NULL, cpu);
 	if (IS_ERR(p)) {
 		err = PTR_ERR(p);
-		goto out;
+		goto out_allowed;
 	}
 
 	if (cpu_online(cpu))
 		goto out_thread;
 
-	check_for_tasks(cpu, p);
-
 	/* Wait for it to sleep (leaving idle task). */
 	while (!idle_cpu(cpu))
 		yield();

@@ -146,10 +153,14 @@ int cpu_down(unsigned int cpu)
 	    == NOTIFY_BAD)
 		BUG();
 
+	check_for_tasks(cpu);
+
 	cpu_run_sbin_hotplug(cpu, "offline");
 
 out_thread:
 	err = kthread_stop(p);
+out_allowed:
+	set_cpus_allowed(current, old_allowed);
 out:
 	unlock_cpu_hotplug();
 	return err;
Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
#18 lore raw
In reply to Srivatsa Vaddagiri
Quoted 6 lines · Srivatsa Vaddagiri

Hi Rusty,
migrate_all_tasks is currently run with rest of the machine
stopped. It iterates thr' the complete task table, turning off cpu affinity of any task that it finds affine to the dying cpu. Depending on the task table size this can take considerable time. All this time machine is stopped, doing nothing.

this is being done to keep things simple and fast - to avoid the special-cases (hotplug hooks) in various bits of scheduler code.

Quoted 7 lines

The solution that I came up with (which can be shot down if you think its not correct/good :-) which meets both the above goals was to have idle task put to the _front_ of the dying CPU's runqueue at the highest priority possible. This cause idle thread to run _immediately_ after kstopmachine thread yields. Idle thread notices that its cpu is offline and dies quickly. Task migration can then be done at leisure in CPU_DEAD notification, when rest of the CPUs are running.

nice. The best thing is that your patch also removes a special-case from the hot path.

kernel/sched.c static inline void enqueue_task(struct t · line 346

Quoted 2 lines
kernel/sched.c renamed from kernel/sched.c~migrate_all_tasks_in_CPU_DEAD +2 -0
+/* Add task at the _front_ of it's priority queue - Used by CPU offline code */
+static inline void __enqueue_task(struct task_struct *p, prio_array_t *array)

btw., there's now an enqueue_task_head() function in the -mm scheduler [to do cache-cold migration], if you build ontop of -mm you'll have one less infrastructure bit to worry about.

the question is, how much actual latency does the current 'freeze everything' solution cause? We should prefer simplicity and debuggability over cleverness of implementation - it's not like we'll have hotplug systems on everyone's desk in the next year or so.

also, even assuming a hotplug CPU system, CPU replacement events are not that common, so the performance of the CPU-down op should not be a big issue. The function depends on the # of tasks only linearly, and we have tons of other code that is linear on the # of tasks - in fact we just finished removing all the quadratic functions.

Ingo

author
#19 lore raw
Quoted 10 lines · Ingo Molnar

the question is, how much actual latency does the current 'freeze
everything' solution cause? We should prefer simplicity and debuggability
over cleverness of implementation - it's not like we'll have hotplug systems on everyone's desk in the next year or so.

also, even assuming a hotplug CPU system, CPU replacement events are not that common, so the performance of the CPU-down op should not be a big issue. The function depends on the # of tasks only linearly, and we have tons of other code that is linear on the # of tasks - in fact we just finished removing all the quadratic functions.

Ingo,
I obtained some latency measurements of migrate_all_tasks() on
a 4-way 1.2 GHz Power4 PPC64 (p630) box. They are as below:

Number of Tasks Cycles (get_cycles) spent in migrate_all_tasks (ms)
===========================================================================

     10536 			803244 (5.3 ms)
     30072  			2587940 (17 ms)


	Extending this to 100000 tasks makes the stoppage time to be for

8 million cycles (~50 ms).

My main concern of stopping the machine for so much time
was not performance, rather the effect it may have on functioning of the system. The fact that we freeze the machine for (possibly) tons of cycles doing nothing but migration made me uncomfortable. _and_ the fact that it can very well be avoided :)

Can we rule out any side effects because of this stoppage? Watchdog timers, cluster heartbeats, jiffies, ..? Not sure ..

It just felt much more "safe" and efficient to delegate migration to more safer time in CPU_DEAD notification, when rest of the machine is running. Plus this avoids the cpu_is_offline check in the more hotter path (load_balance/try_to_wake_up)!!

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
author
#20 lore raw
In reply to Ingo Molnar
Quoted 3 lines · Ingo Molnar

We should prefer simplicity and debuggability over cleverness of implementation - it's not like we'll
have hotplug systems on everyone's desk in the next year or so.

I felt that adding idle task to front of runqueue and thereby avoid the stop-machine overhead to a great extent is simple! Is there any complications you see arising out of this?

Signature · 5 lines
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017