2012/10/19 Tejun Heo <[email protected]>:
> On Fri, Oct 19, 2012 at 09:35:26AM -0400, Frederic Weisbecker wrote:
>> 2012/10/18 Tejun Heo <[email protected]>:
>> > From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
>> > From: Tejun Heo <[email protected]>
>> > Date: Thu, 18 Oct 2012 17:40:30 -0700
>> >
>> > This reverts commit 7e3aa30ac8c904a706518b725c451bb486daaae9.
>> >
>> > The commit incorrectly assumed that fork path always performed
>> > threadgroup_change_begin/end() and depended on that for
>> > synchronization against task exit and cgroup migration paths instead
>> > of explicitly grabbing task_lock().
>> >
>> > threadgroup_change is not locked when forking a new process (as
>> > opposed to a new thread in the same process) and even if it were it
>> > wouldn't be effective as different processes use different threadgroup
>> > locks.
>> >
>> > Revert the incorrect optimization.
>>
>> Ok but there is still no good reason to task_lock() there. But the
>> comment is indeed wrong, how about fixing it instead? I can send you
>> a patch for that.
>
> 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.
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?
Thanks.
--
tejun
2012/10/19 Tejun Heo <[email protected]>:
> 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.
2012/10/20 Frederic Weisbecker <[email protected]>:
> 2012/10/19 Tejun Heo <[email protected]>:
>> 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)
gmail mangled everything :(
Hello, Frederic.
On Sat, Oct 20, 2012 at 02:21:43PM -0400, Frederic Weisbecker wrote:
> 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. :)
> 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.
Thanks.
--
tejun
2012/10/21 Tejun Heo <[email protected]>:
> Hello, Frederic.
>
> On Sat, Oct 20, 2012 at 02:21:43PM -0400, Frederic Weisbecker wrote:
>> 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 :)
>
>> 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 <[email protected]>