Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair()

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Sep 08, 2015 at 06:48:43AM +0800, Wanpeng Li wrote:
> On 9/7/15 10:02 PM, Peter Zijlstra wrote:
> >Please always Cc at least the person who wrote the lines you modify.
> >
> >On Mon, Sep 07, 2015 at 05:45:20PM +0800, Wanpeng Li wrote:
> >>The sleeper task will be normalized when moved from fair_sched_class, in
> >>order that vruntime will be adjusted either the task is running or sleeping
> >>when moved back. The nomalization in switch_to_fair for sleep task will
> >>result in lose fair sleeper bonus in place_entity() once the vruntime -
> >>cfs_rq->min_vruntime is big when moved from fair_sched_class.
> >>
> >>This patch fix it by adjusting vruntime just during migrating as original
> >>codes since the vruntime of the task has usually NOT been normalized in
> >>this case.

> >Sorry, I cannot follow that at all. Maybe its me being sleep deprived,
> >but could you try that again?
> 
> When changing away from the fair class while sleeping, relative vruntime is
> calculated to handle the case sleep when moved from fair_sched_class and
> running when moved to fair_sched_class.

That, or the task being migrated to a different cgroup / cpu while being
outside of the fair class.

Also, the 'relative vruntime' as you call it, is an approximation for
lag. Because we do not compute the 0-lag point (too expensive) we use
min_vruntime as a conservative approximation.

Lag is something you can transfer between runqueues, so that is the
natural state for something that is not associated with a rq.

> The absolute vruntime will be
> calculated in enqueue_entity() either the task is running or sleeping when
> moved back.

Incorrect, enqueue_entity() will only do that conditionally, in the
other cases it will assume se->vruntime is already absolute as you call
it.

attach_task_cfs_rq() must deal with the other cases.

> The fair sleeper bonus should be gained in place_entity() if the
> task is still sleeping. 

And this is still true. place_entity() assumes 'absolute vruntime', no
matter how it got there. If we went to relative/lag, someone needs to go
back to 'absolute'.

> However, after recent commit ( 23ec30ddd7c1306:
> 'sched: add two functions for att(det)aching a task to(from) a cfs_rq'), the
> absolute vruntime will be calculated in switched_to_fair(),

Also, conditionally, to complement the other places.

> so the
> max_vruntime() which is called in place_entity() will select the absolute
> vruntime which is calculated in switched_to_fair() as the se->vruntime and
> lose the fair sleeper bonus.

You cannot loose your sleeper bonus by going to and from relative/lag
(or rather you can, but that's due to min_vruntime being a poor
substitute for the 0-lag point). But since we do this for all rq
transfers we should not make exemptions.


Afaict, the only possibly place for a bug to be here is
vruntime_normalized(), if that somehow gets the conditions wrong we
could fail-to/incorrectly subtract/add min_vruntime, creating a mess.