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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151013120721.GP11639@twins.programming.kicks-ass.net>
Date:	Tue, 13 Oct 2015 14:07:21 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Byungchul Park <byungchul.park@....com>
Cc:	mingo@...nel.org, linux-kernel@...r.kernel.org,
	yuyang.du@...el.com, pjt@...gle.com, efault@....de,
	tglx@...utronix.de
Subject: Re: [PATCH 1/2] sched: make fair sched class can handle the cgroup
 change by other class

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@....com wrote:
> > > > From: Byungchul Park <byungchul.park@....com>
> > > > 
> > > > 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/