From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934479AbcJFK3h (ORCPT ); Thu, 6 Oct 2016 06:29:37 -0400 Received: from merlin.infradead.org ([205.233.59.134]:43546 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752332AbcJFK3a (ORCPT ); Thu, 6 Oct 2016 06:29:30 -0400 Date: Thu, 6 Oct 2016 12:29:16 +0200 From: Peter Zijlstra To: mingo@kernel.org, tglx@linutronix.de, juri.lelli@arm.com, rostedt@goodmis.org, xlpang@redhat.com, bigeasy@linutronix.de Cc: 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: <20161006102916.GM3142@twins.programming.kicks-ass.net> References: <20161003091234.879763059@infradead.org> <20161003091847.704255067@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161003091847.704255067@infradead.org> 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, Oct 03, 2016 at 11:12:38AM +0200, Peter Zijlstra wrote: > There's a number of 'interesting' problems with FUTEX_UNLOCK_PI, all > caused by holding hb->lock while doing the rt_mutex_unlock() > equivalient. > > This patch doesn't attempt to fix any of the actual problems, but > instead reworks the code to not hold hb->lock across the unlock, > paving the way to actually fix the problems later. > > The current reason we hold hb->lock over unlock is that it serializes > against FUTEX_LOCK_PI and avoids new waiters from coming in, this then > ensures the rt_mutex_next_owner() value is stable and can be written > into the user-space futex value before doing the unlock. Such that the > unlock will indeed end up at new_owner. > > This patch recognises that holding rt_mutex::wait_lock results in the > very same guarantee, no new waiters can come in while we hold that > lock -- after all, waiters would need this lock to queue themselves. > > It therefore restructures the code to keep rt_mutex::wait_lock held. > > This (of course) is not entirely straight forward either, see the > comment in rt_mutex_slowunlock(), doing the unlock itself might drop > wait_lock, letting new waiters in. To cure this > rt_mutex_futex_unlock() becomes a variant of rt_mutex_slowunlock() > that return -EAGAIN instead. This ensures the FUTEX_UNLOCK_PI code > aborts and restarts the entire operation. Urgh, I missed a bunch :/ So there's the !new_owner case in wake_futex_pi() which can happen if futex_lock_pi()'s rt_mutex_timed_futex_lock() failed but we still see that task on the futex_q list (it hasn't yet done unqueue_me). I wondered if we could sort this case by making fixup_owner() more interesting, which got me looking at that. And it turns out fixup_owner() relies on futex_pi_unlock() holding hb->lock as well.. It does rt_mutex_owner() while holding wait_lock, but then drops wait_lock to call fixup_pi_state_owner(), assuming the owner it read remains valid. ARGGH, what a mess. Lemme stare at this more..