Set FUTEX_WAITERS under the bucket lock in LOCK_PI

src/runtime/futex.c

@@ -392,6 +392,16 @@ static futex_bucket_t *futex_waiter_follow(futex_bucket_t *b,
     }
 }
 
+/* Whether any waiter on @uaddr is queued in @b. Caller must hold b->lock. */
+static bool bucket_has_uaddr_locked(const futex_bucket_t *b, uint64_t uaddr)
+{
+    for (const futex_waiter_t *w = b->head; w; w = w->next) {
+        if (w->uaddr == uaddr)
+            return true;
+    }
+    return false;
+}
+
 /* Unlink the waiter at *pp from its bucket list and wake it. The bucket lock
  * must be held. Unlinking before the store/signal keeps a woken waiter from
  * observing itself still queued; the release store pairs with the waiter's
@@ -1547,44 +1557,62 @@ static int64_t futex_lock_pi_inner(guest_t *g,
         if (owner_tid != 0 && !thread_find((int64_t) owner_tid))
             return -LINUX_ESRCH;
 
-        /* Set the WAITERS bit so the owner takes the kernel-mediated unlock
-         * path. Retry the CAS in a loop since the owner may release
-         * concurrently.
+        /* Set FUTEX_WAITERS and enqueue under one hold of the bucket lock.
+         *
+         * The bit is what makes the owner release through FUTEX_UNLOCK_PI: its
+         * guest-side CAS(TID->0) fails while the bit is set. Parking on an
+         * owned word without it is therefore a lost wake, since that CAS
+         * succeeds and makes no syscall. Setting the bit before taking the
+         * lock, and then only checking that the word was still owned, allowed
+         * exactly that: the owner could release and retake the lock in guest
+         * code in between, and the word read as owned with the bit gone.
+         *
+         * Under the lock the two cannot come apart. Once the word is seen (or
+         * made) owned with the bit set, the owner can only release through
+         * FUTEX_UNLOCK_PI, which takes this bucket lock after its release and
+         * so finds the waiter enqueued below.
          */
+        pthread_mutex_lock(&b->lock);
+
+        uint32_t cur;
         for (;;) {
-            uint32_t cur;
-            if (!futex_word_load(word, &cur))
+            if (!futex_word_load(word, &cur)) {
+                pthread_mutex_unlock(&b->lock);
                 return -LINUX_EFAULT;
-            if (futex_pi_unowned(cur))
-                break; /* Owner released; retry outer loop */
-            if (futex_pi_has_waiters(cur))
-                break; /* Already set by another waiter */
-            uint32_t desired = futex_pi_set_waiters(cur);
+            }
+            if (futex_pi_unowned(cur) || futex_pi_has_waiters(cur))
+                break;
             bool marked;
-            if (!futex_word_cas(word, &cur, desired, &marked))
+            if (!futex_word_cas(word, &cur, futex_pi_set_waiters(cur),
+                                &marked)) {
+                pthread_mutex_unlock(&b->lock);
                 return -LINUX_EFAULT;
+            }
             if (marked)
-                break; /* WAITERS bit set */
+                break;
         }
 
-        /* Re-check after WAITERS bit: if lock is now free, retry */
-        uint32_t cur;
-        if (!futex_word_load(word, &cur))
-            return -LINUX_EFAULT;
-        if (futex_pi_unowned(cur))
-            continue;
-
-        /* Enqueue and block */
-        pthread_mutex_lock(&b->lock);
-
-        /* Double-check under bucket lock: owner may have released and called
-         * UNLOCK_PI between the current WAITERS set and lock.
-         */
-        if (!futex_word_load(word, &cur)) {
-            pthread_mutex_unlock(&b->lock);
-            return -LINUX_EFAULT;
-        }
         if (futex_pi_unowned(cur)) {
+            /* FUTEX_WAITERS with no TID never matches the fast path's
+             * CAS(0->TID), so take the word here, as futex_lock_pi_atomic does
+             * on Linux. The bit stays while another waiter is queued.
+             * FUTEX_OWNER_DIED is left to the recovery at the top.
+             */
+            if (cur != 0 && !futex_pi_owner_died(cur)) {
+                uint32_t want = tid;
+                if (bucket_has_uaddr_locked(b, uaddr))
+                    want = futex_pi_set_waiters(tid);
+                bool taken;
+                if (!futex_word_cas(word, &cur, want, &taken)) {
+                    pthread_mutex_unlock(&b->lock);
+                    return -LINUX_EFAULT;
+                }
+                if (taken) {
+                    pthread_mutex_unlock(&b->lock);
+                    return 0;
+                }
+            }
+            /* Owner released first, or the word moved: retry. */
             pthread_mutex_unlock(&b->lock);
             continue;
         }
@@ -1656,18 +1684,14 @@ static int64_t futex_lock_pi_inner(guest_t *g,
                                                  memory_order_acquire)) {
                     /* Timeout: dequeue and return */
                     bucket_unlink_locked(b, &waiter);
-                    /* Only clear WAITERS bit if no waiters for this address */
-                    bool has_waiters = false;
-                    for (futex_waiter_t *w = b->head; w; w = w->next) {
-                        if (w->uaddr == uaddr) {
-                            has_waiters = true;
-                            break;
-                        }
-                    }
+
+                    /* Clear under the lock: a waiter that enqueues once it is
+                     * dropped relies on the bit it reads staying set.
+                     */
+                    if (!bucket_has_uaddr_locked(b, uaddr))
+                        futex_clear_waiters_bit(word);
                     pthread_mutex_unlock(&b->lock);
                     pthread_cond_destroy(&waiter.cond);
-                    if (!has_waiters)
-                        futex_clear_waiters_bit(word);
                     return -LINUX_ETIMEDOUT;
                 }
             } else {

tests/test-futex-pi.c

@@ -15,6 +15,9 @@
  *      post-robust-walk state (FUTEX_OWNER_DIED set, TID cleared) must take
  *      the lock over and return 0, not spin on the CAS(0->TID) fast path.
  *
+ *   1c. FUTEX_LOCK_PI on a word with FUTEX_WAITERS set and no TID takes the
+ *      lock over, as Linux does, for the same reason.
+ *
  *   2. FUTEX_LOCK_PI + FUTEX_UNLOCK_PI round-trip: acquire and
  *      release a PI lock from the same thread.
  *
@@ -25,11 +28,19 @@
  *      must only return when a real wake arrives or a signal is
  *      genuinely queued for the thread.
  *
+ *   4. PI waiter against an owner that retakes the lock in guest code: two
+ *      threads take and release one PI lock in a tight loop, entering the
+ *      kernel only on contention, until 2000 acquisitions have gone through
+ *      FUTEX_LOCK_PI. A waiter that parks while the word has no FUTEX_WAITERS
+ *      is never woken, because the owner's next release is a guest-side CAS
+ *      that makes no syscall.
+ *
  * Syscalls exercised: futex(98), clone(220), gettid(178), exit(93)
  */
 
 #include <stdint.h>
 #include <string.h>
+#include <time.h>
 #include <unistd.h>
 #include <sys/mman.h>
 #include <sys/time.h>
@@ -252,6 +263,200 @@ static void test_pi_owner_died_recover(void)
     PASS();
 }
 
+/* Test: LOCK_PI takes a word that has FUTEX_WAITERS but no owner */
+
+static void test_pi_waiters_no_owner(void)
+{
+    TEST("PI LOCK_PI takes a WAITERS-only word");
+
+    /* Linux takes this word over (futex_lock_pi_atomic). The CAS(0->TID) fast
+     * path never matches it, so a retry loop without a takeover spins forever.
+     */
+    pi_lock = FUTEX_WAITERS;
+
+    long r = raw_futex_lock_pi((uint32_t *) &pi_lock);
+    if (r != 0) {
+        printf("FAIL: LOCK_PI on a WAITERS-only word expected 0, got %ld\n", r);
+        fails++;
+        return;
+    }
+
+    uint32_t tid = (uint32_t) raw_gettid();
+    uint32_t val = __atomic_load_n(&pi_lock, __ATOMIC_SEQ_CST);
+    if ((val & FUTEX_TID_MASK) != tid) {
+        FAIL("lock word doesn't contain our TID after takeover");
+        return;
+    }
+
+    raw_futex_unlock_pi((uint32_t *) &pi_lock);
+    PASS();
+}
+
+/* Test: a PI waiter is woken when the owner retakes the lock in guest code */
+
+#define RELOCK_CONTENDED 2000 /* FUTEX_LOCK_PI acquisitions to collect */
+#define RELOCK_BUDGET_SEC 10  /* Give up collecting after this long */
+#define RELOCK_DEADLINE_SEC 2 /* On one FUTEX_LOCK_PI call */
+
+static volatile uint32_t relock_word __attribute__((aligned(4))) = 0;
+static volatile long relock_count = 0;     /* Guarded by relock_word */
+static volatile long relock_iters = 0;     /* Loop passes of both threads */
+static volatile long relock_contended = 0; /* FUTEX_LOCK_PI calls that won */
+static volatile long relock_failure = 0;   /* First failed acquisition */
+static char relock_stack_buf[8192] __attribute__((aligned(16)));
+
+static long relock_clock(int clock, struct timespec *ts)
+{
+    return raw_syscall6(__NR_clock_gettime, clock, (long) ts, 0, 0, 0, 0);
+}
+
+/* The protocol glibc and Rosetta both use: take the lock with a guest-side CAS
+ * and enter the kernel only when that fails. The kernel call carries a deadline
+ * so that a waiter nobody wakes reports ETIMEDOUT rather than hanging the
+ * suite; with the lock changing hands every few microseconds, two seconds is
+ * never reached by a waiter that is woken.
+ */
+static long relock_acquire(uint32_t tid)
+{
+    uint32_t expected = 0;
+    if (__atomic_compare_exchange_n((uint32_t *) &relock_word, &expected, tid,
+                                    0, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
+        return 0;
+
+    /* FUTEX_LOCK_PI takes an absolute CLOCK_REALTIME deadline. */
+    struct timespec deadline;
+    long r = relock_clock(CLOCK_REALTIME, &deadline);
+    if (r != 0)
+        return r;
+    deadline.tv_sec += RELOCK_DEADLINE_SEC;
+    r = raw_syscall6(__NR_futex, (long) &relock_word,
+                     FUTEX_LOCK_PI | FUTEX_PRIVATE, 0, (long) &deadline, 0, 0);
+    if (r == 0)
+        __atomic_fetch_add((long *) &relock_contended, 1, __ATOMIC_SEQ_CST);
+    return r;
+}
+
+static void relock_release(uint32_t tid)
+{
+    /* Fails exactly when FUTEX_WAITERS is set, which is the kernel's way of
+     * asking for the syscall.
+     */
+    uint32_t expected = tid;
+    if (__atomic_compare_exchange_n((uint32_t *) &relock_word, &expected, 0, 0,
+                                    __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
+        return;
+    raw_futex_unlock_pi((uint32_t *) &relock_word);
+}
+
+/* Runs on both threads until they have collected RELOCK_CONTENDED kernel
+ * acquisitions between them. A thread running alone collects none, so the two
+ * need no start signal: whichever starts first keeps going until the other
+ * arrives. Raw syscalls only: the clone child has no TLS.
+ */
+static void relock_loop(void)
+{
+    uint32_t tid = (uint32_t) raw_gettid();
+    struct timespec now;
+    long stop_sec = 0;
+    long iters = 0;
+
+    if (relock_clock(CLOCK_MONOTONIC, &now) == 0)
+        stop_sec = now.tv_sec + RELOCK_BUDGET_SEC;
+
+    for (;;) {
+        if (__atomic_load_n((long *) &relock_failure, __ATOMIC_SEQ_CST) != 0 ||
+            __atomic_load_n((long *) &relock_contended, __ATOMIC_SEQ_CST) >=
+                RELOCK_CONTENDED)
+            break;
+
+        /* The budget ends a run that cannot contend, such as one CPU; the
+         * caller then fails on the count.
+         */
+        if ((iters & 1023) == 0 &&
+            (relock_clock(CLOCK_MONOTONIC, &now) != 0 || now.tv_sec > stop_sec))
+            break;
+
+        long r = relock_acquire(tid);
+        if (r != 0) {
+            long none = 0;
+            __atomic_compare_exchange_n((long *) &relock_failure, &none, r, 0,
+                                        __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
+            break;
+        }
+        relock_count = relock_count + 1;
+        relock_release(tid);
+        iters++;
+    }
+    __atomic_fetch_add((long *) &relock_iters, iters, __ATOMIC_SEQ_CST);
+}
+
+static void test_pi_owner_relock(void)
+{
+    TEST("PI waiter woken across relock");
+
+    relock_word = 0;
+    relock_count = 0;
+    relock_iters = 0;
+    relock_contended = 0;
+    relock_failure = 0;
+
+    void *stack_top = relock_stack_buf + sizeof(relock_stack_buf);
+    int child_tid_val = 0;
+    long ret = raw_clone(0x7d0f00, stack_top, &child_tid_val, 0,
+                         (int *) &child_tid_val);
+    if (ret < 0) {
+        FAIL("clone failed");
+        return;
+    }
+    if (ret == 0) {
+        relock_loop();
+        raw_exit(0);
+    }
+
+    relock_loop();
+
+    /* The child is at most one deadline plus one budget behind. */
+    for (int i = 0; i < (RELOCK_DEADLINE_SEC + RELOCK_BUDGET_SEC + 1) * 100;
+         i++) {
+        if (__atomic_load_n(&child_tid_val, __ATOMIC_SEQ_CST) == 0)
+            break;
+        usleep(10000);
+    }
+    if (__atomic_load_n(&child_tid_val, __ATOMIC_SEQ_CST) != 0) {
+        FAIL("relock child did not exit");
+        return;
+    }
+
+    long failure = __atomic_load_n((long *) &relock_failure, __ATOMIC_SEQ_CST);
+    if (failure != 0) {
+        printf(
+            "FAIL: lock acquisition returned %ld (-110: a parked waiter "
+            "was never woken)\n",
+            failure);
+        fails++;
+        return;
+    }
+
+    /* Fewer means the run could not contend enough to show a lost wake, and a
+     * pass would then say nothing about the kernel path.
+     */
+    long contended =
+        __atomic_load_n((long *) &relock_contended, __ATOMIC_SEQ_CST);
+    if (contended < RELOCK_CONTENDED) {
+        printf("FAIL: %ld of %d FUTEX_LOCK_PI acquisitions in %d s\n",
+               contended, RELOCK_CONTENDED, RELOCK_BUDGET_SEC);
+        fails++;
+        return;
+    }
+    if (relock_count != relock_iters) {
+        printf("FAIL: %ld increments under the lock, expected %ld\n",
+               relock_count, relock_iters);
+        fails++;
+        return;
+    }
+    PASS();
+}
+
 /* Test 3: futex_wait without a signal blocks until woken */
 
 /* Sibling that waits ~1.2 s, flips the futex word, and issues FUTEX_WAKE on the
@@ -389,6 +594,8 @@ int main(void)
     test_futex_eintr();
     test_futex_unaligned();
     test_pi_owner_died_recover();
+    test_pi_waiters_no_owner();
+    test_pi_owner_relock();
     test_pi_dead_owner(); /* Last: uses CLONE_THREAD which may hang on x64 */
 
     SUMMARY("test-futex-pi");