Messages in this thread | | | Date | Tue, 8 Sep 2015 11:49:42 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] sched: fix lose fair sleeper bonus in switch_to_fair() |
| |
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.
| |