Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756606Ab2JTSVq (ORCPT ); Sat, 20 Oct 2012 14:21:46 -0400 Received: from mail-vb0-f46.google.com ([209.85.212.46]:32974 "EHLO mail-vb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750850Ab2JTSVo (ORCPT ); Sat, 20 Oct 2012 14:21:44 -0400 MIME-Version: 1.0 In-Reply-To: <20121019210738.GA1180@google.com> References: <20121008020000.GB2575@localhost> <20121019005922.GG13370@google.com> <20121019193808.GL13370@google.com> <20121019210738.GA1180@google.com> Date: Sat, 20 Oct 2012 14:21:43 -0400 Message-ID: Subject: Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()" From: Frederic Weisbecker To: Tejun Heo Cc: Li Zefan , containers@lists.linux-foundation.org, cgroups@vger.kernel.org, LKML Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1803 Lines: 47 2012/10/19 Tejun Heo : > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/