From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933360AbdDFMRu (ORCPT ); Thu, 6 Apr 2017 08:17:50 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:60033 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752385AbdDFMRi (ORCPT ); Thu, 6 Apr 2017 08:17:38 -0400 Date: Thu, 6 Apr 2017 14:17:28 +0200 From: Peter Zijlstra To: Darren Hart Cc: tglx@linutronix.de, 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: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt_mutex API Message-ID: <20170406121728.h3lj2fg4bbst4663@hirez.programming.kicks-ass.net> References: <20170322103547.756091212@infradead.org> <20170322104151.702962446@infradead.org> <20170405150217.GA27833@fury> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170405150217.GA27833@fury> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 05, 2017 at 08:02:17AM -0700, Darren Hart wrote: > > @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad > > pi_state->owner = new_owner; > > raw_spin_unlock(&new_owner->pi_lock); > > > > /* > > + * We've updated the uservalue, this unlock cannot fail. > > It isn't clear to me what I should understand from this new comment. How does > the value of the uval affect whether or not the pi_state->pi_mutex can be > unlocked or not? Or are you noting that we've set FUTEX_WAITIERS so any valid > userspace operations will be forced intot he kernel and can't race with us since > we hold the hb->lock? With futexes, I think it's important that we be very > explicit in our comment blocks. The critical point is that once you've modified uval we must not fail; there is no way to undo things thereafter. > > */ > > + deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q); > > + > > + raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock); > > spin_unlock(&hb->lock); > > + > > + if (deboost) { > > + wake_up_q(&wake_q); > > Is moving wake_up_q under deboost related to this change or is it just an > optimization since there is no need to wake unless we are deboosting ourselves - > which was true before as well? > > If this is due to the rt_mutex_futex* API, I haven't made the connection. It's how rt_mutex does wakeups, note that later patches clean this up.