Repository navigation
feat(libkernel): implement the umutex, condition variable and rwlock _umtx_op operations as measured on a PS5 - #10789
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head eeb5cf0cc0e8cdf1ff73ee03ed6c1b0abe80c91c. The changed source and test translation units pass GCC 12.2 syntax checks with warnings as errors (suppressing pre-existing unused-parameter/unused-variable warnings); git diff --check and the current-main conventions check pass. I built and ran the exact GuestUmtxOp.cpp test with the changed SyncOnAddress.cpp and real TimedWait.cpp, using temporary host hooks for guest thread ID, guest errno, clock and trace calls: 5/5 runs passed, and the same focused suite passed under ASan+UBSan. The PR's hosted product jobs are path-skipped.
I found a functional omission in the read-lock flags. _umtx_op passes its value to RwlockLock only indirectly today: the dispatcher calls RwlockLock(rwlock, writer, ...) and drops value, while the helper derives reader preference only from rwlock->flags. FreeBSD stable/11's do_rw_rdlock uses the operation's fflag as well as rw_flags when deciding whether URWLOCK_WRITE_WAITERS blocks a reader (source). The PR says reader preference affects writer-before-reader behavior, but none of its tests pass that operation flag.
I reproduced the missing case against the exact PR source: with rwlock.state = URWLOCK_WRITE_WAITERS, rwlock.flags = 0, and a short timeout, _umtx_op(RW_RDLOCK, value = URWLOCK_PREFER_READER, ...) returns ETIMEDOUT and leaves state 0x60000000 instead of taking the read lock. Please thread the read-lock value into the helper and add this regression, or provide a PS5 measurement showing the console ignores this flag. Until then the advertised reader-preference behavior is incomplete.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@MarcZX10 good catch, thanks. Measured on a PS5 (the console's
So the console honours the flag from either place, as |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the updated head 2900cecebe859508f4c51a725e1b8662e2a4bd7c. The implementation now passes the RW_RDLOCK operation flags into RwlockLock and combines URWLOCK_PREFER_READER with the rwlock flags. The regression covers the four reported combinations, including reader preference supplied by the operation and a writer-owner timeout. I rebuilt the changed source, test, and TimedWait translation units with GCC 12.2, linked the focused GuestUmtxOp harness with temporary host hooks, and ran it five times successfully. tools/check_conventions.py and git diff --check also pass against current main. The author reports matching PS5 measurements for the same four cases. Hosted product jobs are skipped by PR workflow policy.
|
Stress check of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@rooklz In addition to the rwlock lost wakeup in #10789 (comment) (still open on
Could you either reject the unsupported mutex before inserting the waiter, or remove the entry on every exceptional exit? A test that checks the queue is clean and reusable after that failure would cover it, without asserting that the priority-protected form has to throw. |
|
Both fixed at @wachi35676's rwlock lost-wakeup: applied your CAS-guarded fix exactly ( @oneandonlydean's dangling Added |
|
@rooklz Thanks, the CAS-guarded waiter publish in
|
|
Both fixed at `6ffb86cbe6`.
I couldn't reproduce the second race empirically even after 60 extended runs of the widened stress test (8 threads, 6000 shorter rounds, write eligibility spread across all threads) with the fix reverted — the window is evidently narrower than what this harness's scheduling can reliably hit. The fix is a direct translation of the mechanism you described (re-validate after the unsynchronized clear, same as the first fix re-validated after a failed CAS), and it's cheap on the common path (one extra load, only taken when `left == 0`). With the fix restored, 20/20 consecutive runs of the widened stress test pass, as do the other suites. `check_conventions.py` passes. |
|
@rooklz Thanks, the condvar test now only checks
There is also a gap when B has already counted: C can unlock after A clears the bit but before A restores it, and restoring the bit after the last unlock sends no wake. The predicate runs under the bucket mutex but A's decrement, clear and recheck don't. Two ways to close it: do A's |
|
Good catch, and the barrier-test suggestion is right that the stress test can't reliably force this. Took the first option: the whole cleanup (`fetch_sub`, `ClearRwlockBits`, recheck) now runs under `BucketOf(key).mutex` at `2210ddc367` — the same mutex `WaitOnKeyLocked`'s predicate holds for its own CAS and `blocked.fetch_add`. Since both critical sections now share that lock, there's no window left for them to interleave at all: either the new waiter's full publish (CAS + fetch_add) completes first, so the cleanup's recheck sees it and restores the bit, or the cleanup finishes first (bit cleared, count still 0), so the new waiter runs its predicate against a clean post-clear state and sets its own bit from scratch. No partial interleaving is possible either way, so this closes it structurally rather than narrowing the window further. 25/25 runs of the widened stress test still pass, as do the other suites. `check_conventions.py` passes. |
2210ddc to
491fe94
Compare
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open PRs #11945, #11899, #11859, #11772, #11087, #11627, #11354, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
@oneandonlydean when you have a moment: the timed-out-writer window from your last comment is addressed in |
|
Re-check of our rwlock lost-wakeup report on head
From my side the lost wakeup is fixed. AI-assisted: yes (Claude Code); results above were run on this machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 491fe942705cef60c011a19222ef6650ac9b3646 against live main 4c7d275bef89d309313d16bc2bc05d37a1b50be2.
The three follow-up commits address the reported lost-wakeup race structurally: the wait predicate publishes its waiter bit/count under BucketOf(key).mutex, and last-waiter cleanup now decrements, clears, and rechecks under that same mutex. A new waiter therefore cannot publish between the clear and recheck. I also checked the priority-protected condition-wait guard: it throws before deadline parsing or incrementing hasWaiters, and the regression checks that no waiter state leaks.
Current-main checks:
git merge-tree --write-tree: clean.git diff --check: passed.python3 tools/check_conventions.py --base upstream/main --head refs/review/upstream-10789: passed.- Main has changed
SyncOnAddress.cppsince the PR base, but those hunks are in the standalone SyncOnAddress wait/wake API; the umtx wait predicate/cleanup changes reviewed here remain intact.
Validation evidence: wachi35676 reports guest_umtx_op 10/10 on Windows and Linux, the 4-thread lost-wakeup stress 10/10 on both, 2- and 8-thread stress 3/3, and the mutex/condvar stress passing. The Windows negative control with the fix commits reversed fails 3/3 and reproduces the stuck waiter state. I did not rerun those platform tests locally; hosted product jobs are skipped.
No blocking finding remains.
491fe94 to
0fb91fb
Compare
…_umtx_op operations as the console does
… the console does
RwlockLock set its waiter bit unconditionally after deciding the lock was unheld, so an unlock landing in that window woke nobody and the waiter published its bit onto an already-free lock and slept forever. Set the bit with a CAS against the value just checked, retrying the freedom check on failure, as the mutex path already does for UMUTEX_CONTESTED. ConditionWait queues its stack waiter and drops the bucket lock before calling MutexUnlock; MutexUnlock throws for a priority-protected umutex, which unwound past the cleanup that removes the queue entry, leaving a dangling pointer for a later ConditionWake to use. Reject the unsupported mutex before the waiter is queued. Depends on: none
…r test The last waiter to leave RwlockLock cleared the waiter bit outside the bucket lock. If a new waiter published that same bit and incremented the blocked count in the window between the decrement and the clear, the clear erased its bit and a later unlock woke nobody. Re-check the blocked count after clearing and restore the bit if a waiter appeared in that window. ConditionWaitRejectsAPriorityProtectedMutexWithoutLeakingAWaiter also required the priority-protected wait to throw; a condition wait on such a mutex is a valid, merely unsupported form, so only the queue cleanliness is asserted now. Extended the rwlock stress test to 8 threads and more, shorter rounds to give the window more chances to show up. Depends on: none
The last waiter's fetch_sub, ClearRwlockBits and recheck ran outside the bucket mutex the predicate runs under, so a new waiter's bit CAS and blocked.fetch_add could still land inside that window: the clear erased a bit the new waiter had just set, and the recheck could see blocked == 0 before the new waiter's fetch_add became visible. Run the whole cleanup under BucketOf(key).mutex, the same lock the predicate holds for its CAS and fetch_add, so the two can no longer interleave. Depends on: none
0fb91fb to
83b1015
Compare
|
Rebased onto current |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The rebased head 83b1015c4fb99e527b72f458848d0230094758a1 preserves the previously approved implementation and tests; its current-main preview is clean, conventions pass, and hosted Windows passes. The reported 540/540 CTest run is author-provided; I did not build or run it locally.
What
_umtx_opthrew for the umutex, condition variable and read-write lock operations. 26 of the 67 title and homebrew executables I have import_umtx_op, and these are the operations FreeBSD's libthr builds its mutexes, condition variables and rwlocks on. This implements them as a payload process on a PS5 (firmware 13.40) answers:MUTEX_TRYLOCK(4), free / contested-freeMUTEX_TRYLOCK/MUTEX_LOCK(5), owned by the callerUNLOCKfrees it)MUTEX_TRYLOCK, owned by another threadEBUSYMUTEX_LOCK/MUTEX_WAIT(17), owned by another, 1 ms timeoutETIMEDOUT,UMUTEX_CONTESTEDleft setMUTEX_UNLOCK(6), not the owner / unownedEPERMMUTEX_WAIT, unownedMUTEX_WAKE(18), contested, no waitersUMUTEX_PRIO_INHERITtrylock/unlockUMUTEX_PRIO_PROTECTtrylockEINVALSET_CEILING(7) on a non-PP objectEINVALCV_SIGNAL(9) /CV_BROADCAST(10), no waitersc_has_waiters0CV_WAIT(8), 1 ms timeoutETIMEDOUT; the mutex is left unlocked,c_has_waiters0RW_RDLOCK(12) /RW_WRLOCK(13) /RW_UNLOCK(14), free0x80000000/ 0RW_RDLOCKagainst a writer, 1 ms timeoutETIMEDOUT,URWLOCK_READ_WAITERSleft setRW_WRLOCKagainst a reader, 1 ms timeoutETIMEDOUT, no waiter bit left,rw_blocked_writers0RW_UNLOCK, unlockedEPERMThe owner a mutex records is
scePthreadGetthreadid, which on the console equalsthr_self. Two answers differ from FreeBSD 11's kern_umtx.c, and this follows the console: relocking a mutex the caller owns succeeds instead ofEBUSY, and a reader that times out leavesURWLOCK_READ_WAITERSset. Blocking, contention and wakeups (who is woken on unlock,UMUTEX_CONTESTEDwhen more than one waiter remains, writers before readers unlessURWLOCK_PREFER_READER) follow kern_umtx.c.CV_WAITqueues the waiter before it unlocks the mutex, so a signal sent between the unlock and the sleep is not lost. Its timeout is atimespecinuaddr2, relative unlessCVWAIT_ABSTIME, onc_clockidwithCVWAIT_CLOCKID.Robust umutexes, unlocking or setting the ceiling of a priority-protected one,
MUTEX_WAKE2and theSEM2,SHMand robust-list operations still throw. Priority inheritance is not applied, andCVWAIT_CHECK_UNPARKINGis ignored. The debt entry records this.Tested
_umtx_opfrom a payload: the table above.guest_umtx_opgainsMutexOwnershipFollowsTheConsole,MutexLockWaitsForTheOwnerToUnlock,ConditionWaitReleasesTheMutexAndWakes,RwlockStateFollowsTheConsoleandRwlockUnlockWakesTheBlockedWriter; the old check thatMUTEX_LOCKthrows is replaced by operation 7'sEINVAL. Linux x86-64 container (Ubuntu 24.04, GCC 13.3, Release): passes 5 of 5 runs.pr_overlap_encoding, ismain's on that runner.check_conventions.pypasses. feat(libkernel): resolve syscall, equeue, shm and threading stubs #9823 also edits_umtx_op'sdefault:lines (it swaps theNotImplemented_nid_no_patchthere for a plain throw). It implements none of these operations, but whichever lands second will need a small rebase there.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)