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

From: Vincent Guittot
Date: Fri Sep 16 2016 - 10:23:50 EST

On 16 September 2016 at 14:16, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 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 !

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.