Skip to content

fix(libkernel): spin and yield when a condition variable wait relocks its mutex - #13725

Open
cocoa55 wants to merge 2 commits into
boykopovar:mainfrom
cocoa55:fix/cond-relock-spin
Open

cocoa55 wants to merge 2 commits into
boykopovar:mainfrom
cocoa55:fix/cond-relock-spin

Conversation

@cocoa55

@cocoa55 cocoa55 commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

What

A woken scePthreadCondWait/scePthreadCondTimedwait (and the pthread_cond_* calls that use them) now takes its mutex back through the same path as scePthreadMutexLock: it tries once, spins and yields by the mutex's spin and yield counts, then blocks. Before this change, WakingLock::lock() in Cond.cpp called the native lock directly. FreeBSD 11.4 re-locks through _mutex_cv_lock, which calls mutex_lock_common, so a woken waiter spins and yields like any other lock. This is a follow-up to #13544.

MutexOperations::SpinAcquire(PthreadMutex, std::timed_mutex&) and its std::recursive_timed_mutex& overload expose Mutex.cpp's existing spin path. The re-lock of a recursive mutex's outer level spins; the inner levels the owner takes back don't. The condition variable clause is removed from the libkernel.md debt entry.

This fixes behaviour only and makes no speed claim.

Tested

Linux (CachyOS, kernel 7.3, GCC, Release, i5-9400F). guest_pthread_mutex_spin adds a token ring: 4 guest threads take 300 turns each through one condition variable, broadcasting after every turn, so every wake re-locks the mutex under contention. It runs for all four mutex types with 2000 spins and 4 yields, and with neither; with scePthreadCondWait and scePthreadCondTimedwait; and at recursive depth 2 for the recursive type. It checks that no two threads are ever inside together and that the turn and total counts are exact.

These pass: all 56 ^guest_pthread tests, guest_cond_timedwait, guest_cond_recursive_mutex, guest_cond_unowned_mutex, both guest_libc_cnd_* tests and all 29 guest_job_manager_* tests. tools/check_conventions.py passes. tools/pr_overlap.py finds no shared lines: #9326 and #12642 change other functions in Mutex.cpp. Not run on Windows or with a title.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

🤖 Generated with Claude Code

… its mutex

FreeBSD 11.4's _mutex_cv_lock takes the mutex back through
mutex_lock_common, so a woken waiter tries once, spins and yields by
the mutex's counts before it blocks. The wait here called the native
lock directly; it now goes through the same spin and yield path as
scePthreadMutexLock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked — new head, test evidence needed

I reviewed head 16375634e31e44e35ff62a7ab513811d5249b292 and found no source-level blocker, but the new condition-wait test has not been verified by the hosted run.

The Windows job passed its build, but its log says No guest core/libs test sources changed; the changed-test runner therefore did not execute guest_pthread_mutex_spin. Linux and specialized jobs were skipped. The PR description reports Linux test results but does not identify the tested commit SHA, so I cannot tie those results to this head.

Please provide the exact tested SHA and output for guest_pthread_mutex_spin and the focused condition/mutex tests, or arrange for CI to select and run the changed test. I will recheck the same evidence on the updated head.

@JJenkx

JJenkx commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Windows run of this head (16375634e3), merged onto main 7351ac7eb1 (clean merge, local only). Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, ANYPS5_ENABLE_SPIRV_TOOLS=ON, no Vulkan driver visible.

  • This PR: guest_pthread_mutex_spin passes 3 of 3 (ctest --repeat until-fail:3).
  • Negative control: not possible. With this PR's source files (core/libs/prx/libkernel/Pthread/include/Mutex.hpp, core/libs/prx/libkernel/Pthread/src/Cond.cpp, core/libs/prx/libkernel/Pthread/src/Mutex.cpp) put back to main's version, the build fails against the rest of the PR, so there is no source-only control.

AI-assisted: yes (Claude Code).

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 4630 docs/dev/debt/libkernel.md
PR 5021 docs/dev/debt/libkernel.md
PR 6149 docs/dev/debt/libkernel.md
PR 6936 docs/dev/debt/libkernel.md
PR 7088 docs/dev/debt/libkernel.md
PR 7105 docs/dev/debt/libkernel.md
PR 7121 docs/dev/debt/libkernel.md
PR 7135 docs/dev/debt/libkernel.md
PR 7149 docs/dev/debt/libkernel.md
PR 7192 docs/dev/debt/libkernel.md
PR 7447 docs/dev/debt/libkernel.md
PR 7464 docs/dev/debt/libkernel.md
PR 7509 docs/dev/debt/libkernel.md
PR 7804 docs/dev/debt/libkernel.md
PR 8605 docs/dev/debt/libkernel.md
PR 9156 docs/dev/debt/libkernel.md
PR 9199 docs/dev/debt/libkernel.md
PR 9242 docs/dev/debt/libkernel.md
PR 9293 docs/dev/debt/libkernel.md
PR 9297 docs/dev/debt/libkernel.md
PR 9302 docs/dev/debt/libkernel.md
PR 9326 core/libs/prx/libkernel/Pthread/src/Mutex.cpp
docs/dev/debt/libkernel.md
PR 9389 docs/dev/debt/libkernel.md
PR 9557 docs/dev/debt/libkernel.md
PR 9566 docs/dev/debt/libkernel.md
PR 9843 docs/dev/debt/libkernel.md
PR 9938 docs/dev/debt/libkernel.md
PR 10025 docs/dev/debt/libkernel.md
PR 10442 docs/dev/debt/libkernel.md
PR 10558 docs/dev/debt/libkernel.md
PR 10645 docs/dev/debt/libkernel.md
PR 10789 docs/dev/debt/libkernel.md
PR 11002 docs/dev/debt/libkernel.md
PR 11215 docs/dev/debt/libkernel.md
PR 11939 docs/dev/debt/libkernel.md
PR 12642 core/libs/prx/libkernel/Pthread/src/Mutex.cpp
PR 12688 docs/dev/debt/libkernel.md
PR 12700 docs/dev/debt/libkernel.md
PR 12925 docs/dev/debt/libkernel.md
PR 12965 docs/dev/debt/libkernel.md

@cocoa55

cocoa55 commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Tested SHA: 16375634e31e44e35ff62a7ab513811d5249b292 (clean tree), Linux (CachyOS, kernel 7.3), GCC 16.2.1, Release, i5-9400F.

  • ctest -R guest_pthread_mutex_spin --repeat until-fail:20: 20 of 20 passed (0.08–0.09 s each).
  • ctest -R "^guest_(pthread_mutex|pthread_timed_locks|cond_)": 7 of 7 passed: guest_pthread_mutexattr, guest_pthread_mutex_errors, guest_pthread_mutex_spin, guest_cond_timedwait, guest_cond_recursive_mutex, guest_cond_unowned_mutex, guest_pthread_timed_locks.
  • ctest -R "^guest_libc_cnd_": 2 of 2 passed.

Negative control: with only Cond.cpp reverted to main (the one file that changes behaviour; Mutex.hpp/Mutex.cpp only add the overloads), the build succeeds and guest_pthread_mutex_spin still passes 20 of 20. So the ring test guards correctness: mutual exclusion, owner and recursive depth after a woken wait re-locks with spin and yield counts set. It does not detect the spinning itself, which can't be observed through the public API. The spin and yield sequence is covered by the SpinAcquire checks from #13544, and the re-lock now calls that same function.

On CI: the PR does change core/libs/tests/GuestPthreadMutexSpin.cpp, so the "No guest core/libs test sources changed" line suggests the runner's selection missed a modified (not added) test file.

Thanks to @JJenkx for the Windows run (3 of 3 on this head).

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@cocoa55 Reviewed at 16375634e3. Please add a deterministic regression check for the condition-variable relock using the configured try/spin/yield path.

The expanded GuestPthreadMutexSpin test passes against both this head and main's Cond.cpp/Mutex.cpp in the same focused native build. The token ring verifies mutual exclusion, recursion and progress, but native blocking already provides those guarantees. It therefore stays green when the relock still calls native.lock() directly, which is the behavior this PR changes. The existing injected callback-count checks cover the generic spin helper, not its connection to a condition-variable wake.

Please keep the ring coverage and add a test seam or observable callback/count check that distinguishes the relock path without timing assumptions. Show it passing with this change and failing when only WakingLock::lock() is reverted to the direct native lock. Cover the recursive outer relock as well as a nonrecursive mutex; the restored inner recursion levels should not spin again.

The mutex yield step counts each yield in a counter that
MutexYieldsForTest_nid_no_patch reads. With no spins and a yield count
of 5, a waiter woken while another thread holds its mutex must yield
exactly 5 times before it blocks, for each mutex type and for the outer
level of a recursive mutex held twice. With WakingLock::lock calling
the native lock directly the count stays 0 and the test aborts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cocoa55

cocoa55 commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Added in 8c4cdf8. The mutex yield step now counts each yield in a relaxed atomic that MutexYieldsForTest_nid_no_patch() reads (same pattern as FileSynchronousWritesForTest_nid_no_patch). CheckCondRelockYields sets 0 spins and 5 yields; the waiter locks, flags and waits; the main thread locks (possible only once the waiter is in the wait) and signals while holding the mutex, so the woken waiter must take the relock path. It waits on the counter, with no sleep, until it reaches 5, unlocks, joins, and requires the final count to be exactly 5. The 5 s deadline only bounds the failing case. It runs for types 1–4 at depth 1 and for a recursive mutex held twice, where the count is still exactly 5: the outer relock yields and the inner level, which calls _rmtx.lock() directly, adds none. (An owner relock succeeds on its first try, so the count shows the inner level adds no yields; it can't tell a direct lock from a spin that succeeds at once.) The ring coverage is unchanged.

At 8c4cdf8 (Linux, CachyOS kernel 7.3, GCC 16.2.1, Release): guest_pthread_mutex_spin 20 of 20 with --repeat until-fail:20; all 56 ^guest_pthread, 3 guest_cond_ and 29 guest_job_manager_ tests pass. With only WakingLock::lock() reverted to { native.lock(); }, it builds and guest_pthread_mutex_spin aborts after 5.10 s with the count at 0.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants