Re: [tip PATCH v6 8/8] RFC: futex: add requeue_pi calls

Thomas Gleixner
2009-03-31T07:31:05Z

On Mon, 30 Mar 2009, Darren Hart wrote:
> +
> + if (requeue_pi) {
> + if (refill_pi_state_cache())
> + return -ENOMEM;
> + if (nr_wake != 1)
> + return -EINVAL;
> + }

Needs a comment

> retry:
> + if (pi_state != NULL) {
> + free_pi_state(pi_state);
> + pi_state = NULL;
> + }

Ditto

> + if (requeue_pi) {
> + /* Attempt to acquire uaddr2 and wake the top_waiter. */
> + ret = futex_proxy_trylock_atomic(uaddr2, hb1, hb2, &key1,
> + &key2, &pi_state);
> +
> + /*
> + * At this point the top_waiter has either taken uaddr2 or is
> + * waiting on it. If the former, then the pi_state will not
> + * exist yet, look it up one more time to ensure we have a
> + * reference to it.
> + */
> + if (ret == 1 && !pi_state) {
> + task_count++;
> + ret = get_futex_value_locked(&curval2, uaddr2);
> + if (!ret)
> + ret = lookup_pi_state(curval2, hb2, &key2,
> + &pi_state);
> + }
> +
> + switch (ret) {
> + case 0:
> + break;
> + case -EFAULT:
> + double_unlock_hb(hb1, hb2);
> + put_futex_key(fshared, &key2);
> + put_futex_key(fshared, &key1);
> + ret = get_user(curval2, uaddr2);
> + if (!ret)
> + goto retry;

Hmm. What happens if futex_proxy_trylock_atomic() succeeds, but
get_futex_value_locked() fails ? I guess its unlikely to happen, but
in case it does are we sure that our state is correct ?

> +
> + /* This can go after we're satisfied with testing. */
> + if (!requeue_pi)
> + WARN_ON(this->rt_waiter);

Keep those WARN_ONs. they are useful when code is changed later. Just make it

WARN_ON(!requeue_pi && this->rt_waiter);
WARN_ON(requeue_pi && !this->rt_waiter);

> + /*
> + * Requeue nr_requeue waiters and possibly one more in the case
> + * of requeue_pi if we couldn't acquire the lock atomically.
> + */
> + if (requeue_pi) {
> + /* This can go after we're satisfied with testing. */
> + WARN_ON(!this->rt_waiter);
> +
> + /* Prepare the waiter to take the rt_mutex. */
> + atomic_inc(&pi_state->refcount);
> + this->pi_state = pi_state;
> + ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
> + this->rt_waiter,
> + this->task, 1);
> + if (ret) {
> + this->pi_state = NULL;
> + free_pi_state(pi_state);
> + goto out_unlock;

What are the reasons of dropping out here ?

> + /*
> + * Ensure the requeue is atomic to avoid races while we process the
> + * wakeup. We only need to hold hb->lock to ensure atomicity as the
> + * wakeup code can't change q.key from uaddr to uaddr2 if we hold that
> + * lock. It can't be requeued from uaddr2 to something else since we
> + * don't support a PI aware source futex for requeue.
> + */
> + spin_lock(&hb->lock);
> + if (!match_futex(&q.key, &key2)) {
> + WARN_ON(q.lock_ptr && (&hb->lock != q.lock_ptr));
> + /*
> + * We were not requeued, handle wakeup from futex1 (uaddr). We
> + * cannot have been unqueued and already hold the lock, no need
> + * to call unqueue_me, just do it directly.
> + */
> + plist_del(&q.list, &q.list.plist);
> + drop_futex_key_refs(&q.key);
> +
> + ret = -ETIMEDOUT;
> + if (to && !to->task) {
> + spin_unlock(&hb->lock);
> + goto out_put_keys;
> + }
> +
> + /*
> + * We expect signal_pending(current), but another thread may
> + * have handled it for us already.
> + */
> + ret = -ERESTARTSYS;
> + if (!abs_time) {
> + spin_unlock(&hb->lock);
> + goto out_put_keys;
> + }

Hmm. Why do we use restart block here ? The timeout is absolute, so
we can just restart the syscall, can't we ?

> + ret = 0;
> + /*
> + * Check if the waker acquired the second futex for us. If the lock_ptr
> + * is NULL, but our key is key2, then the requeue target futex was
> + * uncontended and the waker gave it to us. This is safe without a lock
> + * as futex_requeue() will not release the hb lock until after it's
> + * nulled the lock_ptr and removed us from the hb.
> + */
> + if (!q.lock_ptr)
> + goto out_put_keys;

At this point we hold the rt_mutex, the pi state is correct and the
user space variable at *uaddr2 is updated, right ??

Thanks,

tglx