cgroups: disallow attaching kthreadd

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.

---
 kernel/cgroup.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

@@ -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();
 

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

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

Ooh, and please use Li's new address - [email protected].

Thanks.

tejun

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.

Suggest a third version.

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

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.

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.

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.

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

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

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.

(replaces bouncy CC)

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.

---
 kernel/cgroup.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

@@ -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();
 

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.

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

[+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?

Hm, I thought I did that.

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

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

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:

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?

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

Rebased and separated patches proposed by Mike Galbraith <[email protected]> in https://lkml.org/lkml/2012/1/7/17.

From: Mike Galbraith <[email protected]>

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.

---
 kernel/cpuset.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

@@ -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;

From: Mike Galbraith <[email protected]>

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.

---
 kernel/sched/core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

@@ -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;
 }

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

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.

Hello, David.

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

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?

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

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

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.

Hello, Li.

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

Thanks.

tejun

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.

Hello, David.

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.

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

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

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.

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.

Hello,

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

Thanks.

tejun

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.

Hey, David.

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

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.

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.

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.

Hello, David.

"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

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.

2012年4月7日5:49 David Rientjes <[email protected]>:

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

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

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.

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.

Hello,

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

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.

What about for workqueues?

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.

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

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

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

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

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.

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.

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

Thanks.

tejun

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

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.

---
 kernel/cgroup.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

@@ -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();
 

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

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.

---
 kernel/cgroup.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

@@ -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();
 

Applied to cgroup/for-3.5. Thanks.

tejun