On Wed, Mar 22, 2017 at 11:35:51AM +0100, Peter Zijlstra wrote:
> Part of what makes futex_unlock_pi() intricate is that
> rt_mutex_futex_unlock() -> rt_mutex_slowunlock() can drop
> rt_mutex::wait_lock.
>
> This means we cannot rely on the atomicy of wait_lock, which we would
> like to do in order to not rely on hb->lock so much.
>
> The reason rt_mutex_slowunlock() needs to drop wait_lock is because it
> can race with the rt_mutex fastpath, however futexes have their own
> fast path.
>
> Since futexes already have a bunch of separate rt_mutex accessors,
> complete that set and implement a rt_mutex variant without fastpath
> for them.
Premise makes sense, I'm tripping over some detail - wondering if it is all
related...
>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
> kernel/futex.c | 30 ++++++++++-----------
> kernel/locking/rtmutex.c | 55
> +++++++++++++++++++++++++++++-----------
> kernel/locking/rtmutex_common.h | 9 +++++-
> 3 files changed, 62 insertions(+), 32 deletions(-)
>
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -916,7 +916,7 @@ void exit_pi_state_list(struct task_stru
> pi_state->owner = NULL;
> raw_spin_unlock_irq(&curr->pi_lock);
>
> - rt_mutex_unlock(&pi_state->pi_mutex);
> + rt_mutex_futex_unlock(&pi_state->pi_mutex);
>
> spin_unlock(&hb->lock);
>
> @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad
> pi_state->owner = new_owner;
> raw_spin_unlock(&new_owner->pi_lock);
>
> - raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
> -
> - deboost = rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
> -
> /*
> - * First unlock HB so the waiter does not spin on it once he got woken
> - * up. Second wake up the waiter before the priority is adjusted. If we
> - * deboost first (and lose our higher priority), then the task might get
> - * scheduled away before the wake up can take place.
> + * 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.
> */
> + 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);
> - wake_up_q(&wake_q);
> - if (deboost)
> +
> + 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.
--
Darren Hart
VMware Open Source Technology Center
