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

[Peter Zijlstra]

On Tue, Oct 13, 2015 at 02:04:38PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 13, 2015 at 08:26:45PM +0900, Byungchul Park wrote:
> > 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.
> 
> Ah, see details like that make or break a Changelog, since you've
> clearly thought about it, you might as well write it down and safe me
> the trouble of trying to puzzle it out on me own ;-)
> 
> OK, now that I understand the problem, let me consider it a bit.

So the problem I have with your approach is that I would prefer to
isolate the classes as much as possible. If its not currently of a
class, we should not call into it.

Now, there's a few exceptions to that already, but I would really prefer
not to make it worse.
--
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/