From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753441Ab2GPQx3 (ORCPT ); Mon, 16 Jul 2012 12:53:29 -0400 Received: from mx2.netapp.com ([216.240.18.37]:16139 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751916Ab2GPQxQ (ORCPT ); Mon, 16 Jul 2012 12:53:16 -0400 X-IronPort-AV: E=Sophos;i="4.77,594,1336374000"; d="scan'208";a="666150581" From: "Myklebust, Trond" To: "Kirill A. Shutemov" CC: Andrew Morton , Pavel Emelyanov , Serge Hallyn , "KOSAKI Motohiro" , Al Viro , "Dmitry V. Levin" , "Kirill A. Shutemov" , Doug Ledford , "linux-kernel@vger.kernel.org" , "containers@lists.linux-foundation.org" Subject: Re: [PATCH v2] ns: do not block exit_task_namespaces() for a long time Thread-Topic: [PATCH v2] ns: do not block exit_task_namespaces() for a long time Thread-Index: AQHNY2k1j02wfx58ck6ocQv7UBLk4pcskbqAgAADtAA= Date: Mon, 16 Jul 2012 16:53:00 +0000 Message-ID: <1342457578.4648.19.camel@lade.trondhjem.org> References: <20120713140806.b3d0fda8.akpm@linux-foundation.org> <1342451364-14787-1-git-send-email-kirill.shutemov@linux.intel.com> <1342453174.4648.6.camel@lade.trondhjem.org> <20120716163943.GA15156@otc-wbsnb-06> In-Reply-To: <20120716163943.GA15156@otc-wbsnb-06> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.104.60.115] Content-Type: text/plain; charset="utf-8" Content-ID: <14F4E11193005B418196E91EE761C8FA@tahoe.netapp.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id q6GGra7f028358 On Mon, 2012-07-16 at 19:39 +0300, Kirill A. Shutemov wrote: > On Mon, Jul 16, 2012 at 03:39:36PM +0000, Myklebust, Trond wrote: > > On Mon, 2012-07-16 at 18:09 +0300, Kirill A. Shutemov wrote: > > > From: "Kirill A. Shutemov" > > > > > > On exiting of the last task in a namespace we need to trigger freeing of > > > the namespace. Currently, we call synchronize_rcu() and free_nsproxy() > > > directly on do_exit() path. > > > > > > On my machine synchronize_rcu() blocks for about 0.01 seconds. For > > > comparing: normal exit_group() syscall takes less than 0.0003 seconds. > > > > > > Let's offload synchronize_rcu() and free_nsproxy() to a workqueue. > > > > > > I also move synchronize_rcu() inside free_nsproxy(). It fixes racy > > > put_nsproxy() which calls free_nsproxy() without synchronize_rcu(). > > > I guess it was missed during switch to RCU (see cf7b708). > > > > > > Microbenchmark: > > > > > > : #define _GNU_SOURCE > > > : #include > > > : #include > > > : #include > > > : #include > > > : > > > : int > > > : main(void) > > > : { > > > : int i; > > > : for (i = 0; i < 1024; i++) { > > > : if (fork()) { > > > : wait(NULL); > > > : continue; > > > : } > > > : unshare(CLONE_NEWIPC); > > > : exit(0); > > > : } > > > : return 0; > > > : } > > > > > > Before the patch: > > > > > > real 0m8.335s > > > user 0m0.000s > > > sys 0m0.265s > > > > > > After: > > > > > > real 0m0.569s > > > user 0m0.001s > > > sys 0m0.154s > > > > > > Signed-off-by: Kirill A. Shutemov > > > Acked-by: Serge E. Hallyn > > > --- > > > > > > v2: > > > - Updated description. > > > > > > --- > > > include/linux/nsproxy.h | 1 + > > > kernel/nsproxy.c | 34 +++++++++++++++++++++++----------- > > > 2 files changed, 24 insertions(+), 11 deletions(-) > > > > > > diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h > > > index cc37a55..1d26be7 100644 > > > --- a/include/linux/nsproxy.h > > > +++ b/include/linux/nsproxy.h > > > @@ -24,6 +24,7 @@ struct fs_struct; > > > */ > > > struct nsproxy { > > > atomic_t count; > > > + struct work_struct free_nsproxy_work; > > > struct uts_namespace *uts_ns; > > > struct ipc_namespace *ipc_ns; > > > struct mnt_namespace *mnt_ns; > > > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c > > > index b576f7f..ebc7d40 100644 > > > --- a/kernel/nsproxy.c > > > +++ b/kernel/nsproxy.c > > > @@ -41,13 +41,17 @@ struct nsproxy init_nsproxy = { > > > #endif > > > }; > > > > > > +static void free_nsproxy_work(struct work_struct *work); > > > + > > > static inline struct nsproxy *create_nsproxy(void) > > > { > > > struct nsproxy *nsproxy; > > > > > > nsproxy = kmem_cache_alloc(nsproxy_cachep, GFP_KERNEL); > > > - if (nsproxy) > > > + if (nsproxy) { > > > atomic_set(&nsproxy->count, 1); > > > + INIT_WORK(&nsproxy->free_nsproxy_work, free_nsproxy_work); > > > + } > > > return nsproxy; > > > } > > > > > > @@ -166,6 +170,14 @@ out: > > > > > > void free_nsproxy(struct nsproxy *ns) > > > { > > > + /* > > > + * wait for others to get what they want from this nsproxy. > > > + * > > > + * cannot release this nsproxy via the call_rcu() since > > > + * put_mnt_ns() will want to sleep > > > + */ > > > + synchronize_rcu(); > > > + > > > if (ns->mnt_ns) > > > put_mnt_ns(ns->mnt_ns); > > > if (ns->uts_ns) > > > @@ -178,6 +190,14 @@ void free_nsproxy(struct nsproxy *ns) > > > kmem_cache_free(nsproxy_cachep, ns); > > > } > > > > > > +static void free_nsproxy_work(struct work_struct *work) > > > +{ > > > + struct nsproxy *ns = container_of(work, struct nsproxy, > > > + free_nsproxy_work); > > > + > > > + free_nsproxy(ns); > > > +} > > > + > > > /* > > > * Called from unshare. Unshare all the namespaces part of nsproxy. > > > * On success, returns the new nsproxy. > > > @@ -215,16 +235,8 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new) > > > > > > rcu_assign_pointer(p->nsproxy, new); > > > > > > - if (ns && atomic_dec_and_test(&ns->count)) { > > > - /* > > > - * wait for others to get what they want from this nsproxy. > > > - * > > > - * cannot release this nsproxy via the call_rcu() since > > > - * put_mnt_ns() will want to sleep > > > - */ > > > - synchronize_rcu(); > > > - free_nsproxy(ns); > > > - } > > > + if (ns && atomic_dec_and_test(&ns->count)) > > > + schedule_work(&ns->free_nsproxy_work); > > > > What's wrong with using call_rcu()? The above will cause a workqueue > > thread to block for no good reason. > > See comment to synchronize_rcu(). free_nsproxy() might sleep. > call_rcu() callback invocation might happen from either softirq or process > context, so we can't use it. But call_rcu() should be allowed to call schedule_work(). At least you'd be able to get rid of the 0.01s synchronize_rcu() sleep inside keventd. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I