Messages in this thread | | | From | Vincent Guittot <> | Date | Fri, 16 Sep 2016 16:23:16 +0200 | Subject | Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class |
| |
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.
| |