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.