Re: cgroup attach/fork hooks consistency with the ns_cgroup

[Daniel Lezcano <daniel.lezcano-GANU6spQydw@public.gmane.org>]

Paul Menage wrote:
> On Thu, Jun 18, 2009 at 11:36 AM, Daniel Lezcano<daniel.lezcano-GANU6spQydw@public.gmane.org> wrote:
>   
>> 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.
>   

Yes, that sounds good.
>> 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:

Index: linux-2.6/kernel/fork.c
===================================================================
--- linux-2.6.orig/kernel/fork.c
+++ 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