2012/10/21 Tejun Heo [off-list ref]:

Hello, Frederic.

On Sat, Oct 20, 2012 at 02:21:43PM -0400, Frederic Weisbecker wrote:

quoted

CPU 0
                    CPU 1

cgroup_task_migrate {
        task_lock(p)
        rcu_assign_pointer(tsk->cgroups, newcg);
        task_unlock(tsk);

        write_lock(&css_set_lock);
        if (!list_empty(&tsk->cg_list))
            list_move(&tsk->cg_list, &newcg->tasks);
        write_unlock(&css_set_lock);

                          write_lock(&css_set_lock);
      put_css_set(oldcg);
         list_add(&child->cg_list, &child->cgroups->tasks); (1)

Man, that's confusing. :)

Sorry and I'm currently stuck in some airport and too lazy to reorder
the above lines :)

quoted

On (1), child->cgroups should have the value of newcg and not oldcg
due to the memory ordering implied by the locking of css_set_lock. Now
I can't guarantee that because I'm no memory ordering expert. And even
if it's safe, it's so very non obvious that I now agree with you:
let's revert  the patch and restart with a better base by gathering
all the cgroup fork code in the current cgroup_post_fork place.

Aye aye, let's move everything to cgroup_post_fork() and then we don't
have to worry about grabbing task_lock multiple times.

Agreed. and Acked-by: Frederic Weisbecker [off-list ref]