agc_driver_recorder_tests (full run): allocation stays pinned for the whole lease wait since cf5c3ebe, on NVIDIA and RADV #2833

Activity

1. fma-rc commented on Oct 10, 2026

   

   fma-rc

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Follow-up with the cause. Runs are on main 78aedb4, unmodified build, RTX 5080 and RADV.

   Where: unitShadowTests in tests/Recorder.cpp, at the mutation.Remove(block) of the "retire through the registry" step (line 1816). The test thread holds GpuMutex there. A second full wait follows from the block's unregistration at scope exit, which is why the run takes two lease waits.

   Measured

   Run	Result
   APS5_PIN_WAIT_MS=3000	same still pinned stop, after 6 s (two 3 s waits)
   APS5_RELEASE_UNDER_LOCK=1	no wait; the run goes on and stops at once on the retire published a unit whose memory is no longer registered (tests/Recorder.cpp:1823)

   Same on both devices.

   What holds the pin (from reading the code, consistent with the second row):

   1. cf5c3eb changed HostImport::range from weak_ptr to shared_ptr (Graphics/include/GuestBufferMemory.hpp), so a retired import now keeps the registered range alive.
   2. retireImport (GuestBufferMemory.cpp:200) hands the RetiredImport holder to recorder->Keep when the recorder is not idle. Here it is not: the shadow units were just published.
   3. The pin waiter, on a thread that holds GpuMutex, calls recorder->Sync(). But finish (Graphics/src/Recorder.cpp:3571) does not destroy a batch's kept objects on a thread that holds GpuMutex; it moves them to that thread's deferred list, released by the thread's own outermost unlock (ReleaseDeferredKeeps).
   4. That unlock cannot happen while the thread waits for the lease, so the holder, and with it the range, stays alive until the wait gives up.

   So the wait is made on the thread that must do the release. APS5_RELEASE_UNDER_LOCK=1 destroys kept objects in finish, which is why the wait disappears with it.

   The next stop with that switch looks like a test expectation the same commit made stale, but I have only read this, not shown it: the comment above the step expects the import to retire at the next lookup, after the block was re-registered as five units. Since cf5c3eb it retires inside Remove, while all of the block is still registered, so unit 5 is published.

   Not established: whether any product path removes a guest allocation on a thread that holds GpuMutex with the recorder busy (the waiter's own comment says the mutating thread "may be a driver thread holding it"); I did not trace the callers, and no title run shows it.
2. Silverash-i8 commented on Oct 10, 2026

   

   Silverash-i8

   on Oct 10, 2026

   Contributor

   More actions

   Windows data point, mostly so nobody reads a Windows pass as "not affected": the full run can't reach this there.

   main 57f96fd, Windows 11 (26200), WinLibs GCC 15.2.0 r7, Release, ANYPS5_REQUIRE_VULKAN=1 agc_driver_recorder_tests with no arguments. On an RTX 5060 Laptop GPU (32.0.16.1742) and an Arrow Lake iGPU (32.0.101.8991) it exits 0 in about 2 s, with no pin wait. But the run prints

   host imports are compared, not watched: unit shadows not tested
   host imports are compared, not watched: snapshot patching not tested
   host imports are compared, not watched: draw snapshot reuse not tested
   

   so unitShadowTests returns at Recorder.cpp:1891 on 57f96fd, before the mutation.Remove(block) at line 2021 where you see the stall (your line 1816 on 78aedb4). On Windows host imports go through comparisons rather than the write watch, so this only shows Windows hosts can't catch it, not that the change is fine there.

   AI-assisted (Claude Code); the numbers above are from runs on this machine.
3. MarcZX10 commented on Oct 10, 2026

   

   MarcZX10

   on Oct 10, 2026

   Contributor

   More actions

   I opened PR #2864 with the fix and a focused agc_unit_shadow CTest for the retirement sequence you traced. It flushes the current thread's deferred recorder keeps after synchronization, before the GuestAllocations waiter can deadlock waiting for that same thread to unlock. The test also updates the expected bytes for units that are still covered by the old full registration when retirement occurs.

   Validation so far:

   • Linux x86-64 GCC 12.2 Release build of agc_driver_recorder_tests: passed.
   • agc_unit_shadow: skipped on this host because asynchronous userfaultfd write watch is unavailable; this means I have not yet run the regression sequence on a capable Linux host.
   • PR conventions, intake, Linux/macOS register-context, both macOS write-watch jobs, and all three relinker jobs: passed. Full Linux and Windows PR builds were skipped under the repository workflow policy.

   If you can run PR #2864's focused test on one of the affected Linux setups, the exact command is:

   ctest --test-dir build --output-on-failure -R '^agc_unit_shadow$'

   The CTest sets APS5_PIN_WAIT_MS=3000 and has a 30-second timeout. A pass or a failure log from that test would help verify the fix on the write-watch path this host cannot exercise.
4. fma-rc commented on Oct 10, 2026

   

   fma-rc

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Closing my open point above ("whether any product path removes a guest allocation on a thread that holds GpuMutex"): I found none, so this looks like a test-only stop, not a title hang.

   Checked on main 57f96fd with git grep over core, tests excluded:

   Question	Result
   Files outside core/libs/prx/libSceAgcDriver that mention GpuMutex	none
   Non-test files that construct a GuestAllocations::Mutation	libc/src/GuestHeap.cpp, libkernel/DirectMemory/DirectMemory.cpp, libkernel/Module/src/DynamicLoader.cpp
   Non-test driver code that calls GuestHeapFree, GuestHeapRealloc, munmap_nid, mprotect_nid, DoMunmap or the GuestAllocationsRemove/Unmap/Protect exports	none

   So every registry mutation that can reach the pin wait starts in libc or libkernel, on a thread that cannot hold GpuMutex through those files, and the waiter then takes its own lock_guard (GuestBufferMemory.cpp, the branch for a thread that does not hold the lock). That guard's unlock releases the deferred kept objects, which is the step the test thread cannot reach while it holds the lock itself at mutation.Remove(block).

   Not checked: a guest signal or exception handler that runs on a thread while it is inside the driver and frees or unmaps memory there. Nothing above rules that out, and I have not tried to produce it.

   That leaves the two test-side points from the earlier comment: the full agc_driver_recorder_tests run is not a registered ctest, and it has not passed at any commit I ran since 084aeeb.

   AI-assisted: yes
5. fma-rc commented on Oct 10, 2026

   

   fma-rc

   on Oct 10, 2026

   ContributorAuthor

   More actions

   The next stop of the full run, step (s), is a test expectation that no longer matches the driver, not lost results.

   Setup: #2864 head b2a8590 merged onto main 57f96fd (needed to get past the pin wait), full agc_driver_recorder_tests run with no arguments. RTX 5080 (615.78.08) and Radeon 610M (RADV, Mesa 26.1.4).

   Test source	RTX 5080	RADV, Radeon 610M
   As merged	stops at (s) results pending when their memory left the watch did not reach it	same
   Temporary change: explicit flush before the (s) check	step (s) passes; stops later at (w) a cache over the pages the import reported sees no change	same

   The temporary change, in staleGenerationTests before const auto pixels = readback.Bytes();, copies what the unwatched branch of storageRefreshTests in the same file already does:

   Require(StorageTexture::FindPending(address, surfaceBytes) == image, "...");
   Require(StorageTexture::FlushPending(address, surfaceBytes, nullptr, "stale generation test"), "...");
   recorder.Submit();
   device.WaitQueue();
   recorder.Sync();

   Both Requires hold and the two (s) checks after them pass. So after the Refresh() the image is still registered as pending and an explicit flush puts FF 00 00 FF into guest memory; step (s) only fails because it expects Refresh() alone to store the results into the unwatched range.

   From reading, not bisected: 0e1d25a (fix(agc): avoid redundant transfers for untracked imported textures) added compareUntracked to Texture.cpp, after which a refresh over unchanged untracked memory keeps its results pending. git log -L on staleGenerationTests shows it was last touched on 2026-10-01 (d8c7c0c), before that commit. 0e1d25a is an ancestor of 2f655b1, where I first saw the (s) stop, and not of 084aeeb.

   The stop after that, (w) a cache over the pages the import reported sees no change, is the assertion #1532 reports. I have not looked into it on this build.

   AI-assisted: yes
6. MarcZX10 commented on Oct 10, 2026

   

   MarcZX10

   on Oct 10, 2026

   Contributor

   More actions

   Thanks for the follow-up. This confirms the full recorder executable still has separate stale-generation expectations after the pin-wait issue is bypassed: explicitly flushing gets past (s) but exposes the later (w) assertion on both GPUs. I kept #2864 scoped to the pin-release deadlock and noted that the full executable is not green; the focused regression passes with the fix and fails without it on both devices.
7. adriadam10

   closed this as completedin #2864on Oct 10, 2026