Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1246415 > unrolled thread

[PATCH v2 0/2] sched: account fair load avg consistently

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] sched: account fair load avg consistently <byungchul.park@lge.com> - 2015-10-14 10:50 +0200
    [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class <byungchul.park@lge.com> - 2015-10-14 10:50 +0200
      Re: [PATCH v2 1/2] sched: make each sched class handle its rq  assignment in their own class Peter Zijlstra <peterz@infradead.org> - 2015-10-14 11:10 +0200
        Re: [PATCH v2 1/2] sched: make each sched class handle its rq  assignment in their own class Byungchul Park <byungchul.park@lge.com> - 2015-10-14 11:30 +0200
          Re: [PATCH v2 1/2] sched: make each sched class handle its rq  assignment in their own class Peter Zijlstra <peterz@infradead.org> - 2015-10-14 11:40 +0200
            Re: [PATCH v2 1/2] sched: make each sched class handle its rq  assignment in their own class Byungchul Park <byungchul.park@lge.com> - 2015-10-14 12:20 +0200

#1246415 — [PATCH v2 0/2] sched: account fair load avg consistently

From: Byungchul Park <byungchul.park@lge.com>

* change from v1 to v2
- make set_task_rq() do that role instead of migration callback
- make set_task_rq() do that role instead of move group callback
- remove the dependancy between last_update_time and check for migration

Byungchul Park (2):
  sched: make each sched class handle its rq assignment in their own
    class
  sched: make it possible to account fair class load avg consistently

 include/linux/sched.h |    3 ++
 kernel/sched/core.c   |    1 +
 kernel/sched/fair.c   |   77 ++++++++++++++++++++++---------
 kernel/sched/rt.c     |   14 ++++++
 kernel/sched/sched.h  |  121 ++++++++++++++++++++++++-------------------------
 5 files changed, 132 insertions(+), 84 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1246416 — [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class

From: Byungchul Park <byungchul.park@lge.com>

set_task_rq() which is a commonly used function regardless of sched class
is currently assigning both cfs_rq for fair class and rt_rq for rt class
to a task. but it would be better that a class related operation is done
by its own class respectively. additionally, this patch moves some code's
position to refer for_each_class() macro.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c  |    9 ++++
 kernel/sched/rt.c    |   14 ++++++
 kernel/sched/sched.h |  120 ++++++++++++++++++++++++--------------------------
 3 files changed, 80 insertions(+), 63 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 077076f..07882c2 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8088,6 +8088,14 @@ static void task_move_group_fair(struct task_struct *p)
 	attach_task_cfs_rq(p);
 }
 
+static void set_task_rq_fair(struct task_struct *p, unsigned int cpu)
+{
+	struct task_group *tg = task_group(p);
+
+	p->se.cfs_rq = tg->cfs_rq[cpu];
+	p->se.parent = tg->se[cpu];
+}
+
 void free_fair_sched_group(struct task_group *tg)
 {
 	int i;
@@ -8306,6 +8314,7 @@ const struct sched_class fair_sched_class = {
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	.task_move_group	= task_move_group_fair,
+	.set_task_rq		= set_task_rq_fair,
 #endif
 };
 
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index e3cc163..f1a1320 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2143,6 +2143,16 @@ static void switched_to_rt(struct rq *rq, struct task_struct *p)
 	}
 }
 
+#ifdef CONFIG_RT_GROUP_SCHED
+static void set_task_rq_rt(struct task_struct *p, unsigned int cpu)
+{
+	struct task_group *tg = task_group(p);
+
+	p->rt.rt_rq  = tg->rt_rq[cpu];
+	p->rt.parent = tg->rt_se[cpu];
+}
+#endif
+
 /*
  * Priority of the task has changed. This may cause
  * us to initiate a push or pull.
@@ -2290,6 +2300,10 @@ const struct sched_class rt_sched_class = {
 	.switched_to		= switched_to_rt,
 
 	.update_curr		= update_curr_rt,
+
+#ifdef CONFIG_RT_GROUP_SCHED
+	.set_task_rq		= set_task_rq_rt,
+#endif
 };
 
 #ifdef CONFIG_SCHED_DEBUG
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index af6f252..7f73e89 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -905,69 +905,6 @@ static inline void sched_ttwu_pending(void) { }
 #include "stats.h"
 #include "auto_group.h"
 
-#ifdef CONFIG_CGROUP_SCHED
-
-/*
- * Return the group to which this tasks belongs.
- *
- * We cannot use task_css() and friends because the cgroup subsystem
- * changes that value before the cgroup_subsys::attach() method is called,
- * therefore we cannot pin it and might observe the wrong value.
- *
- * The same is true for autogroup's p->signal->autogroup->tg, the autogroup
- * core changes this before calling sched_move_task().
- *
- * Instead we use a 'copy' which is updated from sched_move_task() while
- * holding both task_struct::pi_lock and rq::lock.
- */
-static inline struct task_group *task_group(struct task_struct *p)
-{
-	return p->sched_task_group;
-}
-
-/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
-static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
-{
-#if defined(CONFIG_FAIR_GROUP_SCHED) || defined(CONFIG_RT_GROUP_SCHED)
-	struct task_group *tg = task_group(p);
-#endif
-
-#ifdef CONFIG_FAIR_GROUP_SCHED
-	p->se.cfs_rq = tg->cfs_rq[cpu];
-	p->se.parent = tg->se[cpu];
-#endif
-
-#ifdef CONFIG_RT_GROUP_SCHED
-	p->rt.rt_rq  = tg->rt_rq[cpu];
-	p->rt.parent = tg->rt_se[cpu];
-#endif
-}
-
-#else /* CONFIG_CGROUP_SCHED */
-
-static inline void set_task_rq(struct task_struct *p, unsigned int cpu) { }
-static inline struct task_group *task_group(struct task_struct *p)
-{
-	return NULL;
-}
-
-#endif /* CONFIG_CGROUP_SCHED */
-
-static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
-{
-	set_task_rq(p, cpu);
-#ifdef CONFIG_SMP
-	/*
-	 * After ->cpu is set up to a new value, task_rq_lock(p, ...) can be
-	 * successfuly executed on another CPU. We must ensure that updates of
-	 * per-task data have been completed by this moment.
-	 */
-	smp_wmb();
-	task_thread_info(p)->cpu = cpu;
-	p->wake_cpu = cpu;
-#endif
-}
-
 /*
  * Tunables that become constants when CONFIG_SCHED_DEBUG is off:
  */
@@ -1222,6 +1159,8 @@ struct sched_class {
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	void (*task_move_group) (struct task_struct *p);
 #endif
+
+	void (*set_task_rq) (struct task_struct *p, unsigned int cpu);
 };
 
 static inline void put_prev_task(struct rq *rq, struct task_struct *prev)
@@ -1239,6 +1178,61 @@ extern const struct sched_class rt_sched_class;
 extern const struct sched_class fair_sched_class;
 extern const struct sched_class idle_sched_class;
 
+#ifdef CONFIG_CGROUP_SCHED
+
+/*
+ * Return the group to which this tasks belongs.
+ *
+ * We cannot use task_css() and friends because the cgroup subsystem
+ * changes that value before the cgroup_subsys::attach() method is called,
+ * therefore we cannot pin it and might observe the wrong value.
+ *
+ * The same is true for autogroup's p->signal->autogroup->tg, the autogroup
+ * core changes this before calling sched_move_task().
+ *
+ * Instead we use a 'copy' which is updated from sched_move_task() while
+ * holding both task_struct::pi_lock and rq::lock.
+ */
+static inline struct task_group *task_group(struct task_struct *p)
+{
+	return p->sched_task_group;
+}
+
+/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
+static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
+{
+	const struct sched_class *class;
+
+	for_each_class(class) {
+		if (class->set_task_rq)
+			class->set_task_rq(p, cpu);
+	}
+}
+
+#else /* CONFIG_CGROUP_SCHED */
+
+static inline void set_task_rq(struct task_struct *p, unsigned int cpu) { }
+static inline struct task_group *task_group(struct task_struct *p)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_CGROUP_SCHED */
+
+static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+	set_task_rq(p, cpu);
+#ifdef CONFIG_SMP
+	/*
+	 * After ->cpu is set up to a new value, task_rq_lock(p, ...) can be
+	 * successfuly executed on another CPU. We must ensure that updates of
+	 * per-task data have been completed by this moment.
+	 */
+	smp_wmb();
+	task_thread_info(p)->cpu = cpu;
+	p->wake_cpu = cpu;
+#endif
+}
 
 #ifdef CONFIG_SMP
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246432 — Re: [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class

On Wed, Oct 14, 2015 at 05:42:09PM +0900, byungchul.park@lge.com wrote:
> +static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> +{
> +	const struct sched_class *class;
> +
> +	for_each_class(class) {
> +		if (class->set_task_rq)
> +			class->set_task_rq(p, cpu);
> +	}
> +}

So I worry about this, because the class structures are not all in the
same translation unit, GCC cannot (without -fwhole-program) optimize
that all away.

This means we'll do 5 cacheline loads and 2 indirect calls, on _every_
cpu migration.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246458 — Re: [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class

On Wed, Oct 14, 2015 at 11:00:16AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 14, 2015 at 05:42:09PM +0900, byungchul.park@lge.com wrote:
> > +static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> > +{
> > +	const struct sched_class *class;
> > +
> > +	for_each_class(class) {
> > +		if (class->set_task_rq)
> > +			class->set_task_rq(p, cpu);
> > +	}
> > +}
> 
> So I worry about this, because the class structures are not all in the
> same translation unit, GCC cannot (without -fwhole-program) optimize
> that all away.

i wondered if it was so, and it was.

> 
> This means we'll do 5 cacheline loads and 2 indirect calls, on _every_
> cpu migration.

i agree with your this concern. to avoid this concern, it can be done by
hard coding as current code.. but we will lose code flexability. i thought
migration overhead was not so important since it hardly happens.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246460 — Re: [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class

On Wed, Oct 14, 2015 at 06:26:30PM +0900, Byungchul Park wrote:
> On Wed, Oct 14, 2015 at 11:00:16AM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 14, 2015 at 05:42:09PM +0900, byungchul.park@lge.com wrote:
> > > +static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> > > +{
> > > +	const struct sched_class *class;
> > > +
> > > +	for_each_class(class) {
> > > +		if (class->set_task_rq)
> > > +			class->set_task_rq(p, cpu);
> > > +	}
> > > +}
> > 
> > So I worry about this, because the class structures are not all in the
> > same translation unit, GCC cannot (without -fwhole-program) optimize
> > that all away.
> 
> i wondered if it was so, and it was.

--combine might be enough, and might make sense for all the
kernel/sched/ file, but that too is not something we currently do.

> > This means we'll do 5 cacheline loads and 2 indirect calls, on _every_
> > cpu migration.
> 
> i agree with your this concern. to avoid this concern, it can be done by
> hard coding as current code.. but we will lose code flexability. i thought
> migration overhead was not so important since it hardly happens.

It happens quite a lot, just not _as_ often as regular context
switching.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246489 — Re: [PATCH v2 1/2] sched: make each sched class handle its rq assignment in their own class

On Wed, Oct 14, 2015 at 11:30:24AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 14, 2015 at 06:26:30PM +0900, Byungchul Park wrote:
> > On Wed, Oct 14, 2015 at 11:00:16AM +0200, Peter Zijlstra wrote:
> > > On Wed, Oct 14, 2015 at 05:42:09PM +0900, byungchul.park@lge.com wrote:
> > > > +static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> > > > +{
> > > > +	const struct sched_class *class;
> > > > +
> > > > +	for_each_class(class) {
> > > > +		if (class->set_task_rq)
> > > > +			class->set_task_rq(p, cpu);
> > > > +	}
> > > > +}
> > > 
> > > So I worry about this, because the class structures are not all in the
> > > same translation unit, GCC cannot (without -fwhole-program) optimize
> > > that all away.
> > 
> > i wondered if it was so, and it was.
> 
> --combine might be enough, and might make sense for all the
> kernel/sched/ file, but that too is not something we currently do.
> 
> > > This means we'll do 5 cacheline loads and 2 indirect calls, on _every_
> > > cpu migration.
> > 
> > i agree with your this concern. to avoid this concern, it can be done by
> > hard coding as current code.. but we will lose code flexability. i thought
> > migration overhead was not so important since it hardly happens.
> 
> It happens quite a lot, just not _as_ often as regular context
> switching.

ok, i will take the optimization into account in the migration case.

thanks,
byungchul

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web