Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756286Ab2JCOwC (ORCPT ); Wed, 3 Oct 2012 10:52:02 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:52370 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756270Ab2JCOwA (ORCPT ); Wed, 3 Oct 2012 10:52:00 -0400 Date: Wed, 3 Oct 2012 07:50:47 -0700 From: "Paul E. McKenney" To: "Srivatsa S. Bhat" Cc: Jiri Kosina , Christoph Lameter , Pekka Enberg , "Paul E. McKenney" , Josh Triplett , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v2] [RFC] mm, slab: release slab_mutex earlier in kmem_cache_destroy() Message-ID: <20121003145047.GA2527@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20121002170149.GC2465@linux.vnet.ibm.com> <20121002233138.GD2465@linux.vnet.ibm.com> <20121003001530.GF2465@linux.vnet.ibm.com> <506C2E02.9080804@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <506C2E02.9080804@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Content-Scanned: Fidelis XPS MAILER x-cbid: 12100314-2876-0000-0000-000000B1180C Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8718 Lines: 202 On Wed, Oct 03, 2012 at 05:52:26PM +0530, Srivatsa S. Bhat wrote: > On 10/03/2012 03:16 PM, Jiri Kosina wrote: > > On Wed, 3 Oct 2012, Jiri Kosina wrote: > > > >> Good question. I believe it should be safe to drop slab_mutex earlier, as > >> cachep has already been unlinked. I am adding slab people and linux-mm to > >> CC (the whole thread on LKML can be found at > >> https://lkml.org/lkml/2012/10/2/296 for reference). > >> > >> How about the patch below? Pekka, Christoph, please? > > > > It turns out that lockdep is actually getting this wrong, so the changelog > > in the previous version wasn't accurate. > > > > Please find patch with updated changelog below. Pekka, Christoph, could > > you please check whether it makes sense to you as well? Thanks. > > > > > > > > > > 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: > > > > === [ cut here ] === > > ====================================================== > > [ INFO: possible circular locking dependency detected ] > > 3.6.0-rc5-00004-g0d8ee37 #143 Not tainted > > ------------------------------------------------------- > > kworker/u:2/40 is trying to acquire lock: > > (rcu_sched_state.barrier_mutex){+.+...}, at: [] _rcu_barrier+0x26/0x1e0 > > > > but task is already holding lock: > > (slab_mutex){+.+.+.}, at: [] kmem_cache_destroy+0x45/0xe0 > > > > which lock already depends on the new lock. > > > > the existing dependency chain (in reverse order) is: > > > > -> #2 (slab_mutex){+.+.+.}: > > [] validate_chain+0x632/0x720 > > [] __lock_acquire+0x309/0x530 > > [] lock_acquire+0x121/0x190 > > [] __mutex_lock_common+0x5c/0x450 > > [] mutex_lock_nested+0x3e/0x50 > > [] cpuup_callback+0x2f/0xbe > > [] notifier_call_chain+0x93/0x140 > > [] __raw_notifier_call_chain+0x9/0x10 > > [] _cpu_up+0xba/0x14e > > [] cpu_up+0xbc/0x117 > > [] smp_init+0x6b/0x9f > > [] kernel_init+0x147/0x1dc > > [] kernel_thread_helper+0x4/0x10 > > > > -> #1 (cpu_hotplug.lock){+.+.+.}: > > [] validate_chain+0x632/0x720 > > [] __lock_acquire+0x309/0x530 > > [] lock_acquire+0x121/0x190 > > [] __mutex_lock_common+0x5c/0x450 > > [] mutex_lock_nested+0x3e/0x50 > > [] get_online_cpus+0x37/0x50 > > [] _rcu_barrier+0xbb/0x1e0 > > [] rcu_barrier_sched+0x10/0x20 > > [] rcu_barrier+0x9/0x10 > > [] deactivate_locked_super+0x49/0x90 > > [] deactivate_super+0x61/0x70 > > [] mntput_no_expire+0x127/0x180 > > [] sys_umount+0x6e/0xd0 > > [] system_call_fastpath+0x16/0x1b > > > > -> #0 (rcu_sched_state.barrier_mutex){+.+...}: > > [] check_prev_add+0x3de/0x440 > > [] validate_chain+0x632/0x720 > > [] __lock_acquire+0x309/0x530 > > [] lock_acquire+0x121/0x190 > > [] __mutex_lock_common+0x5c/0x450 > > [] mutex_lock_nested+0x3e/0x50 > > [] _rcu_barrier+0x26/0x1e0 > > [] rcu_barrier_sched+0x10/0x20 > > [] rcu_barrier+0x9/0x10 > > [] kmem_cache_destroy+0xd1/0xe0 > > [] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack] > > [] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack] > > [] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack] > > [] ops_exit_list+0x39/0x60 > > [] cleanup_net+0xfb/0x1b0 > > [] process_one_work+0x26b/0x4c0 > > [] worker_thread+0x12e/0x320 > > [] kthread+0x9e/0xb0 > > [] kernel_thread_helper+0x4/0x10 > > > > other info that might help us debug this: > > > > Chain exists of: > > rcu_sched_state.barrier_mutex --> cpu_hotplug.lock --> slab_mutex > > > > Possible unsafe locking scenario: > > > > CPU0 CPU1 > > ---- ---- > > lock(slab_mutex); > > lock(cpu_hotplug.lock); > > lock(slab_mutex); > > lock(rcu_sched_state.barrier_mutex); > > > > *** DEADLOCK *** > > === [ cut here ] === > > > > This is actually a false positive. Lockdep has no way of knowing the fact > > that the ABBA can actually never happen, because of special semantics of > > cpu_hotplug.refcount and itss handling in cpu_hotplug_begin(); the mutual > > exclusion there is not achieved through mutex, but through > > cpu_hotplug.refcount. > > > > The "neither cpu_up() nor cpu_down() will proceed past cpu_hotplug_begin() > > until everyone who called get_online_cpus() will call put_online_cpus()" > > semantics is totally invisible to lockdep. > > > > 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_destroy() > > call any more > > - it silences the lockdep false positive warning, as it avoids lockdep ever > > learning about slab_mutex -> cpu_hotplug.lock dependency > > > > Reviewed-by: Paul E. McKenney > > Signed-off-by: Jiri Kosina > > Reviewed-by: Srivatsa S. Bhat > > Earlier I was under the wrong impression that all the calltraces that lockdep > spewed were reflecting the same point in time. So, sorry about that noise! :-) > It is indeed a false-positive and there is no real bug here, and the fix looks > good, provided __kmem_cache_destroy() doesn't expect slab_mutex to be held. I am not so sure about it being a false positive. Consider the following sequence of events: o Thread A starts a CPU-hotplug operation, acquiring the hotplug mutex. o Thread B does a kmem_cache_destroy(), acquiring the slab mutex. o Thread A reaches the slab CPU-hotplug notifier, but cannot acquire the slab mutex because Thread B hold it. o Thread B enters rcu_barrier(), but cannot acquire the hotplug mutex because Thread A holds it. So I would argue that lockdep's output was a bit confusing, but that the deadlock it flagged is real. Or am I still missing something? Thanx, Paul > But, I'm also quite surprised that the put_online_cpus() code as it stands today > doesn't have any checks for the refcount going negative. I believe that such a > check would be valuable to help catch cases where we might end up inadvertently > causing an imbalance between get_online_cpus() and put_online_cpus(). I'll post > that as a separate patch. > > Regards, > Srivatsa S. Bhat > > > --- > > mm/slab.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/mm/slab.c b/mm/slab.c > > index 1133911..693c7cb 100644 > > --- a/mm/slab.c > > +++ b/mm/slab.c > > @@ -2801,12 +2801,12 @@ void kmem_cache_destroy(struct kmem_cache *cachep) > > put_online_cpus(); > > return; > > } > > + mutex_unlock(&slab_mutex); > > > > if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU)) > > rcu_barrier(); > > > > __kmem_cache_destroy(cachep); > > - 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/