Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Removetask_lock() from cgroup_post_fork()"

[Frederic Weisbecker]

2012/10/19 Tejun Heo <tj@xxxxxxxxxx>:
> Hello, Frederic.
>
> On Fri, Oct 19, 2012 at 03:44:20PM -0400, Frederic Weisbecker wrote:
>> > For -stable, I think it's better to revert.  If you want to remove
>> > task_lock, let's do it for 3.8.
>>
>> I don't think that a wrong comment justifies a patch to stable.
>
> I'm not really sure whether it's safe or not.  It seems all usages are
> protected by write locking css_set_lock but maybe I'm missing
> something and as the commit is born out of confusion, I'm very
> inclined to revert it by default.  Are you sure this one is safe?

Thinking about it further, one scenario is worrying me but it
eventually looks safe but by accident.

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)

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.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/