Re: cgroup attach/fork hooks consistency with the ns_cgroup

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting 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:

I think the patch should be fine.

-serge

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