From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752422AbcJCPgh (ORCPT ); Mon, 3 Oct 2016 11:36:37 -0400 Received: from smtprelay0041.hostedemail.com ([216.40.44.41]:56299 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750991AbcJCPga (ORCPT ); Mon, 3 Oct 2016 11:36:30 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::::,RULES_HIT:41:69:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1543:1593:1594:1605:1711:1730:1747:1777:1792:2393:2553:2559:2562:2691:2693:2895:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4605:5007:6261:7875:7903:10004:10400:10848:10967:11026:11232:11473:11658:11914:12043:12198:12291:12294:12296:12438:12683:12740:12760:13439:14096:14097:14181:14659:14721:21080:21324:21433:30012:30034:30051:30054:30055:30070:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: crown35_8fa8ce344ad18 X-Filterd-Recvd-Size: 4510 Date: Mon, 3 Oct 2016 11:36:24 -0400 From: Steven Rostedt To: Peter Zijlstra Cc: mingo@kernel.org, tglx@linutronix.de, juri.lelli@arm.com, 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: <20161003113624.04f1f9f2@gandalf.local.home> In-Reply-To: <20161003091847.704255067@infradead.org> References: <20161003091234.879763059@infradead.org> <20161003091847.704255067@infradead.org> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 03 Oct 2016 11:12:38 +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. > > Signed-off-by: Peter Zijlstra (Intel) > --- > kernel/futex.c | 63 +++++++++++++++++-------------- > kernel/locking/rtmutex.c | 81 ++++++++++++++++++++++++++++++++++++---- > kernel/locking/rtmutex_common.h | 4 - > 3 files changed, 110 insertions(+), 38 deletions(-) > > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -1294,24 +1294,21 @@ static void mark_wake_futex(struct wake_ > static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter, > struct futex_hash_bucket *hb) > { > - struct task_struct *new_owner; > struct futex_pi_state *pi_state = top_waiter->pi_state; > u32 uninitialized_var(curval), newval; > + struct task_struct *new_owner; > WAKE_Q(wake_q); > - bool deboost; > int ret = 0; > > - if (!pi_state) > - return -EINVAL; > + raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock); > > + WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount)); Don't we have a rule where WARN_ON() and BUG_ON() should never have "side effects"? That is, they should only check values, but their contents should not update values. hence have: ret = atomic_inc_not_zero(&pi_state->refcount); WARN_ON_ONCE(!ret); > /* > - * If current does not own the pi_state then the futex is > - * inconsistent and user space fiddled with the futex value. > + * Now that we hold wait_lock, no new waiters can happen on the > + * rt_mutex and new owner is stable. Drop hb->lock. > */ > - if (pi_state->owner != current) > - return -EINVAL; > + spin_unlock(&hb->lock); > Also, as Sebastian has said before, I believe this breaks rt's migrate disable code. As migrate disable and migrate_enable are a nop if preemption is disabled, thus if you hold a raw_spin_lock across a spin_unlock() when the migrate enable will be a nop, and the migrate_disable() will never stop. -- Steve > - raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock); > new_owner = rt_mutex_next_owner(&pi_state->pi_mutex); > > /*