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

[Peter Zijlstra]

On Thu, Sep 15, 2016 at 05:36:58PM +0200, Vincent Guittot wrote:
> On 15 September 2016 at 15:18, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.