Skip to contentSkip to conversation

sched: Simplify move_tasks()

PATCH In review Peter Williams 2 participants 2 messages Aug 4, 2007 Scheduler

lore
Share & tools
lore

Peter Williams

author
#0 lore raw

PATCH 1/1 sched: Simplify move_tasks()

The move_tasks() function is currently multiplexed with two distinct capabilities:

1. attempt to move a specified amount of weighted load from one run queue to another; and
2. attempt to move a specified number of tasks from one run queue to another.

The first of these capabilities is used in two places, load_balance() and load_balance_idle(), and in both of these cases the return value of move_tasks() is used purely to decide if tasks/load were moved and no notice of the actual number of tasks moved is taken.

The second capability is used in exactly one place, active_load_balance(), to attempt to move exactly one task and, as before, the return value is only used as an indicator of success or failure.

This multiplexing of sched_task() was introduced, by me, as part of the smpnice patches and was motivated by the fact that the alternative, one function to move specified load and one to move a single task, would have led to two functions of roughly the same complexity as the old move_tasks() (or the new balance_tasks()). However, the new modular design of the new CFS scheduler allows a simpler solution to be adopted and this patch addresses that solution by:

1. adding a new function, move_one_task(), to be used by active_load_balance(); and
2. making move_tasks() a single purpose function that tries to move a specified weighted load and returns 1 for success and 0 for failure.

One of the consequences of these changes is that neither move_one_task()
or the new move_tasks() care how many tasks sched_class.load_balance() moves and this enables its interface to be simplified by returning the amount of load moved as its result and removing the load_moved pointer from the argument list. This helps simplify the new move_tasks() and slightly reduces the amount of work done in each of sched_class.load_balance()'s implementations.

Further simplification, e.g. changes to balance_tasks(), are possible but (slightly) complicated by the special needs of load_balance_fair() so I've left them to a later patch (if this one gets accepted).

NB Since move_tasks() gets called with two run queue locks held even small reductions in overhead are worthwhile.

Signed-off-by Peter Williams <[email protected]>
Signature · 4 lines
Peter Williams                                   [email protected]

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce
include/linux/sched.h +2 -2
diff -r b97e7dab8f7b include/linux/sched.h
@@ -866,11 +866,11 @@ struct sched_class {
 	struct task_struct * (*pick_next_task) (struct rq *rq, u64 now);
 	void (*put_prev_task) (struct rq *rq, struct task_struct *p, u64 now);
 
-	int (*load_balance) (struct rq *this_rq, int this_cpu,
+	unsigned long (*load_balance) (struct rq *this_rq, int this_cpu,
 			struct rq *busiest,
 			unsigned long max_nr_move, unsigned long max_load_move,
 			struct sched_domain *sd, enum cpu_idle_type idle,
-			int *all_pinned, unsigned long *total_load_moved);
+			int *all_pinned);
 
 	void (*set_curr_task) (struct rq *rq);
 	void (*task_tick) (struct rq *rq, struct task_struct *p);
kernel/sched.c +47 -37

180 lines not on this page · raw message

kernel/sched_fair.c +3 -5
diff -r b97e7dab8f7b kernel/sched_fair.c
@@ -944,11 +944,11 @@ static int cfs_rq_best_prio(struct cfs_r
 	return p->prio;
 }
 
-static int
+static unsigned long
 load_balance_fair(struct rq *this_rq, int this_cpu, struct rq *busiest,
 			unsigned long max_nr_move, unsigned long max_load_move,
 			struct sched_domain *sd, enum cpu_idle_type idle,
-			int *all_pinned, unsigned long *total_load_moved)
+			int *all_pinned)
 {
 	struct cfs_rq *busy_cfs_rq;
 	unsigned long load_moved, total_nr_moved = 0, nr_moved;

@@ -1006,9 +1006,7 @@ load_balance_fair(struct rq *this_rq, in
 			break;
 	}
 
-	*total_load_moved = max_load_move - rem_load_move;
-
-	return total_nr_moved;
+	return max_load_move - rem_load_move;
 }
 
 /*
kernel/sched_idletask.c +2 -2
diff -r b97e7dab8f7b kernel/sched_idletask.c
@@ -37,11 +37,11 @@ static void put_prev_task_idle(struct rq
 {
 }
 
-static int
+static unsigned long
 load_balance_idle(struct rq *this_rq, int this_cpu, struct rq *busiest,
 			unsigned long max_nr_move, unsigned long max_load_move,
 			struct sched_domain *sd, enum cpu_idle_type idle,
-			int *all_pinned, unsigned long *total_load_moved)
+			int *all_pinned)
 {
 	return 0;
 }
kernel/sched_rt.c +5 -4
diff -r b97e7dab8f7b kernel/sched_rt.c
@@ -172,15 +172,16 @@ static struct task_struct *load_balance_
 	return p;
 }
 
-static int
+static unsigned long
 load_balance_rt(struct rq *this_rq, int this_cpu, struct rq *busiest,
 			unsigned long max_nr_move, unsigned long max_load_move,
 			struct sched_domain *sd, enum cpu_idle_type idle,
-			int *all_pinned, unsigned long *load_moved)
+			int *all_pinned)
 {
 	int this_best_prio, best_prio, best_prio_seen = 0;
 	int nr_moved;
 	struct rq_iterator rt_rq_iterator;
+	unsigned long load_moved;
 
 	best_prio = sched_find_first_bit(busiest->rt.active.bitmap);
 	this_best_prio = sched_find_first_bit(this_rq->rt.active.bitmap);

@@ -203,11 +204,11 @@ load_balance_rt(struct rq *this_rq, int 
 	rt_rq_iterator.arg = busiest;
 
 	nr_moved = balance_tasks(this_rq, this_cpu, busiest, max_nr_move,
-			max_load_move, sd, idle, all_pinned, load_moved,
+			max_load_move, sd, idle, all_pinned, &load_moved,
 			this_best_prio, best_prio, best_prio_seen,
 			&rt_rq_iterator);
 
-	return nr_moved;
+	return load_moved;
 }
 
 static void task_tick_rt(struct rq *rq, struct task_struct *p)
#1 lore raw
Quoted 2 lines · Peter Williams

NB Since move_tasks() gets called with two run queue locks held even small reductions in overhead are worthwhile.

thanks, i've applied your patch. It also shaves off a bit of code from sched.o:

   text    data     bss     dec     hex filename
  39216    3618      24   42858    a76a sched.o.before
  39173    3618      24   42815    a73f sched.o.after

Ingo