From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mandeep Singh Baines Subject: Re: [PATCH 2/3] cgroup: remove double-checking locking from attach_task_by_pid Date: Thu, 22 Dec 2011 18:13:33 -0800 Message-ID: <20111223021333.GM13529@google.com> References: <1324601873-20773-1-git-send-email-msb@chromium.org> <1324601873-20773-2-git-send-email-msb@chromium.org> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=beta; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:x-operating-system :user-agent; bh=Zivuw/YnFS8oiMb8C8LOqY7+EYNRfDcyUEX1kb4OqVc=; b=OEj/7W/KhzTIzpWZiNI+OhAQSwqkINGzgP0rBF2dIM+dVL68x3H7ABcqzsC5VIk5lv dq0w1iwEVzhjC99oPgiQ== Content-Disposition: inline In-Reply-To: <1324601873-20773-2-git-send-email-msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Mandeep Singh Baines Cc: Tejun Heo , Li Zefan , Frederic Weisbecker , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, KAMEZAWA Hiroyuki , Oleg Nesterov , Andrew Morton , Paul Menage This is not going to work. Will fix up and resend. Need to goto the top as suggested by Tejun. Mandeep Singh Baines (msb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org) wrote: > By reading group_leader after taking the threadgroup_lock, we can > avoid the double-check locking. This removes the return of > -EAGAIN so we can cleanup cgroup_procs_write at the same time. > > The suggestion was made here: > > https://lkml.org/lkml/2011/12/22/371 > > Suggested-by: Tejun Heo > Signed-off-by: Mandeep Singh Baines > Cc: Li Zefan > Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org > Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: KAMEZAWA Hiroyuki > Cc: Frederic Weisbecker > Cc: Oleg Nesterov > Cc: Andrew Morton > Cc: Paul Menage > --- > kernel/cgroup.c | 40 ++++++---------------------------------- > 1 files changed, 6 insertions(+), 34 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 032139d..a5f7d1b 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -2234,9 +2234,6 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup) > ret= -ESRCH; > goto out_unlock_cgroup; > } > - /* we check later for a group_leader race with de_thread */ > - if (threadgroup) > - tsk = tsk->group_leader; > /* > * even if we're attaching all tasks in the thread group, we > * only need to check permissions on one of them. > @@ -2252,33 +2249,17 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup) > get_task_struct(tsk); > rcu_read_unlock(); > } else { > - if (threadgroup) > - tsk = current->group_leader; > - else > - tsk = current; > + tsk = current; > get_task_struct(tsk); > } > > threadgroup_lock(tsk); > - > - if (threadgroup) { > - if (!thread_group_leader(tsk)) { > - /* > - * a race with de_thread from another thread's exec() > - * may strip us of our leadership, if this happens, > - * there is no choice but to throw this task away and > - * try again (from cgroup_procs_write); this is > - * "double-double-toil-and-trouble-check locking". > - */ > - ret = -EAGAIN; > - goto out_unlock_threadgroup; > - } > - ret = cgroup_attach_proc(cgrp, tsk); > - } else > + if (threadgroup) > + ret = cgroup_attach_proc(cgrp, tsk->group_leader); > + else > ret = cgroup_attach_task(cgrp, tsk); > - > -out_unlock_threadgroup: > threadgroup_unlock(tsk); > + > put_task_struct(tsk); > out_unlock_cgroup: > cgroup_unlock(); > @@ -2292,16 +2273,7 @@ static int cgroup_tasks_write(struct cgroup *cgrp, struct cftype *cft, u64 pid) > > static int cgroup_procs_write(struct cgroup *cgrp, struct cftype *cft, u64 tgid) > { > - int ret; > - do { > - /* > - * attach_proc fails with -EAGAIN if threadgroup leadership > - * changes in the middle of the operation, in which case we need > - * to find the task_struct for the new leader and start over. > - */ > - ret = attach_task_by_pid(cgrp, tgid, true); > - } while (ret == -EAGAIN); > - return ret; > + return attach_task_by_pid(cgrp, tgid, true); > } > > /** > -- > 1.7.3.1 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754138Ab1LWCNw (ORCPT ); Thu, 22 Dec 2011 21:13:52 -0500 Received: from mail-iy0-f174.google.com ([209.85.210.174]:56988 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752765Ab1LWCNt (ORCPT ); Thu, 22 Dec 2011 21:13:49 -0500 Date: Thu, 22 Dec 2011 18:13:33 -0800 From: Mandeep Singh Baines To: Mandeep Singh Baines Cc: Tejun Heo , Li Zefan , Frederic Weisbecker , linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org, cgroups@vger.kernel.org, KAMEZAWA Hiroyuki , Oleg Nesterov , Andrew Morton , Paul Menage Subject: Re: [PATCH 2/3] cgroup: remove double-checking locking from attach_task_by_pid Message-ID: <20111223021333.GM13529@google.com> References: <1324601873-20773-1-git-send-email-msb@chromium.org> <1324601873-20773-2-git-send-email-msb@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1324601873-20773-2-git-send-email-msb@chromium.org> X-Operating-System: Linux/2.6.38.8-gg621 (x86_64) User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is not going to work. Will fix up and resend. Need to goto the top as suggested by Tejun. Mandeep Singh Baines (msb@chromium.org) wrote: > By reading group_leader after taking the threadgroup_lock, we can > avoid the double-check locking. This removes the return of > -EAGAIN so we can cleanup cgroup_procs_write at the same time. > > The suggestion was made here: > > https://lkml.org/lkml/2011/12/22/371 > > Suggested-by: Tejun Heo > Signed-off-by: Mandeep Singh Baines > Cc: Li Zefan > Cc: containers@lists.linux-foundation.org > Cc: cgroups@vger.kernel.org > Cc: KAMEZAWA Hiroyuki > Cc: Frederic Weisbecker > Cc: Oleg Nesterov > Cc: Andrew Morton > Cc: Paul Menage > --- > kernel/cgroup.c | 40 ++++++---------------------------------- > 1 files changed, 6 insertions(+), 34 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 032139d..a5f7d1b 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -2234,9 +2234,6 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup) > ret= -ESRCH; > goto out_unlock_cgroup; > } > - /* we check later for a group_leader race with de_thread */ > - if (threadgroup) > - tsk = tsk->group_leader; > /* > * even if we're attaching all tasks in the thread group, we > * only need to check permissions on one of them. > @@ -2252,33 +2249,17 @@ static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup) > get_task_struct(tsk); > rcu_read_unlock(); > } else { > - if (threadgroup) > - tsk = current->group_leader; > - else > - tsk = current; > + tsk = current; > get_task_struct(tsk); > } > > threadgroup_lock(tsk); > - > - if (threadgroup) { > - if (!thread_group_leader(tsk)) { > - /* > - * a race with de_thread from another thread's exec() > - * may strip us of our leadership, if this happens, > - * there is no choice but to throw this task away and > - * try again (from cgroup_procs_write); this is > - * "double-double-toil-and-trouble-check locking". > - */ > - ret = -EAGAIN; > - goto out_unlock_threadgroup; > - } > - ret = cgroup_attach_proc(cgrp, tsk); > - } else > + if (threadgroup) > + ret = cgroup_attach_proc(cgrp, tsk->group_leader); > + else > ret = cgroup_attach_task(cgrp, tsk); > - > -out_unlock_threadgroup: > threadgroup_unlock(tsk); > + > put_task_struct(tsk); > out_unlock_cgroup: > cgroup_unlock(); > @@ -2292,16 +2273,7 @@ static int cgroup_tasks_write(struct cgroup *cgrp, struct cftype *cft, u64 pid) > > static int cgroup_procs_write(struct cgroup *cgrp, struct cftype *cft, u64 tgid) > { > - int ret; > - do { > - /* > - * attach_proc fails with -EAGAIN if threadgroup leadership > - * changes in the middle of the operation, in which case we need > - * to find the task_struct for the new leader and start over. > - */ > - ret = attach_task_by_pid(cgrp, tgid, true); > - } while (ret == -EAGAIN); > - return ret; > + return attach_task_by_pid(cgrp, tgid, true); > } > > /** > -- > 1.7.3.1 >