Repository navigation
Conversation
|
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.
|
|
Manual follow-up to the overlap report: I reviewed the flagged |
|
Intel and AMD Windows data: Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, every execution test built,
AI-assisted: yes (Claude Code) |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Thanks for picking up #7744. I ran 346cc6ee4 on Linux, and the host read path from the issue still reads the old bytes with this change.
Machine: Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON, full build. NVIDIA GeForce RTX 3070 (driver 595.99.02) and lavapipe (Mesa 26.0.3) on the same host. PR merged onto main 6b8228b2b (clean). ANYPS5_NO_SHADER_CACHE=1, -j1 --timeout 30.
The check is the repro from #7744 as a test: produce the pending surface, start a thread that takes the GPU mutex and calls FlushAllPending("idle"), wait until FindPending on the surface returns null, then GuestMemory::Read the surface. 20 rounds per run; a round fails if the read sees zeros.
| RTX 3070 | lavapipe | |
|---|---|---|
main |
5 of 5 runs fail, 20 of 20 reads stale in each | 5 of 5 runs fail, 20 of 20 stale in each |
| this PR | 10 of 10 runs fail, 20 of 20 reads stale in each | 10 of 10 runs fail, 14 to 20 of 20 stale |
agc_driver_storage_file_write, PR |
3 of 3 + 30 of 30 pass | same |
only the PR's StorageFileWrite.cpp, on main |
10 of 10 fail: concurrent guest read returned before an overlapping image was stored |
same |
So the new case does fail without the fix, but it only covers a reader that calls FlushPending itself before GuestMemory::Read. A plain GuestMemory::Read goes through the flush hook (FlushForAccess in Recorder.cpp), and that path misses the wait twice:
- With no image left in
pending.texturesfor the range (images == 0, the defaultHookFlushCpuBlocks()off), the hook callsPublishShadowsOnlyand never reachesFlushPending. The image being written back is only inpending.flushingat that point, so this is the usual case for the race. - Even with
APS5_HOOK_FLUSH_CPU_BLOCKS=0,FlushPendingstill returns false after the new wait (flushstays empty), so the hook skips itsSyncThroughand does not wait for the recorded store either.
When I tried the same FlushPending-only approach on #7744 it behaved the same way. It only passed after FlushPending returned true after waiting and the hook's images == 0 branch waited on pending.flushing too: 0 stale reads in 200 on both devices. That change is open as #7769 (with the StorageReadDuringIdleStore test that fails on this PR); happy to fold it into whichever lands.
agc_driver_storage_file_write passes on main too here (30 of 30 on each device, idle host), so it doesn't show the race without load.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks @jotitadev, you're right, and I missed the hook path when I looked at this. With Reproduced on an RTX 4090 (Linux) with your |
Add the concurrent flush case from boykopovar#7757 to agc_driver_storage_file_write: while one thread's FlushPending stores the surface under a held GPU mutex, a second thread calls FlushPending and then GuestMemory::Read on the same range, and the read must see the stored bytes. Only the test from boykopovar#7757 is taken here; agc_driver_storage_read_during_idle_store keeps covering the flush hook path. Co-authored-by: MarcZX10 <67475268+MarcZX10@users.noreply.github.com>
|
You’re right: this PR’s direct |
|
Thanks @MarcZX10. Closing this as covered: #7769 has merged (535587c), and it includes your concurrent |
|
Thanks for confirming the direct-reader test is included in merged #7769 alongside the flush-hook coverage. I appreciate the clarification and the credit; that closes the remaining status question on this PR. |
What
When
FlushPendingfinds no pending image for a range, it now waits if an overlapping image is already being written back by another thread, then retries the pending lookup. The wait releases the pending-list mutex, and the GPU-lock holder avoids waiting, preventing a lock cycle. The flush completion path wakes readers after removing in-flight images. Adds a synchronized regression toagc_driver_storage_file_writefor a reader racing an overlapping flush. Fixes #7744.Tested
Linux x86_64, Debian 12.
cmake --build build-scalar --target agc_driver_storage_file_write_tests --parallel 4passed.ctest --test-dir build-scalar --output-on-failure --repeat until-fail:20 -R '^agc_driver_storage_file_write$'passed 20/20 using Mesa llvmpipe. This host lacks the emulator-required LZCNT, BMI1, BMI2, MOVBE, AVX2 and FMA features, so I temporarily bypassed only the host CPU startup guard in the local test build; the test itself ran against lavapipe, and the guard source was restored after the run. On the unmodified build, CTest exits at that guard before the test starts.python3 tools/check_conventions.py --base upstream/mainpassed. No game or homebrew run.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)