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

Groups > linux.kernel > #1480992 > unrolled thread

[PATCH 0/7 v3] sched: reflect sched_entity move into task_group's load

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7 v3] sched: reflect sched_entity move into task_group's load Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-12 09:50 +0200
    [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-12 09:50 +0200
      Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-15 15:20 +0200
        Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-15 17:40 +0200
          Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-16 14:20 +0200
            Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-16 16:30 +0200
              Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-20 14:00 +0200
                Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-20 15:10 +0200
                  Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-22 14:30 +0200
                Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class bsegall@google.com - 2016-09-20 19:00 +0200
                  Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-22 10:40 +0200
                    Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class bsegall@google.com - 2016-09-22 19:20 +0200
      Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Peter Zijlstra <peterz@infradead.org> - 2016-09-16 13:00 +0200
        Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when  switching to fair class Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-16 14:50 +0200
    [PATCH 1/7 v3] sched: factorize attach entity Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-12 09:50 +0200
    [PATCH 3/7 v3] sched: factorize PELT update Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-12 09:50 +0200
      Re: [PATCH 3/7 v3] sched: factorize PELT update Peter Zijlstra <peterz@infradead.org> - 2016-09-15 15:20 +0200
        Re: [PATCH 3/7 v3] sched: factorize PELT update Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-15 15:40 +0200
    [PATCH 6/7 v3] sched: fix task group initialization Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-12 09:50 +0200

#1480992 — [PATCH 0/7 v3] sched: reflect sched_entity move into task_group's load

Ensure that the move of a sched_entity will be reflected in load and
utilization of the task_group hierarchy.

When a sched_entity moves between groups or CPUs, load and utilization
of cfs_rq don't reflect the changes immediately but converge to new values.
As a result, the metrics are no more aligned with the new balance of the
load in the system and next decisions will have a biased view.

This patchset synchronizes load/utilization of sched_entity with its child
cfs_rq (se->my-q) only when tasks move to/from child cfs_rq:
-move between task group
-migration between CPUs
Otherwise, PELT is updated as usual.

Changes since v2:
- Propagate both utilization and load
- Synced sched_entity and se->my_q instead of adding the delta  

Changes since v1:
- This patch needs the patch that fixes issue with rq->leaf_cfs_rq_list
  "sched: fix hierarchical order in rq->leaf_cfs_rq_list" in order to work
  correctly. I haven't sent them as a single patchset because the fix is
  independent of this one
- Merge some functions that are always used together
- During update of blocked load, ensure that the sched_entity is synced
  with the cfs_rq applying changes
- Fix an issue when task changes its cpu affinity

Vincent Guittot (7):
  sched: factorize attach entity
  sched: fix hierarchical order in rq->leaf_cfs_rq_list
  sched: factorize PELT update
  sched: propagate load during synchronous attach/detach
  sched: propagate asynchrous detach
  sched: fix task group initialization
  sched: fix wrong utilization accounting when switching to fair class

 kernel/sched/core.c  |  21 ++--
 kernel/sched/fair.c  | 319 ++++++++++++++++++++++++++++++++++++++++-----------
 kernel/sched/sched.h |   2 +
 3 files changed, 264 insertions(+), 78 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1480993 — [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

When a task switches to fair scheduling class, the period between now and
the last update of its utilization is accounted as running time whatever
happened during this period. This wrong accounting applies to the task
and also to the task group branch.

When changing the property of a running task like its list of allowed CPUs
or its scheduling class, we follow the sequence:
-dequeue task
-put task
-change the property
-set task as current task
-enqueue task

The end of the sequence doesn't follow the normal sequence which is :
-enqueue a task
-then set the task as current task.

This wrong ordering is the root cause of wrong utilization accounting.
Update the sequence to follow the right one:
-dequeue task
-put task
-change the property
-enqueue task
-set task as current task

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3e52d08..7a9c9b9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1105,10 +1105,10 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
 
 	p->sched_class->set_cpus_allowed(p, new_mask);
 
-	if (running)
-		p->sched_class->set_curr_task(rq);
 	if (queued)
 		enqueue_task(rq, p, ENQUEUE_RESTORE);
+	if (running)
+		p->sched_class->set_curr_task(rq);
 }
 
 /*
@@ -3687,10 +3687,10 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
 
 	p->prio = prio;
 
-	if (running)
-		p->sched_class->set_curr_task(rq);
 	if (queued)
 		enqueue_task(rq, p, queue_flag);
+	if (running)
+		p->sched_class->set_curr_task(rq);
 
 	check_class_changed(rq, p, prev_class, oldprio);
 out_unlock:
@@ -4243,8 +4243,6 @@ static int __sched_setscheduler(struct task_struct *p,
 	prev_class = p->sched_class;
 	__setscheduler(rq, p, attr, pi);
 
-	if (running)
-		p->sched_class->set_curr_task(rq);
 	if (queued) {
 		/*
 		 * We enqueue to tail when the priority of a task is
@@ -4255,6 +4253,8 @@ static int __sched_setscheduler(struct task_struct *p,
 
 		enqueue_task(rq, p, queue_flags);
 	}
+	if (running)
+		p->sched_class->set_curr_task(rq);
 
 	check_class_changed(rq, p, prev_class, oldprio);
 	preempt_disable(); /* avoid rq from going away on us */
@@ -5417,10 +5417,10 @@ void sched_setnuma(struct task_struct *p, int nid)
 
 	p->numa_preferred_nid = nid;
 
-	if (running)
-		p->sched_class->set_curr_task(rq);
 	if (queued)
 		enqueue_task(rq, p, ENQUEUE_RESTORE);
+	if (running)
+		p->sched_class->set_curr_task(rq);
 	task_rq_unlock(rq, p, &rf);
 }
 #endif /* CONFIG_NUMA_BALANCING */
@@ -7868,10 +7868,10 @@ void sched_move_task(struct task_struct *tsk)
 
 	sched_change_group(tsk, TASK_MOVE_GROUP);
 
-	if (unlikely(running))
-		tsk->sched_class->set_curr_task(rq);
 	if (queued)
 		enqueue_task(rq, tsk, ENQUEUE_RESTORE | ENQUEUE_MOVE);
+	if (unlikely(running))
+		tsk->sched_class->set_curr_task(rq);
 
 	task_rq_unlock(rq, tsk, &rf);
 }
-- 
1.9.1

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

#1484156 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:
> When a task switches to fair scheduling class, the period between now and
> the last update of its utilization is accounted as running time whatever
> happened during this period. This wrong accounting applies to the task
> and also to the task group branch.
> 
> When changing the property of a running task like its list of allowed CPUs
> or its scheduling class, we follow the sequence:
> -dequeue task
> -put task
> -change the property
> -set task as current task
> -enqueue task
> 
> The end of the sequence doesn't follow the normal sequence which is :
> -enqueue a task
> -then set the task as current task.
> 
> This wrong ordering is the root cause of wrong utilization accounting.
> Update the sequence to follow the right one:
> -dequeue task
> -put task
> -change the property
> -enqueue task
> -set task as current task

But enqueue_entity depends on cfs_rq->curr, which is set by
set_curr_task_fair().

Also, the normalize comment in dequeue_entity() worries me, 'someone'
didn't update that when he moved update_min_vruntime() around.

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

#1484325 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On 15 September 2016 at 15:18, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:
>> When a task switches to fair scheduling class, the period between now and
>> the last update of its utilization is accounted as running time whatever
>> happened during this period. This wrong accounting applies to the task
>> and also to the task group branch.
>>
>> When changing the property of a running task like its list of allowed CPUs
>> or its scheduling class, we follow the sequence:
>> -dequeue task
>> -put task
>> -change the property
>> -set task as current task
>> -enqueue task
>>
>> The end of the sequence doesn't follow the normal sequence which is :
>> -enqueue a task
>> -then set the task as current task.
>>
>> This wrong ordering is the root cause of wrong utilization accounting.
>> Update the sequence to follow the right one:
>> -dequeue task
>> -put task
>> -change the property
>> -enqueue task
>> -set task as current task
>
> But enqueue_entity depends on cfs_rq->curr, which is set by
> set_curr_task_fair().

With this sequence, cfs_rq->curr is null and the cfs_rq is "idle" as
the entity has been dequeued and put back in the rb tree the time to
change the properties.

enqueue_entity use cfs_rq->cur == se for:
- updating current. With this sequence, current is now null so nothing to do
- to skip the enqueue of the se in rb tree. With this sequence, se is
put in the rb tree during the enqueue and take back during the set
task as current task

I don't see any functional issue but we are not doing the same step
with the new sequence

>
> Also, the normalize comment in dequeue_entity() worries me, 'someone'
> didn't update that when he moved update_min_vruntime() around.

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

#1484933 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Thu, Sep 15, 2016 at 05:36:58PM +0200, Vincent Guittot wrote:
> On 15 September 2016 at 15:18, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:

> >> Update the sequence to follow the right one:
> >> -dequeue task
> >> -put task
> >> -change the property
> >> -enqueue task
> >> -set task as current task
> >
> > But enqueue_entity depends on cfs_rq->curr, which is set by
> > set_curr_task_fair().
> 
> With this sequence, cfs_rq->curr is null and the cfs_rq is "idle" as
> the entity has been dequeued and put back in the rb tree the time to
> change the properties.
> 
> enqueue_entity use cfs_rq->cur == se for:
> - updating current. With this sequence, current is now null so nothing to do
> - to skip the enqueue of the se in rb tree. With this sequence, se is
> put in the rb tree during the enqueue and take back during the set
> task as current task
> 
> I don't see any functional issue but we are not doing the same step
> with the new sequence

So I think you're right in that it should work.

I also think we can then simplify enqueue_entity() in that it will never
be possible to enqueue current with your change.

But my brain just isn't working today, so who knows.

> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
> > didn't update that when he moved update_min_vruntime() around.

I now worry more, so we do:

	dequeue_task := dequeue_task_fair (p == current)
	  dequeue_entity
	    update_curr()
	      update_min_vruntime()
	    vruntime -= min_vruntime
	    update_min_vruntime()
	      // use cfs_rq->curr, which we just normalized !

	put_prev_task := put_prev_task_fair
	  put_prev_entity
	    cfs_rq->curr = NULL;


Now the point of the latter update_min_vruntime() is to advance
min_vruntime when the task we removed was the one holding it back.

However, it means that if we do dequeue+enqueue, we're further in the
future (ie. we get penalized).

So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
But as said above, my brain isn't co-operating much today.

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

#1485044 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On 16 September 2016 at 14:16, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Sep 15, 2016 at 05:36:58PM +0200, Vincent Guittot wrote:
>> On 15 September 2016 at 15:18, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:
>
>> >> Update the sequence to follow the right one:
>> >> -dequeue task
>> >> -put task
>> >> -change the property
>> >> -enqueue task
>> >> -set task as current task
>> >
>> > But enqueue_entity depends on cfs_rq->curr, which is set by
>> > set_curr_task_fair().
>>
>> With this sequence, cfs_rq->curr is null and the cfs_rq is "idle" as
>> the entity has been dequeued and put back in the rb tree the time to
>> change the properties.
>>
>> enqueue_entity use cfs_rq->cur == se for:
>> - updating current. With this sequence, current is now null so nothing to do
>> - to skip the enqueue of the se in rb tree. With this sequence, se is
>> put in the rb tree during the enqueue and take back during the set
>> task as current task
>>
>> I don't see any functional issue but we are not doing the same step
>> with the new sequence
>
> So I think you're right in that it should work.
>
> I also think we can then simplify enqueue_entity() in that it will never
> be possible to enqueue current with your change.
>
> But my brain just isn't working today, so who knows.
>
>> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
>> > didn't update that when he moved update_min_vruntime() around.
>
> I now worry more, so we do:
>
>         dequeue_task := dequeue_task_fair (p == current)
>           dequeue_entity
>             update_curr()
>               update_min_vruntime()
>             vruntime -= min_vruntime
>             update_min_vruntime()
>               // use cfs_rq->curr, which we just normalized !

yes but does it really change the cfs_rq->min_vruntime in this case ?

If curr is the task with the smallest vruntime of the cfs_rq,
cfs_rq->min_vruntime has been aligned with curr->vruntime during
update_curr(). So vruntime -= min_vruntime will be for sure less than
cfs_rq->min_vruntime and cfs_rq->min_vruntime stays unchanged

If curr is not the task with the smallest vruntime of the cfs_rq,
cfs_rq->min_vruntime has been aligned with the left most entity. And
vruntime -= min_vruntime will not change anything during the 2nd
update_min_vruntime as it will be either greater than
leftmost->vruntime or less than cfs_rq->min_vruntime.

>
>         put_prev_task := put_prev_task_fair
>           put_prev_entity
>             cfs_rq->curr = NULL;
>
>
> Now the point of the latter update_min_vruntime() is to advance
> min_vruntime when the task we removed was the one holding it back.
>
> However, it means that if we do dequeue+enqueue, we're further in the
> future (ie. we get penalized).
>
> So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
> But as said above, my brain isn't co-operating much today.

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

#1487271 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Fri, Sep 16, 2016 at 04:23:16PM +0200, Vincent Guittot wrote:
> On 16 September 2016 at 14:16, Peter Zijlstra <peterz@infradead.org> wrote:

> >> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
> >> > didn't update that when he moved update_min_vruntime() around.
> >
> > I now worry more, so we do:
> >
> >         dequeue_task := dequeue_task_fair (p == current)
> >           dequeue_entity
> >             update_curr()
> >               update_min_vruntime()
> >             vruntime -= min_vruntime
> >             update_min_vruntime()
> >               // use cfs_rq->curr, which we just normalized !
> 
> yes but does it really change the cfs_rq->min_vruntime in this case ?

So let me see; it does:

	vruntime = cfs_rq->min_vruntime;

	if (curr) // true
	  vruntime = curr->vruntime; // == vruntime - min_vruntime

	if (leftmost) // possible
	  if (curr) // true
	    vruntime = min_vruntime(vruntime, se->vruntime);
	      if (se->vruntime - (curr->vruntime - min_vruntime)) < 0 // false

	min_vruntime = max_vruntime(min_vruntime, vruntime);
	  if ((curr->vruntime - min_vruntime) - min_vruntime) > 0)


The problem is that double subtraction of min_vruntime can wrap.
The thing is, min_vruntime is the 0-point in our modular space, it
normalizes vruntime (ideally min_vruntime would be our 0-lag point,
resulting in vruntime - min_vruntime being the lag).

The moment min_vruntime grows past S64_MAX/2 -2*min_vruntime wraps into
positive space again and the test above becomes true and we'll select
the normalized @curr vruntime as new min_vruntime and weird stuff will
happen.


Also, even it things magically worked out, its still very icky to mix
the normalized vruntime into things.

> >         put_prev_task := put_prev_task_fair
> >           put_prev_entity
> >             cfs_rq->curr = NULL;
> >
> >
> > Now the point of the latter update_min_vruntime() is to advance
> > min_vruntime when the task we removed was the one holding it back.
> >
> > However, it means that if we do dequeue+enqueue, we're further in the
> > future (ie. we get penalized).
> >
> > So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
> > But as said above, my brain isn't co-operating much today.

OK, so not sure we can actually remove it, we do want it to move
min_vruntime forward (sometimes). We just don't want it to do so when
DEQUEUE_SAVE -- we want to get back where we left off, nor do we want to
muck about with touching normalized values.

Another fun corner case is DEQUEUE_SLEEP; in that case we do not
normalize, but we still want advance min_vruntime if this was the one
holding it back.

I ended up with the below, but I'm not sure I like it much. Let me prod
a wee bit more to see if there's not something else we can do.

Google has this patch-set replacing min_vruntime with an actual global
0-lag, which greatly simplifies things. If only they'd post it sometime
:/ /me prods pjt and ben with a sharp stick :-)

---
 kernel/sched/fair.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 986c10c25176..77566a340cbf 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -462,17 +462,23 @@ static inline int entity_before(struct sched_entity *a,
 
 static void update_min_vruntime(struct cfs_rq *cfs_rq)
 {
+	struct sched_entity *curr = cfs_rq->curr;
+
 	u64 vruntime = cfs_rq->min_vruntime;
 
-	if (cfs_rq->curr)
-		vruntime = cfs_rq->curr->vruntime;
+	if (curr) {
+		if (curr->on_rq)
+			vruntime = curr->vruntime;
+		else
+			curr = NULL;
+	}
 
 	if (cfs_rq->rb_leftmost) {
 		struct sched_entity *se = rb_entry(cfs_rq->rb_leftmost,
 						   struct sched_entity,
 						   run_node);
 
-		if (!cfs_rq->curr)
+		if (!curr)
 			vruntime = se->vruntime;
 		else
 			vruntime = min_vruntime(vruntime, se->vruntime);
@@ -3483,8 +3489,16 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	/* return excess runtime on last dequeue */
 	return_cfs_rq_runtime(cfs_rq);
 
-	update_min_vruntime(cfs_rq);
 	update_cfs_shares(cfs_rq);
+
+	/*
+	 * Now advance min_vruntime if @se was the entity holding it back,
+	 * except when: DEQUEUE_SAVE && !DEQUEUE_MOVE, in this case we'll be
+	 * put back on, and if we advance min_vruntime, we'll be placed back
+	 * further than we started -- ie. we'll be penalized.
+	 */
+	if ((flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)) == DEQUEUE_SAVE)
+		update_min_vruntime(cfs_rq);
 }
 
 /*

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

#1487329 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On 20 September 2016 at 13:54, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 16, 2016 at 04:23:16PM +0200, Vincent Guittot wrote:
>> On 16 September 2016 at 14:16, Peter Zijlstra <peterz@infradead.org> wrote:
>
>> >> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
>> >> > didn't update that when he moved update_min_vruntime() around.
>> >
>> > I now worry more, so we do:
>> >
>> >         dequeue_task := dequeue_task_fair (p == current)
>> >           dequeue_entity
>> >             update_curr()
>> >               update_min_vruntime()
>> >             vruntime -= min_vruntime
>> >             update_min_vruntime()
>> >               // use cfs_rq->curr, which we just normalized !
>>
>> yes but does it really change the cfs_rq->min_vruntime in this case ?
>
> So let me see; it does:
>
>         vruntime = cfs_rq->min_vruntime;
>
>         if (curr) // true
>           vruntime = curr->vruntime; // == vruntime - min_vruntime
>
>         if (leftmost) // possible
>           if (curr) // true
>             vruntime = min_vruntime(vruntime, se->vruntime);
>               if (se->vruntime - (curr->vruntime - min_vruntime)) < 0 // false
>
>         min_vruntime = max_vruntime(min_vruntime, vruntime);
>           if ((curr->vruntime - min_vruntime) - min_vruntime) > 0)
>
>
> The problem is that double subtraction of min_vruntime can wrap.
> The thing is, min_vruntime is the 0-point in our modular space, it
> normalizes vruntime (ideally min_vruntime would be our 0-lag point,
> resulting in vruntime - min_vruntime being the lag).
>
> The moment min_vruntime grows past S64_MAX/2 -2*min_vruntime wraps into

fair enough

> positive space again and the test above becomes true and we'll select
> the normalized @curr vruntime as new min_vruntime and weird stuff will
> happen.
>
>
> Also, even it things magically worked out, its still very icky to mix
> the normalized vruntime into things.

I agree

>
>> >         put_prev_task := put_prev_task_fair
>> >           put_prev_entity
>> >             cfs_rq->curr = NULL;
>> >
>> >
>> > Now the point of the latter update_min_vruntime() is to advance
>> > min_vruntime when the task we removed was the one holding it back.
>> >
>> > However, it means that if we do dequeue+enqueue, we're further in the
>> > future (ie. we get penalized).
>> >
>> > So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
>> > But as said above, my brain isn't co-operating much today.
>
> OK, so not sure we can actually remove it, we do want it to move
> min_vruntime forward (sometimes). We just don't want it to do so when
> DEQUEUE_SAVE -- we want to get back where we left off, nor do we want to
> muck about with touching normalized values.
>
> Another fun corner case is DEQUEUE_SLEEP; in that case we do not
> normalize, but we still want advance min_vruntime if this was the one
> holding it back.
>
> I ended up with the below, but I'm not sure I like it much. Let me prod
> a wee bit more to see if there's not something else we can do.
>
> Google has this patch-set replacing min_vruntime with an actual global
> 0-lag, which greatly simplifies things. If only they'd post it sometime
> :/ /me prods pjt and ben with a sharp stick :-)
>
> ---
>  kernel/sched/fair.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 986c10c25176..77566a340cbf 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -462,17 +462,23 @@ static inline int entity_before(struct sched_entity *a,
>
>  static void update_min_vruntime(struct cfs_rq *cfs_rq)
>  {
> +       struct sched_entity *curr = cfs_rq->curr;
> +
>         u64 vruntime = cfs_rq->min_vruntime;
>
> -       if (cfs_rq->curr)
> -               vruntime = cfs_rq->curr->vruntime;
> +       if (curr) {
> +               if (curr->on_rq)
> +                       vruntime = curr->vruntime;
> +               else
> +                       curr = NULL;
> +       }
>
>         if (cfs_rq->rb_leftmost) {
>                 struct sched_entity *se = rb_entry(cfs_rq->rb_leftmost,
>                                                    struct sched_entity,
>                                                    run_node);
>
> -               if (!cfs_rq->curr)
> +               if (!curr)
>                         vruntime = se->vruntime;
>                 else
>                         vruntime = min_vruntime(vruntime, se->vruntime);
> @@ -3483,8 +3489,16 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>         /* return excess runtime on last dequeue */
>         return_cfs_rq_runtime(cfs_rq);
>
> -       update_min_vruntime(cfs_rq);
>         update_cfs_shares(cfs_rq);
> +
> +       /*
> +        * Now advance min_vruntime if @se was the entity holding it back,
> +        * except when: DEQUEUE_SAVE && !DEQUEUE_MOVE, in this case we'll be
> +        * put back on, and if we advance min_vruntime, we'll be placed back
> +        * further than we started -- ie. we'll be penalized.
> +        */
> +       if ((flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)) == DEQUEUE_SAVE)
> +               update_min_vruntime(cfs_rq);
>  }
>
>  /*

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

#1488842 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Tue, Sep 20, 2016 at 03:06:04PM +0200, Vincent Guittot wrote:
> > Also, even it things magically worked out, its still very icky to mix
> > the normalized vruntime into things.
> 
> I agree

In any case, I pushed out a bunch of patches to:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git sched/tmp

they appear to be able to build a kernel, but that's not much testing.

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

#1487523 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

Peter Zijlstra <peterz@infradead.org> writes:

> On Fri, Sep 16, 2016 at 04:23:16PM +0200, Vincent Guittot wrote:
>> On 16 September 2016 at 14:16, Peter Zijlstra <peterz@infradead.org> wrote:
>
>> >> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
>> >> > didn't update that when he moved update_min_vruntime() around.
>> >
>> > I now worry more, so we do:
>> >
>> >         dequeue_task := dequeue_task_fair (p == current)
>> >           dequeue_entity
>> >             update_curr()
>> >               update_min_vruntime()
>> >             vruntime -= min_vruntime
>> >             update_min_vruntime()
>> >               // use cfs_rq->curr, which we just normalized !
>> 
>> yes but does it really change the cfs_rq->min_vruntime in this case ?
>
> So let me see; it does:
>
> 	vruntime = cfs_rq->min_vruntime;
>
> 	if (curr) // true
> 	  vruntime = curr->vruntime; // == vruntime - min_vruntime
>
> 	if (leftmost) // possible
> 	  if (curr) // true
> 	    vruntime = min_vruntime(vruntime, se->vruntime);
> 	      if (se->vruntime - (curr->vruntime - min_vruntime)) < 0 // false
>
> 	min_vruntime = max_vruntime(min_vruntime, vruntime);
> 	  if ((curr->vruntime - min_vruntime) - min_vruntime) > 0)
>
>
> The problem is that double subtraction of min_vruntime can wrap.
> The thing is, min_vruntime is the 0-point in our modular space, it
> normalizes vruntime (ideally min_vruntime would be our 0-lag point,
> resulting in vruntime - min_vruntime being the lag).
>
> The moment min_vruntime grows past S64_MAX/2 -2*min_vruntime wraps into
> positive space again and the test above becomes true and we'll select
> the normalized @curr vruntime as new min_vruntime and weird stuff will
> happen.
>
>
> Also, even it things magically worked out, its still very icky to mix
> the normalized vruntime into things.
>
>> >         put_prev_task := put_prev_task_fair
>> >           put_prev_entity
>> >             cfs_rq->curr = NULL;
>> >
>> >
>> > Now the point of the latter update_min_vruntime() is to advance
>> > min_vruntime when the task we removed was the one holding it back.
>> >
>> > However, it means that if we do dequeue+enqueue, we're further in the
>> > future (ie. we get penalized).
>> >
>> > So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
>> > But as said above, my brain isn't co-operating much today.
>
> OK, so not sure we can actually remove it, we do want it to move
> min_vruntime forward (sometimes). We just don't want it to do so when
> DEQUEUE_SAVE -- we want to get back where we left off, nor do we want to
> muck about with touching normalized values.
>
> Another fun corner case is DEQUEUE_SLEEP; in that case we do not
> normalize, but we still want advance min_vruntime if this was the one
> holding it back.
>
> I ended up with the below, but I'm not sure I like it much. Let me prod
> a wee bit more to see if there's not something else we can do.
>
> Google has this patch-set replacing min_vruntime with an actual global
> 0-lag, which greatly simplifies things. If only they'd post it sometime
> :/ /me prods pjt and ben with a sharp stick :-)
>

No, we don't have any patches like that. I wish, we've screwed up
vruntime a couple of times too.

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

#1488642 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Tue, Sep 20, 2016 at 09:59:08AM -0700, bsegall@google.com wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> > Google has this patch-set replacing min_vruntime with an actual global
> > 0-lag, which greatly simplifies things. If only they'd post it sometime
> > :/ /me prods pjt and ben with a sharp stick :-)
> >
> 
> No, we don't have any patches like that. I wish, we've screwed up
> vruntime a couple of times too.

Hurm, I was sure you guys were doing something entirely different from
what mainline does.

IIRC Andrew Hunter was 'working' on getting that posted. See also:
lkml.kernel.org/r/CADroS=6Ncpao6WWXBxboB6quqUP96EnjbsDUZqNgASd_PzDGaA@mail.gmail.com

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

#1489106 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Sep 20, 2016 at 09:59:08AM -0700, bsegall@google.com wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>> > Google has this patch-set replacing min_vruntime with an actual global
>> > 0-lag, which greatly simplifies things. If only they'd post it sometime
>> > :/ /me prods pjt and ben with a sharp stick :-)
>> >
>> 
>> No, we don't have any patches like that. I wish, we've screwed up
>> vruntime a couple of times too.
>
> Hurm, I was sure you guys were doing something entirely different from
> what mainline does.
>
> IIRC Andrew Hunter was 'working' on getting that posted. See also:
> lkml.kernel.org/r/CADroS=6Ncpao6WWXBxboB6quqUP96EnjbsDUZqNgASd_PzDGaA@mail.gmail.com

Oh, I was looking at the wrong branches. Sigh.

Yeah, we just do an unlocked "se->vruntime -= old->min_vruntime -
new->min_vruntime;" in migrate, and remove/add it in
switched_from/switched_to_fair.

This still isn't replacing min_vruntime with anything cool, perhaps you
and Andrew were using opposite definitions of relative here - we have
fair tasks always having a vruntime that is relative to min_vruntime,
rather than temporarily having the 0-based one when doing
non-SLEEP/WAKEUP.

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

#1484868 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:

> -dequeue task
> -put task
> -change the property
> -enqueue task
> -set task as current task

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3e52d08..7a9c9b9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1105,10 +1105,10 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
>  
>  	p->sched_class->set_cpus_allowed(p, new_mask);
>  
> -	if (running)
> -		p->sched_class->set_curr_task(rq);
>  	if (queued)
>  		enqueue_task(rq, p, ENQUEUE_RESTORE);
> +	if (running)
> +		p->sched_class->set_curr_task(rq);
>  }
>  
>  /*

So one thing that I've wanted to do for a while, but never managed to
come up with a sensible way to do is encapsulate this pattern.

The two options I came up with are:

#define FOO(p, stmt)
({
	struct rq *rq = task_rq(p);
	bool queued = task_on_rq_queued(p);
	bool running = task_current(rq);
	int queue_flags = DEQUEUE_SAVE; /* also ENQUEUE_RESTORE */

	if (queued)
		dequeue_task(rq, p, queue_flags);
	if (running)
		put_prev_task(rq, p);

	stmt;

	if (queued)
		enqueue_task(rq, p, queue_flags);
	if (running)
		set_curr_task(rq, p);
})

and

void foo(struct task_struct *p, void (*func)(struct task_struct *, int *))
{
	struct rq *rq = task_rq(p);
	bool queued = task_on_rq_queued(p);
	bool running = task_current(rq);
	int queue_flags = DEQUEUE_SAVE; /* also ENQUEUE_RESTORE */

	if (queued)
		dequeue_task(rq, p, queue_flags);
	if (running)
		put_prev_task(rq, p);

	func(p, &queue_flags);

	if (queued)
		enqueue_task(rq, p, queue_flags);
	if (running)
		set_curr_task(rq, p);
}

Neither results in particularly pretty code. Although I suppose if I'd
have to pick one I'd go for the macro variant.

Opinions? I'm fine with leaving the code as is, just wanted to throw
this out there.

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

#1484969 — Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

On 16 September 2016 at 12:51, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:
>
> > -dequeue task
> > -put task
> > -change the property
> > -enqueue task
> > -set task as current task
>
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 3e52d08..7a9c9b9 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1105,10 +1105,10 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> >
> >       p->sched_class->set_cpus_allowed(p, new_mask);
> >
> > -     if (running)
> > -             p->sched_class->set_curr_task(rq);
> >       if (queued)
> >               enqueue_task(rq, p, ENQUEUE_RESTORE);
> > +     if (running)
> > +             p->sched_class->set_curr_task(rq);
> >  }
> >
> >  /*
>
> So one thing that I've wanted to do for a while, but never managed to
> come up with a sensible way to do is encapsulate this pattern.
>
> The two options I came up with are:
>
> #define FOO(p, stmt)
> ({
>         struct rq *rq = task_rq(p);
>         bool queued = task_on_rq_queued(p);
>         bool running = task_current(rq);
>         int queue_flags = DEQUEUE_SAVE; /* also ENQUEUE_RESTORE */
>
>         if (queued)
>                 dequeue_task(rq, p, queue_flags);
>         if (running)
>                 put_prev_task(rq, p);
>
>         stmt;
>
>         if (queued)
>                 enqueue_task(rq, p, queue_flags);
>         if (running)
>                 set_curr_task(rq, p);
> })
>
> and
>
> void foo(struct task_struct *p, void (*func)(struct task_struct *, int *))
> {
>         struct rq *rq = task_rq(p);
>         bool queued = task_on_rq_queued(p);
>         bool running = task_current(rq);
>         int queue_flags = DEQUEUE_SAVE; /* also ENQUEUE_RESTORE */
>
>         if (queued)
>                 dequeue_task(rq, p, queue_flags);
>         if (running)
>                 put_prev_task(rq, p);
>
>         func(p, &queue_flags);
>
>         if (queued)
>                 enqueue_task(rq, p, queue_flags);
>         if (running)
>                 set_curr_task(rq, p);
> }
>
> Neither results in particularly pretty code. Although I suppose if I'd
> have to pick one I'd go for the macro variant.
>
> Opinions? I'm fine with leaving the code as is, just wanted to throw
> this out there.

I'm not convinced by using such encapsulation as it adds the
constraint of having a function to pass which is not always the case
and it hides a bit whats happen to this function
What about creating a task_FOO_save and a task_FOO_save macro  ? something like

#define task_FOO_save(p, rq, flags)
({
        bool queued = task_on_rq_queued(p);
        bool running = task_current(rq);
        int queue_flags = DEQUEUE_SAVE; /* also ENQUEUE_RESTORE */

        if (queued)
                dequeue_task(rq, p, queue_flags);
        if (running)
                put_prev_task(rq, p);
        flags = queued | running << 1;
})

#define task_FOO_restore(p, rq, flags)
({
        bool queued = flags & 0x1;
        bool running = flags & 0x2;
        int queue_flags = ENQUEUE_RESTORE;

        if (queued)
                enqueue_task(rq, p, queue_flags);
        if (running)
                set_curr_task(rq, p);
})

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

#1480994 — [PATCH 1/7 v3] sched: factorize attach entity

Factorize post_init_entity_util_avg and part of attach_task_cfs_rq
in one function attach_entity_cfs_rq

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a6820b3..dfd9c0c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -694,9 +694,7 @@ void init_entity_runnable_average(struct sched_entity *se)
 }
 
 static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
-static int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq);
-static void update_tg_load_avg(struct cfs_rq *cfs_rq, int force);
-static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se);
+static void attach_entity_cfs_rq(struct sched_entity *se);
 
 /*
  * With new tasks being created, their initial util_avgs are extrapolated
@@ -761,9 +759,7 @@ void post_init_entity_util_avg(struct sched_entity *se)
 		}
 	}
 
-	update_cfs_rq_load_avg(now, cfs_rq, false);
-	attach_entity_load_avg(cfs_rq, se);
-	update_tg_load_avg(cfs_rq, false);
+	attach_entity_cfs_rq(se);
 }
 
 #else /* !CONFIG_SMP */
@@ -8498,9 +8494,8 @@ static void detach_task_cfs_rq(struct task_struct *p)
 	update_tg_load_avg(cfs_rq, false);
 }
 
-static void attach_task_cfs_rq(struct task_struct *p)
+static void attach_entity_cfs_rq(struct sched_entity *se)
 {
-	struct sched_entity *se = &p->se;
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	u64 now = cfs_rq_clock_task(cfs_rq);
 
@@ -8516,6 +8511,14 @@ static void attach_task_cfs_rq(struct task_struct *p)
 	update_cfs_rq_load_avg(now, cfs_rq, false);
 	attach_entity_load_avg(cfs_rq, se);
 	update_tg_load_avg(cfs_rq, false);
+}
+
+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);
+
+	attach_entity_cfs_rq(se);
 
 	if (!vruntime_normalized(p))
 		se->vruntime += cfs_rq->min_vruntime;
-- 
1.9.1

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

#1480996 — [PATCH 3/7 v3] sched: factorize PELT update

Every time, we modify load/utilization of sched_entity, we start to sync
it with its cfs_rq. This update is done is different ways:
-when attaching/detaching a sched_entity, we update cfs_rq and then we
sync the entity with the cfs_rq.
-when enqueueing/dequeuing the sched_entity, we update both sched_entity
and cfs_rq metrics to now.

Use update_load_avg everytime we have to update and sync cfs_rq and
sched_entity before changing the state of a sched_enity

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 68 ++++++++++++++---------------------------------------
 1 file changed, 17 insertions(+), 51 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 264119a..0aa1d7d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3086,7 +3086,8 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
 }
 
 /* Update task and its cfs_rq load average */
-static inline void update_load_avg(struct sched_entity *se, int update_tg)
+static inline void update_load_avg(struct sched_entity *se, int update_tg,
+				   int skip_aging)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	u64 now = cfs_rq_clock_task(cfs_rq);
@@ -3097,7 +3098,8 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
 	 * 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,
+	if (se->avg.last_update_time && !skip_aging)
+		__update_load_avg(now, cpu, &se->avg,
 			  se->on_rq * scale_load_down(se->load.weight),
 			  cfs_rq->curr == se, NULL);
 
@@ -3115,26 +3117,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.
-	 *
-	 * Or we're fresh through post_init_entity_util_avg().
-	 */
-	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;
@@ -3154,9 +3136,6 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
  */
 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);
 
 	sub_positive(&cfs_rq->avg.load_avg, se->avg.load_avg);
 	sub_positive(&cfs_rq->avg.load_sum, se->avg.load_sum);
@@ -3171,34 +3150,20 @@ 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, decayed;
-
-	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);
-	}
-
-	decayed = update_cfs_rq_load_avg(now, cfs_rq, !migrated);
 
 	cfs_rq->runnable_load_avg += sa->load_avg;
 	cfs_rq->runnable_load_sum += sa->load_sum;
 
-	if (migrated)
+	if (!sa->last_update_time) {
 		attach_entity_load_avg(cfs_rq, se);
-
-	if (decayed || migrated)
 		update_tg_load_avg(cfs_rq, 0);
+	}
 }
 
 /* Remove the runnable load generated by se from cfs_rq's runnable load average */
 static inline void
 dequeue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	update_load_avg(se, 1);
-
 	cfs_rq->runnable_load_avg =
 		max_t(long, cfs_rq->runnable_load_avg - se->avg.load_avg, 0);
 	cfs_rq->runnable_load_sum =
@@ -3272,7 +3237,8 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
 	return 0;
 }
 
-static inline void update_load_avg(struct sched_entity *se, int not_used)
+static inline void update_load_avg(struct sched_entity *se,
+				   int not_used1, int not_used2)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	struct rq *rq = rq_of(cfs_rq);
@@ -3420,6 +3386,7 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	if (renorm && !curr)
 		se->vruntime += cfs_rq->min_vruntime;
 
+	update_load_avg(se, 1, 0);
 	enqueue_entity_load_avg(cfs_rq, se);
 	account_entity_enqueue(cfs_rq, se);
 	update_cfs_shares(cfs_rq);
@@ -3494,6 +3461,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	 * Update run-time statistics of the 'current'.
 	 */
 	update_curr(cfs_rq);
+	update_load_avg(se, 1, 0);
 	dequeue_entity_load_avg(cfs_rq, se);
 
 	update_stats_dequeue(cfs_rq, se, flags);
@@ -3572,7 +3540,7 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
 		 */
 		update_stats_wait_end(cfs_rq, se);
 		__dequeue_entity(cfs_rq, se);
-		update_load_avg(se, 1);
+		update_load_avg(se, 1, 0);
 	}
 
 	update_stats_curr_start(cfs_rq, se);
@@ -3674,7 +3642,7 @@ static void put_prev_entity(struct cfs_rq *cfs_rq, struct sched_entity *prev)
 		/* Put 'current' back into the tree. */
 		__enqueue_entity(cfs_rq, prev);
 		/* in !on_rq case, update occurred at dequeue */
-		update_load_avg(prev, 0);
+		update_load_avg(prev, 0, 0);
 	}
 	cfs_rq->curr = NULL;
 }
@@ -3690,7 +3658,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 	/*
 	 * Ensure that runnable average is periodically updated.
 	 */
-	update_load_avg(curr, 1);
+	update_load_avg(curr, 1, 0);
 	update_cfs_shares(cfs_rq);
 
 #ifdef CONFIG_SCHED_HRTICK
@@ -4579,7 +4547,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 		if (cfs_rq_throttled(cfs_rq))
 			break;
 
-		update_load_avg(se, 1);
+		update_load_avg(se, 1, 0);
 		update_cfs_shares(cfs_rq);
 	}
 
@@ -4638,7 +4606,7 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 		if (cfs_rq_throttled(cfs_rq))
 			break;
 
-		update_load_avg(se, 1);
+		update_load_avg(se, 1, 0);
 		update_cfs_shares(cfs_rq);
 	}
 
@@ -8517,7 +8485,6 @@ 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);
-	u64 now = cfs_rq_clock_task(cfs_rq);
 
 	if (!vruntime_normalized(p)) {
 		/*
@@ -8529,7 +8496,7 @@ static void detach_task_cfs_rq(struct task_struct *p)
 	}
 
 	/* Catch up with the cfs_rq and remove our load when we leave */
-	update_cfs_rq_load_avg(now, cfs_rq, false);
+	update_load_avg(se, 0, 0);
 	detach_entity_load_avg(cfs_rq, se);
 	update_tg_load_avg(cfs_rq, false);
 }
@@ -8537,7 +8504,6 @@ static void detach_task_cfs_rq(struct task_struct *p)
 static void attach_entity_cfs_rq(struct sched_entity *se)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
-	u64 now = cfs_rq_clock_task(cfs_rq);
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	/*
@@ -8548,7 +8514,7 @@ static void attach_entity_cfs_rq(struct sched_entity *se)
 #endif
 
 	/* Synchronize task with its cfs_rq */
-	update_cfs_rq_load_avg(now, cfs_rq, false);
+	update_load_avg(se, 0, !sched_feat(ATTACH_AGE_LOAD));
 	attach_entity_load_avg(cfs_rq, se);
 	update_tg_load_avg(cfs_rq, false);
 }
-- 
1.9.1

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

#1484157 — Re: [PATCH 3/7 v3] sched: factorize PELT update

On Mon, Sep 12, 2016 at 09:47:48AM +0200, Vincent Guittot wrote:
> @@ -3690,7 +3658,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>  	/*
>  	 * Ensure that runnable average is periodically updated.
>  	 */
> -	update_load_avg(curr, 1);
> +	update_load_avg(curr, 1, 0);
>  	update_cfs_shares(cfs_rq);
>  

I would find something like: update_load_avg(curr, UPDATE_TG), eg, make
the second argument a bitflag instead of two arguments, much more
readable.

Do however check that it doesn't generate retarded code if you do that.

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

#1484180 — Re: [PATCH 3/7 v3] sched: factorize PELT update

On 15 September 2016 at 15:09, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 12, 2016 at 09:47:48AM +0200, Vincent Guittot wrote:
>> @@ -3690,7 +3658,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>>       /*
>>        * Ensure that runnable average is periodically updated.
>>        */
>> -     update_load_avg(curr, 1);
>> +     update_load_avg(curr, 1, 0);
>>       update_cfs_shares(cfs_rq);
>>
>
> I would find something like: update_load_avg(curr, UPDATE_TG), eg, make
> the second argument a bitflag instead of two arguments, much more
> readable.

OK, I'm going to use bitflag
>
> Do however check that it doesn't generate retarded code if you do that.

OK

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

#1480997 — [PATCH 6/7 v3] sched: fix task group initialization

The moves of tasks are now propagated down to root and the utilization
of cfs_rq reflects reality so it doesn't need to be estimated at init.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 25533ba..ad6ca84 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8863,7 +8863,7 @@ void online_fair_sched_group(struct task_group *tg)
 		se = tg->se[i];
 
 		raw_spin_lock_irq(&rq->lock);
-		post_init_entity_util_avg(se);
+		attach_entity_cfs_rq(se);
 		sync_throttle(tg, i);
 		raw_spin_unlock_irq(&rq->lock);
 	}
-- 
1.9.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web