cgroup attach/fork hooks consistency with the ns_cgroup

PATCH Daniel Lezcano 4 participants 9 messages Jun 17, 2009 – Jun 19, 2009 7 messages not in the archive

Subsystems (4)

MM core Scheduler exec and binfmt cgroup

lore

Share & tools

lore

Daniel Lezcano

author

2009-06-17 15:35 UTC #0 lore raw

Hi,

I noticed two different behaviours, the second one looks weird for me:

1) when the cgroup is manually created:
mkdir /cgroup/foo
echo $$ > /cgroup/foo/tasks

only the "attach" callback is called as expected.

2) when the cgroup is automatically created via the ns_cgroup with the
clone function and the namespace flags,

the "attach" *and* the "fork" callbacks are called.

IMHO, these two different behaviours look inconsistent. Won't this lead to some problems or a specific code to handle both cases if a cgroup is using the fork and the attach hooks ?

For example, let's imagine we create a control group which shows the number of tasks running. We have a global atomic and we display its value in the cgroupfs.

When a task attaches to the cgroup, we do atomic_inc in the attach callback. For all its child, the fork hook will do atomic_inc and exit hook will do atomic_dec.

If we create the cgroup manually like the case 1) that works. But if we use the control group with the ns_cgroup the task counter will be set to 2 for the first tasks entering the cgroup because the attach callback will increment the counter and the fork callback will increment it again.

In attachment a source code to illustrate the example.

Shouldn't the ns_cgroup_clone be called after the cgroup_fork_callbacks in copy_process function ? So we don't call the fork callback for the first tasks and we keep the consistency ?

Thanks
-- Daniel

---
 include/linux/cgroup_subsys.h |    1 
 kernel/Makefile               |    2 
 kernel/cgroup_bug.c           |   87 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)

kernel/cgroup_bug.c new file +87 -0

Index: linux-2.6/kernel/cgroup_bug.c
===================================================================
@@ -0,0 +1,87 @@
+#include <linux/cgroup.h>
+#include <asm/atomic.h>
+
+struct bug_cg {
+	struct cgroup_subsys_state css;
+	atomic_t ntasks;
+};
+
+static inline struct bug_cg *bug_cgroup(struct cgroup *cgroup)
+{
+	return container_of(cgroup_subsys_state(cgroup, bug_subsys_id),
+			    struct bug_cg, css);
+}
+
+static struct cgroup_subsys_state *bug_cgroup_create(struct cgroup_subsys *ss,
+						     struct cgroup *cgroup)
+{
+	struct bug_cg *bug_cg;
+
+	bug_cg = kzalloc(sizeof(*bug_cg), GFP_KERNEL);
+	if (!bug_cg)
+		return ERR_PTR(-ENOMEM);
+
+	atomic_set(&bug_cg->ntasks, 0);
+
+	return &bug_cg->css;
+}
+
+static void bug_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup *cgroup)
+{
+	struct bug_cg *bug_cg = bug_cgroup(cgroup);
+	kfree(bug_cg);
+}
+
+static void bug_cgroup_attach(struct cgroup_subsys *ss, struct cgroup *cgroup,
+			      struct cgroup *old_cgroup,
+			      struct task_struct *task)
+{
+	struct bug_cg *bug_cg = bug_cgroup(cgroup);
+
+	atomic_inc(&bug_cg->ntasks);
+}
+
+static int bug_cgroup_fork(struct cgroup_subsys *ss, struct task_struct *task,
+			   unsigned long clone_flags)
+{
+	struct cgroup *cgroup = task_cgroup(task, bug_subsys_id);
+
+	atomic_inc(&bug_cgroup(cgroup)->ntasks);
+
+	return 0;
+}
+
+static void bug_cgroup_exit(struct cgroup_subsys *ss, struct task_struct *task)
+{
+	struct cgroup *cgroup = task_cgroup(task, bug_subsys_id);
+
+	atomic_dec(&bug_cgroup(cgroup)->ntasks);
+}
+
+static u64 task_count_read(struct cgroup *cgroup, struct cftype *cft)
+{
+	return atomic_read(&bug_cgroup(cgroup)->ntasks);
+}
+
+static struct cftype files[] = {
+	{
+		.name = "ntask",
+		.read_u64 = task_count_read,
+	},
+};
+
+static int bug_cgroup_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
+{
+	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
+}
+
+struct cgroup_subsys bug_subsys = {
+	.name		= "bug",
+	.subsys_id	= bug_subsys_id,
+	.create		= bug_cgroup_create,
+	.destroy	= bug_cgroup_destroy,
+	.populate	= bug_cgroup_populate,
+	.attach         = bug_cgroup_attach,
+	.fork		= bug_cgroup_fork,
+	.exit		= bug_cgroup_exit,
+};

include/linux/cgroup_subsys.h +1 -0

Index: linux-2.6/include/linux/cgroup_subsys.h
===================================================================
@@ -21,6 +21,7 @@ SUBSYS(debug)
 
 #ifdef CONFIG_CGROUP_NS
 SUBSYS(ns)
+SUBSYS(bug)
 #endif
 
 /* */

kernel/Makefile +1 -1

Index: linux-2.6/kernel/Makefile
===================================================================
@@ -60,7 +60,7 @@ obj-$(CONFIG_CGROUPS) += cgroup.o
 obj-$(CONFIG_CGROUP_DEBUG) += cgroup_debug.o
 obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
-obj-$(CONFIG_CGROUP_NS) += ns_cgroup.o
+obj-$(CONFIG_CGROUP_NS) += ns_cgroup.o cgroup_bug.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
 obj-$(CONFIG_PID_NS) += pid_namespace.o

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

2009-06-17 21:26 UTC #2 lore raw

Quoted 33 lines · Daniel Lezcano

Hi,
I noticed two different behaviours, the second one looks weird for me:
1) when the cgroup is manually created:
mkdir /cgroup/foo
echo $$ > /cgroup/foo/tasks
only the "attach" callback is called as expected.
2) when the cgroup is automatically created via the ns_cgroup with the
clone function and the namespace flags,
the "attach" *and* the "fork" callbacks are called.
IMHO, these two different behaviours look inconsistent. Won't this lead to some problems or a specific code to handle both cases if a cgroup is using the fork and the attach hooks ?
For example, let's imagine we create a control group which shows the number of tasks running. We have a global atomic and we display its value in the cgroupfs.
When a task attaches to the cgroup, we do atomic_inc in the attach callback. For all its child, the fork hook will do atomic_inc and exit hook will do atomic_dec.
If we create the cgroup manually like the case 1) that works. But if we use the control group with the ns_cgroup the task counter will be set to 2 for the first tasks entering the cgroup because the attach callback will increment the counter and the fork callback will increment it again.

Quoted 5 lines

In attachment a source code to illustrate the example.
Shouldn't the ns_cgroup_clone be called after the cgroup_fork_callbacks in copy_process function ? So we don't call the fork callback for the first tasks and we keep the consistency ?

The ns cgroup is really only good for preventing root in a container from escaping its cgroup-imposed limits. The same can be done today using smack or selinux, and eventually will be possible using user namespaces. Would anyone object to removing ns_cgroup?

It won't just remove kernel/ns_cgroup.c, but some subtle code in fork.c, nsproxy.c, and of course cgroup.c as well.

There admittedly is minute convenience gain in not having to manually create a new cgroup and attach a cloned child to it, but that wasn't the intent of the cgroup.

-serge

A message that is not in the archive (sent off-list or not yet received): 20090617212614.GA26781-r/[email protected]

Li Zefan

2009-06-18 01:21 UTC #4 lore raw

Quoted 5 lines

The ns cgroup is really only good for preventing root in a container from escaping its cgroup-imposed limits. The same can be done today using smack or selinux, and eventually will be possible using user namespaces. Would anyone object to removing ns_cgroup?

I vote for removing it. :)

Quoted 3 lines

It won't just remove kernel/ns_cgroup.c, but some subtle code in fork.c, nsproxy.c, and of course cgroup.c as well.

Yeah, regarding to cgroup, cgroup_clone() and cgroup_is_descendant() can be removed. cgroup_clone() is somewhat ugly I think.

Quoted 4 lines

There admittedly is minute convenience gain in not having to manually create a new cgroup and attach a cloned child to it, but that wasn't the intent of the cgroup.

Paul Menage

2009-06-18 01:21 UTC #5 lore raw

In reply to a message not in the archive

Quoted 5 lines · Serge E. Hallyn

The ns cgroup is really only good for preventing root in a container from escaping its cgroup-imposed limits. The same can be done today using smack or selinux, and eventually will be possible using user namespaces. Would anyone object to removing ns_cgroup?

Sounds reasonable to me. It feels to me that there ought to be some good way to integrate namespaces and cgroups, but I'm not quite sure exactly how, and ns_cgroup sort of hovers in the "toy" category rather than something very useful.

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830906171821v3c97f176y65bd4b7fa9a405e9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Serge E. Hallyn

2009-06-18 13:45 UTC #7 lore raw

Quoted 11 lines · Paul Menage

Quoted 5 lines · Serge E. Hallyn
The ns cgroup is really only good for preventing root in a container from escaping its cgroup-imposed limits. The same can be done today using smack or selinux, and eventually will be possible using user namespaces. Would anyone object to removing ns_cgroup?
Sounds reasonable to me. It feels to me that there ought to be some good way to integrate namespaces and cgroups, but I'm not quite sure exactly how, and ns_cgroup sort of hovers in the "toy" category rather than something very useful.

So the question becomes: does the presence of the ns cgroup constitute an API? Can we just yank it out?

Daniel, AFAIK liblxc is the only thing that actually uses it. Do you mind manually moving the container init into a new cgroup?

-serge

A message that is not in the archive (sent off-list or not yet received): 20090618134527.GA3186-r/[email protected]

Daniel Lezcano

author

2009-06-18 18:36 UTC #9 lore raw

Quoted 15 lines · Serge E. Hallyn

Quoted 7 lines · Paul Menage
Quoted 5 lines · Serge E. Hallyn
The ns cgroup is really only good for preventing root in a container from escaping its cgroup-imposed limits. The same can be done today using smack or selinux, and eventually will be possible using user namespaces. Would anyone object to removing ns_cgroup?
Quoted 5 lines
Sounds reasonable to me. It feels to me that there ought to be some good way to integrate namespaces and cgroups, but I'm not quite sure exactly how, and ns_cgroup sort of hovers in the "toy" category rather than something very useful.

Quoted 6 lines

So the question becomes: does the presence of the ns cgroup constitute an API? Can we just yank it out?
Daniel, AFAIK liblxc is the only thing that actually uses it. Do you mind manually moving the container init into a new cgroup?

It is not a big deal to manually create and attach to the cgroup and I would prefer to remove the ns_cgroup if that helps to keep the cgroup behaviour consistent.
But the behaviour will change, no ? I mean the ns_cgroup clones the cgroup parent so we inherit its values. If we remove the ns_cgroup, that won't happen and I will have to handle some specific cases. For example, cpuset.cpus and cpuset.mems won't be filled and I will not be able to attach until I set them, so I have to be aware of this control group in the code and copy the values from the parent. That's annoying, I tried to have the liblxc cgroup code generic so it supports any kind of cgroup :/

There isn't a rule saying that we will inherit the values set by the parent ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the same time, no ?

By the way, Andrew complained when we changed the name of the ns_cgroup "node_<pid>" by "<pid>" that we don't have to change the api when something is in the kernel. IMO, he won't be happy if we remove the ns_cgroup :/
Maybe, we can first fix the ns_cgroup hook problem by moving the ns_cgroup_clone after cgroup_fork_callbacks and propose a cgroup related to the namespaces. When we activate this new cgroup that disables the ns_cgroup and we put this cgroup_namespace as default in the Kconfig, no ?

Thanks
-- Daniel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Menage

2009-06-18 18:41 UTC #11 lore raw

Quoted 4 lines · Daniel Lezcano

There isn't a rule saying that we will inherit the values set by the parent ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the same time, no ?

There's no rule either way, but there is the backward-compatibility aspects of cpusets.

One way around that would be to add a "cgroup.clone_children" control file - if you write 1 to it (it defaults to 0) then all mkdir operations do a clone (i.e. pre-populate the child with appropriate defaults even if that's not the normal behaviour for the subsystem. That would avoid compatibility issues.

Quoted 2 lines

Maybe, we can first fix the ns_cgroup hook problem by moving the ns_cgroup_clone after cgroup_fork_callbacks

You mean fork the task into the parent cgroup, then clone the new
cgroup and reattach to the new cgroup? That could work, although I'm not sure whether it would be bad to have the new task briefly appearing in the parent cgroups.

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830906181141w1669d154j22277070ae221a76-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Daniel Lezcano

author

2009-06-18 20:08 UTC #13 lore raw

Quoted 7 lines · Paul Menage

Quoted 4 lines · Daniel Lezcano
There isn't a rule saying that we will inherit the values set by the parent ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the same time, no ?

Quoted 9 lines

There's no rule either way, but there is the backward-compatibility aspects of cpusets.
One way around that would be to add a "cgroup.clone_children" control file - if you write 1 to it (it defaults to 0) then all mkdir operations do a clone (i.e. pre-populate the child with appropriate defaults even if that's not the normal behaviour for the subsystem. That would avoid compatibility issues.

Yes, that sounds good.

Quoted 9 lines

Quoted 3 lines
Maybe, we can first fix the ns_cgroup hook problem by moving the ns_cgroup_clone after cgroup_fork_callbacks
You mean fork the task into the parent cgroup, then clone the new
cgroup and reattach to the new cgroup? That could work, although I'm not sure whether it would be bad to have the new task briefly appearing in the parent cgroups.

I tried the following patch:

kernel/fork.c +0 -6

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -1150,12 +1150,6 @@ static struct task_struct *copy_process(
        if (clone_flags & CLONE_THREAD)
                p->tgid = current->tgid;
 
-       if (current->nsproxy != p->nsproxy) {
-               retval = ns_cgroup_clone(p, pid);
-               if (retval)
-                       goto bad_fork_free_graph;
-       }
-
        p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ?

child_tidptr : NULL;
/*
* Clear TID on mm_release()?
@@ -1204,6 +1198,12 @@ static struct task_struct *copy_process(

        if (retval)
                goto bad_fork_free_graph;

+ if (current->nsproxy != p->nsproxy) {
+ retval = ns_cgroup_clone(p, pid);
+ if (retval)
+ goto bad_fork_cgroup_callbacks;
+ }
+

        /* Need tasklist lock for parent etc handling! */
        write_lock_irq(&tasklist_lock);

That seems to fix the inconsistency problem but maybe I am missing something...
Excepting kernel race condition in the copy_process function I may have missed, I don't see the difference with a program forking and adding the task in the child cgroup (or the child process adds itself right after the fork), the child will briefly appear to the parent cgroup, no ?

-- Daniel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

2009-06-19 13:59 UTC #15 lore raw

Quoted 23 lines · Daniel Lezcano

Quoted 7 lines · Paul Menage
Quoted 4 lines · Daniel Lezcano
There isn't a rule saying that we will inherit the values set by the parent ? If it is case, maybe we can remove the ns_cgroup and fix the cpuset at the same time, no ?
Quoted 9 lines
There's no rule either way, but there is the backward-compatibility aspects of cpusets.
One way around that would be to add a "cgroup.clone_children" control file - if you write 1 to it (it defaults to 0) then all mkdir operations do a clone (i.e. pre-populate the child with appropriate defaults even if that's not the normal behaviour for the subsystem. That would avoid compatibility issues.
Yes, that sounds good.
Quoted 4 lines
Quoted 3 lines
Maybe, we can first fix the ns_cgroup hook problem by moving the ns_cgroup_clone after cgroup_fork_callbacks

Quoted 6 lines

Quoted 5 lines
You mean fork the task into the parent cgroup, then clone the new
cgroup and reattach to the new cgroup? That could work, although I'm not sure whether it would be bad to have the new task briefly appearing in the parent cgroups.
I tried the following patch:

I think the patch should be fine.

-serge

Quoted 41 lines

Index: linux-2.6/kernel/fork.c
===================================================================
kernel/fork.c +0 -6
@@ -1150,12 +1150,6 @@ static struct task_struct *copy_process(
       if (clone_flags & CLONE_THREAD)
               p->tgid = current->tgid;
 
-       if (current->nsproxy != p->nsproxy) {
-               retval = ns_cgroup_clone(p, pid);
-               if (retval)
-                       goto bad_fork_free_graph;
-       }
-
       p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ?  
 child_tidptr : NULL;
       /*
* Clear TID on mm_release()?
kernel/fork.c +6 -0
@@ -1204,6 +1198,12 @@ static struct task_struct *copy_process(
       if (retval)
               goto bad_fork_free_graph;
 
+       if (current->nsproxy != p->nsproxy) {
+               retval = ns_cgroup_clone(p, pid);
+               if (retval)
+                       goto bad_fork_cgroup_callbacks;
+       }
+
       /* Need tasklist lock for parent etc handling! */
       write_lock_irq(&tasklist_lock);
 
That seems to fix the inconsistency problem but maybe I am missing something...
Excepting kernel race condition in the copy_process function I may have missed, I don't see the difference with a program forking and adding the task in the child cgroup (or the child process adds itself right after the fork), the child will briefly appear to the parent cgroup, no ?
-- Daniel