Re: [patch] cgroups: disallow attaching kthreadd

From: David Rientjes <rientjes@google.com>
To: Mike Galbraith <mgalbraith@suse.de>
Cc: Tejun Heo <tj@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Li Zefan <lizf@cn.fujitsu.com>, Paul Menage <paul@paulmenage.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [patch] cgroups: disallow attaching kthreadd
Date: Wed, 4 Apr 2012 04:29:09 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.00.1204040419450.23137@chino.kir.corp.google.com> (raw)
In-Reply-To: <1333535915.7188.18.camel@marge.simpson.net>

On Wed, 4 Apr 2012, Mike Galbraith wrote:

> > We've been through this several times now iterating between two different 
> > functional changes.  I appreciate the persistence, but please, again, 
> > explain why you are doing this at the cgroups level rather than the 
> > cpusets level?
> > 
> > The last time we discussed this, you had proposed a patch to only do this 
> > for cpusets after the points I'm about to bring up for the fifth time.  
> > Peter ended up not responding and as I remember it didn't have strong 
> > feelings against doing it only for cpusets.  And here we are, yet again, 
> > back to the cgroups version.
> 
> Suggest a third version.
> 

I've already acked your kernel/cpuset.c version as an extension of 
6d7b2f5f9e88 ("cpusets: prevent PF_THREAD_BOUND tasks from attaching 
to non-root cpusets") which took care of PF_THREAD_BOUND attachment back 
in 2.6.30.

The undeniable fact is that PF_THREAD_BOUND is special for cpusets since 
we can't possibly allow its cpu affinity to change after kthread_bind() 
and we don't want inconsistencies in our cpusets whereas threads attached 
to a cpuset can have a disjoint set of allowable nodes.  Do we special 
case PF_THREAD_BOUND for any other cgroup?  No.  And that's what you're 
trying to introduce here and it's completely unnecessary.

> Ok, so you NAK this way, Peter NAKS the other way, and the bug lives on
> forever.  So be it.
> 

I've never seen a nack from Peter on this, I only remember discussing 
whether this needs to be isolated to only cpusets or whether it needs to 
be a generic cgroup thing and I've always argued in favor of localizing it 
to cpusets because that cgroup happens to care about cpu affinity where 
others don't and this is why cgroups have ->can_attach() functions.  If a 
cgroup were created to have nothing to do with cpu affinity (only for 
collecting statistics for threads within it, for example), there's 
absolutely no reason why we need to exclude kthreadd.

You know I've been very supportive of getting this fix included for 
cpusets in the past and I very much appreciate your time and patience in 
the review cycle.  I'm hoping we can finally do this.