From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758517AbcH3Lx6 (ORCPT ); Tue, 30 Aug 2016 07:53:58 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:59480 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757907AbcH3Lx5 (ORCPT ); Tue, 30 Aug 2016 07:53:57 -0400 Date: Tue, 30 Aug 2016 13:53:42 +0200 From: Peter Zijlstra To: Waiman Long Cc: Linus Torvalds , Jason Low , Ding Tianhong , Thomas Gleixner , Will Deacon , Ingo Molnar , Imre Deak , Linux Kernel Mailing List , Davidlohr Bueso , Tim Chen , Terry Rudd , "Paul E. McKenney" , Jason Low , Chris Wilson , Daniel Vetter Subject: Re: [RFC][PATCH -v2 4/4] locking/mutex: Add lock handoff to avoid starvation Message-ID: <20160830115342.GH10168@twins.programming.kicks-ass.net> References: <20160825183734.113736626@infradead.org> <20160825184324.934871397@infradead.org> <57BF6A73.1050505@hpe.com> <20160825222328.GN10138@twins.programming.kicks-ass.net> <57C05287.8000708@hpe.com> <20160826151818.GO10121@twins.programming.kicks-ass.net> <57C0D372.9030301@hpe.com> <20160829154109.GU10121@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160829154109.GU10121@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 29, 2016 at 05:41:09PM +0200, Peter Zijlstra wrote: > On Fri, Aug 26, 2016 at 07:40:34PM -0400, Waiman Long wrote: > > On 08/26/2016 11:18 AM, Peter Zijlstra wrote: > > > >Still need to look at adding spinning to the handoff case. > > >Also need to look at writing (much) better changelogs, they stink. > > > > > > > I have looked at the handoff code and I didn't see any problem. > > So I found (or rather the buildbot did) a problem with it. > > locking-selftest has testcases like: > > > lock(&A); > if (trylock(&A)) > /* fail */ > > and > > ww_lock(&A) > if (ww_lock(&A) != -EDEADLK) > /* fail */ > > But with the 'trylock' accepting the lock if owner==current, in order to > accept the hand-off, this breaks in interesting ways. > > Now, ARCH_MIN_TASKALIGN is at least 8 (mips, s390, parisc) which would > give us one more FLAG bit to play with. > > > The below seems to make things happy again.. Much simpler solution... only accept handoffs when we're stuck in the wait loop (which precludes doing recursive locking, since that would've failed much earlier). Now, let me look at that spinner patch you sent. --- --- kernel/locking/mutex.c.mod 2016-08-30 11:08:15.410551744 +0200 +++ kernel/locking/mutex.c 2016-08-30 13:38:30.185550669 +0200 @@ -69,7 +69,7 @@ /* * Actual trylock that will work on any unlocked state. */ -static inline bool __mutex_trylock(struct mutex *lock) +static inline bool __mutex_trylock(struct mutex *lock, const bool handoff) { unsigned long owner, curr = (unsigned long)current; @@ -78,8 +78,10 @@ unsigned long old; if (__owner_task(owner)) { - if ((unsigned long)__owner_task(owner) == curr) + if (handoff && unlikely(__owner_task(owner) == current)) { + smp_mb(); /* ACQUIRE */ return true; + } return false; } @@ -134,6 +136,10 @@ return list_first_entry(&lock->wait_list, struct mutex_waiter, list) == waiter; } +/* + * Give up ownership to a specific task, when @task = NULL, this is equivalent + * to a regular unlock. + */ static void __mutex_handoff(struct mutex *lock, struct task_struct *task) { unsigned long owner = atomic_long_read(&lock->owner); @@ -148,7 +154,7 @@ new = (owner & MUTEX_FLAG_WAITERS); new |= (unsigned long)task; - old = atomic_long_cmpxchg(&lock->owner, owner, new); + old = atomic_long_cmpxchg_release(&lock->owner, owner, new); if (old == owner) break; @@ -425,7 +431,7 @@ break; /* Try to acquire the mutex if it is unlocked. */ - if (__mutex_trylock(lock)) { + if (__mutex_trylock(lock, false)) { osq_unlock(&lock->osq); return true; } @@ -570,7 +576,12 @@ preempt_disable(); mutex_acquire_nest(&lock->dep_map, subclass, 0, nest_lock, ip); - if (__mutex_trylock(lock) || mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) { + /* + * The first __mutex_trylock() must not accept handoffs, otherwise its + * possible to allow recursive lock attempts by accident. + */ + if (__mutex_trylock(lock, false) || + mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) { /* got the lock, yay! */ lock_acquired(&lock->dep_map, ip); if (use_ww_ctx) { @@ -588,7 +599,7 @@ /* * Once more, try to acquire the lock. */ - if (__mutex_trylock(lock)) + if (__mutex_trylock(lock, false)) goto skip_wait; debug_mutex_lock_common(lock, &waiter); @@ -601,7 +612,7 @@ if (__mutex_waiter_is_first(lock, &waiter)) __mutex_set_flag(lock, MUTEX_FLAG_WAITERS); - if (__mutex_trylock(lock)) + if (__mutex_trylock(lock, false)) goto remove_waiter; lock_contended(&lock->dep_map, ip); @@ -629,7 +640,7 @@ schedule_preempt_disabled(); spin_lock_mutex(&lock->wait_lock, flags); - if (__mutex_trylock(lock)) + if (__mutex_trylock(lock, true)) break; if (__mutex_waiter_is_first(lock, &waiter)) @@ -923,7 +934,7 @@ */ int __sched mutex_trylock(struct mutex *lock) { - bool locked = __mutex_trylock(lock); + bool locked = __mutex_trylock(lock, false); if (locked) mutex_acquire(&lock->dep_map, 0, 1, _RET_IP_);