From patchwork Thu Aug 9 05:12:15 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Pitre X-Patchwork-Id: 1298691 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 7F0DDDFF7B for ; Thu, 9 Aug 2012 05:16:03 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SzL2j-0002rz-Na; Thu, 09 Aug 2012 05:12:21 +0000 Received: from relais.videotron.ca ([24.201.245.36]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SzL2g-0002rl-5l for linux-arm-kernel@lists.infradead.org; Thu, 09 Aug 2012 05:12:18 +0000 MIME-version: 1.0 Received: from xanadu.home ([24.201.196.149]) by VL-VM-MR001.ip.videotron.ca (Oracle Communications Messaging Exchange Server 7u4-22.01 64bit (built Apr 21 2011)) with ESMTP id <0M8H00GM42GFMB80@VL-VM-MR001.ip.videotron.ca> for linux-arm-kernel@lists.infradead.org; Thu, 09 Aug 2012 01:12:16 -0400 (EDT) Date: Thu, 09 Aug 2012 01:12:15 -0400 (EDT) From: Nicolas Pitre To: Will Deacon Subject: Re: RFC: mutex: hung tasks on SMP platforms with asm-generic/mutex-xchg.h In-reply-to: <20120807115647.GA12828@mudshark.cambridge.arm.com> Message-id: References: <20120807115647.GA12828@mudshark.cambridge.arm.com> User-Agent: Alpine 2.02 (LFD 1266 2009-07-14) X-Spam-Note: CRM114 invocation failed X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [24.201.245.36 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Chris Mason , Peter Zijlstra , Arnd Bergmann , linux-kernel@vger.kernel.org, Ingo Molnar , Thomas Gleixner , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Tue, 7 Aug 2012, Will Deacon wrote: > Hello, > > ARM recently moved to asm-generic/mutex-xchg.h for its mutex implementation > after our previous implementation was found to be missing some crucial > memory barriers. However, I'm seeing some problems running hackbench on > SMP platforms due to the way in which the MUTEX_SPIN_ON_OWNER code operates. > > The symptoms are that a bunch of hackbench tasks are left waiting on an > unlocked mutex and therefore never get woken up to claim it. I think this > boils down to the following sequence: > > > Task A Task B Task C Lock value > 0 1 > 1 lock() 0 > 2 lock() 0 > 3 spin(A) 0 > 4 unlock() 1 > 5 lock() 0 > 6 cmpxchg(1,0) 0 > 7 contended() -1 > 8 lock() 0 > 9 spin(C) 0 > 10 unlock() 1 > 11 cmpxchg(1,0) 0 > 12 unlock() 1 > > > At this point, the lock is unlocked, but Task B is in an uninterruptible > sleep with nobody to wake it up. > > The following patch fixes the problem by ensuring we put the lock into > the contended state if we acquire it from the spin loop on the slowpath > but I'd like to be sure that this won't cause problems with other mutex > implementations: > > > diff --git a/kernel/mutex.c b/kernel/mutex.c > index a307cc9..27b7887 100644 > --- a/kernel/mutex.c > +++ b/kernel/mutex.c > @@ -170,7 +170,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > if (owner && !mutex_spin_on_owner(lock, owner)) > break; > > - if (atomic_cmpxchg(&lock->count, 1, 0) == 1) { > + if (atomic_cmpxchg(&lock->count, 1, -1) == 1) { > lock_acquired(&lock->dep_map, ip); > mutex_set_owner(lock); > preempt_enable(); > > > All comments welcome. Well... after thinking about this for a while, I came to the conclusion that the mutex_spin_on_owner code is indeed breaking the contract between the xchg lock fast path and the slow path. The xchg fast path will always set the count to 0 and rely on the slow path to restore a possible pre-existing negative count. So the above change would be needed for correctness in the xchg case, even if it always forces the unlock into the slow path. As I mentioned previously, we don't want to force the slow path in all cases though. The atomic decrement based fast path doesn't need that, so I'd suggest this fix instead: That would be needed for the stable tree as well. A further cleanup could remove all definitions of __mutex_slowpath_needs_to_unlock() given that they're all set to 1 except for the xchg fast path, in which case __MUTEX_XCHG_FAST_PATH could be reused instead. Of course that might tilt the balance towards using mutex-dec.h on ARM v6 and above instead of mutex-xchg.h. But that is an orthogonal issue, and that doesn't remove the need for fixing the xchg based case for correctness. Nicolas diff --git a/include/asm-generic/mutex-xchg.h b/include/asm-generic/mutex-xchg.h index 580a6d35c7..60964844c8 100644 --- a/include/asm-generic/mutex-xchg.h +++ b/include/asm-generic/mutex-xchg.h @@ -108,4 +108,6 @@ __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *)) return prev; } +#define __MUTEX_XCHG_FAST_PATH + #endif diff --git a/kernel/mutex.c b/kernel/mutex.c index a307cc9c95..c6a26a4f1c 100644 --- a/kernel/mutex.c +++ b/kernel/mutex.c @@ -161,6 +161,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, for (;;) { struct task_struct *owner; + int locked_val; /* * If there's an owner, wait for it to either @@ -170,7 +171,19 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, if (owner && !mutex_spin_on_owner(lock, owner)) break; - if (atomic_cmpxchg(&lock->count, 1, 0) == 1) { +#ifdef __MUTEX_XCHG_FAST_PATH + /* + * The fast path based on xchg sets a transient 0 count, + * relying on the slow path to restore a possible + * pre-existing contended count. Without checking the + * waiters' list we must presume possible contension here. + */ + locked_val = -1; +#else + locked_val = 0; +#endif + + if (atomic_cmpxchg(&lock->count, 1, locked_val) == 1) { lock_acquired(&lock->dep_map, ip); mutex_set_owner(lock); preempt_enable();