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