sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()

RFC PATCH In review Dmitry Adamushko 3 participants 5 messages Jul 24, 2008 – Jul 25, 2008

Share & tools

Dmitry Adamushko

author

2008-07-24 22:12 UTC #0 lore raw

RFC PATCH 1/2 sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()

From: Dmitry Adamushko <[email protected]>
Subject: sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()

---

sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()

(1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr() after releasing the rq-lock;

(2) in load_balance() and load_balance_idle()

ensure that we don't get 'busiest' which can disappear as a result of cpu_down() while we are manipulating it. For this goal, we choose 'busiest' only amongst 'cpu_active_map' cpus.

load_balance() and load_balance_idle() get called with preemption being disabled so synchronize_sched() in cpu_down() should get us synced.

IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for can't be manipulated/accessed by the load-balancer.

Signed-off-by Dmitry Adamushko <[email protected]>

kernel/sched.c +23 -4

index 6acf749..b4ccc8b 100644
@@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 	struct rq *busiest;
 	unsigned long flags;
 
-	cpus_setall(*cpus);
+	/*
+	 * Ensure that we don't get 'busiest' which can disappear
+	 * as a result of cpu_down() while we are manipulating it.
+	 *
+	 * load_balance() gets called with preemption being disabled
+	 * so synchronize_sched() in cpu_down() should get us synced.
+	 */
+	*cpus = cpu_active_map;
 
 	/*
 	 * When power savings policy is enabled for the parent domain, idle

@@ -3571,7 +3578,14 @@ load_balance_newidle(int this_cpu, struct rq *this_rq, struct sched_domain *sd,
 	int sd_idle = 0;
 	int all_pinned = 0;
 
-	cpus_setall(*cpus);
+	/*
+	 * Ensure that we don't get 'busiest' which can disappear
+	 * as a result of cpu_down() while we are manipulating it.
+	 *
+	 * load_balance_newidle() gets called with preemption being disabled
+	 * so synchronize_sched() in cpu_down() should get us synced.
+	 */
+	*cpus = cpu_active_map;
 
 	/*
 	 * When power savings policy is enabled for the parent domain, idle

@@ -5764,9 +5778,14 @@ int set_cpus_allowed_ptr(struct task_struct *p, const cpumask_t *new_mask)
 		goto out;
 
 	if (migrate_task(p, any_online_cpu(*new_mask), &req)) {
-		/* Need help from migration thread: drop lock and wait. */
+		/* Need to wait for migration thread (might exit: take ref). */
+		struct task_struct *mt = rq->migration_thread;
+
+		get_task_struct(mt);
 		task_rq_unlock(rq, &flags);
-		wake_up_process(rq->migration_thread);
+		wake_up_process(mt);
+		put_task_struct(mt);
+
 		wait_for_completion(&req.done);
 		tlb_migrate_finish(p->mm);
 		return 0;

Peter Zijlstra

Acked-by on v1 1/2

2008-07-25 11:40 UTC #1 lore raw

Quoted 17 lines · Dmitry Adamushko

From: Dmitry Adamushko <[email protected]>
Subject: sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
---
sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
(1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr() after releasing the rq-lock;
(2) in load_balance() and load_balance_idle()
ensure that we don't get 'busiest' which can disappear as a result of cpu_down() while we are manipulating it. For this goal, we choose 'busiest' only amongst 'cpu_active_map' cpus.

Quoted 7 lines

load_balance() and load_balance_idle() get called with preemption being disabled so synchronize_sched() in cpu_down() should get us synced.
IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for can't be manipulated/accessed by the load-balancer.
Signed-off-by Dmitry Adamushko <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Quoted 7 lines

kernel/sched.c +0 -0

index 6acf749..b4ccc8b 100644
@@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 	struct rq *busiest;
 	unsigned long flags;

kernel/sched.c static int load_balance(int this_cpu, struct rq *this_rq, · line 3419

Quoted 10 lines

kernel/sched.c +8 -1

 
-	cpus_setall(*cpus);
+	/*
+	 * Ensure that we don't get 'busiest' which can disappear
+	 * as a result of cpu_down() while we are manipulating it.
+	 *
+	 * load_balance() gets called with preemption being disabled
+	 * so synchronize_sched() in cpu_down() should get us synced.
+	 */
+	*cpus = cpu_active_map;

This is going to be painful on -rt... there it can be preempted. I guess we can put get_online_cpus() around it or something..

Quoted 37 lines

kernel/sched.c +15 -3

 	/*
 	 * When power savings policy is enabled for the parent domain, idle

@@ -3571,7 +3578,14 @@ load_balance_newidle(int this_cpu, struct rq *this_rq, struct sched_domain *sd,
 	int sd_idle = 0;
 	int all_pinned = 0;
 
-	cpus_setall(*cpus);
+	/*
+	 * Ensure that we don't get 'busiest' which can disappear
+	 * as a result of cpu_down() while we are manipulating it.
+	 *
+	 * load_balance_newidle() gets called with preemption being disabled
+	 * so synchronize_sched() in cpu_down() should get us synced.
+	 */
+	*cpus = cpu_active_map;
 
 	/*
 	 * When power savings policy is enabled for the parent domain, idle

@@ -5764,9 +5778,14 @@ int set_cpus_allowed_ptr(struct task_struct *p, const cpumask_t *new_mask)
 		goto out;
 
 	if (migrate_task(p, any_online_cpu(*new_mask), &req)) {
-		/* Need help from migration thread: drop lock and wait. */
+		/* Need to wait for migration thread (might exit: take ref). */
+		struct task_struct *mt = rq->migration_thread;
+
+		get_task_struct(mt);
 		task_rq_unlock(rq, &flags);
-		wake_up_process(rq->migration_thread);
+		wake_up_process(mt);
+		put_task_struct(mt);
+
 		wait_for_completion(&req.done);
 		tlb_migrate_finish(p->mm);
 		return 0;

Dmitry Adamushko

author

2008-07-25 11:52 UTC #2 lore raw

2008/7/25 Peter Zijlstra <[email protected]>:

Quoted 21 lines · Dmitry Adamushko

Quoted 20 lines
From: Dmitry Adamushko <[email protected]>
Subject: sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
---
sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
(1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr() after releasing the rq-lock;
(2) in load_balance() and load_balance_idle()
ensure that we don't get 'busiest' which can disappear as a result of cpu_down() while we are manipulating it. For this goal, we choose 'busiest' only amongst 'cpu_active_map' cpus.
load_balance() and load_balance_idle() get called with preemption being disabled so synchronize_sched() in cpu_down() should get us synced.

Quoted 6 lines

Quoted 4 lines
IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for can't be manipulated/accessed by the load-balancer.
Signed-off-by Dmitry Adamushko <[email protected]>
Acked-by Peter Zijlstra <[email protected]>

Thanks.

Quoted 9 lines

kernel/sched.c +0 -0

index 6acf749..b4ccc8b 100644
@@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
      struct rq *busiest;
      unsigned long flags;

Quoted 12 lines

kernel/sched.c static int load_balance(int this_cpu, struct rq *this_rq, · line 3419
Quoted 9 lines
kernel/sched.c +8 -1
-     cpus_setall(*cpus);
+     /*
+      * Ensure that we don't get 'busiest' which can disappear
+      * as a result of cpu_down() while we are manipulating it.
+      *
+      * load_balance() gets called with preemption being disabled
+      * so synchronize_sched() in cpu_down() should get us synced.
+      */
+     *cpus = cpu_active_map;
This is going to be painful on -rt... there it can be preempted. I guess we can put get_online_cpus() around it or something..

I've considered using get_online_cpus() for a moment but dropped this idea exactly because I thought it would harm us latency-wise. cpu_down() and cpu_up() may take quite a long time to complete and load_balance() && load_balance_idle() would need to wait all this time. And they both are kind of generic (primary) scheduler operations.

but yea, my scheme relies on the fact that load_balance() && load_balance_idle() are atomic one way or another wrt. cpu_clear() + synchronize_sched() in cpu_down().

[ speculating here ] I'd rather add an additional mechanism which would be light-weight for load_balance() and add synch_this_mechanism() (alike to synchonise_sched()) in cpu_down() as perhaps we don't care that much on how fast the later one is.

Signature · 2 lines

Best regards,
Dmitry Adamushko

Peter Zijlstra

2008-07-25 12:13 UTC #3 lore raw

Quoted 60 lines · Dmitry Adamushko

2008/7/25 Peter Zijlstra <[email protected]>:
Quoted 21 lines · Dmitry Adamushko
Quoted 20 lines
From: Dmitry Adamushko <[email protected]>
Subject: sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
---
sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
(1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr() after releasing the rq-lock;
(2) in load_balance() and load_balance_idle()
ensure that we don't get 'busiest' which can disappear as a result of cpu_down() while we are manipulating it. For this goal, we choose 'busiest' only amongst 'cpu_active_map' cpus.
load_balance() and load_balance_idle() get called with preemption being disabled so synchronize_sched() in cpu_down() should get us synced.
Quoted 6 lines
Quoted 4 lines
IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for can't be manipulated/accessed by the load-balancer.
Signed-off-by Dmitry Adamushko <[email protected]>
Acked-by Peter Zijlstra <[email protected]>
Thanks.
Quoted 9 lines
Quoted 9 lines
kernel/sched.c +0 -0
index 6acf749..b4ccc8b 100644
@@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
      struct rq *busiest;
      unsigned long flags;
 
Quoted 12 lines
kernel/sched.c static int load_balance(int this_cpu, struct rq *this_rq, · line 3419
Quoted 9 lines
kernel/sched.c +8 -1
-     cpus_setall(*cpus);
+     /*
+      * Ensure that we don't get 'busiest' which can disappear
+      * as a result of cpu_down() while we are manipulating it.
+      *
+      * load_balance() gets called with preemption being disabled
+      * so synchronize_sched() in cpu_down() should get us synced.
+      */
+     *cpus = cpu_active_map;
This is going to be painful on -rt... there it can be preempted. I guess we can put get_online_cpus() around it or something..
I've considered using get_online_cpus() for a moment but dropped this idea exactly because I thought it would harm us latency-wise. cpu_down() and cpu_up() may take quite a long time to complete and load_balance() && load_balance_idle() would need to wait all this time. And they both are kind of generic (primary) scheduler operations.

Quoted 8 lines

but yea, my scheme relies on the fact that load_balance() && load_balance_idle() are atomic one way or another wrt. cpu_clear() + synchronize_sched() in cpu_down().
[ speculating here ] I'd rather add an additional mechanism which would be light-weight for load_balance() and add synch_this_mechanism() (alike to synchonise_sched()) in cpu_down() as perhaps we don't care that much on how fast the later one is.

Right, I suppose we could stick in an SRCU domain or something to do that.

So we do:

  srcu_read_lock();
  load_balance();
  srcu_read_unlock();

and then in cpu_down():

  srcu_synchronize();

Gautham R Shenoy

2008-07-25 22:32 UTC #4 lore raw

In reply to Dmitry Adamushko

Quoted 53 lines · Dmitry Adamushko

2008/7/25 Peter Zijlstra <[email protected]>:
Quoted 21 lines · Dmitry Adamushko
Quoted 20 lines
From: Dmitry Adamushko <[email protected]>
Subject: sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
---
sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
(1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr() after releasing the rq-lock;
(2) in load_balance() and load_balance_idle()
ensure that we don't get 'busiest' which can disappear as a result of cpu_down() while we are manipulating it. For this goal, we choose 'busiest' only amongst 'cpu_active_map' cpus.
load_balance() and load_balance_idle() get called with preemption being disabled so synchronize_sched() in cpu_down() should get us synced.
Quoted 6 lines
Quoted 4 lines
IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for can't be manipulated/accessed by the load-balancer.
Signed-off-by Dmitry Adamushko <[email protected]>
Acked-by Peter Zijlstra <[email protected]>
Thanks.
Quoted 9 lines
Quoted 9 lines
kernel/sched.c +0 -0
index 6acf749..b4ccc8b 100644
@@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
      struct rq *busiest;
      unsigned long flags;
 
Quoted 12 lines
kernel/sched.c static int load_balance(int this_cpu, struct rq *this_rq, · line 3419
Quoted 9 lines
kernel/sched.c +8 -1
-     cpus_setall(*cpus);
+     /*
+      * Ensure that we don't get 'busiest' which can disappear
+      * as a result of cpu_down() while we are manipulating it.
+      *
+      * load_balance() gets called with preemption being disabled
+      * so synchronize_sched() in cpu_down() should get us synced.
+      */
+     *cpus = cpu_active_map;
This is going to be painful on -rt... there it can be preempted. I guess we can put get_online_cpus() around it or something..

Quoted 2 lines

I've considered using get_online_cpus() for a moment but dropped this idea exactly because I thought it would harm us latency-wise.

get_online_cpus() can be made to be extremely lightweight (as simple as updating a per_cpu variable). But yes, if a cpu-hotplug operation is in progress one might block there.. So probably we need the try_ variant here..

Quoted 27 lines

cpu_down() and cpu_up() may take quite a long time to complete and load_balance() && load_balance_idle() would need to wait all this time. And they both are kind of generic (primary) scheduler operations.
but yea, my scheme relies on the fact that load_balance() && load_balance_idle() are atomic one way or another wrt. cpu_clear() + synchronize_sched() in cpu_down().
[ speculating here ] I'd rather add an additional mechanism which would be light-weight for load_balance() and add synch_this_mechanism() (alike to synchonise_sched()) in cpu_down() as perhaps we don't care that much on how fast the later one is.
Signature · 7 lines
Best regards,
Dmitry Adamushko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 2 lines

Thanks and Regards
gautham