diff --git a/src/runtime/futex.c b/src/runtime/futex.c index 6f93835c..e8faff0d 100644 --- a/src/runtime/futex.c +++ b/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 { diff --git a/tests/test-futex-pi.c b/tests/test-futex-pi.c index e9c62258..2093549a 100644 --- a/tests/test-futex-pi.c +++ b/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 #include +#include #include #include #include @@ -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");