From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx118.postini.com [74.125.245.118]) by kanga.kvack.org (Postfix) with SMTP id EADD96B005D for ; Wed, 3 Oct 2012 10:56:13 -0400 (EDT) Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 3 Oct 2012 20:26:10 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q93Eu8a19634240 for ; Wed, 3 Oct 2012 20:26:08 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q93KPjWP002376 for ; Thu, 4 Oct 2012 01:55:45 +0530 Message-ID: <506C51E0.1000602@linux.vnet.ibm.com> Date: Wed, 03 Oct 2012 20:25:28 +0530 From: "Srivatsa S. Bhat" MIME-Version: 1.0 Subject: Re: [PATCH v2] [RFC] 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> <506C2E02.9080804@linux.vnet.ibm.com> <20121003145047.GA2527@linux.vnet.ibm.com> In-Reply-To: <20121003145047.GA2527@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: paulmck@linux.vnet.ibm.com Cc: Jiri Kosina , Christoph Lameter , Pekka Enberg , "Paul E. McKenney" , Josh Triplett , linux-kernel@vger.kernel.org, linux-mm@kvack.org On 10/03/2012 08:20 PM, Paul E. McKenney wrote: > 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. This can't happen. Because kmem_cache_destroy() will call get_online_cpus() before trying to acquire slab mutex. And it sleeps waiting at get_online_cpus() because the hotplug lock has already been acquired by Thread A. > > 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? > So the key point is, Thread A is a hotplug writer. Thread B becomes a hotplug reader the moment it calls get_online_cpus(). So they can't co-exist/run together. They will get serialized. That is, Thread A runs to completion, releases hotplug lock. Only then thread B gets past get_online_cpus(). Regards, Srivatsa S. Bhat >> 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, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org