Re: [PATCH 1/2] sched: make fair sched class can handle the cgroup change by other class

[Byungchul Park]

On Tue, Oct 13, 2015 at 11:06:54AM +0200, Peter Zijlstra wrote:
> On Mon, Oct 05, 2015 at 06:16:23PM +0900, byungchul.park@xxxxxxx wrote:
> > From: Byungchul Park <byungchul.park@xxxxxxx>
> > 
> > Original fair sched class can handle the cgroup change occured within its
> > class with task_move_group_fair(), but there is no way to know it if the
> > change happened outside. This patch makes the fair sched class can handle
> > the change of cgroup which happened even at other sched class.
> > 
> > Additionally, it makes sched_move_task() more flexable so that any other
> > sched class can add task_move_group_xx() callback easily in future when
> > it is needed.
> 
> I don't get the problem... when !fair, set_task_rq() will do what needs
> doing.

set_task_rq() changes se's cfs_rq properly.

> 
> The only reason we need task_move_group_fair() is the extra accounting
> required when we actually _are_ of the fair class, it needs to
> unaccount, move and reaccount.

i agree with you mostly. but let's consider following sequence.

1. switch se's class from fair to rt
2. change se's group within the rt class
3. switch se's class back to fair

now, se->avg.last_update_time has a wrong value which is not synced with
the current cfs_rq yet before calling attach_entity_load_avg(). so
ATTACH_AGE_LOAD won't work expectedly. to be honest with you, no problem
if we disable ATTACH_AGE_LOAD. but i think ATTACH_AGE_LOAD is a valuable
feature, so i hope this patch will be added so that the ATTACH_AGE_LOAD
feature works properly.

this patch can add a very small overhead when changing se's group, but
i think that kind of small overhead is reasonable because those events
hardly occure. in addition, please consider similar kind of problem
solved in 2/2 patch. migration in the rt class also causes same problem.
i also considered flexability of code for adding each sched class's
callback functions when it needs in future.

IMHO, it is clear that these 2 patches makes ATTACH_AGE_LOAD work more
properly. but if you think the overhead introduced by these patches
is not reasonable, please let me know. i will follow your decision.

> 
> If we're not fair, the whole switched_from/to stuff should do that for
> us, no?
> 
> So please explain the problem.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/