From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757264AbcJHQ4B (ORCPT ); Sat, 8 Oct 2016 12:56:01 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:56629 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752845AbcJHQzw (ORCPT ); Sat, 8 Oct 2016 12:55:52 -0400 Date: Sat, 8 Oct 2016 18:55:40 +0200 From: Peter Zijlstra To: Thomas Gleixner Cc: mingo@kernel.org, juri.lelli@arm.com, rostedt@goodmis.org, xlpang@redhat.com, bigeasy@linutronix.de, linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com, jdesfossez@efficios.com, bristot@redhat.com Subject: Re: [RFC][PATCH 4/4] futex: Rewrite FUTEX_UNLOCK_PI Message-ID: <20161008165540.GI3568@worktop.programming.kicks-ass.net> References: <20161003091234.879763059@infradead.org> <20161003091847.704255067@infradead.org> <20161007112143.GJ3117@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.22.1 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Oct 08, 2016 at 05:53:49PM +0200, Thomas Gleixner wrote: > On Fri, 7 Oct 2016, Peter Zijlstra wrote: > > Solve all that by: > > > > - using futex specific rt_mutex calls that lack the fastpath, futexes > > have their own fastpath anyway. This makes that > > rt_mutex_futex_unlock() doesn't need to drop rt_mutex::wait_lock > > and the unlock is guaranteed if we manage to update user state. > > > > - make futex_unlock_pi() drop hb->lock early and only use > > rt_mutex::wait_lock to serialize against rt_mutex waiters > > update the futex value and unlock. > > > > - in case futex and rt_mutex disagree on waiters, side with rt_mutex > > and simply clear the user value. This works because either there > > really are no waiters left, or futex_lock_pi() triggers the > > lock-steal path and fixes up the WAITERS flag. > > I stared at this for a few hours and while I'm not yet done analyzing all > possible combinations I found at least one thing which is broken: > > CPU 0 CPU 1 > > unlock_pi(f) > .... > unlock(hb->lock) > *f = new_owner_tid | WAITERS; > > lock_pi(f) > lock(hb->lock) > uval = *f; > topwaiter = futex_top_waiter(); > attach_to_pi_state(uval, topwaiter->pistate); > pid = uval & TID_MASK; > if (pid != task_pid_vnr(pistate->owner)) > return -EINVAL; > .... > pistate->owner = newowner; > > So in this case we tell the caller on CPU 1 that the futex is in > inconsistent state, because pistate->owner still points to the unlocking > task while the user space value alread shows the new owner. So this sanity > check triggers and we simply fail while we should not. It's [10] in the > state matrix above attach_to_pi_state(). Urgh, yes. I think I can cure that, by taking pi_state->pi_mutex.wait_lock in attach_to_pi_state(), but blergh. > I suspect that there are more issues like this, especially since I did not > look at requeue_pi yet, but by now my brain is completely fried. Yes, I know about fried brains :-( This stuff has far too many moving parts. I've been staring at this stuff far too long. Also, we need better tools to stress this stuff.