[patch] cgroups: disallow attaching kthreadd

[patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

The hazzards of moving kthreadd into a non-root cgroup is still present
in mainline.  Last go 'round stalled with Peter not liking the
cpuset,cpu per controller specific exclusion.  I agree that total
exclusion is the better option, and below is a respin doing that.

cgroups: disallow attaching kthreadd

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
---
 kernel/cgroup.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -1894,6 +1895,14 @@ int cgroup_attach_task(struct cgroup *cg
 	if (tsk->flags & PF_EXITING)
 		return -ESRCH;
 
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task)
+		return -EINVAL;
+
 	/* Nothing to do if the task is already in that cgroup */
 	oldcgrp = task_cgroup_from_root(tsk, root);
 	if (cgrp == oldcgrp)
@@ -2172,6 +2181,17 @@ static int attach_task_by_pid(struct cgr
 
 	if (threadgroup)
 		tsk = tsk->group_leader;
+
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task) {
+		ret = -EINVAL;
+		goto out_unlock_cgroup;
+	}
+
 	get_task_struct(tsk);
 	rcu_read_unlock();
 

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Tue, Apr 03, 2012 at 07:58:26PM +0200, Mike Galbraith wrote:
> @@ -1894,6 +1895,14 @@ int cgroup_attach_task(struct cgroup *cg
>  	if (tsk->flags & PF_EXITING)
>  		return -ESRCH;
>  
> +	/*
> +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> +	 * with no rt_runtime allocated.  Just say no.
> +	 */
> +	if (tsk == kthreadd_task)
> +		return -EINVAL;
> +
>  	/* Nothing to do if the task is already in that cgroup */
>  	oldcgrp = task_cgroup_from_root(tsk, root);
>  	if (cgrp == oldcgrp)
> @@ -2172,6 +2181,17 @@ static int attach_task_by_pid(struct cgr
>  
>  	if (threadgroup)
>  		tsk = tsk->group_leader;
> +
> +	/*
> +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> +	 * with no rt_runtime allocated.  Just say no.
> +	 */
> +	if (tsk == kthreadd_task) {
> +		ret = -EINVAL;
> +		goto out_unlock_cgroup;
> +	}

If we have this test here, do we need the same check in
cgroup_attach_task()/

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Tue, 2012-04-03 at 11:49 -0700, Tejun Heo wrote: 
> On Tue, Apr 03, 2012 at 07:58:26PM +0200, Mike Galbraith wrote:
> > @@ -1894,6 +1895,14 @@ int cgroup_attach_task(struct cgroup *cg
> >  	if (tsk->flags & PF_EXITING)
> >  		return -ESRCH;
> >  
> > +	/*
> > +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> > +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> > +	 * with no rt_runtime allocated.  Just say no.
> > +	 */
> > +	if (tsk == kthreadd_task)
> > +		return -EINVAL;
> > +
> >  	/* Nothing to do if the task is already in that cgroup */
> >  	oldcgrp = task_cgroup_from_root(tsk, root);
> >  	if (cgrp == oldcgrp)
> > @@ -2172,6 +2181,17 @@ static int attach_task_by_pid(struct cgr
> >  
> >  	if (threadgroup)
> >  		tsk = tsk->group_leader;
> > +
> > +	/*
> > +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> > +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> > +	 * with no rt_runtime allocated.  Just say no.
> > +	 */
> > +	if (tsk == kthreadd_task) {
> > +		ret = -EINVAL;
> > +		goto out_unlock_cgroup;
> > +	}
> 
> If we have this test here, do we need the same check in
> cgroup_attach_task()/

It looked to me like yes, because cgroup_attach_task() can be called
directly.  For the problem at hand, user script doing bad things, no,
it's not needed.

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Tue, 3 Apr 2012, Mike Galbraith wrote:

> The hazzards of moving kthreadd into a non-root cgroup is still present
> in mainline.  Last go 'round stalled with Peter not liking the
> cpuset,cpu per controller specific exclusion.  I agree that total
> exclusion is the better option, and below is a respin doing that.
> 

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.

There's _nothing_ wrong with attaching a kthread to most cgroups.  We do 
it for memcg.  And now you're trying to break it for ABSOLUTELY NO REASON.

Both points you mention in your changelog below have to do with bad 
_cpuset_ behavior, not _cgroup_ behavior.

So nack in this form, but I'm pretty sure you already knew that.

> cgroups: disallow attaching kthreadd
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2012-04-04 at 00:15 -0700, David Rientjes wrote: 
> On Tue, 3 Apr 2012, Mike Galbraith wrote:
> 
> > The hazzards of moving kthreadd into a non-root cgroup is still present
> > in mainline.  Last go 'round stalled with Peter not liking the
> > cpuset,cpu per controller specific exclusion.  I agree that total
> > exclusion is the better option, and below is a respin doing that.
> > 
> 
> 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.

> There's _nothing_ wrong with attaching a kthread to most cgroups.  We do 
> it for memcg.  And now you're trying to break it for ABSOLUTELY NO REASON.

Oh, caps made that so much more legible.

One, I don't see what it's breaking, and two, the reason for this repeat
is that the last attempt with cpuset,cpu exclusion did not fly.

I don't care how it gets fixed.  I just thought I should mention that
the problem is still alive upstream, did that, and was told I should try
this way again with CCs.

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

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

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.

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2012-04-04 at 04:29 -0700, David Rientjes wrote:

> 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.

Well, he didn't actually say "NAK", but was against it, which means
pretty much the same thing as NAK to me.. we can call it a nak.

> 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.

Whatever floats the boat works for me.  Peter likes generic, you like
targeted.  I like generic best too fwtw, but I don't get to make the
command decision, else you'd be saying revert, and I'd be saying NAK :)

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 4 Apr 2012, Mike Galbraith wrote:

> > 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.
> 
> Well, he didn't actually say "NAK", but was against it, which means
> pretty much the same thing as NAK to me.. we can call it a nak.
> 

People are able to change their minds and after our discussion about 
cgroups vs cpusets, I was under the impression we were at a common 
understanding of the problem.

Cpusets are a cgroup.  It wouldn't make much sense to NACK a patch that 
does this to cpusets if you're arguing in favor of doing it for all 
cgroups.

Your changelog mentions only cpusets issues, not cgroup issues.

So please propose your cpusets version so that the issue can be fixed.  If 
Peter wants to extend that to cgroups later, that's a discussion we can 
have at that time.  However, the bug being addressed here is for cpusets 
and is deserving of a patch now rather than later.  I'm concerned we're 
just going to drop this again and it will live on.

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Wed, Apr 04, 2012 at 04:34:38AM +0200, Mike Galbraith wrote:
> It looked to me like yes, because cgroup_attach_task() can be called
> directly.  For the problem at hand, user script doing bad things, no,
> it's not needed.

Then that's a kernel bug.  We're just trying to protect userland from
shotting itself, so please just keep the one in the user-facing
interface.

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Wed, Apr 04, 2012 at 02:17:36PM -0700, David Rientjes wrote:
> On Wed, 4 Apr 2012, Mike Galbraith wrote:
> 
> > > 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.
> > 
> > Well, he didn't actually say "NAK", but was against it, which means
> > pretty much the same thing as NAK to me.. we can call it a nak.
> > 
> 
> People are able to change their minds and after our discussion about 
> cgroups vs cpusets, I was under the impression we were at a common 
> understanding of the problem.
> 
> Cpusets are a cgroup.  It wouldn't make much sense to NACK a patch that 
> does this to cpusets if you're arguing in favor of doing it for all 
> cgroups.
> 
> Your changelog mentions only cpusets issues, not cgroup issues.
> 
> So please propose your cpusets version so that the issue can be fixed.  If 
> Peter wants to extend that to cgroups later, that's a discussion we can 
> have at that time.  However, the bug being addressed here is for cpusets 
> and is deserving of a patch now rather than later.  I'm concerned we're 
> just going to drop this again and it will live on.

I don't see much problem with the proposed solution and am gonna take
it unless there are pretty good reasons not to.

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Wed, Apr 04, 2012 at 04:06:48PM -0700, Tejun Heo wrote:
> On Wed, Apr 04, 2012 at 04:34:38AM +0200, Mike Galbraith wrote:
> > It looked to me like yes, because cgroup_attach_task() can be called
> > directly.  For the problem at hand, user script doing bad things, no,
> > it's not needed.
> 
> Then that's a kernel bug.  We're just trying to protect userland from
> shotting itself, so please just keep the one in the user-facing
> interface.

Ooh, and please use Li's new address - lizefan@huawei.com.

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Wed, 4 Apr 2012, Tejun Heo wrote:

> I don't see much problem with the proposed solution and am gonna take
> it unless there are pretty good reasons not to.
> 

The only cgroup that cannot allow kthreadd is cpusets and that's 
illustrated by Mike's changelog.  Restricting it for all cgroups is 
completely unnecessary, as I've already said, and is the entire reason we 
have ->can_attach() functions for each cgroup.

Also see Paul Menage's suggestion for only isolating this fix to cpusets 
as well at 
http://lkml.indiana.edu/hypermail/linux/kernel/1110.2/01280.html.

Thanks.

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

(replaces bouncy CC)

On Wed, 2012-04-04 at 16:09 -0700, Tejun Heo wrote:

> I don't see much problem with the proposed solution and am gonna take
> it unless there are pretty good reasons not to.

Below is user interface only patchlet.  I find it cleaner than the
cpuset,cpu variant, but don't have strong feelings either way.

If this one don't fly, I'll just declare buglet immortal, he ain't worth
near the electrons he's already wasted ;-) 

cgroups: disallow attaching kthreadd 

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
---
 kernel/cgroup.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -2172,6 +2181,17 @@ static int attach_task_by_pid(struct cgr
 
 	if (threadgroup)
 		tsk = tsk->group_leader;
+
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task) {
+		ret = -EINVAL;
+		goto out_unlock_cgroup;
+	}
+
 	get_task_struct(tsk);
 	rcu_read_unlock();
 

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Mike Galbraith wrote:

> Below is user interface only patchlet.  I find it cleaner than the
> cpuset,cpu variant, but don't have strong feelings either way.
> 
> If this one don't fly, I'll just declare buglet immortal, he ain't worth
> near the electrons he's already wasted ;-) 
> 

Why can't you simply propose the cpusets version that both Paul and I 
liked?  There's _no_ reason to do this for every cgroup, current or 
future.

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2012-04-04 at 21:58 -0700, David Rientjes wrote: 
> On Thu, 5 Apr 2012, Mike Galbraith wrote:
> 
> > Below is user interface only patchlet.  I find it cleaner than the
> > cpuset,cpu variant, but don't have strong feelings either way.
> > 
> > If this one don't fly, I'll just declare buglet immortal, he ain't worth
> > near the electrons he's already wasted ;-) 
> > 
> 
> Why can't you simply propose the cpusets version that both Paul and I 
> liked?  There's _no_ reason to do this for every cgroup, current or 
> future.

I submitted that, and it didn't fly.  I like the generic exclusion
better, so I submit that for consideration.

>From my POV, the problem is known, decision makers can decide any way
they want, but _I_ have posted my last variant.  If this one doesn't
fly, too bad for patchlet, it's not the first one to ever go splat :)

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Mike Galbraith wrote:

> I submitted that, and it didn't fly.  I like the generic exclusion
> better, so I submit that for consideration.
> 

[+akpm]

The last time we went through this, it was left after Andrew had fixed it 
up when the cpusets version was merged in -mm without any disagreement 
from Peter who was cc'd and that version was acked both by myself and Paul 
Menage at https://lkml.org/lkml/2011/12/14/402.  Andrew dropped it and 
asked for a repost since there was some on-going scheduler work going on 
in linux-next that caused that version not to apply.  No follow-up was 
ever offered.

Why have we now gone in a completely different direction again?

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2012-04-04 at 23:07 -0700, David Rientjes wrote: 
> On Thu, 5 Apr 2012, Mike Galbraith wrote:
> 
> > I submitted that, and it didn't fly.  I like the generic exclusion
> > better, so I submit that for consideration.
> > 
> 
> [+akpm]
> 
> The last time we went through this, it was left after Andrew had fixed it 
> up when the cpusets version was merged in -mm without any disagreement 
> from Peter who was cc'd and that version was acked both by myself and Paul 
> Menage at https://lkml.org/lkml/2011/12/14/402.  Andrew dropped it and 
> asked for a repost since there was some on-going scheduler work going on 
> in linux-next that caused that version not to apply.  No follow-up was 
> ever offered.

Hm, I thought I did that.

> Why have we now gone in a completely different direction again?

I already said that after Peter griped and suggested global, I thought
about it, and liked that better.  The submitted patchlet can either fly
or die.  It's not a big deal.

If I need a reason to like global better, posix like: the user shall not
fool around with my "mom" vs a wishy washy comedy variant of same ;-)

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Mike Galbraith wrote:

> > The last time we went through this, it was left after Andrew had fixed it 
> > up when the cpusets version was merged in -mm without any disagreement 
> > from Peter who was cc'd and that version was acked both by myself and Paul 
> > Menage at https://lkml.org/lkml/2011/12/14/402.  Andrew dropped it and 
> > asked for a repost since there was some on-going scheduler work going on 
> > in linux-next that caused that version not to apply.  No follow-up was 
> > ever offered.
> 
> Hm, I thought I did that.
> 

There's no reply from you to Andrew's email unless it was private.

> > Why have we now gone in a completely different direction again?
> 
> I already said that after Peter griped and suggested global, I thought
> about it, and liked that better.

I think you're taking Peter's questions as a nack.  He asked a question, I 
answered it.  He didn't participate in the thread after October 20.  
Andrew's email to you asking for a new version is December 14 with these 
lines:

Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>

He's been cc'd the whole time.  Looking through the lengthy emails, he 
never actually nack'd _any_ version of this patch.  He asked why not do it 
for all cgroups.  That's it.

If cpusets is a cgroup, why would he nack a patch that does it for cpusets 
that addresses a cpusets problem if he was asking to do it for _all_ 
cgroups including cpusets?

> The submitted patchlet can either fly
> or die.  It's not a big deal.
> 

I'm hoping you will take this bug more seriously.

[patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

Rebased and separated patches proposed by Mike Galbraith 
<mgalbraith@suse.de> in https://lkml.org/lkml/2012/1/7/17.

[patch 1/2] cpusets: disallow attaching kthreadd

[David Rientjes]

From: Mike Galbraith <efault@gmx.de>

A worker thread spawned by kthreadd may acquire PF_THREAD_BOUND and never
be able to leave a cpuset, rending the cpuset immortal.  Save the user
some unexpected trouble, just say no.

Acked-by: Paul Menage <paul@paulmenage.org>
Signed-off-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/cpuset.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -53,6 +53,7 @@
 #include <linux/time.h>
 #include <linux/backing-dev.h>
 #include <linux/sort.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <linux/atomic.h>
@@ -1389,9 +1390,11 @@ static int cpuset_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 		 * unnecessary.  Thus, cpusets are not applicable for such
 		 * threads.  This prevents checking for success of
 		 * set_cpus_allowed_ptr() on all attached tasks before
-		 * cpus_allowed may be changed.
+		 * cpus_allowed may be changed.  We also disallow attaching
+		 * kthreadd to prevent a child from becoming trapped should it
+		 * later acquire PF_THREAD_BOUND.
 		 */
-		if (task->flags & PF_THREAD_BOUND)
+		if (task->flags & PF_THREAD_BOUND || task == kthreadd_task)
 			return -EINVAL;
 		if ((ret = security_task_setscheduler(task)))
 			return ret;

[patch 2/2] cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

From: Mike Galbraith <efault@gmx.de>

An RT workqueue worker thread spawned in a cpu cgroup with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.
Save the user some unexpected trouble, just say no.

Acked-by: Paul Menage <paul@paulmenage.org>
Signed-off-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/sched/core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -72,6 +72,7 @@
 #include <linux/slab.h>
 #include <linux/init_task.h>
 #include <linux/binfmts.h>
+#include <linux/kthread.h>
 
 #include <asm/switch_to.h>
 #include <asm/tlb.h>
@@ -7636,6 +7637,13 @@ static int cpu_cgroup_can_attach(struct cgroup *cgrp,
 		if (task->sched_class != &fair_sched_class)
 			return -EINVAL;
 #endif
+		/*
+		 * Disallow kthreadd since it can fork workers for an RT
+		 * workqueue in a cgroup which may or may not have rt_runtime
+		 * allocated.
+		 */
+		if (task == kthreadd_task)
+			return -EINVAL;
 	}
 	return 0;
 }

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Wed, 2012-04-04 at 23:49 -0700, David Rientjes wrote: 
> On Thu, 5 Apr 2012, Mike Galbraith wrote:
> 
> > > The last time we went through this, it was left after Andrew had fixed it 
> > > up when the cpusets version was merged in -mm without any disagreement 
> > > from Peter who was cc'd and that version was acked both by myself and Paul 
> > > Menage at https://lkml.org/lkml/2011/12/14/402.  Andrew dropped it and 
> > > asked for a repost since there was some on-going scheduler work going on 
> > > in linux-next that caused that version not to apply.  No follow-up was 
> > > ever offered.
> > 
> > Hm, I thought I did that.
> > 
> 
> There's no reply from you to Andrew's email unless it was private.

Maybe I didn't, busy as hell happens a lot these days.

> > > Why have we now gone in a completely different direction again?
> > 
> > I already said that after Peter griped and suggested global, I thought
> > about it, and liked that better.
> 
> I think you're taking Peter's questions as a nack.  He asked a question, I 
> answered it.  He didn't participate in the thread after October 20.  
> Andrew's email to you asking for a new version is December 14 with these 
> lines:
> 
> Acked-by: David Rientjes <rientjes@google.com>
> Acked-by: Paul Menage <paul@paulmenage.org>
> Cc: Tejun Heo <htejun@gmail.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> He's been cc'd the whole time.  Looking through the lengthy emails, he 
> never actually nack'd _any_ version of this patch.  He asked why not do it 
> for all cgroups.  That's it.

That question made perfect sense to me.  Putting a global resource in
any bin of any sort makes little if any sense.

> I'm hoping you will take this bug more seriously.

Look, I merely reminded Tehun that the problem had never been resolved.
I was asked to post, and did that.  Don't get upset with me for posting
the solution I like best.  If I weren't taking the bug seriously enough
(small as it is), I wouldn't have reminded Tehun.

I fixed my customers problems the global way months ago, could have and
likely should have ignored the fact that buglet lives on in mainline.

-Mike

Re: [patch] cgroups: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Mike Galbraith wrote:

> That question made perfect sense to me.  Putting a global resource in
> any bin of any sort makes little if any sense.
> 

This has nothing to do with global resources, it has to do with 
PF_THREAD_BOUND since those threads cannot move amongst these cgroups.

For cgroups that do not have anything to do with cpu affinity, 
PF_THREAD_BOUND means nothing to their attachment rules.  This patch is 
addressing kthreadd since it, as the changelog says, has the ability to 
spawn threads that become PF_THREAD_BOUND.  By logic, we then can't allow 
kthreadd to be attached to cgroups where PF_THREAD_BOUND matters either.

I know we've been through several iterations on this over the past several 
months and the patch getting dropped a couple times along the way, but I 
really do appreciate your time and effort spent in developing this fix, 
it's very helpful.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

On Thu, Apr 05, 2012 at 12:14:48AM -0700, David Rientjes wrote:
> Rebased and separated patches proposed by Mike Galbraith 
> <mgalbraith@suse.de> in https://lkml.org/lkml/2012/1/7/17.

Umm... David, I'm with Peter on this one and don't want kthreadd to go
anywhere other than the root cgroup.  If you know of a valid use case,
please bring one up; otherwise, I'm applying Mike's patch.

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Thu, Apr 05, 2012 at 06:47:11AM +0200, Mike Galbraith wrote:
> cgroups: disallow attaching kthreadd 
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <mgalbraith@suse.de>

Li, I'm gonna route this through for-3.4-fixes.  Can you please ack
it?

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Tejun Heo wrote:

> > Rebased and separated patches proposed by Mike Galbraith 
> > <mgalbraith@suse.de> in https://lkml.org/lkml/2012/1/7/17.
> 
> Umm... David, I'm with Peter on this one and don't want kthreadd to go
> anywhere other than the root cgroup.  If you know of a valid use case,
> please bring one up; otherwise, I'm applying Mike's patch.
> 

Yeah, I know a valid use case, and I'm surprised you don't.

Google is moving in a direction where nothing will be assigned to the root 
memcg.  We already have a seperate memcg for accounting of memory 
allocated by the kernel, i.e. memory not accounted toward any user job.  
We will be doing this more aggressively in the future once we have setup 
memcg hierarchies to differentiate the memory usage of the kernel 
including workqueues created by kthreadd and have complete coverage of 
memory accounting such as slab and memory allocated directly from 
__get_free_pages().  We don't want anything in the root memcg itself.

It's also possible to attach kthreadd to the cpuacct cgroup for similar 
accounting.  The idea is that not all cgroups impose limits, either for 
memcg (where memory.limit_in_bytes == ULONG_MAX) or cpuacct, but rather 
purely for accounting.

For cpusets and the cpu cgroup, I completely agree with disallowing 
kthreadd and that's why I've passed along these patches.  However, it's 
not necessary (or even preferred for our usecase) to restrict kthreadd 
from being attached to all cgroups.  Yes, it's a global resource.  We want 
to account for the memory of that global resource.

Thanks.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello, David.

On Thu, Apr 05, 2012 at 02:26:34PM -0700, David Rientjes wrote:
> Yeah, I know a valid use case, and I'm surprised you don't.

Yeah, I'm pretty good at surprising people that way.

> Google is moving in a direction where nothing will be assigned to the root 
> memcg.  We already have a seperate memcg for accounting of memory 
> allocated by the kernel, i.e. memory not accounted toward any user job.  
> We will be doing this more aggressively in the future once we have setup 
> memcg hierarchies to differentiate the memory usage of the kernel 
> including workqueues created by kthreadd and have complete coverage of 
> memory accounting such as slab and memory allocated directly from 
> __get_free_pages().  We don't want anything in the root memcg itself.

Ambitious and I'm not even sure that's even possible without fallback
default group.  There just are things which are system-wide.  What's
wrong with using root cgroup for that?

> It's also possible to attach kthreadd to the cpuacct cgroup for similar 
> accounting.  The idea is that not all cgroups impose limits, either for 
> memcg (where memory.limit_in_bytes == ULONG_MAX) or cpuacct, but rather 
> purely for accounting.
> 
> For cpusets and the cpu cgroup, I completely agree with disallowing 
> kthreadd and that's why I've passed along these patches.  However, it's 
> not necessary (or even preferred for our usecase) to restrict kthreadd 
> from being attached to all cgroups.  Yes, it's a global resource.  We want 
> to account for the memory of that global resource.

Unfortunately, mainline cgroup is moving towards single hierarchy -
well, that's the plan anyway - and in that light ->can_attach() is
essentially broken and will thus be grdually phased out.  From the
look of the current users, I don't think it's gonna be hard.  cpu
cgroup would have to learn to ignore tasks with special scheduling
class and the blkcg silliness needs to go away but that seems to be
it.

So, hmmm, how about just using root cgroup for the fallback?

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

On Thu, Apr 05, 2012 at 02:37:04PM -0700, Tejun Heo wrote:
> look of the current users, I don't think it's gonna be hard.  cpu
> cgroup would have to learn to ignore tasks with special scheduling
> class and the blkcg silliness needs to go away but that seems to be
> it.

Ooh, right, cgroup-freezer needs to learn how to move tasks across
cgroups in different states but that has been in the plan for some
time now.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Tejun Heo wrote:

> Ambitious and I'm not even sure that's even possible without fallback
> default group.  There just are things which are system-wide.  What's
> wrong with using root cgroup for that?
> 

Yes, they act system-wide but that doesn't mean their memory usage or cpu 
need to be accounted together.  The key is that all cgroups, current or 
future, aren't necessarily for limiting those system-wide resources but 
rather can provide useful insight into their cost by monitoring either 
their memory or cpu through these two cgroups.

kthreadd certainly is not the only system-wide resource on the kernel; so 
why are you not arguing that all PF_KTHREAD threads not be allowed into 
non-root cgroups?  We actually have many kthreads in a memcg that _is_ 
limited to a specific amount of memory together with other system daemons 
that are killable if it becomes oom.

The reason we're discussing kthreadd here is because it has the funny 
ability to fork other kthreads that become PF_THREAD_BOUND.  Usually the 
only PF_THREAD_BOUND threads are ones created at boot.  However, if a 
kthread is started for a specific cpu, it gets this bit set to stay on 
that cpu via kthread_bind().  That's what causes an inconsistency for only 
two types of cgroups: cpusets and cpu and we don't allow them to move 
amongst them because of that.

That's what this patch series addresses.  Please don't unnecessarily limit 
our ability with kthreadd or kthreads in general for accounting of system 
activity.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello, David.

On Thu, Apr 05, 2012 at 03:03:06PM -0700, David Rientjes wrote:
> Yes, they act system-wide but that doesn't mean their memory usage or cpu 
> need to be accounted together.  The key is that all cgroups, current or 
> future, aren't necessarily for limiting those system-wide resources but 
> rather can provide useful insight into their cost by monitoring either 
> their memory or cpu through these two cgroups.
> 
> kthreadd certainly is not the only system-wide resource on the kernel; so 
> why are you not arguing that all PF_KTHREAD threads not be allowed into 
> non-root cgroups?  We actually have many kthreads in a memcg that _is_ 
> limited to a specific amount of memory together with other system daemons 
> that are killable if it becomes oom.
> 
> The reason we're discussing kthreadd here is because it has the funny 
> ability to fork other kthreads that become PF_THREAD_BOUND.  Usually the 
> only PF_THREAD_BOUND threads are ones created at boot.  However, if a 
> kthread is started for a specific cpu, it gets this bit set to stay on 
> that cpu via kthread_bind().  That's what causes an inconsistency for only 
> two types of cgroups: cpusets and cpu and we don't allow them to move 
> amongst them because of that.

That and because ramifications of putting kthreadd is difficult to
predict it being the root of all kthreads.  It's fine to be able to
put specific kthreads into cgroups if doing such makes sense for that
type of kthreads.

> That's what this patch series addresses.  Please don't unnecessarily limit 
> our ability with kthreadd or kthreads in general for accounting of system 
> activity.

I can see your point but the problem is that it conflicts with the
long term direction cgroup is taking and that cgroup seems generally
over-engineered to allow too many things which aren't too necessary to
the point where it's a giant pain in the ass for the subsystems and
people involved, so I'm far more likely to go for chopping down and
restricting stuff if it's not strictly necessary.  Somethings are just
stupid to allow and constrict future development and maintenance
unnecessarily which tends to be more important than supporting some
acrobatic use cases.

It's not like we have concrete plan regarding single hierarchy thing
at the moment, so it could be possible to support the use case you're
describing but please bear in mind that such use case might not be of
high priority.  Hmm... I'll think more about it.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

On Thu, Apr 05, 2012 at 03:24:00PM -0700, Tejun Heo wrote:
> at the moment, so it could be possible to support the use case you're
> describing but please bear in mind that such use case might not be of
> high priority.  Hmm... I'll think more about it.

To clarify a bit.  By "such use case", I'm referring to putting
kthreadd into !root cgroup or trying to eliminate (as opposed to
minimizing to a reasonable level, which IMHO should be possible w/o
messing with kthreadd) resource usage from root cgroup.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Tejun Heo wrote:

> > The reason we're discussing kthreadd here is because it has the funny 
> > ability to fork other kthreads that become PF_THREAD_BOUND.  Usually the 
> > only PF_THREAD_BOUND threads are ones created at boot.  However, if a 
> > kthread is started for a specific cpu, it gets this bit set to stay on 
> > that cpu via kthread_bind().  That's what causes an inconsistency for only 
> > two types of cgroups: cpusets and cpu and we don't allow them to move 
> > amongst them because of that.
> 
> That and because ramifications of putting kthreadd is difficult to
> predict it being the root of all kthreads.  It's fine to be able to
> put specific kthreads into cgroups if doing such makes sense for that
> type of kthreads.
> 

But it's not fine to put kthreadd in a cgroup to monitor cpu or memory of 
workqueues?  We're using the forking property of attaching all children to 
the same cgroup as its parent.  Modules calling daemonize() get reparented 
to kthreadd but are not moved amongst cgroups in which they were created, 
why wouldn't they be attached to the root cgroup too if you're going to 
mandate this for all cgroups?  We need no cpuset or cpu consideration here 
because daemonize() does not adjust the cpu affinity of the user thread.

> > That's what this patch series addresses.  Please don't unnecessarily limit 
> > our ability with kthreadd or kthreads in general for accounting of system 
> > activity.
> 
> I can see your point but the problem is that it conflicts with the
> long term direction cgroup is taking and that cgroup seems generally
> over-engineered to allow too many things which aren't too necessary to
> the point where it's a giant pain in the ass for the subsystems and
> people involved, so I'm far more likely to go for chopping down and
> restricting stuff if it's not strictly necessary.

How does kthreadd's placement conflict with the long-term direction of 
cgroups?  We don't need to have a hierarchy for monitoring, we can easily 
iterate over all memcgs or cpu cgroups that house kernel threads or other 
daemons and add their memory.usage_in_bytes or cpuacct.usage if we want 
the aggregate.  The point is that we want to monitor the memory and cpu of 
a set of tasks and cgroups is naturally the perfect interface for doing 
this and already has cgroups such as memcg and cpuacct that allow it.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello,

On Thu, Apr 05, 2012 at 03:55:11PM -0700, David Rientjes wrote:
> > I can see your point but the problem is that it conflicts with the
> > long term direction cgroup is taking and that cgroup seems generally
> > over-engineered to allow too many things which aren't too necessary to
> > the point where it's a giant pain in the ass for the subsystems and
> > people involved, so I'm far more likely to go for chopping down and
> > restricting stuff if it's not strictly necessary.
> 
> How does kthreadd's placement conflict with the long-term direction of 
> cgroups?

Because there's only single hierarchy, random subsystem saying no on
->can_attach() doesn't work.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Tejun Heo wrote:

> > > I can see your point but the problem is that it conflicts with the
> > > long term direction cgroup is taking and that cgroup seems generally
> > > over-engineered to allow too many things which aren't too necessary to
> > > the point where it's a giant pain in the ass for the subsystems and
> > > people involved, so I'm far more likely to go for chopping down and
> > > restricting stuff if it's not strictly necessary.
> > 
> > How does kthreadd's placement conflict with the long-term direction of 
> > cgroups?
> 
> Because there's only single hierarchy, random subsystem saying no on
> ->can_attach() doesn't work.
> 

The cgroups of interest here, cpusets and cpu, which have restrictions for 
PF_THREAD_BOUND threads is disjoint from memcg and cpuacct, which I'm 
saying it's useful for.  There's no requirement to mount them all.  But if 
you do try to do memory accounting in the presence of cpusets, though, it 
should be restricted.

We could still move to a single hierarchy, though, if we introduced a 
change to allow PF_THREAD_BOUND to attach to both cpusets and cpu iff 
its bound cpu is allowed and then do -EINVAL if its changed while still 
attached.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hey, David.

On Thu, Apr 05, 2012 at 04:05:43PM -0700, David Rientjes wrote:
> The cgroups of interest here, cpusets and cpu, which have restrictions for 
> PF_THREAD_BOUND threads is disjoint from memcg and cpuacct, which I'm 
> saying it's useful for.  There's no requirement to mount them all.  But if 
> you do try to do memory accounting in the presence of cpusets, though, it 
> should be restricted.
> 
> We could still move to a single hierarchy, though, if we introduced a 
> change to allow PF_THREAD_BOUND to attach to both cpusets and cpu iff 
> its bound cpu is allowed and then do -EINVAL if its changed while still 
> attached.

Yeah, making the affected controllers to deal with them somehow
definitely is an option and will probably have to be chosen for some
other cases too (e.g. rt tasks).  I still feel lukewarm about the
ability to put kthreadd to !root cgroup tho.  I mean, for any kind of
sane accounting, forked kthreads would have to be further classified
and where kthreadd lives and fork happens wouldn't matter all that
much.  It feels like we're fussing over stuff which isn't that
significant either way.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Thu, 5 Apr 2012, Tejun Heo wrote:

> > We could still move to a single hierarchy, though, if we introduced a 
> > change to allow PF_THREAD_BOUND to attach to both cpusets and cpu iff 
> > its bound cpu is allowed and then do -EINVAL if its changed while still 
> > attached.
> 
> Yeah, making the affected controllers to deal with them somehow
> definitely is an option and will probably have to be chosen for some
> other cases too (e.g. rt tasks).

For cpusets, it's easy if update_nodemask() iterates over threads and 
finds if any PF_THREAD_BOUND thread is bound to a cpu that is disallowed 
by the new nodemask.

It's a very typical configuration to have threads within a cpuset attached 
to a set of mems to use sched_setaffinity() for a subset of those cpus as 
well.  A PF_THREAD_BOUND thread is then perfectly legitimate in a cpuset 
where its cpu intersects with the set of mems but with the above 
restriction that update_nodemask() fails with -EINVAL if its changed to be 
disjoint.

> I still feel lukewarm about the
> ability to put kthreadd to !root cgroup tho.  I mean, for any kind of
> sane accounting, forked kthreads would have to be further classified
> and where kthreadd lives and fork happens wouldn't matter all that
> much.

Forked kthreads may not have to be further classified if the memcg cgroup, 
for example, is mounted after boot and you want to track the memory usage 
of all workqueues or kmem incurred by certain syscalls that don't get 
attributed to the thread calling the syscall because the memory allocation 
is done by a kthread.

The problem is that I'm not in a position to say what everybodys use-case 
is or isn't so I'm biased toward unnecessarily restricting it unless 
absolutely necessary (for cpusets and cpu _until_ we move in the direction 
where update_nodemask(), for example, can fail when PF_THREAD_BOUND 
threads are attached).

I'd also hate to see a cgroup come along in the future, perhaps for 
security where you can only access certain portions of a thread's state if 
they are in the same cgroup, where this will make even more sense beyond 
memcg and cpuacct and then we get into a discussion about why kthreadd is 
prohibited and need to recall this discussion that it's only really 
cpusets and cpu.

> It feels like we're fussing over stuff which isn't that
> significant either way.
> 

Well, I'm fussing over it because the patch being considered unnecessary 
requires that kthreadd can't be moved anywhere and I know one company is 
trying to move in a direction where nothing is in the root memcg.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Li Zefan]

Tejun Heo wrote:

> On Thu, Apr 05, 2012 at 02:37:04PM -0700, Tejun Heo wrote:
>> look of the current users, I don't think it's gonna be hard.  cpu
>> cgroup would have to learn to ignore tasks with special scheduling
>> class and the blkcg silliness needs to go away but that seems to be
>> it.
> 
> Ooh, right, cgroup-freezer needs to learn how to move tasks across
> cgroups in different states but that has been in the plan for some
> time now.
> 


What do you plan to do with cpusets? cpuset.cpus and cpuset.mems are
empty when a cgroup is created, and thus task attaching will fail.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello, David.

On Thu, Apr 05, 2012 at 04:40:06PM -0700, David Rientjes wrote:
> Well, I'm fussing over it because the patch being considered unnecessary 
> requires that kthreadd can't be moved anywhere and I know one company is 
> trying to move in a direction where nothing is in the root memcg.

"Nothing in the root memcg" can't be a goal in and of itself.  You
want that to achieve some functional goal and I'm trying to say this
specific kthreadd change doesn't necessarily affect the goal - better
accounting - all that much.  If root group is gonna be completely
empty otherwise, just combine information from it.  Even if that
doesn't work, assigning specific kthreads to appropriate cgroups after
the creation wouldn't be too far off.  I just don't see how relevant
it actually would be.

If we want all controlles to play by the same rules, which is
necessary for having a unified hierarchy, I wanna keep those rules
simple.  If bound kthreads in !root cgroups cause issues for some and
there aren't quite strong reasons to do otherwise, I would just
restrict them in the root.  It's not like those kthreads are
cgroup-aware in any form anyway.

I don't know.  Just proceed without kthreadd in the root.  If the
fallouts are big enough and can't be easily worked around, let's talk
then.

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello, Li.

On Fri, Apr 06, 2012 at 03:50:14PM +0800, Li Zefan wrote:
> What do you plan to do with cpusets? cpuset.cpus and cpuset.mems are
> empty when a cgroup is created, and thus task attaching will fail.

Not much.  Maybe just filp the default so that they don't apply any
limit on creation?

Thanks.

-- 
tejun

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Peter Zijlstra]

On Fri, 2012-04-06 at 08:52 -0700, Tejun Heo wrote:
> Hello, David.
> 
> On Thu, Apr 05, 2012 at 04:40:06PM -0700, David Rientjes wrote:
> > Well, I'm fussing over it because the patch being considered unnecessary 
> > requires that kthreadd can't be moved anywhere and I know one company is 
> > trying to move in a direction where nothing is in the root memcg.
> 
> "Nothing in the root memcg" can't be a goal in and of itself.  You
> want that to achieve some functional goal and I'm trying to say this
> specific kthreadd change doesn't necessarily affect the goal - better
> accounting - all that much.  If root group is gonna be completely
> empty otherwise, just combine information from it.  Even if that
> doesn't work, assigning specific kthreads to appropriate cgroups after
> the creation wouldn't be too far off.  I just don't see how relevant
> it actually would be.
> 
> If we want all controlles to play by the same rules, which is
> necessary for having a unified hierarchy, I wanna keep those rules
> simple.  If bound kthreads in !root cgroups cause issues for some and
> there aren't quite strong reasons to do otherwise, I would just
> restrict them in the root.  It's not like those kthreads are
> cgroup-aware in any form anyway.
> 
> I don't know.  Just proceed without kthreadd in the root.  If the
> fallouts are big enough and can't be easily worked around, let's talk
> then.

Furthermore, the whole point of kthreadd's existence is so that we could
create kthreads without context. Placing it in a cgroup will ensure all
subsequently created kthreads do have context (including possible idle
threads). This seems like a particularly bad idea.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Fri, 6 Apr 2012, Tejun Heo wrote:

> "Nothing in the root memcg" can't be a goal in and of itself.  You
> want that to achieve some functional goal and I'm trying to say this
> specific kthreadd change doesn't necessarily affect the goal - better
> accounting - all that much.  If root group is gonna be completely
> empty otherwise, just combine information from it.

That doesn't work, the kmem extension to memcg does not allow slab to be 
properly accounted to threads in the root memcg.  From 
Documentation/cgroups/memory.txt section 2.7:

"Kernel memory limits are not imposed for the root cgroup. Usage for the 
root cgroup may or may not be accounted."

Kernel memory, I'm sure we'll all agree, is particularly pertinent for 
kthreadd and would really be the most interesting reason to ever move it 
to a different memcg.  This is the use-case that I cited when I said we're 
moving in a direction to have nothing in the root memcg, for finer-grained 
accounting of kernel memory.

> Even if that
> doesn't work, assigning specific kthreads to appropriate cgroups after
> the creation wouldn't be too far off.  I just don't see how relevant
> it actually would be.
> 

What about for workqueues?

> If we want all controlles to play by the same rules, which is
> necessary for having a unified hierarchy, I wanna keep those rules
> simple.

That's a different goal, in my opinion.  We already talked about how we 
relax the restriction on PF_THREAD_BOUND for both cpusets and cpuacct by 
doing -EINVAL for changes to a cgroup that include such threads that are 
invalid (for cpusets, changing cpuset.mems to be different than the bound 
cpu; for cpu, force rt_runtime for attachment).  If we do that then we 
don't need any special handling of kthreadd either, which is the direction 
in which I thought this discussion was going.

In the interim, however, we need a restriction in place for kthreadd for 
both cgroups until such time as that is made a reality.  It's not like 
we're going to dictate everyone use a signal hierarchy in 3.4.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Fri, 6 Apr 2012, Peter Zijlstra wrote:

> Furthermore, the whole point of kthreadd's existence is so that we could
> create kthreads without context. Placing it in a cgroup will ensure all
> subsequently created kthreads do have context (including possible idle
> threads). This seems like a particularly bad idea.
> 

I don't see it as context if the only thing you're doing is accounting 
with memcg (for slab) or or cpu.  We're simply collecting statistics for a 
set of threads (possibly all kthreads, including kthreadd) and the best 
way to do this is leveraging the existing functionality of cgroups to 
setup the threads we want to collect for and the memcg kmem accounting is 
particularly a good indicator of kernel vs userjob-triggered slab 
allocation.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Hiroyuki Kamezawa]

2012年4月7日5:49 David Rientjes <rientjes@google.com>:
> On Fri, 6 Apr 2012, Peter Zijlstra wrote:
>
>> Furthermore, the whole point of kthreadd's existence is so that we could
>> create kthreads without context. Placing it in a cgroup will ensure all
>> subsequently created kthreads do have context (including possible idle
>> threads). This seems like a particularly bad idea.
>>
>
> I don't see it as context if the only thing you're doing is accounting
> with memcg (for slab) or or cpu.  We're simply collecting statistics for a
> set of threads (possibly all kthreads, including kthreadd) and the best
> way to do this is leveraging the existing functionality of cgroups to
> setup the threads we want to collect for and the memcg kmem accounting is
> particularly a good indicator of kernel vs userjob-triggered slab
> allocation.

I'm sorry if I didn't read e-mails while a trip....let me understand...

 - Tejun at el, tries to disallow to move kthreadd into cgroups other than root.
 - You wants to account kthreadd's activity under memg at el.

Then, 2 question from me is....
1. If this patch only affects kthreadd, you can move other
    kthread. Is this correct ?

I assume yes, if the created kthread itself has characteristics for
working for some specific users(as vhost) it can be moved to correct
cgroup by hand or by hook(vhost does this.)....do you need to move
kthreadd rather than each kthreads ?

2. You just want to see all resources usage. So, if memcg
    can show correct accounting in root cgroup, is it enough ?

IIRC, with current tcp mem accounting, root cgroup shows the all
system usage. Oh, it doesn't seem to be what you want.  I think this
can be fixed. Could you clarify your request against memcg ? Anyway,
kmem/slab accounting isn't implemented yet.

Thanks,
-Kame

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[David Rientjes]

On Sun, 8 Apr 2012, Hiroyuki Kamezawa wrote:

> I'm sorry if I didn't read e-mails while a trip....let me understand...
> 
>  - Tejun at el, tries to disallow to move kthreadd into cgroups other than root.
>  - You wants to account kthreadd's activity under memg at el.
> 

memcg isn't the only use case, you can also use it for cpuacct and any 
other future cgroup that wants to do so.

> Then, 2 question from me is....
> 1. If this patch only affects kthreadd, you can move other
>     kthread. Is this correct ?
> 

Yes, but not those that are forked by kthreadd at runtime without some 
delay between the fork and attaching it to a different cgroup.  With memcg 
slab accounting, for example, allocating the task_struct for the kthread 
should appropriately be accounted for in a non-root memcg.

Keep in mind that this isn't only limited to memcg, I've just used it to 
illustrate how account for slab is useful.  I don't know everybody's use 
cases where they may want to do the same thing so I've been arguing 
against unnecessarily restricting it from all cgroups other than those 
that require it (cpuset and cpu).

Using cpusets as an example, we can even completely remove the restriction 
on attaching all PF_THREAD_BOUND threads by not allowing them only if the 
destination cpuset has mems that are disjoint from the bound cpu and 
disallow changing the mems to be disjoint from a member's bound cpu.

So then we won't even need the restriction for some PF_THREAD_BOUND 
threads for cpusets.  It's not even a rare restriction: users can easily 
use sched_setaffinity() to restrict their scheduling to a subset of their 
set of allowed mems.

I haven't looked at the changes needed for the cpu cgroup, but I'm 
assuming it will be even easier: disallow attaching kthreadd without rt 
allocation.

Re: [patch] cgroups: disallow attaching kthreadd

[Peter Zijlstra]

On Wed, 2012-04-04 at 23:07 -0700, David Rientjes wrote:
> On Thu, 5 Apr 2012, Mike Galbraith wrote:
> 
> > I submitted that, and it didn't fly.  I like the generic exclusion
> > better, so I submit that for consideration.
> > 
> 
> [+akpm]
> 
> The last time we went through this, it was left after Andrew had fixed it 
> up when the cpusets version was merged in -mm without any disagreement 
> from Peter who was cc'd and that version was acked both by myself and Paul 
> Menage at https://lkml.org/lkml/2011/12/14/402.  Andrew dropped it and 
> asked for a repost since there was some on-going scheduler work going on 
> in linux-next that caused that version not to apply.  No follow-up was 
> ever offered.
> 
> Why have we now gone in a completely different direction again?

I really absolutely hate the cpuset only feature. The whole point of
kthreadd's existence is to provide a clean environment to spawn kthreads
from. Adding kthreadd to controllers violates this premise.

New and fresh kthreads should always get a bare minimum state, this
includes not being part of any particular cgroup.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Peter Zijlstra]

On Fri, 2012-04-06 at 13:46 -0700, David Rientjes wrote:
> That doesn't work, the kmem extension to memcg does not allow slab to be 
> properly accounted to threads in the root memcg.  From 
> Documentation/cgroups/memory.txt section 2.7:
> 
> "Kernel memory limits are not imposed for the root cgroup. Usage for the 
> root cgroup may or may not be accounted." 

To me that reads like: kmem/memcg is broken.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Peter Zijlstra]

On Mon, 2012-04-09 at 17:52 -0700, David Rientjes wrote:
> On Sun, 8 Apr 2012, Hiroyuki Kamezawa wrote:
> 
> > I'm sorry if I didn't read e-mails while a trip....let me understand...
> > 
> >  - Tejun at el, tries to disallow to move kthreadd into cgroups other than root.
> >  - You wants to account kthreadd's activity under memg at el.
> > 
> 
> memcg isn't the only use case, you can also use it for cpuacct and any 
> other future cgroup that wants to do so.

fwiw, it would be ever so nice to get rid of cpuacct -- its a horrendous
piece of crap.

Anyway, memcg is more than an bean counter, it can do actual
enforcement, so by allowing it outside the root group you allow it into
an enforced group, making it so that you cannot spawn more threads at
some point, which might mean making your kernel not work properly.

Just fix memcg already to account whatever you want for the root group.

Re: [patch 0/2] cpusets, cpu_cgroup: disallow attaching kthreadd

[Tejun Heo]

Hello,

On Mon, Apr 09, 2012 at 05:52:50PM -0700, David Rientjes wrote:
> On Sun, 8 Apr 2012, Hiroyuki Kamezawa wrote:
> 
> > I'm sorry if I didn't read e-mails while a trip....let me understand...
> > 
> >  - Tejun at el, tries to disallow to move kthreadd into cgroups other than root.
> >  - You wants to account kthreadd's activity under memg at el.
> > 
> 
> memcg isn't the only use case, you can also use it for cpuacct and any 
> other future cgroup that wants to do so.
> 
> > Then, 2 question from me is....
> > 1. If this patch only affects kthreadd, you can move other
> >     kthread. Is this correct ?
> > 
> 
> Yes, but not those that are forked by kthreadd at runtime without some 
> delay between the fork and attaching it to a different cgroup.  With memcg 
> slab accounting, for example, allocating the task_struct for the kthread 
> should appropriately be accounted for in a non-root memcg.
> 
> Keep in mind that this isn't only limited to memcg, I've just used it to 
> illustrate how account for slab is useful.  I don't know everybody's use 
> cases where they may want to do the same thing so I've been arguing 
> against unnecessarily restricting it from all cgroups other than those 
> that require it (cpuset and cpu).
> 
> Using cpusets as an example, we can even completely remove the restriction 
> on attaching all PF_THREAD_BOUND threads by not allowing them only if the 
> destination cpuset has mems that are disjoint from the bound cpu and 
> disallow changing the mems to be disjoint from a member's bound cpu.

So, the scheduler / arch people don't seem too happy with the prospect
of basic system kthreads living in !root cgroups and memcg people seem
willing to fix up whatever accounting deficiency in root memcg and I'm
not convinced the left use cases are all that useful / important.
There inherently are system-wide things (things happen w/o task
context and thus w/o any cgroup associated).  Just leave them in the
rootcg.  If you have to move kthreads to !root cgroups for accounting,
then do so on your own.  The only downsides are small delays and kmem
accounting inaccuracy from kthread allocation.

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

Hello, Mike.

This is dragging out too long but can you please update the patch so
that both kthreadd and PF_THREAD_BOUND tasks are disallowed from !root
cgroups?

Thanks.

-- 
tejun

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Fri, 2012-04-20 at 10:57 -0700, Tejun Heo wrote: 
> Hello, Mike.
> 
> This is dragging out too long but can you please update the patch so
> that both kthreadd and PF_THREAD_BOUND tasks are disallowed from !root
> cgroups?

cgroups: disallow attaching kthreadd or PF_THREAD_BOUND threads

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/cgroup.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -2172,6 +2173,17 @@ static int attach_task_by_pid(struct cgr
 
 	if (threadgroup)
 		tsk = tsk->group_leader;
+
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task || (tsk->flags & PF_THREAD_BOUND)) {
+		ret = -EINVAL;
+		goto out_unlock_cgroup;
+	}
+
 	get_task_struct(tsk);
 	rcu_read_unlock();
 

Re: [patch] cgroups: disallow attaching kthreadd

[Li Zefan]

Mike Galbraith wrote:

> On Fri, 2012-04-20 at 10:57 -0700, Tejun Heo wrote: 
>> Hello, Mike.
>>
>> This is dragging out too long but can you please update the patch so
>> that both kthreadd and PF_THREAD_BOUND tasks are disallowed from !root
>> cgroups?
> 
> cgroups: disallow attaching kthreadd or PF_THREAD_BOUND threads
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <mgalbraith@suse.de>

> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/cgroup.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -60,6 +60,7 @@
>  #include <linux/eventfd.h>
>  #include <linux/poll.h>
>  #include <linux/flex_array.h> /* used in cgroup_attach_proc */
> +#include <linux/kthread.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -2172,6 +2173,17 @@ static int attach_task_by_pid(struct cgr
>  
>  	if (threadgroup)
>  		tsk = tsk->group_leader;
> +
> +	/*
> +	 * Workqueue threads may acquire PF_THREAD_BOUND and become
> +	 * trapped in a cpuset, or RT worker may be born in a cgroup
> +	 * with no rt_runtime allocated.  Just say no.
> +	 */
> +	if (tsk == kthreadd_task || (tsk->flags & PF_THREAD_BOUND)) {
> +		ret = -EINVAL;
> +		goto out_unlock_cgroup;


forgot rcu_read_unlock() ?

Otherwise Acked-by: Li Zefan <lizefan@huawei.com>

> +	}
> +
>  	get_task_struct(tsk);
>  	rcu_read_unlock();

Re: [patch] cgroups: disallow attaching kthreadd

[Mike Galbraith]

On Sat, 2012-04-21 at 14:54 +0800, Li Zefan wrote:

> forgot rcu_read_unlock() ?

Eek, we don't need another one of those. Thanks.

> Otherwise Acked-by: Li Zefan <lizefan@huawei.com>

cgroups: disallow attaching kthreadd or PF_THREAD_BOUND threads

Allowing kthreadd to be moved to a non-root group makes no sense, it being
a global resource, and needlessly leads unsuspecting users toward trouble.

1. An RT workqueue worker thread spawned in a task group with no rt_runtime
allocated is not schedulable.  Simple user error, but harmful to the box.

2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
rendering the cpuset immortal.

Save the user some unexpected trouble, just say no.

Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/kthread.h>
 
 #include <linux/atomic.h>
 
@@ -2172,6 +2173,18 @@ static int attach_task_by_pid(struct cgr
 
 	if (threadgroup)
 		tsk = tsk->group_leader;
+
+	/*
+	 * Workqueue threads may acquire PF_THREAD_BOUND and become
+	 * trapped in a cpuset, or RT worker may be born in a cgroup
+	 * with no rt_runtime allocated.  Just say no.
+	 */
+	if (tsk == kthreadd_task || (tsk->flags & PF_THREAD_BOUND)) {
+		ret = -EINVAL;
+		rcu_read_unlock();
+		goto out_unlock_cgroup;
+	}
+
 	get_task_struct(tsk);
 	rcu_read_unlock();
 

Re: [patch] cgroups: disallow attaching kthreadd

[Tejun Heo]

On Sat, Apr 21, 2012 at 09:13:46AM +0200, Mike Galbraith wrote:
> On Sat, 2012-04-21 at 14:54 +0800, Li Zefan wrote:
> 
> > forgot rcu_read_unlock() ?
> 
> Eek, we don't need another one of those. Thanks.
> 
> > Otherwise Acked-by: Li Zefan <lizefan@huawei.com>
> 
> cgroups: disallow attaching kthreadd or PF_THREAD_BOUND threads
> 
> Allowing kthreadd to be moved to a non-root group makes no sense, it being
> a global resource, and needlessly leads unsuspecting users toward trouble.
> 
> 1. An RT workqueue worker thread spawned in a task group with no rt_runtime
> allocated is not schedulable.  Simple user error, but harmful to the box.
> 
> 2. A worker thread which acquires PF_THREAD_BOUND can never leave a cpuset,
> rendering the cpuset immortal.
> 
> Save the user some unexpected trouble, just say no.
> 
> Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Li Zefan <lizefan@huawei.com>

Applied to cgroup/for-3.5.  Thanks.

-- 
tejun