From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755670AbdCGSPE (ORCPT ); Tue, 7 Mar 2017 13:15:04 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:43437 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754309AbdCGSOJ (ORCPT ); Tue, 7 Mar 2017 13:14:09 -0500 Date: Tue, 7 Mar 2017 15:08:17 +0100 (CET) From: Thomas Gleixner To: Peter Zijlstra 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, dvhart@infradead.org Subject: Re: [PATCH -v5 10/14] futex: Pull rt_mutex_futex_unlock() out from under hb->lock In-Reply-To: <20170304093559.415341088@infradead.org> Message-ID: References: <20170304092717.762954142@infradead.org> <20170304093559.415341088@infradead.org> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 4 Mar 2017, Peter Zijlstra wrote: > @@ -1035,6 +1037,9 @@ static int attach_to_pi_state(u32 __user > * has dropped the hb->lock in between queue_me() and unqueue_me_pi(), > * which in turn means that futex_lock_pi() still has a reference on > * our pi_state. > + * > + * IOW, we cannot race against the unlocked put_pi_state() in > + * futex_unlock_pi(). That 'IOW' made my head spin for a while. I rather prefer to spell it out more explicitely: * The waiter holding a reference on @pi_state protects also * against the unlocked put_pi_state() in futex_unlock_pi(), * futex_lock_pi() and futex_wait_requeue_pi() as it cannot go to 0 * and consequentely free pi state before we can take a reference * ourself. > */ > WARN_ON(!atomic_read(&pi_state->refcount)); > > @@ -1378,47 +1383,33 @@ static void mark_wake_futex(struct wake_ > smp_store_release(&q->lock_ptr, NULL); > } > > -static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter, > - struct futex_hash_bucket *hb) Please add a comment, that the caller must hold a reference on @pi_state > +static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state) > { > - 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; > + bool deboost = false; > DEFINE_WAKE_Q(wake_q); > - bool deboost; > int ret = 0; > > - if (!pi_state) > - return -EINVAL; > - > - /* > - * If current does not own the pi_state then the futex is > - * inconsistent and user space fiddled with the futex value. > - */ > - if (pi_state->owner != current) > - return -EINVAL; > - > raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock); > new_owner = rt_mutex_next_owner(&pi_state->pi_mutex); > - > - /* > - * When we interleave with futex_lock_pi() where it does > - * rt_mutex_timed_futex_lock(), we might observe @this futex_q waiter, > - * but the rt_mutex's wait_list can be empty (either still, or again, > - * depending on which side we land). > - * > - * When this happens, give up our locks and try again, giving the > - * futex_lock_pi() instance time to complete and unqueue_me(). > - */ > if (!new_owner) { > - raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock); > - return -EAGAIN; > + /* > + * Since we held neither hb->lock nor wait_lock when coming > + * into this function, we could have raced with futex_lock_pi() > + * such that it will have removed the waiter that brought us > + * here. Hmm. That's not entirely correct. There are two cases: lock_pi() queue_me() <- Makes it visible as waiter in the hash bucket unlock(hb->lock) [1] rtmutex_futex_lock() [2] lock(hb->lock) Both [1] and [2] are valid reasons why the top waiter is not a rtmutex waiter. Thanks, tglx