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

[Vincent Guittot <vincent.guittot@linaro.org>]

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.