Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964913Ab2JCPuM (ORCPT ); Wed, 3 Oct 2012 11:50:12 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:56493 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964898Ab2JCPuJ (ORCPT ); Wed, 3 Oct 2012 11:50:09 -0400 Message-ID: <506C5E73.6080503@linux.vnet.ibm.com> Date: Wed, 03 Oct 2012 21:19:07 +0530 From: "Srivatsa S. Bhat" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0 MIME-Version: 1.0 To: Jiri Kosina CC: Christoph Lameter , Pekka Enberg , "Paul E. McKenney" , "Paul E. McKenney" , Josh Triplett , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v4] mm, slab: release slab_mutex earlier in kmem_cache_destroy() References: <20121002170149.GC2465@linux.vnet.ibm.com> <20121002233138.GD2465@linux.vnet.ibm.com> <20121003001530.GF2465@linux.vnet.ibm.com> <0000013a26fb253a-fb5df733-ad41-47c1-af1d-3d6739e417de-000000@email.amazonses.com> <506C52FC.4040305@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit x-cbid: 12100315-1618-0000-0000-000002953D5E Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3529 Lines: 104 On 10/03/2012 08:35 PM, Jiri Kosina wrote: > On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: > >>> diff --git a/mm/slab_common.c b/mm/slab_common.c >>> index 9c21725..90c3053 100644 >>> --- a/mm/slab_common.c >>> +++ b/mm/slab_common.c >>> @@ -166,6 +166,7 @@ void kmem_cache_destroy(struct kmem_cache *s) >>> s->refcount--; >>> if (!s->refcount) { >>> list_del(&s->list); >>> + mutex_unlock(&slab_mutex); >>> >>> if (!__kmem_cache_shutdown(s)) { >> >> __kmem_cache_shutdown() calls __cache_shrink(). And __cache_shrink() has this >> comment over it: >> /* Called with slab_mutex held to protect against cpu hotplug */ >> >> So, I guess the question is whether to modify your patch to hold the slab_mutex >> while calling this function, or to update the comment on top of this function >> saying that we are OK to call this function (even without slab_mutex) when we >> are inside a get/put_online_cpus() section. >> >>> if (s->flags & SLAB_DESTROY_BY_RCU) >>> @@ -179,8 +180,9 @@ void kmem_cache_destroy(struct kmem_cache *s) >>> s->name); >>> dump_stack(); >> >> There is a list_add() before this dump_stack(). I assume we need to hold the >> slab_mutex while calling it. > > Gah, of course it is, thanks for spotting this. > > > From: Jiri Kosina > Subject: [PATCH] mm, slab: release slab_mutex earlier in kmem_cache_destroy() > > Commit 1331e7a1bbe1 ("rcu: Remove _rcu_barrier() dependency on > __stop_machine()") introduced slab_mutex -> cpu_hotplug.lock > dependency through kmem_cache_destroy() -> rcu_barrier() -> > _rcu_barrier() -> get_online_cpus(). > > Lockdep thinks that this might actually result in ABBA deadlock, > and reports it as below: > [...] > This patch therefore moves the unlock of slab_mutex so that rcu_barrier() > is being called with it unlocked. It has two advantages: > > - it slightly reduces hold time of slab_mutex; as it's used to protect > the cachep list, it's not necessary to hold it over kmem_cache_free() > call any more > - it silences the lockdep false positive warning, as it avoids lockdep ever > learning about slab_mutex -> cpu_hotplug.lock dependency > > Signed-off-by: Jiri Kosina Reviewed-by: Srivatsa S. Bhat Hmm.. We can't do much about readability I guess... :( Regards, Srivatsa S. Bhat > --- > mm/slab_common.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 9c21725..069a24e6 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -168,6 +168,7 @@ void kmem_cache_destroy(struct kmem_cache *s) > list_del(&s->list); > > if (!__kmem_cache_shutdown(s)) { > + mutex_unlock(&slab_mutex); > if (s->flags & SLAB_DESTROY_BY_RCU) > rcu_barrier(); > > @@ -175,12 +176,14 @@ void kmem_cache_destroy(struct kmem_cache *s) > kmem_cache_free(kmem_cache, s); > } else { > list_add(&s->list, &slab_caches); > + mutex_unlock(&slab_mutex); > printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n", > s->name); > dump_stack(); > } > + } else { > + mutex_unlock(&slab_mutex); > } > - mutex_unlock(&slab_mutex); > put_online_cpus(); > } > EXPORT_SYMBOL(kmem_cache_destroy); > -- 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/