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

Groups > linux.kernel > #1210424 > unrolled thread

[PATCH v4 0/5] sync a se with its cfs_rq when att(det)aching it

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/5] sync a se with its cfs_rq when att(det)aching it byungchul.park@lge.com - 2015-08-20 13:30 +0200
    [PATCH v4 2/5] sched: make task_move_group_fair adjust cfs_rq's load in case of queued byungchul.park@lge.com - 2015-08-20 13:30 +0200
    [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq byungchul.park@lge.com - 2015-08-20 13:30 +0200
      Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Byungchul Park <byungchul.park@lge.com> - 2015-08-20 13:40 +0200
        Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Peter Zijlstra <peterz@infradead.org> - 2015-08-31 17:30 +0200
          Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Peter Zijlstra <peterz@infradead.org> - 2015-08-31 17:40 +0200
          Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Byungchul Park <byungchul.park@lge.com> - 2015-09-01 02:30 +0200
            Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Byungchul Park <byungchul.park@lge.com> - 2015-09-01 05:00 +0200
            Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Peter Zijlstra <peterz@infradead.org> - 2015-09-01 17:10 +0200
              Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Byungchul Park <byungchul.park@lge.com> - 2015-09-02 04:40 +0200
                Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Peter Zijlstra <peterz@infradead.org> - 2015-09-02 10:20 +0200
                  Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a  task to(from) a cfs_rq Ingo Molnar <mingo@kernel.org> - 2015-09-02 10:40 +0200
    [PATCH v4 4/5] sched: sync a se with its cfs_rq when switching sched class to fair class byungchul.park@lge.com - 2015-08-20 13:30 +0200
    [PATCH v4 1/5] sched: add two functions adjusting cfs_rq's load when att(det)aching a se byungchul.park@lge.com - 2015-08-20 13:30 +0200
    Re: [PATCH v4 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-22 09:50 +0200
    Re: [PATCH v4 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-26 02:00 +0200

#1210424 — [PATCH v4 0/5] sync a se with its cfs_rq when att(det)aching it

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

there are 3 problems when att(det)aching a se to(from) its cfs_rq.

problem 1. se's average load is not accounted with new cfs_rq in queued case,
when a task changes its cgroup.

problem 2. cfs_rq->avg.load_avg becomes larger and larger whenever changing
cgroup to another. we have to sync se's average load with prev cfs_rq.

problem 3. current code does not sync it with its cfs_rq when switching
sched class to fair class. if we can always assume that a se has been
detached from fair class for a long time enough for its average load to
become useless, at the time attaching it to its fair class cfs_rq, then
current code is acceptable. but the assumption is not always true.

patch 1/5, does code refactoring for further patches.
patch 2/5, solves the problem 1.
patch 3/5, solves the problem 2.
patch 4/5, solves the problem 3.
patch 5/5, does code refactoring for better readability.

change from v3 to v4
* clean up code as Peter suggests
* use upper case letter in comment properly

change from v2 to v3 (logic is same as v2)
* fix up my poor english in commit message and comment
* break down big patches into more patches for being reviewed easily
* supplement cover letter messages

change from v1 to v2
* introduce two functions for adjusting vruntime and load when attaching
  and detaching.
* call the introduced functions instead of switched_from(to)_fair() directly
  in task_move_group_fair().
* add decaying logic for a se which has detached from a cfs_rq.

thanks,
byungchul

Byungchul Park (5):
  sched: add two functions adjusting cfs_rq's load when att(det)aching
    a se
  sched: make task_move_group_fair adjust cfs_rq's load in case of
    queued
  sched: sync a se with prev cfs_rq when changing cgroup
  sched: sync a se with its cfs_rq when switching sched class to fair
    class
  sched: add two functions for att(det)aching a task to(from) a cfs_rq

 kernel/sched/fair.c |  222 ++++++++++++++++++++++++++-------------------------
 1 file changed, 113 insertions(+), 109 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]

#1210425 — [PATCH v4 2/5] sched: make task_move_group_fair adjust cfs_rq's load in case of queued

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

se's average load should be added to new cfs_rq, not only in case of
!queued but also in case of queued. older code managed cfs_rq's blocked
load separately. in that case, the blocked load was meaningful only in
case that the se is in !queued. but now load tracking code is changed,
it is not true. code adjusting cfs_rq's average load should be changed.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a43023b..9f93c8b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8041,13 +8041,12 @@ static void task_move_group_fair(struct task_struct *p, int queued)
 		se->vruntime -= cfs_rq_of(se)->min_vruntime;
 	set_task_rq(p, task_cpu(p));
 	se->depth = se->parent ? se->parent->depth + 1 : 0;
-	if (!queued) {
-		cfs_rq = cfs_rq_of(se);
+	cfs_rq = cfs_rq_of(se);
+	if (!queued)
 		se->vruntime += cfs_rq->min_vruntime;
 
-		/* Virtually synchronize task with its new cfs_rq */
-		attach_entity_load_avg(cfs_rq, se);
-	}
+	/* Virtually synchronize task with its new cfs_rq */
+	attach_entity_load_avg(cfs_rq, se);
 }
 
 void free_fair_sched_group(struct task_group *tg)
-- 
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]

#1210426 — [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

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

this patch introduces two functions for adjusting a task's vruntime
and the target cfs_rq's average load so that redundancy code doing
same things can be simplified. and it provides better readability.
switched_from_fair, switched_to_fair and task_move_group_fair can
use introduced functions and get simplified.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |  137 ++++++++++++++++++++++-----------------------------
 1 file changed, 60 insertions(+), 77 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index af6ad5f..53d0e30 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7918,21 +7918,47 @@ prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
 		check_preempt_curr(rq, p, 0);
 }
 
-static void switched_from_fair(struct rq *rq, struct task_struct *p)
+static inline bool vruntime_normalized(struct task_struct *p)
 {
+	int queued = task_on_rq_queued(p);
 	struct sched_entity *se = &p->se;
-	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 
 	/*
-	 * Ensure the task's vruntime is normalized, so that when it's
-	 * switched back to the fair class the enqueue_entity(.flags=0) will
-	 * do the right thing.
-	 *
 	 * If it's queued, then the dequeue_entity(.flags=0) will already
-	 * have normalized the vruntime, if it's !queued, then only when
-	 * the task is sleeping will it still have non-normalized vruntime.
+	 * have normalized the vruntime.
+	 */
+	if (queued)
+		return true;
+
+	/*
+	 * When !queued, vruntime of the task has usually NOT been normalized.
+	 * But there are some cases where it has already been normalized:
+	 *
+	 * - A forked child which is waiting for being woken up by
+	 *   wake_up_new_task().
+	 * - A task which has been woken up by try_to_wake_up() and
+	 *   waiting for actually being woken up by sched_ttwu_pending().
 	 */
-	if (!task_on_rq_queued(p) && p->state != TASK_RUNNING) {
+	if (!se->sum_exec_runtime || p->state == TASK_WAKING)
+		return true;
+
+	/*
+	 * If it's !queued, then only when the task is sleeping it has a
+	 * non-normalized vruntime, that is, when the task is being migrated
+	 * it has a normailized vruntime.
+	 */
+	if (p->state == TASK_RUNNING)
+		return true;
+
+	return false;
+}
+
+static void detach_task_cfs_rq(struct task_struct *p)
+{
+	struct sched_entity *se = &p->se;
+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
+
+	if (!vruntime_normalized(p)) {
 		/*
 		 * Fix up our vruntime so that the current sleep doesn't
 		 * cause 'unlimited' sleep bonus.
@@ -7945,9 +7971,10 @@ static void switched_from_fair(struct rq *rq, struct task_struct *p)
 	detach_entity_load_avg(cfs_rq, se);
 }
 
-static void switched_to_fair(struct rq *rq, struct task_struct *p)
+static void attach_task_cfs_rq(struct task_struct *p)
 {
 	struct sched_entity *se = &p->se;
+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	/*
@@ -7958,33 +7985,32 @@ static void switched_to_fair(struct rq *rq, struct task_struct *p)
 #endif
 
 	/* Synchronize task with its cfs_rq */
-	attach_entity_load_avg(cfs_rq_of(&p->se), &p->se);
+	attach_entity_load_avg(cfs_rq, se);
 
-	if (!task_on_rq_queued(p)) {
+	if (!vruntime_normalized(p))
+		se->vruntime += cfs_rq->min_vruntime;
+}
 
+static void switched_from_fair(struct rq *rq, struct task_struct *p)
+{
+	detach_task_cfs_rq(p);
+}
+
+static void switched_to_fair(struct rq *rq, struct task_struct *p)
+{
+	attach_task_cfs_rq(p);
+
+	if (task_on_rq_queued(p)) {
 		/*
-		 * Ensure the task has a non-normalized vruntime when it is switched
-		 * back to the fair class with !queued, so that enqueue_entity() at
-		 * wake-up time will do the right thing.
-		 *
-		 * If it's queued, then the enqueue_entity(.flags=0) makes the task
-		 * has non-normalized vruntime, if it's !queued, then it still has
-		 * normalized vruntime.
+		 * We were most likely switched from sched_rt, so
+		 * kick off the schedule if running, otherwise just see
+		 * if we can still preempt the current task.
 		 */
-		if (p->state != TASK_RUNNING)
-			se->vruntime += cfs_rq_of(se)->min_vruntime;
-		return;
+		if (rq->curr == p)
+			resched_curr(rq);
+		else
+			check_preempt_curr(rq, p, 0);
 	}
-
-	/*
-	 * We were most likely switched from sched_rt, so
-	 * kick off the schedule if running, otherwise just see
-	 * if we can still preempt the current task.
-	 */
-	if (rq->curr == p)
-		resched_curr(rq);
-	else
-		check_preempt_curr(rq, p, 0);
 }
 
 /* Account for a task changing its policy or group.
@@ -8021,55 +8047,12 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
 #ifdef CONFIG_FAIR_GROUP_SCHED
 static void task_move_group_fair(struct task_struct *p, int queued)
 {
-	struct sched_entity *se = &p->se;
-	struct cfs_rq *cfs_rq;
-
-	/*
-	 * If the task was not on the rq at the time of this cgroup movement
-	 * it must have been asleep, sleeping tasks keep their ->vruntime
-	 * absolute on their old rq until wakeup (needed for the fair sleeper
-	 * bonus in place_entity()).
-	 *
-	 * If it was on the rq, we've just 'preempted' it, which does convert
-	 * ->vruntime to a relative base.
-	 *
-	 * Make sure both cases convert their relative position when migrating
-	 * to another cgroup's rq. This does somewhat interfere with the
-	 * fair sleeper stuff for the first placement, but who cares.
-	 */
-	/*
-	 * When !queued, vruntime of the task has usually NOT been normalized.
-	 * But there are some cases where it has already been normalized:
-	 *
-	 * - Moving a forked child which is waiting for being woken up by
-	 *   wake_up_new_task().
-	 * - Moving a task which has been woken up by try_to_wake_up() and
-	 *   waiting for actually being woken up by sched_ttwu_pending().
-	 *
-	 * To prevent boost or penalty in the new cfs_rq caused by delta
-	 * min_vruntime between the two cfs_rqs, we skip vruntime adjustment.
-	 */
-	if (!queued && (!se->sum_exec_runtime || p->state == TASK_WAKING))
-		queued = 1;
-
-	cfs_rq = cfs_rq_of(se);
-	if (!queued)
-		se->vruntime -= cfs_rq->min_vruntime;
-
-	/* Synchronize task with its prev cfs_rq */
-	detach_entity_load_avg(cfs_rq, se);
+	detach_task_cfs_rq(p);
 	set_task_rq(p, task_cpu(p));
 
 	/* Tell se's cfs_rq has been changed -- migrated */
 	p->se.avg.last_update_time = 0;
-
-	se->depth = se->parent ? se->parent->depth + 1 : 0;
-	cfs_rq = cfs_rq_of(se);
-	if (!queued)
-		se->vruntime += cfs_rq->min_vruntime;
-
-	/* Virtually synchronize task with its new cfs_rq */
-	attach_entity_load_avg(cfs_rq, se);
+	attach_task_cfs_rq(p);
 }
 
 void free_fair_sched_group(struct task_group *tg)
-- 
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]

#1210437 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Thu, Aug 20, 2015 at 08:22:00PM +0900, byungchul.park@lge.com wrote:
> From: Byungchul Park <byungchul.park@lge.com>
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index af6ad5f..53d0e30 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7918,21 +7918,47 @@ prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
>  		check_preempt_curr(rq, p, 0);
>  }
>  
> -static void switched_from_fair(struct rq *rq, struct task_struct *p)
> +static inline bool vruntime_normalized(struct task_struct *p)
>  {
> +	int queued = task_on_rq_queued(p);
>  	struct sched_entity *se = &p->se;
> -	struct cfs_rq *cfs_rq = cfs_rq_of(se);
>  
>  	/*
> -	 * Ensure the task's vruntime is normalized, so that when it's
> -	 * switched back to the fair class the enqueue_entity(.flags=0) will
> -	 * do the right thing.
> -	 *
>  	 * If it's queued, then the dequeue_entity(.flags=0) will already
> -	 * have normalized the vruntime, if it's !queued, then only when
> -	 * the task is sleeping will it still have non-normalized vruntime.
> +	 * have normalized the vruntime.
> +	 */
> +	if (queued)
> +		return true;
> +
> +	/*
> +	 * When !queued, vruntime of the task has usually NOT been normalized.
> +	 * But there are some cases where it has already been normalized:
> +	 *
> +	 * - A forked child which is waiting for being woken up by
> +	 *   wake_up_new_task().
> +	 * - A task which has been woken up by try_to_wake_up() and
> +	 *   waiting for actually being woken up by sched_ttwu_pending().
>  	 */
> -	if (!task_on_rq_queued(p) && p->state != TASK_RUNNING) {
> +	if (!se->sum_exec_runtime || p->state == TASK_WAKING)
> +		return true;
> +
> +	/*
> +	 * If it's !queued, then only when the task is sleeping it has a
> +	 * non-normalized vruntime, that is, when the task is being migrated
> +	 * it has a normailized vruntime.
> +	 */

i tried to change your XXX comment. i think it can be explaned like this.
don't you think so? i want to hear any opinions about this.

thanks,
byungchul

> +	if (p->state == TASK_RUNNING)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void detach_task_cfs_rq(struct task_struct *p)
> +{
> +	struct sched_entity *se = &p->se;
> +	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> +
> +	if (!vruntime_normalized(p)) {
>  		/*
>  		 * Fix up our vruntime so that the current sleep doesn't
>  		 * cause 'unlimited' sleep bonus.
> @@ -7945,9 +7971,10 @@ static void switched_from_fair(struct rq *rq, struct task_struct *p)
>  	detach_entity_load_avg(cfs_rq, se);
>  }
>  
> -static void switched_to_fair(struct rq *rq, struct task_struct *p)
> +static void attach_task_cfs_rq(struct task_struct *p)
>  {
>  	struct sched_entity *se = &p->se;
> +	struct cfs_rq *cfs_rq = cfs_rq_of(se);
>  
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>  	/*
> @@ -7958,33 +7985,32 @@ static void switched_to_fair(struct rq *rq, struct task_struct *p)
>  #endif
>  
>  	/* Synchronize task with its cfs_rq */
> -	attach_entity_load_avg(cfs_rq_of(&p->se), &p->se);
> +	attach_entity_load_avg(cfs_rq, se);
>  
> -	if (!task_on_rq_queued(p)) {
> +	if (!vruntime_normalized(p))
> +		se->vruntime += cfs_rq->min_vruntime;
> +}
>  
> +static void switched_from_fair(struct rq *rq, struct task_struct *p)
> +{
> +	detach_task_cfs_rq(p);
> +}
> +
> +static void switched_to_fair(struct rq *rq, struct task_struct *p)
> +{
> +	attach_task_cfs_rq(p);
> +
> +	if (task_on_rq_queued(p)) {
>  		/*
> -		 * Ensure the task has a non-normalized vruntime when it is switched
> -		 * back to the fair class with !queued, so that enqueue_entity() at
> -		 * wake-up time will do the right thing.
> -		 *
> -		 * If it's queued, then the enqueue_entity(.flags=0) makes the task
> -		 * has non-normalized vruntime, if it's !queued, then it still has
> -		 * normalized vruntime.
> +		 * We were most likely switched from sched_rt, so
> +		 * kick off the schedule if running, otherwise just see
> +		 * if we can still preempt the current task.
>  		 */
> -		if (p->state != TASK_RUNNING)
> -			se->vruntime += cfs_rq_of(se)->min_vruntime;
> -		return;
> +		if (rq->curr == p)
> +			resched_curr(rq);
> +		else
> +			check_preempt_curr(rq, p, 0);
>  	}
> -
> -	/*
> -	 * We were most likely switched from sched_rt, so
> -	 * kick off the schedule if running, otherwise just see
> -	 * if we can still preempt the current task.
> -	 */
> -	if (rq->curr == p)
> -		resched_curr(rq);
> -	else
> -		check_preempt_curr(rq, p, 0);
>  }
>  
>  /* Account for a task changing its policy or group.
> @@ -8021,55 +8047,12 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>  static void task_move_group_fair(struct task_struct *p, int queued)
>  {
> -	struct sched_entity *se = &p->se;
> -	struct cfs_rq *cfs_rq;
> -
> -	/*
> -	 * If the task was not on the rq at the time of this cgroup movement
> -	 * it must have been asleep, sleeping tasks keep their ->vruntime
> -	 * absolute on their old rq until wakeup (needed for the fair sleeper
> -	 * bonus in place_entity()).
> -	 *
> -	 * If it was on the rq, we've just 'preempted' it, which does convert
> -	 * ->vruntime to a relative base.
> -	 *
> -	 * Make sure both cases convert their relative position when migrating
> -	 * to another cgroup's rq. This does somewhat interfere with the
> -	 * fair sleeper stuff for the first placement, but who cares.
> -	 */
> -	/*
> -	 * When !queued, vruntime of the task has usually NOT been normalized.
> -	 * But there are some cases where it has already been normalized:
> -	 *
> -	 * - Moving a forked child which is waiting for being woken up by
> -	 *   wake_up_new_task().
> -	 * - Moving a task which has been woken up by try_to_wake_up() and
> -	 *   waiting for actually being woken up by sched_ttwu_pending().
> -	 *
> -	 * To prevent boost or penalty in the new cfs_rq caused by delta
> -	 * min_vruntime between the two cfs_rqs, we skip vruntime adjustment.
> -	 */
> -	if (!queued && (!se->sum_exec_runtime || p->state == TASK_WAKING))
> -		queued = 1;
> -
> -	cfs_rq = cfs_rq_of(se);
> -	if (!queued)
> -		se->vruntime -= cfs_rq->min_vruntime;
> -
> -	/* Synchronize task with its prev cfs_rq */
> -	detach_entity_load_avg(cfs_rq, se);
> +	detach_task_cfs_rq(p);
>  	set_task_rq(p, task_cpu(p));
>  
>  	/* Tell se's cfs_rq has been changed -- migrated */
>  	p->se.avg.last_update_time = 0;
> -
> -	se->depth = se->parent ? se->parent->depth + 1 : 0;
> -	cfs_rq = cfs_rq_of(se);
> -	if (!queued)
> -		se->vruntime += cfs_rq->min_vruntime;
> -
> -	/* Virtually synchronize task with its new cfs_rq */
> -	attach_entity_load_avg(cfs_rq, se);
> +	attach_task_cfs_rq(p);
>  }
>  
>  void free_fair_sched_group(struct task_group *tg)
> -- 
> 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/
--
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]

#1216264 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Thu, Aug 20, 2015 at 08:35:16PM +0900, Byungchul Park wrote:
> On Thu, Aug 20, 2015 at 08:22:00PM +0900, byungchul.park@lge.com wrote:

> > +	/*
> > +	 * If it's !queued, then only when the task is sleeping it has a
> > +	 * non-normalized vruntime, that is, when the task is being migrated
> > +	 * it has a normailized vruntime.
> > +	 */
> 
> i tried to change your XXX comment. i think it can be explaned like this.
> don't you think so? i want to hear any opinions about this.
> 
> > +	if (p->state == TASK_RUNNING)
> > +		return true;


--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7943,11 +7943,10 @@ static inline bool vruntime_normalized(s
 		return true;
 
 	/*
-	 * If it's !queued, then only when the task is sleeping it has a
-	 * non-normalized vruntime, that is, when the task is being migrated
-	 * it has a normalized vruntime.
+	 * If it's !queued, sleeping tasks have a normalized vruntime,
+	 * see dequeue_entity().
 	 */
-	if (p->state == TASK_RUNNING)
+	if (!p->se.on_rq)
 		return true;
 
 	return false;


Does that make sense?

I think using p->state for this is fragile, as we could be racy with any
random blocking primitive that does set_current_state() _before_
actually calling into the scheduler.


--
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]

#1216269 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Mon, Aug 31, 2015 at 05:21:38PM +0200, Peter Zijlstra wrote:

> +	if (!p->se.on_rq)
>  		return true;
>  
>  	return false;
> 
> 
> Does that make sense?

It does not.. never mind me.
--
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]

#1216533 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Mon, Aug 31, 2015 at 05:21:38PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 20, 2015 at 08:35:16PM +0900, Byungchul Park wrote:
> > On Thu, Aug 20, 2015 at 08:22:00PM +0900, byungchul.park@lge.com wrote:
> 
> > > +	/*
> > > +	 * If it's !queued, then only when the task is sleeping it has a
> > > +	 * non-normalized vruntime, that is, when the task is being migrated
> > > +	 * it has a normailized vruntime.
> > > +	 */
> > 
> > i tried to change your XXX comment. i think it can be explaned like this.
> > don't you think so? i want to hear any opinions about this.
> > 
> > > +	if (p->state == TASK_RUNNING)
> > > +		return true;
> 
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7943,11 +7943,10 @@ static inline bool vruntime_normalized(s
>  		return true;
>  
>  	/*
> -	 * If it's !queued, then only when the task is sleeping it has a
> -	 * non-normalized vruntime, that is, when the task is being migrated
> -	 * it has a normalized vruntime.
> +	 * If it's !queued, sleeping tasks have a normalized vruntime,
> +	 * see dequeue_entity().

check the condition "!(flags & DEQUEUE_SLEEP)" for doing normalizing in
dequeue_entity(). i think you have to keep my original comment, or
modify your comment to something like below.

before - If it's !queued, sleeping tasks have a normalized vruntime,
after - If it's !queued, sleeping tasks have a non-normalize vruntime,

but.. i think it would be better that you keep my original comment..

>  	 */
> -	if (p->state == TASK_RUNNING)
> +	if (!p->se.on_rq)
>  		return true;
>  
>  	return false;
> 
> 
> Does that make sense?
> 
> I think using p->state for this is fragile, as we could be racy with any
> random blocking primitive that does set_current_state() _before_
> actually calling into the scheduler.
> 
> 
> --
> 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]

#1216569 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Tue, Sep 01, 2015 at 09:28:49AM +0900, Byungchul Park wrote:
> On Mon, Aug 31, 2015 at 05:21:38PM +0200, Peter Zijlstra wrote:
> > On Thu, Aug 20, 2015 at 08:35:16PM +0900, Byungchul Park wrote:
> > > On Thu, Aug 20, 2015 at 08:22:00PM +0900, byungchul.park@lge.com wrote:
> > 
> > > > +	/*
> > > > +	 * If it's !queued, then only when the task is sleeping it has a
> > > > +	 * non-normalized vruntime, that is, when the task is being migrated
> > > > +	 * it has a normailized vruntime.
> > > > +	 */
> > > 
> > > i tried to change your XXX comment. i think it can be explaned like this.
> > > don't you think so? i want to hear any opinions about this.
> > > 
> > > > +	if (p->state == TASK_RUNNING)
> > > > +		return true;
> > 
> > 
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7943,11 +7943,10 @@ static inline bool vruntime_normalized(s
> >  		return true;
> >  
> >  	/*
> > -	 * If it's !queued, then only when the task is sleeping it has a
> > -	 * non-normalized vruntime, that is, when the task is being migrated
> > -	 * it has a normalized vruntime.
> > +	 * If it's !queued, sleeping tasks have a normalized vruntime,
> > +	 * see dequeue_entity().
> 
> check the condition "!(flags & DEQUEUE_SLEEP)" for doing normalizing in
> dequeue_entity(). i think you have to keep my original comment, or
> modify your comment to something like below.
> 
> before - If it's !queued, sleeping tasks have a normalized vruntime,
> after - If it's !queued, sleeping tasks have a non-normalize vruntime,
> 
> but.. i think it would be better that you keep my original comment..

i mean "it would be better that you leave my original comment unchanged".

> 
> >  	 */
> > -	if (p->state == TASK_RUNNING)
> > +	if (!p->se.on_rq)
> >  		return true;
> >  
> >  	return false;
> > 
> > 
> > Does that make sense?
> > 
> > I think using p->state for this is fragile, as we could be racy with any
> > random blocking primitive that does set_current_state() _before_
> > actually calling into the scheduler.
> > 
> > 
> > --
> > 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/
--
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]

#1216919 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Tue, Sep 01, 2015 at 09:28:49AM +0900, Byungchul Park wrote:

> check the condition "!(flags & DEQUEUE_SLEEP)" for doing normalizing in
> dequeue_entity(). i think you have to keep my original comment, or
> modify your comment to something like below.
> 
> before - If it's !queued, sleeping tasks have a normalized vruntime,
> after - If it's !queued, sleeping tasks have a non-normalize vruntime,
> 
> but.. i think it would be better that you keep my original comment..

The comment we can talk about later, but I think the condition:

> > -	if (p->state == TASK_RUNNING)
> > +	if (!p->se.on_rq)

is important now. Both are broken in different ways.

	p->state == TASK_RUNNING

is broken in this scenario:

	CPU0					CPU1

	set_current_state(TASK_UNINTERRUPTIBLE);

						sched_move_task()
						  task_move_group_fair()
						    vruntime_normalized() == true
	if (!cond)
		schedule();
	__set_current_state(TASK_RUNNING);


Now the proposed replacement:

	!p->se.on_rq

is equally broken, because (as you point out) clearing it isn't
conditional on DEQUEUE_SLEEP.


And the problem with tracking the vruntime state is that while it helps
detach_task_cfs_rq(), attach_task_cfs_rq() is still left wondering what
it should return to.

So we do indeed need something to determine, based on the current state,
if vruntime should be normalized.

/me ponders moar
--
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]

#1217289 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Tue, Sep 01, 2015 at 05:03:43PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 01, 2015 at 09:28:49AM +0900, Byungchul Park wrote:
> 
> > check the condition "!(flags & DEQUEUE_SLEEP)" for doing normalizing in
> > dequeue_entity(). i think you have to keep my original comment, or
> > modify your comment to something like below.
> > 
> > before - If it's !queued, sleeping tasks have a normalized vruntime,
> > after - If it's !queued, sleeping tasks have a non-normalize vruntime,
> > 
> > but.. i think it would be better that you keep my original comment..
> 
> The comment we can talk about later, but I think the condition:
> 
> > > -	if (p->state == TASK_RUNNING)
> > > +	if (!p->se.on_rq)
> 
> is important now. Both are broken in different ways.
> 
> 	p->state == TASK_RUNNING
> 
> is broken in this scenario:
> 
> 	CPU0					CPU1
> 
> 	set_current_state(TASK_UNINTERRUPTIBLE);

here, the target task has not been dequeued yet (the task will be 
dequeued within schedule()). so, queued is true when sched_move_task()
is called. sched_move_task()->dequeue_task(flag=0) will normalize as
we expect. that is anyway no problem. however, i also think the 
condition here can make us confused, too.

i think "p->state == TASK_RUNNING" condition can work anyway, which
already exists in original code. the condition is only for checking
if the task is being migrated or in sleep. how about this appoach
below to make code more readable?

> 
> 						sched_move_task()
> 						  task_move_group_fair()
> 						    vruntime_normalized() == true
> 	if (!cond)
> 		schedule();
> 	__set_current_state(TASK_RUNNING);
> 
> 
> Now the proposed replacement:
> 
> 	!p->se.on_rq
> 
> is equally broken, because (as you point out) clearing it isn't
> conditional on DEQUEUE_SLEEP.
> 

i think we must take a task's on_rq into account instead of se's on_rq,
because current code set se's on_rq to 0 even when migrating a task. but
task's on_rq would be set to TASK_ON_RQ_MIGRATING when migrating the
task. there it would be ok if we use task's on_rq to check if it has
a normailzed vruntime or not like below.

---

From 9545e34c2f6fd195f18d3111b5c1d1b03600cdcd Mon Sep 17 00:00:00 2001
From: Byungchul Park <byungchul.park@lge.com>
Date: Wed, 2 Sep 2015 10:59:58 +0900
Subject: [PATCH] sched: make vruntime_normalized() cleaner

in both TASK_ON_RQ_QUEUED case and TASK_ON_RQ_MIGRATING case, the
target task have a normalized vruntime by dequeue_entity(.flags=0).
so we don't need to check this separately with a different condition
statement.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 53d0e30..dfe8754 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7920,14 +7920,14 @@ prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
 
 static inline bool vruntime_normalized(struct task_struct *p)
 {
-	int queued = task_on_rq_queued(p);
 	struct sched_entity *se = &p->se;
 
 	/*
-	 * If it's queued, then the dequeue_entity(.flags=0) will already
-	 * have normalized the vruntime.
+	 * In both TASK_ON_RQ_QUEUED case and TASK_ON_RQ_MIGRATING case,
+	 * the dequeue_entity(.flags=0) will already have normalized the
+	 * vruntime.
 	 */
-	if (queued)
+	if (p->on_rq)
 		return true;
 
 	/*
@@ -7942,14 +7942,6 @@ static inline bool vruntime_normalized(struct task_struct *p)
 	if (!se->sum_exec_runtime || p->state == TASK_WAKING)
 		return true;
 
-	/*
-	 * If it's !queued, then only when the task is sleeping it has a
-	 * non-normalized vruntime, that is, when the task is being migrated
-	 * it has a normailized vruntime.
-	 */
-	if (p->state == TASK_RUNNING)
-		return true;
-
 	return false;
 }
 
-- 
1.7.9.5

> 
> And the problem with tracking the vruntime state is that while it helps
> detach_task_cfs_rq(), attach_task_cfs_rq() is still left wondering what
> it should return to.
> 
> So we do indeed need something to determine, based on the current state,
> if vruntime should be normalized.
> 
> /me ponders moar
> --
> 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]

#1217404 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

On Wed, Sep 02, 2015 at 11:33:15AM +0900, Byungchul Park wrote:
> +++ b/kernel/sched/fair.c
> @@ -7920,14 +7920,14 @@ prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
>  
>  static inline bool vruntime_normalized(struct task_struct *p)
>  {
>  	struct sched_entity *se = &p->se;
>  
>  	/*
> +	 * In both TASK_ON_RQ_QUEUED case and TASK_ON_RQ_MIGRATING case,
> +	 * the dequeue_entity(.flags=0) will already have normalized the
> +	 * vruntime.
>  	 */
> +	if (p->on_rq)
>  		return true;
>  

You're right. And yes this is nicer. Thanks!

I've folded it into your 5/5 patch.
--
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]

#1217419 — Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Sep 02, 2015 at 11:33:15AM +0900, Byungchul Park wrote:
> > +++ b/kernel/sched/fair.c
> > @@ -7920,14 +7920,14 @@ prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
> >  
> >  static inline bool vruntime_normalized(struct task_struct *p)
> >  {
> >  	struct sched_entity *se = &p->se;
> >  
> >  	/*
> > +	 * In both TASK_ON_RQ_QUEUED case and TASK_ON_RQ_MIGRATING case,

s/         In both the TASK_ON_RQ_QUEUED and TASK_ON_RQ_MIGRATING cases,

> > +	 * the dequeue_entity(.flags=0) will already have normalized the
> > +	 * vruntime.

Thanks,

	Ingo
--
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]

#1210428 — [PATCH v4 4/5] sched: sync a se with its cfs_rq when switching sched class to fair class

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

we have to sync a se with its cfs_rq, when switching sched class to
fair class. current code does not sync it because the se average load
won't be valid any more if it has been dettached for a long time. however
the se's average load would be valid just after being detached from
cfs_rq, so we need to sync it in that case, e.g. priority inheritance.

to solve the problem that a se average load becomes more useless over
time, this patch decays its average load even for the duration that
the se has been detached, when it gets attached to the cfs_rq.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d58e9aa..af6ad5f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2712,6 +2712,18 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
 
 static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	/*
+	 * In case of migration and cgroup-change, more care should be taken
+	 * because se's cfs_rq was changed, that means calling __update_load_avg
+	 * with new cfs_rq->avg.last_update_time is meaningless. So we skip the
+	 * update here. We have to update it with prev cfs_rq just before changing
+	 * se's cfs_rq, and get here soon.
+	 */
+	if (se->avg.last_update_time) {
+		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
+				  &se->avg, 0, 0, NULL);
+	}
+
 	se->avg.last_update_time = cfs_rq->avg.last_update_time;
 	cfs_rq->avg.load_avg += se->avg.load_avg;
 	cfs_rq->avg.load_sum += se->avg.load_sum;
@@ -7945,6 +7957,9 @@ static void switched_to_fair(struct rq *rq, struct task_struct *p)
 	se->depth = se->parent ? se->parent->depth + 1 : 0;
 #endif
 
+	/* Synchronize task with its cfs_rq */
+	attach_entity_load_avg(cfs_rq_of(&p->se), &p->se);
+
 	if (!task_on_rq_queued(p)) {
 
 		/*
@@ -8044,6 +8059,10 @@ static void task_move_group_fair(struct task_struct *p, int queued)
 	/* Synchronize task with its prev cfs_rq */
 	detach_entity_load_avg(cfs_rq, se);
 	set_task_rq(p, task_cpu(p));
+
+	/* Tell se's cfs_rq has been changed -- migrated */
+	p->se.avg.last_update_time = 0;
+
 	se->depth = se->parent ? se->parent->depth + 1 : 0;
 	cfs_rq = cfs_rq_of(se);
 	if (!queued)
-- 
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]

#1210430 — [PATCH v4 1/5] sched: add two functions adjusting cfs_rq's load when att(det)aching a se

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

this patch introduces two functions adjusting cfs_rq's average load when
att(det)aching a se to(from) the cfs_rq, and let them use these functions,
and cleans up some code.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |   77 +++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4d5f97b..a43023b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2664,8 +2664,8 @@ static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
 /* Group cfs_rq's load_avg is used for task_h_load and update_cfs_share */
 static inline int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
 {
-	int decayed;
 	struct sched_avg *sa = &cfs_rq->avg;
+	int decayed;
 
 	if (atomic_long_read(&cfs_rq->removed_load_avg)) {
 		long r = atomic_long_xchg(&cfs_rq->removed_load_avg, 0);
@@ -2695,33 +2695,52 @@ static inline int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
 static inline void update_load_avg(struct sched_entity *se, int update_tg)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
-	int cpu = cpu_of(rq_of(cfs_rq));
 	u64 now = cfs_rq_clock_task(cfs_rq);
+	int cpu = cpu_of(rq_of(cfs_rq));
 
 	/*
 	 * Track task load average for carrying it to new CPU after migrated, and
 	 * track group sched_entity load average for task_h_load calc in migration
 	 */
 	__update_load_avg(now, cpu, &se->avg,
-		se->on_rq * scale_load_down(se->load.weight), cfs_rq->curr == se, NULL);
+			  se->on_rq * scale_load_down(se->load.weight),
+			  cfs_rq->curr == se, NULL);
 
 	if (update_cfs_rq_load_avg(now, cfs_rq) && update_tg)
 		update_tg_load_avg(cfs_rq, 0);
 }
 
+static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+	se->avg.last_update_time = cfs_rq->avg.last_update_time;
+	cfs_rq->avg.load_avg += se->avg.load_avg;
+	cfs_rq->avg.load_sum += se->avg.load_sum;
+	cfs_rq->avg.util_avg += se->avg.util_avg;
+	cfs_rq->avg.util_sum += se->avg.util_sum;
+}
+
+static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+	__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
+			  &se->avg, se->on_rq * scale_load_down(se->load.weight),
+			  cfs_rq->curr == se, NULL);
+
+	cfs_rq->avg.load_avg = max_t(long, cfs_rq->avg.load_avg - se->avg.load_avg, 0);
+	cfs_rq->avg.load_sum = max_t(s64,  cfs_rq->avg.load_sum - se->avg.load_sum, 0);
+	cfs_rq->avg.util_avg = max_t(long, cfs_rq->avg.util_avg - se->avg.util_avg, 0);
+	cfs_rq->avg.util_sum = max_t(s32,  cfs_rq->avg.util_sum - se->avg.util_sum, 0);
+}
+
 /* Add the load generated by se into cfs_rq's load average */
 static inline void
 enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
 	struct sched_avg *sa = &se->avg;
 	u64 now = cfs_rq_clock_task(cfs_rq);
-	int migrated = 0, decayed;
+	int migrated, decayed;
 
-	if (sa->last_update_time == 0) {
-		sa->last_update_time = now;
-		migrated = 1;
-	}
-	else {
+	migrated = !sa->last_update_time;
+	if (!migrated) {
 		__update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
 			se->on_rq * scale_load_down(se->load.weight),
 			cfs_rq->curr == se, NULL);
@@ -2732,12 +2751,8 @@ enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 	cfs_rq->runnable_load_avg += sa->load_avg;
 	cfs_rq->runnable_load_sum += sa->load_sum;
 
-	if (migrated) {
-		cfs_rq->avg.load_avg += sa->load_avg;
-		cfs_rq->avg.load_sum += sa->load_sum;
-		cfs_rq->avg.util_avg += sa->util_avg;
-		cfs_rq->avg.util_sum += sa->util_sum;
-	}
+	if (migrated)
+		attach_entity_load_avg(cfs_rq, se);
 
 	if (decayed || migrated)
 		update_tg_load_avg(cfs_rq, 0);
@@ -2752,7 +2767,7 @@ dequeue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 	cfs_rq->runnable_load_avg =
 		max_t(long, cfs_rq->runnable_load_avg - se->avg.load_avg, 0);
 	cfs_rq->runnable_load_sum =
-		max_t(s64, cfs_rq->runnable_load_sum - se->avg.load_sum, 0);
+		max_t(s64,  cfs_rq->runnable_load_sum - se->avg.load_sum, 0);
 }
 
 /*
@@ -2820,6 +2835,11 @@ static inline void
 dequeue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) {}
 static inline void remove_entity_load_avg(struct sched_entity *se) {}
 
+static inline void
+attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) {}
+static inline void
+detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) {}
+
 static inline int idle_balance(struct rq *rq)
 {
 	return 0;
@@ -7909,25 +7929,10 @@ static void switched_from_fair(struct rq *rq, struct task_struct *p)
 		se->vruntime -= cfs_rq->min_vruntime;
 	}
 
-#ifdef CONFIG_SMP
 	/* Catch up with the cfs_rq and remove our load when we leave */
-	__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq), &se->avg,
-		se->on_rq * scale_load_down(se->load.weight), cfs_rq->curr == se, NULL);
-
-	cfs_rq->avg.load_avg =
-		max_t(long, cfs_rq->avg.load_avg - se->avg.load_avg, 0);
-	cfs_rq->avg.load_sum =
-		max_t(s64, cfs_rq->avg.load_sum - se->avg.load_sum, 0);
-	cfs_rq->avg.util_avg =
-		max_t(long, cfs_rq->avg.util_avg - se->avg.util_avg, 0);
-	cfs_rq->avg.util_sum =
-		max_t(s32, cfs_rq->avg.util_sum - se->avg.util_sum, 0);
-#endif
+	detach_entity_load_avg(cfs_rq, se);
 }
 
-/*
- * We switched to the sched_fair class.
- */
 static void switched_to_fair(struct rq *rq, struct task_struct *p)
 {
 	struct sched_entity *se = &p->se;
@@ -8040,14 +8045,8 @@ static void task_move_group_fair(struct task_struct *p, int queued)
 		cfs_rq = cfs_rq_of(se);
 		se->vruntime += cfs_rq->min_vruntime;
 
-#ifdef CONFIG_SMP
 		/* Virtually synchronize task with its new cfs_rq */
-		p->se.avg.last_update_time = cfs_rq->avg.last_update_time;
-		cfs_rq->avg.load_avg += p->se.avg.load_avg;
-		cfs_rq->avg.load_sum += p->se.avg.load_sum;
-		cfs_rq->avg.util_avg += p->se.avg.util_avg;
-		cfs_rq->avg.util_sum += p->se.avg.util_sum;
-#endif
+		attach_entity_load_avg(cfs_rq, se);
 	}
 }
 
-- 
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]

#1211422

On Thu, Aug 20, 2015 at 08:21:55PM +0900, byungchul.park@lge.com wrote:
> From: Byungchul Park <byungchul.park@lge.com>

i am not sure if i did it here, in v4, as peter suggested fully. i did
not fully undertand what he commented on v3. i am waiting for any
feedback e.g. answering to my question, just reviewing now and so on.

> 
> there are 3 problems when att(det)aching a se to(from) its cfs_rq.
> 
> problem 1. se's average load is not accounted with new cfs_rq in queued case,
> when a task changes its cgroup.
> 
> problem 2. cfs_rq->avg.load_avg becomes larger and larger whenever changing
> cgroup to another. we have to sync se's average load with prev cfs_rq.
> 
> problem 3. current code does not sync it with its cfs_rq when switching
> sched class to fair class. if we can always assume that a se has been
> detached from fair class for a long time enough for its average load to
> become useless, at the time attaching it to its fair class cfs_rq, then
> current code is acceptable. but the assumption is not always true.
> 
> patch 1/5, does code refactoring for further patches.
> patch 2/5, solves the problem 1.
> patch 3/5, solves the problem 2.
> patch 4/5, solves the problem 3.
> patch 5/5, does code refactoring for better readability.
> 
> change from v3 to v4
> * clean up code as Peter suggests
> * use upper case letter in comment properly
> 
> change from v2 to v3 (logic is same as v2)
> * fix up my poor english in commit message and comment
> * break down big patches into more patches for being reviewed easily
> * supplement cover letter messages
> 
> change from v1 to v2
> * introduce two functions for adjusting vruntime and load when attaching
>   and detaching.
> * call the introduced functions instead of switched_from(to)_fair() directly
>   in task_move_group_fair().
> * add decaying logic for a se which has detached from a cfs_rq.
> 
> thanks,
> byungchul
> 
> Byungchul Park (5):
>   sched: add two functions adjusting cfs_rq's load when att(det)aching
>     a se
>   sched: make task_move_group_fair adjust cfs_rq's load in case of
>     queued
>   sched: sync a se with prev cfs_rq when changing cgroup
>   sched: sync a se with its cfs_rq when switching sched class to fair
>     class
>   sched: add two functions for att(det)aching a task to(from) a cfs_rq
> 
>  kernel/sched/fair.c |  222 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 113 insertions(+), 109 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/
--
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]

#1213445

On Thu, Aug 20, 2015 at 08:21:55PM +0900, byungchul.park@lge.com wrote:
> From: Byungchul Park <byungchul.park@lge.com>
> 

i am sorry for making a push ; but is there anything for me to improve
for this patch set?

and i forgot to add scheduler related persons to CC and do it.

> there are 3 problems when att(det)aching a se to(from) its cfs_rq.
> 
> problem 1. se's average load is not accounted with new cfs_rq in queued case,
> when a task changes its cgroup.
> 
> problem 2. cfs_rq->avg.load_avg becomes larger and larger whenever changing
> cgroup to another. we have to sync se's average load with prev cfs_rq.
> 
> problem 3. current code does not sync it with its cfs_rq when switching
> sched class to fair class. if we can always assume that a se has been
> detached from fair class for a long time enough for its average load to
> become useless, at the time attaching it to its fair class cfs_rq, then
> current code is acceptable. but the assumption is not always true.
> 
> patch 1/5, does code refactoring for further patches.
> patch 2/5, solves the problem 1.
> patch 3/5, solves the problem 2.
> patch 4/5, solves the problem 3.
> patch 5/5, does code refactoring for better readability.
> 
> change from v3 to v4
> * clean up code as Peter suggests
> * use upper case letter in comment properly
> 
> change from v2 to v3 (logic is same as v2)
> * fix up my poor english in commit message and comment
> * break down big patches into more patches for being reviewed easily
> * supplement cover letter messages
> 
> change from v1 to v2
> * introduce two functions for adjusting vruntime and load when attaching
>   and detaching.
> * call the introduced functions instead of switched_from(to)_fair() directly
>   in task_move_group_fair().
> * add decaying logic for a se which has detached from a cfs_rq.
> 
> thanks,
> byungchul
> 
> Byungchul Park (5):
>   sched: add two functions adjusting cfs_rq's load when att(det)aching
>     a se
>   sched: make task_move_group_fair adjust cfs_rq's load in case of
>     queued
>   sched: sync a se with prev cfs_rq when changing cgroup
>   sched: sync a se with its cfs_rq when switching sched class to fair
>     class
>   sched: add two functions for att(det)aching a task to(from) a cfs_rq
> 
>  kernel/sched/fair.c |  222 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 113 insertions(+), 109 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/
--
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