On 16 September 2016 at 14:16, Peter Zijlstra <[email protected]> wrote: > On Thu, Sep 15, 2016 at 05:36:58PM +0200, Vincent Guittot wrote: >> On 15 September 2016 at 15:18, Peter Zijlstra <[email protected]> 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.
