From mboxrd@z Thu Jan 1 00:00:00 1970 From: Frederic Weisbecker Subject: Re: [RFC PATCH] cgroup: Remove task_lock() from cgroup_post_fork() Date: Thu, 22 Dec 2011 10:33:54 +0100 Message-ID: <20111222093351.GN17668@somewhere> References: <1324516711-26913-1-git-send-email-fweisbec@gmail.com> <4EF2F095.90207@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=ZMgCLUSKIU5OQ2xTrxDREv6YuFC/tXEEMKOdaRfPQns=; b=Eo614oFXsEzfNZ3TBTNgVr037framv3lKmQeiMox+Jw/6C4vLw8UgBXwiLE7q8x2aq 20SsouJfIEWAZgrZUPTSAn2defjYuri3CPJgaLXaxgoYWJv5Z7R0fnsGes+Uzm/tDyKX FTSSLNXi4dkFSgJPP2oahPguYhlP952ugdcyg= Content-Disposition: inline In-Reply-To: <4EF2F095.90207-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Li Zefan Cc: Containers , LKML , Oleg Nesterov , Mandeep Singh Baines , Tejun Heo , Cgroups , Andrew Morton , Paul Menage On Thu, Dec 22, 2011 at 04:55:49PM +0800, Li Zefan wrote: > Frederic Weisbecker wrote: > > cgroup_post_fork() is protected between threadgroup_change_begin() > > and threadgroup_change_end() against concurrent changes of the > > child's css_set in cgroup_task_migrate(). Also the child can't > > exit and call cgroup_exit() at this stage, this means it's css_set > > can't be changed with init_css_set concurrently. > > > > For these reasons, we don't need to hold task_lock() on the child > > because it's css_set can only remain stable in this place. > > > > Let's remove the lock there. > > > > NOTE: We could do something else: move threadgroup_change_end() > > before cgroup_post_fork() and keep the task_lock() which, combined > > with the css_set_lock, would be enough to synchronize against > > cgroup_task_migrate()'s change on child->cgroup and its cglist. > > Because outside that, the threadgroup lock doesn't appear to be > > needed on cgroup_post_fork(). > > > > To narrow the scope of the threadgroup lock? I think it's preferable to keep > cgroup_post_fork() inside the lock, to make things simpler and we have > the same lock rule for both cgroup_fork() and cgroup_post_fork(). Ok! > > Let's debate! > > > > Signed-off-by: Frederic Weisbecker > > Cc: Li Zefan > > Cc: Tejun Heo > > Cc: Containers > > Cc: Cgroups > > Cc: KAMEZAWA Hiroyuki > > Cc: Oleg Nesterov > > Cc: Andrew Morton > > Cc: Paul Menage > > Cc: Mandeep Singh Baines > > --- > > kernel/cgroup.c | 11 ++++++++--- > > 1 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > > index 4936d88..d0dbf72 100644 > > --- a/kernel/cgroup.c > > +++ b/kernel/cgroup.c > > @@ -4622,10 +4622,15 @@ void cgroup_post_fork(struct task_struct *child) > > { > > if (use_task_css_set_links) { > > write_lock(&css_set_lock); > > - task_lock(child); > > - if (list_empty(&child->cg_list)) > > + if (list_empty(&child->cg_list)) { > > + /* > > + * It's safe to use child->cgroups without task_lock() > > + * here because we are protected through > > + * threadgroup_change_begin() against concurrent > > + * css_set change in cgroup_task_migrate() > > + */ > > Also explain why it won't race with cgroup_exit()? You were not quite confident > about that before Oleg's explanation. ;) hehe indeed :) Will update, thanks! > > list_add(&child->cg_list, &child->cgroups->tasks); > > - task_unlock(child); > > + } > > write_unlock(&css_set_lock); > > } > > }