Refactoring sched_entity and sched_rt_entity.

RFC PATCH In review Dario Faggioli 3 participants 15 messages Jan 4, 2011

Subsystems (3)

Scheduler Proc filesystem Per-task delay accounting

lore

Share & tools

lore

Dario Faggioli

author

2011-01-04 15:55 UTC #0 lore raw

cover Refactoring sched_entity and sched_rt_entity.

Hi everybody,

After having heard Peter talking about it many times, I finally decided to give this thing a spin. It is basically the idea of putting the current sched_entity and sched_rt_entity together in an union, contained in a more general structure which hosts the common fields of the twos. For example, it came out here: http://lkml.org/lkml/2009/7/9/153

Therefore, this patchset _DOES_NOT_ do that! :-O

In fact, this is some preliminary refactoring which I think it's needed before union-ify the two data structure, and on which I would like to get some preliminary comments, before going ahead with the wrong approach.

Basically, I renamed struct sched_entity to struct sched_cfs_entity and put it and sched_rt_entity inside a more general struct sched_entity... Is that clear? :-)
The patches just cope with that. Yes, there's some work done by a patch and undone by the next one. I now it's annoying, and I'm sorry, but I did such for the sake of reviewability and for making each patch compile cleanly.
As for the name of CFS's scheduling entities, sched_fair_entity would probably have been better, but it's longer, and "_cfs" is already present in many places, so I went for it... No big deal in changing that, apart from stay-within-80-characters-per-line issues! :-P

They're not inside an union yet, because I'm not sure on how to treat the task group case. In fact, tasks can only have _just_one_ scheduling policy at a time, and thus, for example, they need the run_list _or_ the rb_node for queueing (if the task is RT or fair, respectively), which is perfect with respect to using an union.
OTOH, groups are always considered both fair _and_ RT entities, for example they're always queued in _both_ an RT run_list and a fair rb-tree. So I can't put them in an union, because I need both at the same time!
Suggestions on how to deal with that will be appreciated.

I compiled and tested this on x86_64, and for [!]SMP, [!]CGROUP_SCHED, [!]FAIR_GROUP_SCHED, [!]RT_GROUP_SCHED and [!]SCHEDSTATS, and some combinations of these, but can't guarantee full coverage of all the possibilities yet... Working on it, in the meanwhile, forgive me if you find some configuration that does not build. ;-D

And as usual, comments of any kind are very very welcome! :-)

Thanks and Regards,
Dario Faggioli

[RFC][PATCH 0/3] Refactoring sched_entity and sched_rt_entity.

 include/linux/init_task.h |    8 +-
 include/linux/sched.h     |   35 ++-
 kernel/kthread.c          |    2 +-
 kernel/posix-cpu-timers.c |    4 +-
 kernel/sched.c            |  210 +++++++++------
 kernel/sched_debug.c      |   20 +-
 kernel/sched_fair.c       |  636 ++++++++++++++++++++++++---------------------
 kernel/sched_rt.c         |   58 ++--
 8 files changed, 534 insertions(+), 439 deletions(-)

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Dario Faggioli

author

2011-01-04 16:01 UTC #1 lore raw

RFC PATCH 1/3 sched: rename sched_entity to sched_cfs_entity.

Using the name struct sched_entity' for the scheduling entity of the fair scheduling class is not fair to the other scheduling classes, which have to use things like struct sched_rt_entity'.

Moreover, changing that would make it possible to put all the `struct sched_*_entity' of all the scheduling classes in a more general structure (which will also contains all the common fields). This would make the code easier to understand and, if we manage in using an union for the class-specific fields, it would also save some memory.

Therefore, this commit turns sched_entity' into sched_cfs_entity', wherever it is being used.

Signed-off-by Dario Faggioli <[email protected]>

---
 fs/proc/base.c            |    2 +-
 include/linux/init_task.h |    4 +-
 include/linux/sched.h     |    6 +-
 kernel/delayacct.c        |    2 +-
 kernel/exit.c             |    2 +-
 kernel/posix-cpu-timers.c |   10 +-
 kernel/sched.c            |  152 ++++++------
 kernel/sched_debug.c      |  117 +++++-----
 kernel/sched_fair.c       |  611 +++++++++++++++++++++++----------------------
 kernel/sched_rt.c         |   26 +-
 kernel/sched_stoptask.c   |    2 +-
 11 files changed, 479 insertions(+), 455 deletions(-)

fs/proc/base.c +1 -1

index 08cba2c..9591d6e 100644
@@ -356,7 +356,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 static int proc_pid_schedstat(struct task_struct *task, char *buffer)
 {
 	return sprintf(buffer, "%llu %llu %lu\n",
-			(unsigned long long)task->se.sum_exec_runtime,
+			(unsigned long long)task->cfs.sum_exec_runtime,
 			(unsigned long long)task->sched_info.run_delay,
 			task->sched_info.pcount);
 }

include/linux/init_task.h +2 -2

index caa151f..8baee0b 100644
@@ -142,8 +142,8 @@ extern struct cred init_cred;
 	.cpus_allowed	= CPU_MASK_ALL,					\
 	.mm		= NULL,						\
 	.active_mm	= &init_mm,					\
-	.se		= {						\
-		.group_node 	= LIST_HEAD_INIT(tsk.se.group_node),	\
+	.cfs		= {						\
+		.group_node 	= LIST_HEAD_INIT(tsk.cfs.group_node),	\
 	},								\
 	.rt		= {						\
 		.run_list	= LIST_HEAD_INIT(tsk.rt.run_list),	\

include/linux/sched.h +3 -3

index 777cd01..55adf37 100644
@@ -1137,7 +1137,7 @@ struct sched_statistics {
 };
 #endif
 
-struct sched_entity {
+struct sched_cfs_entity {
 	struct load_weight	load;		/* for load-balancing */
 	struct rb_node		run_node;
 	struct list_head	group_node;

@@ -1155,7 +1155,7 @@ struct sched_entity {
 #endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
-	struct sched_entity	*parent;
+	struct sched_cfs_entity *parent;
 	/* rq on which this entity is (to be) queued: */
 	struct cfs_rq		*cfs_rq;
 	/* rq "owned" by this entity/group: */

@@ -1206,7 +1206,7 @@ struct task_struct {
 	int prio, static_prio, normal_prio;
 	unsigned int rt_priority;
 	const struct sched_class *sched_class;
-	struct sched_entity se;
+	struct sched_cfs_entity cfs;
 	struct sched_rt_entity rt;
 
 #ifdef CONFIG_PREEMPT_NOTIFIERS

kernel/delayacct.c +1 -1

index ead9b61..f314f56 100644
@@ -128,7 +128,7 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	 */
 	t1 = tsk->sched_info.pcount;
 	t2 = tsk->sched_info.run_delay;
-	t3 = tsk->se.sum_exec_runtime;
+	t3 = tsk->cfs.sum_exec_runtime;
 
 	d->cpu_count += t1;

kernel/exit.c +1 -1

index 676149a..01087fc 100644
@@ -132,7 +132,7 @@ static void __exit_signal(struct task_struct *tsk)
 		sig->inblock += task_io_get_inblock(tsk);
 		sig->oublock += task_io_get_oublock(tsk);
 		task_io_accounting_add(&sig->ioac, &tsk->ioac);
-		sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
+		sig->sum_sched_runtime += tsk->cfs.sum_exec_runtime;
 	}
 
 	sig->nr_threads--;

kernel/posix-cpu-timers.c +5 -5

index 05bb717..801169c 100644
@@ -248,7 +248,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
 	do {
 		times->utime = cputime_add(times->utime, t->utime);
 		times->stime = cputime_add(times->stime, t->stime);
-		times->sum_exec_runtime += t->se.sum_exec_runtime;
+		times->sum_exec_runtime += t->cfs.sum_exec_runtime;
 	} while_each_thread(tsk, t);
 out:
 	rcu_read_unlock();

@@ -508,7 +508,7 @@ static void cleanup_timers(struct list_head *head,
 void posix_cpu_timers_exit(struct task_struct *tsk)
 {
 	cleanup_timers(tsk->cpu_timers,
-		       tsk->utime, tsk->stime, tsk->se.sum_exec_runtime);
+		       tsk->utime, tsk->stime, tsk->cfs.sum_exec_runtime);
 
 }
 void posix_cpu_timers_exit_group(struct task_struct *tsk)

@@ -518,7 +518,7 @@ void posix_cpu_timers_exit_group(struct task_struct *tsk)
 	cleanup_timers(tsk->signal->cpu_timers,
 		       cputime_add(tsk->utime, sig->utime),
 		       cputime_add(tsk->stime, sig->stime),
-		       tsk->se.sum_exec_runtime + sig->sum_sched_runtime);
+		       tsk->cfs.sum_exec_runtime + sig->sum_sched_runtime);
 }
 
 static void clear_dead_task(struct k_itimer *timer, union cpu_time_count now)

@@ -949,7 +949,7 @@ static void check_thread_timers(struct task_struct *tsk,
 		struct cpu_timer_list *t = list_first_entry(timers,
 						      struct cpu_timer_list,
 						      entry);
-		if (!--maxfire || tsk->se.sum_exec_runtime < t->expires.sched) {
+		if (!--maxfire || tsk->cfs.sum_exec_runtime < t->expires.sched) {
 			tsk->cputime_expires.sched_exp = t->expires.sched;
 			break;
 		}

@@ -1276,7 +1276,7 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
 		struct task_cputime task_sample = {
 			.utime = tsk->utime,
 			.stime = tsk->stime,
-			.sum_exec_runtime = tsk->se.sum_exec_runtime
+			.sum_exec_runtime = tsk->cfs.sum_exec_runtime
 		};
 
 		if (task_cputime_expired(&task_sample, &tsk->cputime_expires))

kernel/sched.c +76 ~~-76~~

424 lines not on this page · raw message

kernel/sched_debug.c +59 ~~-58~~

185 lines not on this page · raw message

kernel/sched_fair.c +317 ~~-294~~

1,371 lines not on this page · raw message

kernel/sched_rt.c +13 ~~-13~~

index c914ec7..2bba302 100644
@@ -628,16 +628,16 @@ static void update_curr_rt(struct rq *rq)
 	if (!task_has_rt_policy(curr))
 		return;
 
-	delta_exec = rq->clock_task - curr->se.exec_start;
+	delta_exec = rq->clock_task - curr->cfs.exec_start;
 	if (unlikely((s64)delta_exec < 0))
 		delta_exec = 0;
 
-	schedstat_set(curr->se.statistics.exec_max, max(curr->se.statistics.exec_max, delta_exec));
+	schedstat_set(curr->cfs.statistics.exec_max, max(curr->cfs.statistics.exec_max, delta_exec));
 
-	curr->se.sum_exec_runtime += delta_exec;
+	curr->cfs.sum_exec_runtime += delta_exec;
 	account_group_exec_runtime(curr, delta_exec);
 
-	curr->se.exec_start = rq->clock_task;
+	curr->cfs.exec_start = rq->clock_task;
 	cpuacct_charge(curr, delta_exec);
 
 	sched_rt_avg_update(rq, delta_exec);

@@ -1099,7 +1099,7 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
 	} while (rt_rq);
 
 	p = rt_task_of(rt_se);
-	p->se.exec_start = rq->clock_task;
+	p->cfs.exec_start = rq->clock_task;
 
 	return p;
 }

@@ -1126,13 +1126,13 @@ static struct task_struct *pick_next_task_rt(struct rq *rq)
 static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
 {
 	update_curr_rt(rq);
-	p->se.exec_start = 0;
+	p->cfs.exec_start = 0;
 
 	/*
 	 * The previous task needs to be made eligible for pushing
 	 * if it is still active
 	 */
-	if (p->se.on_rq && p->rt.nr_cpus_allowed > 1)
+	if (p->cfs.on_rq && p->rt.nr_cpus_allowed > 1)
 		enqueue_pushable_task(rq, p);
 }

@@ -1283,7 +1283,7 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
 				     !cpumask_test_cpu(lowest_rq->cpu,
 						       &task->cpus_allowed) ||
 				     task_running(rq, task) ||
-				     !task->se.on_rq)) {
+				     !task->cfs.on_rq)) {
 
 				raw_spin_unlock(&lowest_rq->lock);
 				lowest_rq = NULL;

@@ -1317,7 +1317,7 @@ static struct task_struct *pick_next_pushable_task(struct rq *rq)
 	BUG_ON(task_current(rq, p));
 	BUG_ON(p->rt.nr_cpus_allowed <= 1);
 
-	BUG_ON(!p->se.on_rq);
+	BUG_ON(!p->cfs.on_rq);
 	BUG_ON(!rt_task(p));
 
 	return p;

@@ -1463,7 +1463,7 @@ static int pull_rt_task(struct rq *this_rq)
 		 */
 		if (p && (p->prio < this_rq->rt.highest_prio.curr)) {
 			WARN_ON(p == src_rq->curr);
-			WARN_ON(!p->se.on_rq);
+			WARN_ON(!p->cfs.on_rq);
 
 			/*
 			 * There's a chance that p is higher in priority

@@ -1534,7 +1534,7 @@ static void set_cpus_allowed_rt(struct task_struct *p,
 	 * Update the migration status of the RQ if we have an RT task
 	 * which is running AND changing its weight value.
 	 */
-	if (p->se.on_rq && (weight != p->rt.nr_cpus_allowed)) {
+	if (p->cfs.on_rq && (weight != p->rt.nr_cpus_allowed)) {
 		struct rq *rq = task_rq(p);
 
 		if (!task_current(rq, p)) {

@@ -1701,7 +1701,7 @@ static void watchdog(struct rq *rq, struct task_struct *p)
 		p->rt.timeout++;
 		next = DIV_ROUND_UP(min(soft, hard), USEC_PER_SEC/HZ);
 		if (p->rt.timeout > next)
-			p->cputime_expires.sched_exp = p->se.sum_exec_runtime;
+			p->cputime_expires.sched_exp = p->cfs.sum_exec_runtime;
 	}
 }

@@ -1737,7 +1737,7 @@ static void set_curr_task_rt(struct rq *rq)
 {
 	struct task_struct *p = rq->curr;
 
-	p->se.exec_start = rq->clock_task;
+	p->cfs.exec_start = rq->clock_task;
 
 	/* The running task is never eligible for pushing */
 	dequeue_pushable_task(rq, p);

kernel/sched_stoptask.c +1 -1

index 2bf6b47..763732d 100644
@@ -26,7 +26,7 @@ static struct task_struct *pick_next_task_stop(struct rq *rq)
 {
 	struct task_struct *stop = rq->stop;
 
-	if (stop && stop->se.on_rq)
+	if (stop && stop->cfs.on_rq)
 		return stop;
 
 	return NULL;

Signature · 8 lines

1.7.2.3

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Dario Faggioli

author

2011-01-04 16:02 UTC #2 lore raw

RFC PATCH 2/3 sched: make `struct sched_entity' independent from the scheduling class.

In reply to Dario Faggioli

Now that the fair scheduling entity structure is called sched_cfs_entity we can use the name sched_entity for a more general data structure, which accommodates both sched_cfs_entity and sched_rt_entity, along with all the fields that are common and useful for both the classes.

In fact, it sounds a bit awkward for fields like on_rq or exec_start, that are meaningful and actually used from within sched_rt.c (for RT tasks and groups scheduling), to be placed in some other scheduling class' scheduling entity.

This commit addresses that. Notice that it places sched_cfs_entity and sched_rt_entity inside sched_entity as full structure, it doesn't use an union for them yet.

No fields are added, and the memory occupation of this solution should be pretty much the same of the previous one, apart from the case where just one of CONFIG_FAIR_GROUP_SCHED and CONFIG_RT_GROUP_SCHED is defined, which is a bit more expensive than before.

Signed-off-by Dario Faggioli <[email protected]>

---
 fs/proc/base.c            |    2 +-
 include/linux/init_task.h |    8 +-
 include/linux/sched.h     |   33 ++++--
 kernel/delayacct.c        |    2 +-
 kernel/exit.c             |    2 +-
 kernel/kthread.c          |    2 +-
 kernel/posix-cpu-timers.c |   14 ++--
 kernel/sched.c            |  262 ++++++++++++++++++++++++++-------------------
 kernel/sched_debug.c      |  109 ++++++++++---------
 kernel/sched_fair.c       |  184 +++++++++++++++++--------------
 kernel/sched_rt.c         |   80 +++++++-------
 kernel/sched_stoptask.c   |    2 +-
 12 files changed, 385 insertions(+), 315 deletions(-)

fs/proc/base.c +1 -1

index 9591d6e..08cba2c 100644
@@ -356,7 +356,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 static int proc_pid_schedstat(struct task_struct *task, char *buffer)
 {
 	return sprintf(buffer, "%llu %llu %lu\n",
-			(unsigned long long)task->cfs.sum_exec_runtime,
+			(unsigned long long)task->se.sum_exec_runtime,
 			(unsigned long long)task->sched_info.run_delay,
 			task->sched_info.pcount);
 }

include/linux/init_task.h +4 -4

index 8baee0b..cd6238b 100644
@@ -142,11 +142,11 @@ extern struct cred init_cred;
 	.cpus_allowed	= CPU_MASK_ALL,					\
 	.mm		= NULL,						\
 	.active_mm	= &init_mm,					\
-	.cfs		= {						\
-		.group_node 	= LIST_HEAD_INIT(tsk.cfs.group_node),	\
+	.se.cfs		= {						\
+		.group_node 	= LIST_HEAD_INIT(tsk.se.cfs.group_node),\
 	},								\
-	.rt		= {						\
-		.run_list	= LIST_HEAD_INIT(tsk.rt.run_list),	\
+	.se.rt		= {						\
+		.run_list	= LIST_HEAD_INIT(tsk.se.rt.run_list),	\
 		.time_slice	= HZ, 					\
 		.nr_cpus_allowed = NR_CPUS,				\
 	},								\

include/linux/sched.h +21 ~~-12~~

index 55adf37..df358ba 100644
@@ -1141,18 +1141,8 @@ struct sched_cfs_entity {
 	struct load_weight	load;		/* for load-balancing */
 	struct rb_node		run_node;
 	struct list_head	group_node;
-	unsigned int		on_rq;
 
-	u64			exec_start;
-	u64			sum_exec_runtime;
 	u64			vruntime;
-	u64			prev_sum_exec_runtime;
-
-	u64			nr_migrations;
-
-#ifdef CONFIG_SCHEDSTATS
-	struct sched_statistics statistics;
-#endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	struct sched_cfs_entity *parent;

@@ -1179,6 +1169,26 @@ struct sched_rt_entity {
 #endif
 };
 
+struct sched_entity {
+	unsigned int on_rq;
+	u64 exec_start;
+	u64 sum_exec_runtime;
+	u64 prev_sum_exec_runtime;
+	u64 nr_migrations;
+
+	/*union {
+		struct sched_cfs_entity cfs;
+		struct sched_rt_entity rt;
+	};*/
+	struct sched_cfs_entity cfs;
+	struct sched_rt_entity rt;
+
+#ifdef CONFIG_SCHEDSTATS
+	struct sched_statistics statistics;
+#endif
+
+};
+
 struct rcu_node;
 
 enum perf_event_task_context {

@@ -1206,8 +1216,7 @@ struct task_struct {
 	int prio, static_prio, normal_prio;
 	unsigned int rt_priority;
 	const struct sched_class *sched_class;
-	struct sched_cfs_entity cfs;
-	struct sched_rt_entity rt;
+	struct sched_entity se;
 
 #ifdef CONFIG_PREEMPT_NOTIFIERS
 	/* list of struct preempt_notifier: */

kernel/delayacct.c +1 -1

index f314f56..ead9b61 100644
@@ -128,7 +128,7 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	 */
 	t1 = tsk->sched_info.pcount;
 	t2 = tsk->sched_info.run_delay;
-	t3 = tsk->cfs.sum_exec_runtime;
+	t3 = tsk->se.sum_exec_runtime;
 
 	d->cpu_count += t1;

kernel/exit.c +1 -1

index 01087fc..676149a 100644
@@ -132,7 +132,7 @@ static void __exit_signal(struct task_struct *tsk)
 		sig->inblock += task_io_get_inblock(tsk);
 		sig->oublock += task_io_get_oublock(tsk);
 		task_io_accounting_add(&sig->ioac, &tsk->ioac);
-		sig->sum_sched_runtime += tsk->cfs.sum_exec_runtime;
+		sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
 	}
 
 	sig->nr_threads--;

kernel/kthread.c +1 -1

index 5355cfd..af3b914 100644
@@ -184,7 +184,7 @@ void kthread_bind(struct task_struct *p, unsigned int cpu)
 	}
 
 	p->cpus_allowed = cpumask_of_cpu(cpu);
-	p->rt.nr_cpus_allowed = 1;
+	p->se.rt.nr_cpus_allowed = 1;
 	p->flags |= PF_THREAD_BOUND;
 }
 EXPORT_SYMBOL(kthread_bind);

kernel/posix-cpu-timers.c +7 -7

index 801169c..6cf4636 100644
@@ -248,7 +248,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
 	do {
 		times->utime = cputime_add(times->utime, t->utime);
 		times->stime = cputime_add(times->stime, t->stime);
-		times->sum_exec_runtime += t->cfs.sum_exec_runtime;
+		times->sum_exec_runtime += t->se.sum_exec_runtime;
 	} while_each_thread(tsk, t);
 out:
 	rcu_read_unlock();

@@ -508,7 +508,7 @@ static void cleanup_timers(struct list_head *head,
 void posix_cpu_timers_exit(struct task_struct *tsk)
 {
 	cleanup_timers(tsk->cpu_timers,
-		       tsk->utime, tsk->stime, tsk->cfs.sum_exec_runtime);
+		       tsk->utime, tsk->stime, tsk->se.sum_exec_runtime);
 
 }
 void posix_cpu_timers_exit_group(struct task_struct *tsk)

@@ -518,7 +518,7 @@ void posix_cpu_timers_exit_group(struct task_struct *tsk)
 	cleanup_timers(tsk->signal->cpu_timers,
 		       cputime_add(tsk->utime, sig->utime),
 		       cputime_add(tsk->stime, sig->stime),
-		       tsk->cfs.sum_exec_runtime + sig->sum_sched_runtime);
+		       tsk->se.sum_exec_runtime + sig->sum_sched_runtime);
 }
 
 static void clear_dead_task(struct k_itimer *timer, union cpu_time_count now)

@@ -949,7 +949,7 @@ static void check_thread_timers(struct task_struct *tsk,
 		struct cpu_timer_list *t = list_first_entry(timers,
 						      struct cpu_timer_list,
 						      entry);
-		if (!--maxfire || tsk->cfs.sum_exec_runtime < t->expires.sched) {
+		if (!--maxfire || tsk->se.sum_exec_runtime < t->expires.sched) {
 			tsk->cputime_expires.sched_exp = t->expires.sched;
 			break;
 		}

@@ -966,7 +966,7 @@ static void check_thread_timers(struct task_struct *tsk,
 			ACCESS_ONCE(sig->rlim[RLIMIT_RTTIME].rlim_max);
 
 		if (hard != RLIM_INFINITY &&
-		    tsk->rt.timeout > DIV_ROUND_UP(hard, USEC_PER_SEC/HZ)) {
+		    tsk->se.rt.timeout > DIV_ROUND_UP(hard, USEC_PER_SEC/HZ)) {
 			/*
 			 * At the hard limit, we just die.
 			 * No need to calculate anything else now.

@@ -974,7 +974,7 @@ static void check_thread_timers(struct task_struct *tsk,
 			__group_send_sig_info(SIGKILL, SEND_SIG_PRIV, tsk);
 			return;
 		}
-		if (tsk->rt.timeout > DIV_ROUND_UP(soft, USEC_PER_SEC/HZ)) {
+		if (tsk->se.rt.timeout > DIV_ROUND_UP(soft, USEC_PER_SEC/HZ)) {
 			/*
 			 * At the soft limit, send a SIGXCPU every second.
 			 */

@@ -1276,7 +1276,7 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
 		struct task_cputime task_sample = {
 			.utime = tsk->utime,
 			.stime = tsk->stime,
-			.sum_exec_runtime = tsk->cfs.sum_exec_runtime
+			.sum_exec_runtime = tsk->se.sum_exec_runtime
 		};
 
 		if (task_cputime_expired(&task_sample, &tsk->cputime_expires))

kernel/sched.c +152 ~~-110~~

606 lines not on this page · raw message

kernel/sched_debug.c +55 ~~-54~~

171 lines not on this page · raw message

kernel/sched_fair.c +101 ~~-83~~

543 lines not on this page · raw message

kernel/sched_rt.c +40 ~~-40~~

266 lines not on this page · raw message

kernel/sched_stoptask.c +1 -1

index 763732d..2bf6b47 100644
@@ -26,7 +26,7 @@ static struct task_struct *pick_next_task_stop(struct rq *rq)
 {
 	struct task_struct *stop = rq->stop;
 
-	if (stop && stop->cfs.on_rq)
+	if (stop && stop->se.on_rq)
 		return stop;
 
 	return NULL;

Signature · 8 lines

1.7.2.3

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Dario Faggioli

author

2011-01-04 16:03 UTC #3 lore raw

RFC PATCH 3/3 sched: beautify access to some common fields.

In reply to Dario Faggioli

Fields like on_rq or statistics are accessed quite frequently from CFS within implementation, although they're no longer part of sched_cfs_entity (they're in the general sched_entity).

This commit provides some macros that makes the code which deals with them a bit less cumbersome.

Signed-off-by Dario Faggioli <[email protected]>

---
 kernel/sched_fair.c |  111 ++++++++++++++++++++++++++-------------------------
 1 files changed, 56 insertions(+), 55 deletions(-)

kernel/sched_fair.c +56 ~~-55~~

index e9b8260..0c3006e 100644
@@ -107,6 +107,11 @@ static inline struct sched_entity *se_of_cfs_se(struct sched_cfs_entity *cfs_se)
 	return container_of(cfs_se, struct sched_entity, cfs);
 }
 
+static inline int cfs_se_on_rq(struct sched_cfs_entity *cfs_se)
+{
+	return se_of_cfs_se(cfs_se)->on_rq;
+}
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 
 /* cpu runqueue to which this cfs_rq is attached */

@@ -455,6 +460,8 @@ static struct sched_cfs_entity *__pick_last_entity(struct cfs_rq *cfs_rq)
  * Scheduling class statistics methods:
  */
 
+#define cfs_se_statistics(cfs_se)	((se_of_cfs_se(cfs_se))->statistics)
+
 #ifdef CONFIG_SCHED_DEBUG
 int sched_proc_update_handler(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp,

@@ -521,8 +528,7 @@ static u64 __sched_period(unsigned long nr_running)
  */
 static u64 sched_slice(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 {
-	struct sched_entity *se = se_of_cfs_se(cfs_se);
-	u64 slice = __sched_period(cfs_rq->nr_running + !se->on_rq);
+	u64 slice = __sched_period(cfs_rq->nr_running + !cfs_se_on_rq(cfs_se));
 
 	for_each_sched_cfs_entity(cfs_se) {
 		struct load_weight *load;

@@ -531,7 +537,7 @@ static u64 sched_slice(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 		cfs_rq = cfs_rq_of(cfs_se);
 		load = &cfs_rq->load;
 
-		if (unlikely(!se_of_cfs_se(cfs_se)->on_rq)) {
+		if (unlikely(!cfs_se_on_rq(cfs_se))) {
 			lw = cfs_rq->load;
 
 			update_load_add(&lw, cfs_se->load.weight);

@@ -564,13 +570,12 @@ static inline void
 __update_curr(struct cfs_rq *cfs_rq, struct sched_cfs_entity *curr,
 	      unsigned long delta_exec)
 {
-	struct sched_entity *curr_se = se_of_cfs_se(curr);
 	unsigned long delta_exec_weighted;
 
-	schedstat_set(curr_se->statistics.exec_max,
-		      max((u64)delta_exec, curr_se->statistics.exec_max));
+	schedstat_set(cfs_se_statistics(curr).exec_max,
+		      max((u64)delta_exec, cfs_se_statistics(curr).exec_max));
 
-	curr_se->sum_exec_runtime += delta_exec;
+	se_of_cfs_se(curr)->sum_exec_runtime += delta_exec;
 	schedstat_add(cfs_rq, exec_clock, delta_exec);
 	delta_exec_weighted = calc_delta_fair(delta_exec, curr);

@@ -616,7 +621,7 @@ static void update_curr(struct cfs_rq *cfs_rq)
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 {
-	schedstat_set(se_of_cfs_se(cfs_se)->statistics.wait_start,
+	schedstat_set(cfs_se_statistics(cfs_se).wait_start,
 		      rq_of(cfs_rq)->clock);
 }

@@ -637,20 +642,20 @@ update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 {
+	schedstat_set(cfs_se_statistics(cfs_se).wait_max,
+		      max(cfs_se_statistics(cfs_se).wait_max,
+		      rq_of(cfs_rq)->clock - cfs_se_statistics(cfs_se).wait_start));
+	schedstat_set(cfs_se_statistics(cfs_se).wait_count,
+		      cfs_se_statistics(cfs_se).wait_count+1);
+	schedstat_set(cfs_se_statistics(cfs_se).wait_sum,
+		      cfs_se_statistics(cfs_se).wait_sum +
+		      rq_of(cfs_rq)->clock - cfs_se_statistics(cfs_se).wait_start);
 #ifdef CONFIG_SCHEDSTATS
-	/* #ifdef-ing like this suppresses a gcc warning */
-	struct sched_entity *se = se_of_cfs_se(cfs_se);
-
-	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
-		      rq_of(cfs_rq)->clock - se->statistics.wait_start));
-	schedstat_set(se->statistics.wait_count, se->statistics.wait_count+1);
-	schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +
-		      rq_of(cfs_rq)->clock - se->statistics.wait_start);
 	if (cfs_entity_is_task(cfs_se)) {
 		trace_sched_stat_wait(cfs_task_of(cfs_se), rq_of(cfs_rq)->clock -
-				      se->statistics.wait_start);
+				      cfs_se_statistics(cfs_se).wait_start);
 	}
-	schedstat_set(se->statistics.wait_start, 0);
+	schedstat_set(cfs_se_statistics(cfs_se).wait_start, 0);
 #endif
 }

@@ -787,9 +792,7 @@ static void reweight_entity(struct cfs_rq *cfs_rq,
 			    struct sched_cfs_entity *cfs_se,
 			    unsigned long weight)
 {
-	struct sched_entity *se = se_of_cfs_se(cfs_se);
-
-	if (se->on_rq) {
+	if (cfs_se_on_rq(cfs_se)) {
 		/* commit outstanding execution time */
 		if (cfs_rq->curr == cfs_se)
 			update_curr(cfs_rq);

@@ -798,7 +801,7 @@ static void reweight_entity(struct cfs_rq *cfs_rq,
 
 	update_load_set(&cfs_se->load, weight);
 
-	if (se->on_rq)
+	if (cfs_se_on_rq(cfs_se))
 		account_entity_enqueue(cfs_rq, cfs_se);
 }

@@ -860,44 +863,45 @@ enqueue_sleeper(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 {
 #ifdef CONFIG_SCHEDSTATS
 	struct task_struct *tsk = NULL;
-	struct sched_entity *se = se_of_cfs_se(cfs_se);
 
 	if (cfs_entity_is_task(cfs_se))
 		tsk = cfs_task_of(cfs_se);
 
-	if (se->statistics.sleep_start) {
-		u64 delta = rq_of(cfs_rq)->clock - se->statistics.sleep_start;
+	if (cfs_se_statistics(cfs_se).sleep_start) {
+		u64 delta = rq_of(cfs_rq)->clock -
+			    cfs_se_statistics(cfs_se).sleep_start;
 
 		if ((s64)delta < 0)
 			delta = 0;
 
-		if (unlikely(delta > se->statistics.sleep_max))
-			se->statistics.sleep_max = delta;
+		if (unlikely(delta > cfs_se_statistics(cfs_se).sleep_max))
+			cfs_se_statistics(cfs_se).sleep_max = delta;
 
-		se->statistics.sleep_start = 0;
-		se->statistics.sum_sleep_runtime += delta;
+		cfs_se_statistics(cfs_se).sleep_start = 0;
+		cfs_se_statistics(cfs_se).sum_sleep_runtime += delta;
 
 		if (tsk) {
 			account_scheduler_latency(tsk, delta >> 10, 1);
 			trace_sched_stat_sleep(tsk, delta);
 		}
 	}
-	if (se->statistics.block_start) {
-		u64 delta = rq_of(cfs_rq)->clock - se->statistics.block_start;
+	if (cfs_se_statistics(cfs_se).block_start) {
+		u64 delta = rq_of(cfs_rq)->clock -
+			    cfs_se_statistics(cfs_se).block_start;
 
 		if ((s64)delta < 0)
 			delta = 0;
 
-		if (unlikely(delta > se->statistics.block_max))
-			se->statistics.block_max = delta;
+		if (unlikely(delta > cfs_se_statistics(cfs_se).block_max))
+			cfs_se_statistics(cfs_se).block_max = delta;
 
-		se->statistics.block_start = 0;
-		se->statistics.sum_sleep_runtime += delta;
+		cfs_se_statistics(cfs_se).block_start = 0;
+		cfs_se_statistics(cfs_se).sum_sleep_runtime += delta;
 
 		if (tsk) {
 			if (tsk->in_iowait) {
-				se->statistics.iowait_sum += delta;
-				se->statistics.iowait_count++;
+				cfs_se_statistics(cfs_se).iowait_sum += delta;
+				cfs_se_statistics(cfs_se).iowait_count++;
 				trace_sched_stat_iowait(tsk, delta);
 			}

@@ -1031,12 +1035,11 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se,
 #ifdef CONFIG_SCHEDSTATS
 		if (cfs_entity_is_task(cfs_se)) {
 			struct task_struct *tsk = cfs_task_of(cfs_se);
-			struct sched_entity *se = se_of_cfs_se(cfs_se);
 
 			if (tsk->state & TASK_INTERRUPTIBLE)
-				se->statistics.sleep_start = rq_of(cfs_rq)->clock;
+				cfs_se_statistics(cfs_se).sleep_start = rq_of(cfs_rq)->clock;
 			if (tsk->state & TASK_UNINTERRUPTIBLE)
-				se->statistics.block_start = rq_of(cfs_rq)->clock;
+				cfs_se_statistics(cfs_se).block_start = rq_of(cfs_rq)->clock;
 		}
 #endif
 	}

@@ -1104,10 +1107,8 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_cfs_entity *curr)
 static void
 set_next_entity(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 {
-	struct sched_entity *se = se_of_cfs_se(cfs_se);
-
 	/* 'current' is not kept within the tree. */
-	if (se->on_rq) {
+	if (cfs_se_on_rq(cfs_se)) {
 		/*
 		 * Any task has to be enqueued before it get to execute on
 		 * a CPU. So account for the time it spent waiting on the

@@ -1126,11 +1127,13 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_cfs_entity *cfs_se)
 	 * when there are only lesser-weight tasks around):
 	 */
 	if (rq_of(cfs_rq)->load.weight >= 2*cfs_se->load.weight) {
-		se->statistics.slice_max = max(se->statistics.slice_max,
-			se->sum_exec_runtime - se->prev_sum_exec_runtime);
+		cfs_se_statistics(cfs_se).slice_max = max(cfs_se_statistics(cfs_se).slice_max,
+			se_of_cfs_se(cfs_se)->sum_exec_runtime -
+			se_of_cfs_se(cfs_se)->prev_sum_exec_runtime);
 	}
 #endif
-	se->prev_sum_exec_runtime = se->sum_exec_runtime;
+	se_of_cfs_se(cfs_se)->prev_sum_exec_runtime =
+				se_of_cfs_se(cfs_se)->sum_exec_runtime;
 }
 
 static int wakeup_preempt_entity(struct sched_cfs_entity *curr,

@@ -1158,17 +1161,15 @@ static struct sched_cfs_entity *pick_next_entity(struct cfs_rq *cfs_rq)
 static void
 put_prev_entity(struct cfs_rq *cfs_rq, struct sched_cfs_entity *prev)
 {
-	struct sched_entity *prev_se = se_of_cfs_se(prev);
-
 	/*
 	 * If still on the runqueue then deactivate_task()
 	 * was not called and update_curr() has to be done:
 	 */
-	if (prev_se->on_rq)
+	if (cfs_se_on_rq(prev))
 		update_curr(cfs_rq);
 
 	check_spread(cfs_rq, prev);
-	if (prev_se->on_rq) {
+	if (cfs_se_on_rq(prev)) {
 		update_stats_wait_start(cfs_rq, prev);
 		/* Put 'current' back into the tree. */
 		__enqueue_entity(cfs_rq, prev);

@@ -1279,11 +1280,11 @@ static inline void hrtick_update(struct rq *rq)
 static void
 enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 {
-	struct cfs_rq *cfs_rq;
 	struct sched_cfs_entity *cfs_se = &p->se.cfs;
+	struct cfs_rq *cfs_rq;
 
 	for_each_sched_cfs_entity(cfs_se) {
-		if (se_of_cfs_se(cfs_se)->on_rq)
+		if (cfs_se_on_rq(cfs_se))
 			break;
 		cfs_rq = cfs_rq_of(cfs_se);
 		enqueue_entity(cfs_rq, cfs_se, flags);

@@ -1505,7 +1506,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 	if (sync && balanced)
 		return 1;
 
-	schedstat_inc(p, se.statistics.nr_wakeups_affine_attempts);
+	schedstat_inc(p, cfs_se_statistics(cfs_se).nr_wakeups_affine_attempts);
 	tl_per_task = cpu_avg_load_per_task(this_cpu);
 
 	if (balanced ||

@@ -1517,7 +1518,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 		 * there is no bad imbalance.
 		 */
 		schedstat_inc(sd, ttwu_move_affine);
-		schedstat_inc(p, se.statistics.nr_wakeups_affine);
+		schedstat_inc(p, cfs_se_statistics(cfs_se).nr_wakeups_affine);
 
 		return 1;
 	}

@@ -1902,7 +1903,7 @@ preempt:
 	 * Also, during early boot the idle thread is in the fair class,
 	 * for obvious reasons its a bad idea to schedule back to it.
 	 */
-	if (unlikely(!se_of_cfs_se(cfs_se)->on_rq || curr == rq->idle))
+	if (unlikely(!cfs_se_on_rq(cfs_se) || curr == rq->idle))
 		return;
 
 	if (sched_feat(LAST_BUDDY) && scale && cfs_entity_is_task(cfs_se))

Signature · 8 lines

1.7.2.3

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Lucas De Marchi

2011-01-04 16:18 UTC #4 lore raw

In reply to Dario Faggioli

Hi Dario

Quoted 27 lines · Dario Faggioli

Hi everybody,
After having heard Peter talking about it many times, I finally decided to give this thing a spin. It is basically the idea of putting the current sched_entity and sched_rt_entity together in an union, contained in a more general structure which hosts the common fields of the twos. For example, it came out here: http://lkml.org/lkml/2009/7/9/153
Therefore, this patchset _DOES_NOT_ do that! :-O
In fact, this is some preliminary refactoring which I think it's needed before union-ify the two data structure, and on which I would like to get some preliminary comments, before going ahead with the wrong approach.
Basically, I renamed struct sched_entity to struct sched_cfs_entity and put it and sched_rt_entity inside a more general struct sched_entity... Is that clear? :-)
The patches just cope with that. Yes, there's some work done by a patch and undone by the next one. I now it's annoying, and I'm sorry, but I did such for the sake of reviewability and for making each patch compile cleanly.
As for the name of CFS's scheduling entities, sched_fair_entity would probably have been better, but it's longer, and "_cfs" is already present in many places, so I went for it... No big deal in changing that, apart from stay-within-80-characters-per-line issues! :-P

Quoted 11 lines

They're not inside an union yet, because I'm not sure on how to treat the task group case. In fact, tasks can only have _just_one_ scheduling policy at a time, and thus, for example, they need the run_list _or_ the rb_node for queueing (if the task is RT or fair, respectively), which is perfect with respect to using an union.
OTOH, groups are always considered both fair _and_ RT entities, for example they're always queued in _both_ an RT run_list and a fair rb-tree. So I can't put them in an union, because I need both at the same time!
Suggestions on how to deal with that will be appreciated.

In fact I created the patchset but I didn't have time to finish, polish, test and send. There are several corner cases that must be treated that made me think if it was really worth the work.

In this repo you can find a greatly outdated branch that treats some of the troubles you'll need to think about:

git://git.profusion.mobi/users/lucas/linux-2.6.git schedentity

And this was a rebase I did, but irc it was not working well:

git://git.profusion.mobi/users/lucas/linux-2.6.git schedentity-rebased

Lucas De Marchi

Dario Faggioli

author

2011-01-04 18:32 UTC #5 lore raw

Quoted 2 lines · Lucas De Marchi

Hi Dario

Hi there,

Quoted 6 lines

Quoted 2 lines
Suggestions on how to deal with that will be appreciated.
In fact I created the patchset but I didn't have time to finish, polish, test and send.

Heh, I know that feeling! :-)

Quoted 3 lines

There are several corner cases that must be treated that made me think if it was really worth the work.

Ok, again, while git-clone and git-push do their jobs, feel free to share some of these concerns of yours with me, because I'm not sure I'm getting here...

Quoted 5 lines

In this repo you can find a greatly outdated branch that treats some of the troubles you'll need to think about:
git://git.profusion.mobi/users/lucas/linux-2.6.git schedentity

Ok, this is mine (it'll be ready in a while, I'm pushing it up):

git://gitorious.org/sched_deadline/linux-26.git schedentity

Right now I'm pushing my schedentity branch and cloning yours! :-D Unfortunately, both seems to be quite long operations... :-(

Quoted 2 lines

And this was a rebase I did, but irc it was not working well:

Well, I'll be very busy in the next days, but I plan to keep this updated enough, so feel free to comment and/or use it as a basis for further patching if you think it could help.

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Peter Zijlstra

2011-01-04 16:19 UTC #6 lore raw

In reply to Dario Faggioli

Quoted 9 lines · Dario Faggioli

They're not inside an union yet, because I'm not sure on how to treat the task group case. In fact, tasks can only have _just_one_ scheduling policy at a time, and thus, for example, they need the run_list _or_ the rb_node for queueing (if the task is RT or fair, respectively), which is perfect with respect to using an union.
OTOH, groups are always considered both fair _and_ RT entities, for example they're always queued in _both_ an RT run_list and a fair rb-tree. So I can't put them in an union, because I need both at the same time!

Just like its now, keep a sched_entity per class.

struct task_group {

#ifdef CONFIG_FAIR_GROUP_SCHED
	struct sched_entity **cfs_se;
	...

#endif

#ifdef CONFIG_RT_GROUP_SCHED
	struct sched_entity **rt_se;
	...

#endif

}

Quoted 1 line

And as usual, comments of any kind are very very welcome! :-)

Yay!

I see once clash with my current ttwu patch set though, see:

http://lkml.org/lkml/2011/1/4/228

But that should be easy to resolve.

Dario Faggioli

author

2011-01-04 18:12 UTC #7 lore raw

Quoted 12 lines · Peter Zijlstra

Quoted 4 lines
OTOH, groups are always considered both fair _and_ RT entities, for example they're always queued in _both_ an RT run_list and a fair rb-tree. So I can't put them in an union, because I need both at the same time!
Just like its now, keep a sched_entity per class.
struct task_group {
#ifdef CONFIG_FAIR_GROUP_SCHED
	struct sched_entity **cfs_se;
	...

Quoted 9 lines

#endif

#ifdef CONFIG_RT_GROUP_SCHED
	struct sched_entity **rt_se;
	...

#endif

}

Well, sure this can be done. But what about the common fields? I guess you're suggesting to use something like `struct sched_entity_common' and putting them there, aren't you?

If yes, I'm fine with that, although it'll add one more level of indirection for those fields (e.g., p->se.comm.on_rq). Are we cool with this?

Quoted 6 lines

I see once clash with my current ttwu patch set though, see:
http://lkml.org/lkml/2011/1/4/228
But that should be easy to resolve.

It's based on current tip, and yes, I plan to keep it updated and solve the clashes as soon as I run into them. :-)

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Peter Zijlstra

2011-01-04 19:26 UTC #8 lore raw

Quoted 14 lines · Dario Faggioli

Quoted 5 lines

struct task_group {

#ifdef CONFIG_FAIR_GROUP_SCHED
      struct sched_entity **cfs_se;
      ...

Quoted 9 lines

#endif

#ifdef CONFIG_RT_GROUP_SCHED
      struct sched_entity **rt_se;
      ...

#endif

}

Quoted 7 lines

Well, sure this can be done. But what about the common fields? I guess you're suggesting to use something like `struct sched_entity_common' and putting them there, aren't you?
If yes, I'm fine with that, although it'll add one more level of indirection for those fields (e.g., p->se.comm.on_rq). Are we cool with this?

I'm pretty sure we're mis-understanding each other here, I meant to keep two complete sched_entity structures (both containing the common/fair/rt parts). That way, one can use the fair and the other the rt part.

Lucas De Marchi

2011-01-04 16:37 UTC #9 lore raw

In reply to Dario Faggioli

Quoted 10 lines · Dario Faggioli

They're not inside an union yet, because I'm not sure on how to treat the task group case. In fact, tasks can only have _just_one_ scheduling policy at a time, and thus, for example, they need the run_list _or_ the rb_node for queueing (if the task is RT or fair, respectively), which is perfect with respect to using an union.
OTOH, groups are always considered both fair _and_ RT entities, for example they're always queued in _both_ an RT run_list and a fair rb-tree. So I can't put them in an union, because I need both at the same time!
Suggestions on how to deal with that will be appreciated.

Don't forget the PI case too. You will need to change rt_mutex_setprio() to keep a copy of sched_cfs_entity in struct rt_mutex_waiter.

Peter, isn't sched_fair_entity a better name?

Lucas De Marchi

Peter Zijlstra

2011-01-04 16:41 UTC #10 lore raw

Quoted 4 lines · Lucas De Marchi

Peter, isn't sched_fair_entity a better name?

Yeah, it probably is..

Dario Faggioli

author

2011-01-04 18:08 UTC #11 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines
Peter, isn't sched_fair_entity a better name?
Yeah, it probably is..

As you wish. I know it's just one letter longer, but be prepared for it to break a lot of lines, since you may find four of it in a row! :-P

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Dario Faggioli

author

2011-01-04 17:58 UTC #12 lore raw

In reply to Lucas De Marchi

Quoted 6 lines · Lucas De Marchi

Quoted 1 line
Suggestions on how to deal with that will be appreciated.
Don't forget the PI case too. You will need to change rt_mutex_setprio() to keep a copy of sched_cfs_entity in struct rt_mutex_waiter.

Mmm... do I? While I'm cloning your git, could you elaborate a bit on why, because I don't seem to see that... :-P

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]

Lucas De Marchi

2011-01-04 18:30 UTC #13 lore raw

Quoted 9 lines · Dario Faggioli

Quoted 6 lines · Lucas De Marchi
Quoted 1 line
Suggestions on how to deal with that will be appreciated.
Don't forget the PI case too. You will need to change rt_mutex_setprio() to keep a copy of sched_cfs_entity in struct rt_mutex_waiter.
Mmm... do I? While I'm cloning your git, could you elaborate a bit on why, because I don't seem to see that... :-P

Suppose a RT task blocks on a PI-mutex, the lock owner will be boosted to RT and go through a class change in rt_mutex_setprio(). Since now a class change reinitializes the class-specific, if fair and rt fields are on the same memory space, we need to save the sched_fair_entity before changing the class to RT and put it again when going back to the fair class.

Quoting Peter about this:

[ Initially I was thinking not, because the task slept we'll have to
reinsert it in the rb-tree anyway, but upon further consideration that'll loose the old vruntime setting, which can lead to an unseemly gain of time in place_entity()'s never backward check failing.

So yes, we'd have to place a copy of the old sched_entity in struct rt_mutex_waiter, not very hard to do. ]

As a side-note: irc this is one thing i didn't do when i prepared that patches.

Lucas De Marchi

Dario Faggioli

author

2011-01-04 18:57 UTC #14 lore raw

Quoted 10 lines · Lucas De Marchi

Quoted 2 lines
Mmm... do I? While I'm cloning your git, could you elaborate a bit on why, because I don't seem to see that... :-P
Suppose a RT task blocks on a PI-mutex, the lock owner will be boosted to RT and go through a class change in rt_mutex_setprio(). Since now a class change reinitializes the class-specific, if fair and rt fields are on the same memory space, we need to save the sched_fair_entity before changing the class to RT and put it again when going back to the fair class.

Well, I know, but you're deactivating+dequeueing and then activating+enqueueing it back and forth within the proper scheduling class, so that shouldn't be a big deal...

Actually, the point might be that forgetting something like, e.g., vruntime would then lead to unexpected behaviour when the task is back to fair scheduling, but do we need to cache the whole sched_[cfs| fair]_entity for that?

Quoted 10 lines

Quoting Peter about this:
[ Initially I was thinking not, because the task slept we'll have to
reinsert it in the rb-tree anyway, but upon further consideration that'll loose the old vruntime setting, which can lead to an unseemly gain of time in place_entity()'s never backward check failing.
So yes, we'd have to place a copy of the old sched_entity in struct rt_mutex_waiter, not very hard to do. ]

Ok, exactly, I now see that, and it's probably not hard to do... But I'm now thinking of how many fields must be saved to avoid this. Surely, not these ones:
...

	struct rb_node          run_node;
	struct list_head        group_node;

#ifdef CONFIG_FAIR_GROUP_SCHED
	struct sched_cfs_entity *parent;
	/* rq on which this entity is (to be) queued: */
	struct cfs_rq           *cfs_rq;
	/* rq "owned" by this entity/group: */
	struct cfs_rq           *my_q;

#endif
...

I'll double check, but if it's just a matter of vruntime and a couple of other u64, isn't it worth to just save them?

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]