On Wed, Mar 22, 2017 at 11:35:55AM +0100, Peter Zijlstra wrote:
> There's a number of 'interesting' problems, all caused by holding
> hb->lock while doing the rt_mutex_unlock() equivalient.
>
> Notably:
>
> - a PI inversion on hb->lock; and,
>
> - a DL crash because of pointer instability.
A DL crash? What is this? Can you elaborate a bit?
>
> Because of all the previous patches that:
>
> - allow us to do rt_mutex_futex_unlock() without dropping wait_lock;
> which in turn allows us to rely on wait_lock atomicy.
>
> - changed locking rules to cover {uval,pi_state} with wait_lock.
>
> - simplified the waiter conundrum.
>
> We can now quite simply pull rt_mutex_futex_unlock() out from under
> hb->lock, a pi_state reference and wait_lock are sufficient.
OK, owe. I think I've traced most of this through. I have a few gray areas
still, and will continue through the series to see if that addresses them.
A few thoughts as they occurred to me below.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> kernel/futex.c | 154
> +++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 100 insertions(+), 54 deletions(-)
>
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
...
> @@ -1380,48 +1387,40 @@ 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)
> +/*
> + * 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;
Nit: Based on what I've seen from Thomas and others, I ask for declarations in
decreasing order of line length. So deboost should have stayed where it was.
>
> /*
> @@ -2232,7 +2229,8 @@ static int fixup_pi_state_owner(u32 __us
> /*
> * We are here either because we stole the rtmutex from the
> * previous highest priority waiter or we are the highest priority
> - * waiter but failed to get the rtmutex the first time.
> + * waiter but have failed to get the rtmutex the first time.
> + *
> * We have to replace the newowner TID in the user space variable.
> * This must be atomic as we have to preserve the owner died bit here.
> *
> @@ -2249,7 +2247,7 @@ static int fixup_pi_state_owner(u32 __us
> if (get_futex_value_locked(&uval, uaddr))
> goto handle_fault;
>
> - while (1) {
> + for (;;) {
As far as I'm aware, there is no difference and both are used throughout the
kernel (with the while version having 50% more instances). Is there more to this
than personal preference?
> newval = (uval & FUTEX_OWNER_DIED) | newtid;
>
> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
> @@ -2345,6 +2343,10 @@ static int fixup_owner(u32 __user *uaddr
> /*
> * Got the lock. We might not be the anticipated owner if we
> * did a lock-steal - fix up the PI-state in that case:
> + *
> + * We can safely read pi_state->owner without holding wait_lock
> + * because we now own the rt_mutex, only the owner will attempt
> + * to change it.
This seems to contradict the Serialization and lifetime rules:
+ * pi_mutex->wait_lock:
+ *
+ * {uval, pi_state}
+ *
+ * (and pi_mutex 'obviously')
It would seem that simply holding pi_mutex is sufficient for serialization on
pi_state->owner then.
...
> @@ -2738,10 +2748,36 @@ static int futex_unlock_pi(u32 __user *u
> */
> top_waiter = futex_top_waiter(hb, &key);
> if (top_waiter) {
> - ret = wake_futex_pi(uaddr, uval, top_waiter, hb);
> + struct futex_pi_state *pi_state = top_waiter->pi_state;
> +
> + ret = -EINVAL;
> + if (!pi_state)
> + goto out_unlock;
> +
> + /*
> + * 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)
> + goto out_unlock;
> +
> + /*
> + * Grab a reference on the pi_state and drop hb->lock.
> + *
> + * The reference ensures pi_state lives, dropping the hb->lock
> + * is tricky.. wake_futex_pi() will take rt_mutex::wait_lock to
> + * close the races against futex_lock_pi(), but in case of
> + * _any_ fail we'll abort and retry the whole deal.
s/fail/failure/
--
Darren Hart
VMware Open Source Technology Center
