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

Groups > linux.kernel > #1414727 > unrolled thread

[PATCH v4 0/5] sched/fair: Fix attach and detach sched avgs for task group change or sched class change

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/5] sched/fair: Fix attach and detach sched avgs for task group change or sched class change Yuyang Du <yuyang.du@intel.com> - 2016-06-06 10:20 +0200
    [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group Yuyang Du <yuyang.du@intel.com> - 2016-06-06 10:20 +0200
      Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Peter Zijlstra <peterz@infradead.org> - 2016-06-06 12:00 +0200
        Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Yuyang Du <yuyang.du@intel.com> - 2016-06-07 05:40 +0200
      Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-06 14:40 +0200
        Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Yuyang Du <yuyang.du@intel.com> - 2016-06-07 05:10 +0200
          Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-07 10:10 +0200
            Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice  when switching to fair or changing task group Yuyang Du <yuyang.du@intel.com> - 2016-06-08 04:20 +0200
    [PATCH v4 5/5] sched/fair: Add inline to detach_entity_load_evg() Yuyang Du <yuyang.du@intel.com> - 2016-06-06 10:20 +0200
    [PATCH v4 4/5] sched/fair: Move load and util avgs from wake_up_new_task() to sched_fork() Yuyang Du <yuyang.du@intel.com> - 2016-06-06 10:20 +0200
    [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg() Yuyang Du <yuyang.du@intel.com> - 2016-06-06 10:20 +0200
      Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg() Matt Fleming <matt@codeblueprint.co.uk> - 2016-06-06 15:40 +0200
        Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg() Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-06 15:50 +0200
          Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg() Matt Fleming <matt@codeblueprint.co.uk> - 2016-06-06 16:10 +0200
          Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg() Peter Zijlstra <peterz@infradead.org> - 2016-06-06 16:30 +0200

#1414727 — [PATCH v4 0/5] sched/fair: Fix attach and detach sched avgs for task group change or sched class change

Hi Peter,

It seems the problem is worse, the attach twice issue can happen not only as
Vincent raised when task moves between groups, but also when switching to fair
class. In addition, for newly forked task, the detach and attach also have
problems.

Thanks a lot to Vincent. This new version mainly addresses his comments to
reword the description of the problem.

Thanks,
Yuyang

--

Yuyang Du (5):
  sched/fair: Clean up attach_entity_load_avg()
  sched/fair: Fix attaching task sched avgs twice when switching to
    fair or changing task group
  sched/fair: Skip detach sched avgs for new task when changing task
    groups
  sched/fair: Move load and util avgs from wake_up_new_task() to
    sched_fork()
  sched/fair: Add inline to detach_entity_load_evg()

 kernel/sched/auto_group.c |    2 +-
 kernel/sched/core.c       |   13 ++++----
 kernel/sched/fair.c       |   76 ++++++++++++++++++++-------------------------
 kernel/sched/sched.h      |    6 ++--
 4 files changed, 45 insertions(+), 52 deletions(-)

-- 
1.7.9.5

[toc] | [next] | [standalone]

#1414729 — [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

Vincent reported that the first task to a new task group's cfs_rq will
be attached in attach_task_cfs_rq() and once more when it is enqueued
(see https://lkml.org/lkml/2016/5/25/388).

Actually, it is worse. The sched avgs can be sometimes attached twice
not only when we change task groups but also when we switch to fair class,
these two scenarios will descripbe in the following respectively.

1) Switch to fair class:

The sched class change is done like this:

	if (queued)
	  enqueue_task();
	check_class_changed()
	  switched_from()
	  switched_to()

If the task is on_rq, it should have already been enqueued, which
MAY have attached sched avgs to the cfs_rq, if so, we shouldn't attach
it again in switched_to(), otherwise, we attach it twice.

To address both the on_rq and !on_rq cases, as well as both the task
was switched from fair and otherwise, the simplest solution is to reset
the task's last_update_time to 0, when the task is switched from fair.
Then let task enqueue do the sched avgs attachment only once.

2) Change between fair task groups:

The task groups are changed like this:

	if (queued)
          dequeue_task()
	task_move_group()
	if (queued)
          enqueue_task()

Unlike the switch to fair class case, if the task is on_rq, it will be
enqueued after we move task groups, so the simplest solution is to reset
the task's last_update_time when we do task_move_group(), but not to
attach sched avgs in task_move_group(), and then let enqueue_task() do
the sched avgs attachment.

Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c |   47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 34e658b..28b3415 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2933,7 +2933,8 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
 		update_tg_load_avg(cfs_rq, 0);
 }
 
-static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
+/* Virtually synchronize task with its cfs_rq */
+static inline 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;
@@ -2944,19 +2945,6 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
 	cfs_rq_util_change(cfs_rq);
 }
 
-static inline void attach_age_load_task(struct rq *rq, struct task_struct *p)
-{
-	struct sched_entity *se = &p->se;
-
-	if (!sched_feat(ATTACH_AGE_LOAD))
-		return;
-
-	if (se->avg.last_update_time) {
-		__update_load_avg(cfs_rq_of(se)->avg.last_update_time, cpu_of(rq),
-				  &se->avg, 0, 0, NULL);
-	}
-}
-
 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)),
@@ -3031,6 +3019,11 @@ static inline u64 cfs_rq_last_update_time(struct cfs_rq *cfs_rq)
 }
 #endif
 
+static inline void reset_task_last_update_time(struct task_struct *p)
+{
+	p->se.avg.last_update_time = 0;
+}
+
 /*
  * Task first catches up with cfs_rq, and then subtract
  * itself from the cfs_rq (task must be off the queue now).
@@ -3083,10 +3076,8 @@ 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 void attach_age_load_task(struct rq *rq, struct task_struct *p) {}
+static inline void reset_task_last_update_time(struct task_struct *p) {}
 
 static inline int idle_balance(struct rq *rq)
 {
@@ -8372,9 +8363,6 @@ static void attach_task_cfs_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, se);
-
 	if (!vruntime_normalized(p))
 		se->vruntime += cfs_rq->min_vruntime;
 }
@@ -8382,16 +8370,18 @@ static void attach_task_cfs_rq(struct task_struct *p)
 static void switched_from_fair(struct rq *rq, struct task_struct *p)
 {
 	detach_task_cfs_rq(p);
+	reset_task_last_update_time(p);
+	/*
+	 * If we change back to fair class, we will attach the sched
+	 * avgs when we are enqueued, which will be done only once. We
+	 * won't have the chance to consistently age the avgs before
+	 * attaching them, so we have to continue with the last updated
+	 * sched avgs when we were detached.
+	 */
 }
 
 static void switched_to_fair(struct rq *rq, struct task_struct *p)
 {
-	/*
-	 * If we change between classes, age the averages before attaching them.
-	 * XXX: we could have just aged the entire load away if we've been
-	 * absent from the fair class for too long.
-	 */
-	attach_age_load_task(rq, p);
 	attach_task_cfs_rq(p);
 
 	if (task_on_rq_queued(p)) {
@@ -8444,6 +8434,11 @@ static void task_move_group_fair(struct task_struct *p)
 	detach_task_cfs_rq(p);
 	set_task_rq(p, task_cpu(p));
 	attach_task_cfs_rq(p);
+	/*
+	 * This assures we will attach the sched avgs when we are enqueued,
+	 * which will be done only once.
+	 */
+	reset_task_last_update_time(p);
 }
 
 void free_fair_sched_group(struct task_group *tg)
-- 
1.7.9.5

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

#1414853 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

On Mon, Jun 06, 2016 at 08:20:38AM +0800, Yuyang Du wrote:
> Vincent reported that the first task to a new task group's cfs_rq will
> be attached in attach_task_cfs_rq() and once more when it is enqueued
> (see https://lkml.org/lkml/2016/5/25/388).
> 
> Actually, it is worse. The sched avgs can be sometimes attached twice
> not only when we change task groups but also when we switch to fair class,
> these two scenarios will descripbe in the following respectively.
> 
> 1) Switch to fair class:
> 
> The sched class change is done like this:
> 
> 	if (queued)
> 	  enqueue_task();
> 	check_class_changed()
> 	  switched_from()
> 	  switched_to()
> 
> If the task is on_rq, it should have already been enqueued, which
> MAY have attached sched avgs to the cfs_rq, if so, we shouldn't attach
> it again in switched_to(), otherwise, we attach it twice.
> 
> To address both the on_rq and !on_rq cases, as well as both the task
> was switched from fair and otherwise, the simplest solution is to reset
> the task's last_update_time to 0, when the task is switched from fair.
> Then let task enqueue do the sched avgs attachment only once.
> 
> 2) Change between fair task groups:
> 
> The task groups are changed like this:
> 
> 	if (queued)
>           dequeue_task()
> 	task_move_group()
> 	if (queued)
>           enqueue_task()
> 
> Unlike the switch to fair class case, if the task is on_rq, it will be
> enqueued after we move task groups, so the simplest solution is to reset
> the task's last_update_time when we do task_move_group(), but not to
> attach sched avgs in task_move_group(), and then let enqueue_task() do
> the sched avgs attachment.

So this patch completely removes the detach->attach aging you moved
around in the previous patch -- leading me to wonder what the purpose of
the previous patch was.

Also, this Changelog completely fails to mention this fact, nor does it
explain why this is 'right'.

> +/* Virtually synchronize task with its cfs_rq */

I don't feel this comment actually enlightens the function much.

> @@ -8372,9 +8363,6 @@ static void attach_task_cfs_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, se);
> -
>  	if (!vruntime_normalized(p))
>  		se->vruntime += cfs_rq->min_vruntime;
>  }

You leave attach/detach asymmetric and not a comment in sight explaining
why.

> @@ -8382,16 +8370,18 @@ static void attach_task_cfs_rq(struct task_struct *p)
>  static void switched_from_fair(struct rq *rq, struct task_struct *p)
>  {
>  	detach_task_cfs_rq(p);
> +	reset_task_last_update_time(p);
> +	/*
> +	 * If we change back to fair class, we will attach the sched
> +	 * avgs when we are enqueued, which will be done only once. We
> +	 * won't have the chance to consistently age the avgs before
> +	 * attaching them, so we have to continue with the last updated
> +	 * sched avgs when we were detached.
> +	 */

This comment needs improvement; it confuses.

> @@ -8444,6 +8434,11 @@ static void task_move_group_fair(struct task_struct *p)
>  	detach_task_cfs_rq(p);
>  	set_task_rq(p, task_cpu(p));
>  	attach_task_cfs_rq(p);
> +	/*
> +	 * This assures we will attach the sched avgs when we are enqueued,

"ensures" ? Also, more confusion.

> +	 * which will be done only once.
> +	 */
> +	reset_task_last_update_time(p);
>  }

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

#1415660 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

On Mon, Jun 06, 2016 at 11:54:50AM +0200, Peter Zijlstra wrote:
> > The task groups are changed like this:
> > 
> > 	if (queued)
> >           dequeue_task()
> > 	task_move_group()
> > 	if (queued)
> >           enqueue_task()
> > 
> > Unlike the switch to fair class case, if the task is on_rq, it will be
> > enqueued after we move task groups, so the simplest solution is to reset
> > the task's last_update_time when we do task_move_group(), but not to
> > attach sched avgs in task_move_group(), and then let enqueue_task() do
> > the sched avgs attachment.
> 
> So this patch completely removes the detach->attach aging you moved
> around in the previous patch -- leading me to wonder what the purpose of
> the previous patch was.
 
Basically, they address different issues, and should not be conflated.

> Also, this Changelog completely fails to mention this fact, nor does it
> explain why this is 'right'.
 
I should have explained this in the changelog. It is "right", because when a
task switches to fair, it is most likely "we could have just aged the entire
load away" as the XXX comment said. But despite that, basically we have a lost
record time, aging or not aging by that time, it doesn't occur to me one is
definitely better than the other. I will make a comment in the code explaining
this. You think?

> > +/* Virtually synchronize task with its cfs_rq */
> 
> I don't feel this comment actually enlightens the function much.
 
Synchronize without update, so virtually, :)

> > @@ -8372,9 +8363,6 @@ static void attach_task_cfs_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, se);
> > -
> >  	if (!vruntime_normalized(p))
> >  		se->vruntime += cfs_rq->min_vruntime;
> >  }
> 
> You leave attach/detach asymmetric and not a comment in sight explaining
> why.
 
So it is asymmetric because we uniformly attach in enqueue. I will explain.
This also relates to the following code comments and your comments.

> > @@ -8382,16 +8370,18 @@ static void attach_task_cfs_rq(struct task_struct *p)
> >  static void switched_from_fair(struct rq *rq, struct task_struct *p)
> >  {
> >  	detach_task_cfs_rq(p);
> > +	reset_task_last_update_time(p);
> > +	/*
> > +	 * If we change back to fair class, we will attach the sched
> > +	 * avgs when we are enqueued, which will be done only once. We
> > +	 * won't have the chance to consistently age the avgs before
> > +	 * attaching them, so we have to continue with the last updated
> > +	 * sched avgs when we were detached.
> > +	 */
> 
> This comment needs improvement; it confuses.
> 
> > @@ -8444,6 +8434,11 @@ static void task_move_group_fair(struct task_struct *p)
> >  	detach_task_cfs_rq(p);
> >  	set_task_rq(p, task_cpu(p));
> >  	attach_task_cfs_rq(p);
> > +	/*
> > +	 * This assures we will attach the sched avgs when we are enqueued,
> 
> "ensures" ? Also, more confusion.
> 
> > +	 * which will be done only once.
> > +	 */
> > +	reset_task_last_update_time(p);
> >  }
> 

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

#1414992 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

Hi Yuyang,

On 6 June 2016 at 02:20, Yuyang Du <yuyang.du@intel.com> wrote:
> Vincent reported that the first task to a new task group's cfs_rq will
> be attached in attach_task_cfs_rq() and once more when it is enqueued
> (see https://lkml.org/lkml/2016/5/25/388).
>
> Actually, it is worse. The sched avgs can be sometimes attached twice
> not only when we change task groups but also when we switch to fair class,
> these two scenarios will descripbe in the following respectively.
>
> 1) Switch to fair class:
>
> The sched class change is done like this:
>
>         if (queued)
>           enqueue_task();
>         check_class_changed()
>           switched_from()
>           switched_to()
>
> If the task is on_rq, it should have already been enqueued, which
> MAY have attached sched avgs to the cfs_rq, if so, we shouldn't attach
> it again in switched_to(), otherwise, we attach it twice.
>
> To address both the on_rq and !on_rq cases, as well as both the task
> was switched from fair and otherwise, the simplest solution is to reset
> the task's last_update_time to 0, when the task is switched from fair.
> Then let task enqueue do the sched avgs attachment only once.
>
> 2) Change between fair task groups:
>
> The task groups are changed like this:
>
>         if (queued)
>           dequeue_task()
>         task_move_group()
>         if (queued)
>           enqueue_task()
>
> Unlike the switch to fair class case, if the task is on_rq, it will be
> enqueued after we move task groups, so the simplest solution is to reset
> the task's last_update_time when we do task_move_group(), but not to
> attach sched avgs in task_move_group(), and then let enqueue_task() do
> the sched avgs attachment.

According to the review of the previous version
http://www.gossamer-threads.com/lists/linux/kernel/2450678#2450678,
only the use case switch to fair with a task that has never been
queued as a CFS task before, can have the issue and this will not
happen for other use cases described above.
So can you limit the description in the commit message to just this
use case unless you have discovered new use cases and in this case
please add the description.

Then, the problem with this use case, comes that last_update_time == 0
has a special meaning ( task has migrated ) and we initialize
last_update_time with this value. A much more simple solution would be
to prevent last_update_time to be initialized with this special value.
We can initialize the last_update_time of a sched_entity to 1 as an
example which is easier than these changes. Furthermore,  this patch
makes a task to be never decayed for the time that elapsed between its
detach and its next enqueue on a cfs_rq which can be quite long
duration and will make the task been far higher than it should.

>
> Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> ---
>  kernel/sched/fair.c |   47 +++++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 34e658b..28b3415 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2933,7 +2933,8 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
>                 update_tg_load_avg(cfs_rq, 0);
>  }
>
> -static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +/* Virtually synchronize task with its cfs_rq */
> +static inline 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;
> @@ -2944,19 +2945,6 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
>         cfs_rq_util_change(cfs_rq);
>  }
>
> -static inline void attach_age_load_task(struct rq *rq, struct task_struct *p)
> -{
> -       struct sched_entity *se = &p->se;
> -
> -       if (!sched_feat(ATTACH_AGE_LOAD))
> -               return;
> -
> -       if (se->avg.last_update_time) {
> -               __update_load_avg(cfs_rq_of(se)->avg.last_update_time, cpu_of(rq),
> -                                 &se->avg, 0, 0, NULL);
> -       }
> -}
> -
>  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)),
> @@ -3031,6 +3019,11 @@ static inline u64 cfs_rq_last_update_time(struct cfs_rq *cfs_rq)
>  }
>  #endif
>
> +static inline void reset_task_last_update_time(struct task_struct *p)
> +{
> +       p->se.avg.last_update_time = 0;
> +}
> +
>  /*
>   * Task first catches up with cfs_rq, and then subtract
>   * itself from the cfs_rq (task must be off the queue now).
> @@ -3083,10 +3076,8 @@ 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 void attach_age_load_task(struct rq *rq, struct task_struct *p) {}
> +static inline void reset_task_last_update_time(struct task_struct *p) {}
>
>  static inline int idle_balance(struct rq *rq)
>  {
> @@ -8372,9 +8363,6 @@ static void attach_task_cfs_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, se);
> -
>         if (!vruntime_normalized(p))
>                 se->vruntime += cfs_rq->min_vruntime;
>  }
> @@ -8382,16 +8370,18 @@ static void attach_task_cfs_rq(struct task_struct *p)
>  static void switched_from_fair(struct rq *rq, struct task_struct *p)
>  {
>         detach_task_cfs_rq(p);
> +       reset_task_last_update_time(p);
> +       /*
> +        * If we change back to fair class, we will attach the sched
> +        * avgs when we are enqueued, which will be done only once. We
> +        * won't have the chance to consistently age the avgs before
> +        * attaching them, so we have to continue with the last updated
> +        * sched avgs when we were detached.
> +        */
>  }
>
>  static void switched_to_fair(struct rq *rq, struct task_struct *p)
>  {
> -       /*
> -        * If we change between classes, age the averages before attaching them.
> -        * XXX: we could have just aged the entire load away if we've been
> -        * absent from the fair class for too long.
> -        */
> -       attach_age_load_task(rq, p);
>         attach_task_cfs_rq(p);
>
>         if (task_on_rq_queued(p)) {
> @@ -8444,6 +8434,11 @@ static void task_move_group_fair(struct task_struct *p)
>         detach_task_cfs_rq(p);
>         set_task_rq(p, task_cpu(p));
>         attach_task_cfs_rq(p);
> +       /*
> +        * This assures we will attach the sched avgs when we are enqueued,
> +        * which will be done only once.
> +        */
> +       reset_task_last_update_time(p);
>  }
>
>  void free_fair_sched_group(struct task_group *tg)
> --
> 1.7.9.5
>

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

#1415651 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

Hi Vincent,

On Mon, Jun 06, 2016 at 02:32:39PM +0200, Vincent Guittot wrote:
> > Unlike the switch to fair class case, if the task is on_rq, it will be
> > enqueued after we move task groups, so the simplest solution is to reset
> > the task's last_update_time when we do task_move_group(), but not to
> > attach sched avgs in task_move_group(), and then let enqueue_task() do
> > the sched avgs attachment.
> 
> According to the review of the previous version
> http://www.gossamer-threads.com/lists/linux/kernel/2450678#2450678,
> only the use case switch to fair with a task that has never been
> queued as a CFS task before, can have the issue and this will not
> happen for other use cases described above.
> So can you limit the description in the commit message to just this
> use case unless you have discovered new use cases and in this case
> please add the description.

I assure you that I will, :)
 
> Then, the problem with this use case, comes that last_update_time == 0
> has a special meaning ( task has migrated ) and we initialize
> last_update_time with this value.

In general, the meaning of last_update_time == 0 is: not yet attached to
any cfs queue.

> A much more simple solution would be
> to prevent last_update_time to be initialized with this special value.
> We can initialize the last_update_time of a sched_entity to 1 as an
> example which is easier than these changes.

Then at least, we must differentiate CONFIG_FAIR_GROUP_SCHED and
!CONFIG_FAIR_GROUP_SCHED. And I am not sure whether this is a simpler
solution, as I haven't sorted out the whole thing. IMO, this solution
seems a little too hacky and I'd prefer not if we have alternative.

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

#1415822 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

On 6 June 2016 at 21:05, Yuyang Du <yuyang.du@intel.com> wrote:
> Hi Vincent,
>
> On Mon, Jun 06, 2016 at 02:32:39PM +0200, Vincent Guittot wrote:
>> > Unlike the switch to fair class case, if the task is on_rq, it will be
>> > enqueued after we move task groups, so the simplest solution is to reset
>> > the task's last_update_time when we do task_move_group(), but not to
>> > attach sched avgs in task_move_group(), and then let enqueue_task() do
>> > the sched avgs attachment.
>>
>> According to the review of the previous version
>> http://www.gossamer-threads.com/lists/linux/kernel/2450678#2450678,
>> only the use case switch to fair with a task that has never been
>> queued as a CFS task before, can have the issue and this will not
>> happen for other use cases described above.
>> So can you limit the description in the commit message to just this
>> use case unless you have discovered new use cases and in this case
>> please add the description.
>
> I assure you that I will, :)
>
>> Then, the problem with this use case, comes that last_update_time == 0
>> has a special meaning ( task has migrated ) and we initialize
>> last_update_time with this value.
>
> In general, the meaning of last_update_time == 0 is: not yet attached to
> any cfs queue.
>
>> A much more simple solution would be
>> to prevent last_update_time to be initialized with this special value.
>> We can initialize the last_update_time of a sched_entity to 1 as an
>> example which is easier than these changes.
>
> Then at least, we must differentiate CONFIG_FAIR_GROUP_SCHED and
> !CONFIG_FAIR_GROUP_SCHED. And I am not sure whether this is a simpler

Why do you want to differentiate ? we already have
sa->last_update_time = 0; in init_entity_runnable_average. We just
have to change it by sa->last_update_time = 1;

> solution, as I haven't sorted out the whole thing. IMO, this solution
> seems a little too hacky and I'd prefer not if we have alternative.

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

#1416829 — Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

On Tue, Jun 07, 2016 at 10:09:14AM +0200, Vincent Guittot wrote:
> >> A much more simple solution would be
> >> to prevent last_update_time to be initialized with this special value.
> >> We can initialize the last_update_time of a sched_entity to 1 as an
> >> example which is easier than these changes.
> >
> > Then at least, we must differentiate CONFIG_FAIR_GROUP_SCHED and
> > !CONFIG_FAIR_GROUP_SCHED. And I am not sure whether this is a simpler
> 
> Why do you want to differentiate ? we already have
> sa->last_update_time = 0; in init_entity_runnable_average. We just
> have to change it by sa->last_update_time = 1;

If !CONFIG_FAIR_GROUP_SCHED, we don't have task_move_group_fair().
If last_update_time != 0, we won't attach it for new task. Correct?

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

#1414731 — [PATCH v4 5/5] sched/fair: Add inline to detach_entity_load_evg()

detach_entity_load_evg() is only called by detach_task_cfs_rq(), so
explicitly add inline attribute to it.

Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a5a7149..1b6adfc 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2941,7 +2941,8 @@ static inline void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_en
 	cfs_rq_util_change(cfs_rq);
 }
 
-static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
+/* Catch up with the cfs_rq and remove our load when we leave */
+static inline 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),
@@ -8342,7 +8343,6 @@ static void detach_task_cfs_rq(struct task_struct *p)
 		se->vruntime -= cfs_rq->min_vruntime;
 	}
 
-	/* Catch up with the cfs_rq and remove our load when we leave */
 	detach_entity_load_avg(cfs_rq, se);
 }
 
-- 
1.7.9.5

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

#1414732 — [PATCH v4 4/5] sched/fair: Move load and util avgs from wake_up_new_task() to sched_fork()

Move new task initialization to sched_fork(). For initial non-fair class
task, the first switched_to_fair() will do the attach correctly.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/core.c  |    5 +++--
 kernel/sched/fair.c  |   14 +++++---------
 kernel/sched/sched.h |    2 +-
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ae5b8a8..77a8a2b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2370,6 +2370,9 @@ int sched_fork(unsigned long clone_flags, struct task_struct *p)
 	if (p->sched_class->task_fork)
 		p->sched_class->task_fork(p);
 
+	/* Initialize new task's sched averages */
+	init_entity_sched_avg(&p->se);
+
 	/*
 	 * The child is not yet in the pid-hash so no cgroup attach races,
 	 * and the cgroup is pinned to this child due to cgroup_fork()
@@ -2510,8 +2513,6 @@ void wake_up_new_task(struct task_struct *p)
 	struct rq_flags rf;
 	struct rq *rq;
 
-	/* Initialize new task's runnable average */
-	init_entity_runnable_average(&p->se);
 	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
 #ifdef CONFIG_SMP
 	/*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 370e284..a5a7149 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -668,8 +668,8 @@ static unsigned long task_h_load(struct task_struct *p);
 #define LOAD_AVG_MAX 47742 /* maximum possible load avg */
 #define LOAD_AVG_MAX_N 345 /* number of full periods to produce LOAD_AVG_MAX */
 
-/* Give new sched_entity start runnable values to heavy its load in infant time */
-void init_entity_runnable_average(struct sched_entity *se)
+/* Give new sched_entity start load values to heavy its load in infant time */
+void init_entity_sched_avg(struct sched_entity *se)
 {
 	struct sched_avg *sa = &se->avg;
 
@@ -738,12 +738,8 @@ void post_init_entity_util_avg(struct sched_entity *se)
 static inline unsigned long cfs_rq_runnable_load_avg(struct cfs_rq *cfs_rq);
 static inline unsigned long cfs_rq_load_avg(struct cfs_rq *cfs_rq);
 #else
-void init_entity_runnable_average(struct sched_entity *se)
-{
-}
-void post_init_entity_util_avg(struct sched_entity *se)
-{
-}
+void init_entity_sched_avg(struct sched_entity *se) { }
+void post_init_entity_util_avg(struct sched_entity *se) { }
 #endif
 
 /*
@@ -8492,7 +8488,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
 
 		init_cfs_rq(cfs_rq);
 		init_tg_cfs_entry(tg, cfs_rq, se, i, parent->se[i]);
-		init_entity_runnable_average(se);
+		init_entity_sched_avg(se);
 		post_init_entity_util_avg(se);
 	}
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c139ec4..bc9c99e 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1321,7 +1321,7 @@ extern void init_dl_task_timer(struct sched_dl_entity *dl_se);
 
 unsigned long to_ratio(u64 period, u64 runtime);
 
-extern void init_entity_runnable_average(struct sched_entity *se);
+extern void init_entity_sched_avg(struct sched_entity *se);
 extern void post_init_entity_util_avg(struct sched_entity *se);
 
 #ifdef CONFIG_NO_HZ_FULL
-- 
1.7.9.5

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

#1414736 — [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg()

attach_entity_load_avg() is called (indirectly) from:

 - switched_to_fair(): switch between classes to fair
 - task_move_group_fair(): move between task groups
 - enqueue_entity_load_avg(): enqueue entity

Only in switched_to_fair() is it possible that the task's last_update_time
is not 0 and therefore the task needs sched avgs update, so move the task
sched avgs update to switched_to_fair() only. In addition, the code is
refactored and code comments are updated.

No functionality change.

Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c |   43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c6dd8ba..34e658b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2935,24 +2935,6 @@ 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)
 {
-	if (!sched_feat(ATTACH_AGE_LOAD))
-		goto skip_aging;
-
-	/*
-	 * If we got migrated (either between CPUs or between cgroups) we'll
-	 * have aged the average right before clearing @last_update_time.
-	 */
-	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);
-
-		/*
-		 * XXX: we could have just aged the entire load away if we've been
-		 * absent from the fair class for too long.
-		 */
-	}
-
-skip_aging:
 	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;
@@ -2962,6 +2944,19 @@ skip_aging:
 	cfs_rq_util_change(cfs_rq);
 }
 
+static inline void attach_age_load_task(struct rq *rq, struct task_struct *p)
+{
+	struct sched_entity *se = &p->se;
+
+	if (!sched_feat(ATTACH_AGE_LOAD))
+		return;
+
+	if (se->avg.last_update_time) {
+		__update_load_avg(cfs_rq_of(se)->avg.last_update_time, cpu_of(rq),
+				  &se->avg, 0, 0, NULL);
+	}
+}
+
 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)),
@@ -3091,6 +3086,7 @@ 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 void attach_age_load_task(struct rq *rq, struct task_struct *p) {}
 
 static inline int idle_balance(struct rq *rq)
 {
@@ -8390,6 +8386,12 @@ static void switched_from_fair(struct rq *rq, struct task_struct *p)
 
 static void switched_to_fair(struct rq *rq, struct task_struct *p)
 {
+	/*
+	 * If we change between classes, age the averages before attaching them.
+	 * XXX: we could have just aged the entire load away if we've been
+	 * absent from the fair class for too long.
+	 */
+	attach_age_load_task(rq, p);
 	attach_task_cfs_rq(p);
 
 	if (task_on_rq_queued(p)) {
@@ -8441,11 +8443,6 @@ static void task_move_group_fair(struct task_struct *p)
 {
 	detach_task_cfs_rq(p);
 	set_task_rq(p, task_cpu(p));
-
-#ifdef CONFIG_SMP
-	/* Tell se's cfs_rq has been changed -- migrated */
-	p->se.avg.last_update_time = 0;
-#endif
 	attach_task_cfs_rq(p);
 }
 
-- 
1.7.9.5

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

#1415035 — Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg()

On Mon, 06 Jun, at 08:20:37AM, Yuyang Du wrote:
> attach_entity_load_avg() is called (indirectly) from:
> 
>  - switched_to_fair(): switch between classes to fair
>  - task_move_group_fair(): move between task groups
>  - enqueue_entity_load_avg(): enqueue entity
> 
> Only in switched_to_fair() is it possible that the task's last_update_time
> is not 0 and therefore the task needs sched avgs update, so move the task
> sched avgs update to switched_to_fair() only. In addition, the code is
> refactored and code comments are updated.
> 
> No functionality change.
> 
> Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> ---
>  kernel/sched/fair.c |   43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)

What about when tasks are moved between groups on CONFIG_SMP=n ?
Doesn't this change fail to age the load in that case?

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

#1415045 — Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg()

Hi Matt,

On 6 June 2016 at 15:30, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 06 Jun, at 08:20:37AM, Yuyang Du wrote:
>> attach_entity_load_avg() is called (indirectly) from:
>>
>>  - switched_to_fair(): switch between classes to fair
>>  - task_move_group_fair(): move between task groups
>>  - enqueue_entity_load_avg(): enqueue entity
>>
>> Only in switched_to_fair() is it possible that the task's last_update_time
>> is not 0 and therefore the task needs sched avgs update, so move the task
>> sched avgs update to switched_to_fair() only. In addition, the code is
>> refactored and code comments are updated.
>>
>> No functionality change.
>>
>> Signed-off-by: Yuyang Du <yuyang.du@intel.com>
>> ---
>>  kernel/sched/fair.c |   43 ++++++++++++++++++++-----------------------
>>  1 file changed, 20 insertions(+), 23 deletions(-)
>
> What about when tasks are moved between groups on CONFIG_SMP=n ?
> Doesn't this change fail to age the load in that case?

The load average stuff is only enable for SMP system but it's not used
for UP system.
To be honest, I don't know why

Vincent

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

#1415060 — Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg()

On Mon, 06 Jun, at 03:40:18PM, Vincent Guittot wrote:
> 
> The load average stuff is only enable for SMP system but it's not used
> for UP system.

Duh, right. My bad.

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

#1415117 — Re: [PATCH v4 1/5] sched/fair: Clean up attach_entity_load_avg()

On Mon, Jun 06, 2016 at 03:40:18PM +0200, Vincent Guittot wrote:
> > What about when tasks are moved between groups on CONFIG_SMP=n ?
> > Doesn't this change fail to age the load in that case?
> 
> The load average stuff is only enable for SMP system but it's not used
> for UP system.
> To be honest, I don't know why

We'll likely change that 'soon', because the cpufreq_schedutil thing
also needs these numbers on UP.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web