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