Repository navigation
fix(agc): land GPU writes over guest memory before a host write into it - #6926
EagleBearerYS wants to merge 11 commits into
Conversation
A host write into guest memory (a file, AIO or APR read, a socket receive) went straight to the pages. Storage image results pending over the range were stored later, when something read the memory, and a store already recorded or in flight landed whenever its batch ran. Either could overwrite the host's bytes: a write-back recorded before an APR read landed over the file data after it, and a host write into part of a 64 KiB block left the rest of the block without the image's results. libc now calls a host write observer at the start of every GuestArena::HostWrite scope, on every platform. The driver registers one that does what its own guest stores do: it stores the results pending over the range and waits for every recorded or in-flight GPU write there, so the host's bytes land last, as on the console. The driver's own import window on macOS skips it. The test reads a file into guest memory under a HostWrite scope over a storage image's results, with the image's store recorded before the read and with the results still pending, over a whole 64 KiB block and over 4 KiB inside one.
GuestArenaSetHostWriteObserver_nid_postfix becomes GuestArenaSetHostWriteHook_nid_no_patch, taking the address as std::uintptr_t, and the driver's GuestMemory::FlushBeforeHostWrite becomes PrepareHostWrite, so they read as one pair with the host read hook boykopovar#6234 adds (GuestArenaSetHostReadHook_nid_no_patch and PrepareHostRead). The setter is a helper that must not become a NID, so it takes the _nid_no_patch suffix. Behaviour is unchanged.
|
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.
|
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The right place to fix this (the host write itself, rather than a store at every flip), and the fast path is cheap (82-98 ns against 7 ns for a 64 KiB write with nothing to wait for). Three questions on the slow path and on teardown.
- When a host write does overlap pending results,
PrepareHostWritewaits for the in-flight batches and the store. The call comes from whatever thread did theread()/recv()/APR read: file I/O threads, the network thread, and the title's own loader threads. A loader that streams into a buffer the GPU is still writing now stalls on a GPU round trip per read, which is the stall the description describes as the cost of the flip-time store, only moved. The PR measures the empty case only. Please add counters (hook calls, calls that waited, time waited) to the[rescache]-style profile line and post them for Epic Mickey's first-level load, so the cost of the slow path is seen on a real title, not only in the test. GuestArenaSetHostWriteHook_nid_no_patch(&PrepareHostWrite)is registered inDriver::Driverand cleared in~Driver. A thread inside aHostWritescope when the driver is destroyed can call the hook after the clear started, or whileDrivermembers it uses are gone. State that the hook is an atomic pointer and what makesPrepareHostWritesafe once the driver is stopping (it should do nothing afterstop()).- The lock audit is in the description, but nothing enforces it. A future
HostWritecall under a mutex the GPU wait needs (the GPU mutex, a libSceNet lock) deadlocks. Add a check in the hook that throws when the thread holds the GPU mutex outside the supported case, the wayDrawPipeline::Draindoes, so a violation is an error and not a hang.
Also: the description says the title has not run the fix yet ("Related", not "Fixes"). A log line from Epic Mickey (PPSA15657) on this branch reaching the first level is what the maintainers will ask for; it is worth getting from the reporter before this is looked at in detail.
Only the driver's destructor cleared the host write hook, so a host write made after Driver::Shutdown still ran PrepareHostWrite and the flush hook behind it: the queued-label branch takes the GPU mutex and records labels against the reset device, and on Linux exit() runs the destructor while title threads still run. stop() now clears the hook before the workers are joined, sets a stopped flag PrepareHostWrite checks after counting itself in flight, and waits for the calls already inside to leave. The wait is bounded to 1 s, as a thread ExitProcess killed inside the hook never leaves. The test records a storage image's store over a range, shuts the driver down, then makes a host write over the range and checks that it no longer reaches the flush hook.
A host write waits for the GPU work over its range before its bytes land. Every HostWrite site is a guest I/O path that holds no driver lock, but one made with GuestMemory::GpuMutex held would take the flush hook's nested locked wait, which hangs if that work needs another thread's hold of the mutex to complete. PrepareHostWrite now counts such a call (GuestMemory::LockedHostWrites) and logs the first one with its thread tag, address and size, then continues. Under APS5_ASSERT_GPU_LOCK=1 it aborts with a [lock] ASSERT: line instead. The debt notes the rule. The test makes a host write while holding the GPU mutex, checks that it returns and that the count moved by one, and that a host write without the mutex is not counted.
Under APS5_PROFILE_DRAW, PrepareHostWrite reads the calling thread's GPU waits (Recorder::ThreadWaitedMs, given to GuestMemory by the driver as it gives ShaderMemory) before and after its flush, and counts the calls, the calls that waited, and the total and longest wait. A [hostwrite] line reports them every 10 s with the other profile lines. The test makes a host write over a range with a recorded store, which counts one call that waited, and one over an idle range, which counts a call that did not.
|
@CnRJay Thanks. All three are in, each with its test in the same commit:
|
|
AMD / Windows run of this PR at 276f67d: Radeon 780M (RDNA 3, 0x15bf), AMD driver 25.10.20.01 (32.0.21020.1007), Windows 11 Home 10.0.26100, WinLibs GCC 15.2.0 r7, Release. Every ctest entry built from the test targets this PR changes passes (2):
AI-assisted: yes (runs made on the device above). |
|
Thanks @keravyn-ops for the AMD runs here, on #6911 and on #6944; they're in each PR's Tested and GPUs-tested lines now. |
|
NVIDIA data point for this PR: head
Not checked: whether the new tests fail on AI-assisted: yes (Claude Code). |
|
Thanks @TheSovietWaffle for the RTX 3080 runs; they're in the Tested and GPUs-tested lines now. |
main 4827f36 adds the shader disk cache include to the driver lifecycle (boykopovar#6641) where this branch adds the guest arena's. Keep both.
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
A Linux run on NVIDIA and lavapipe, with the hook taken out to see what the new tests catch.
Machine: Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON. NVIDIA GeForce RTX 3070 (driver 595.99.02) and lavapipe (Mesa 26.0.3, LLVM 21.1.8) on the same host. PR merged onto main 12a71f3f5 (clean). ANYPS5_NO_SHADER_CACHE=1, -j1 --timeout 30.
| Run | RTX 3070 | lavapipe |
|---|---|---|
agc_driver_host_write_hook, agc_driver_host_write_pending_storage |
pass 3/3 | pass |
^agc (638 tests), PR vs main |
no new failures (636/638; agc_driver_f16_fma_denormal_modes and agc_driver_fma_mix_f16_denormal_modes fail on main on this GPU too) |
638/638 on both |
The tests don't build on main (they use HostWritesCounted), so as the negative control I removed only the GuestArenaSetHostWriteHook_nid_no_patch(&GuestMemory::PrepareHostWrite) line from Driver::Driver. Both tests fail on both devices, 3/3 on the 3070:
host write over pending storage, a store recorded before a 64 KiB host write: FAILED, byte 0x0 of the file read was overwritten by the image's store after it (0x0, the file has 0x40)
host write over pending storage, a store recorded before a 4 KiB host write: FAILED, byte 0x3000 of the file read was overwritten by the image's store after it (0x0, the file has 0x40)
host write over pending storage, a 64 KiB host write over pending results: the file's bytes survived the image's store and its texels landed around them
host write over pending storage, a 4 KiB host write over pending results: FAILED, byte 0x5 outside the file read holds 0x0, not the image's 0x47, so its results there never landed
So on Linux the file-read path reaches the driver only through this hook, and three of the four cases catch it missing. The third case, a 64 KiB host write over pending results, passes without the hook on both devices, so it does not cover the fix on this platform.
Run on 276f67d36; the current head be2be36b5 only merges main, the PR's five commits are unchanged.
Not run: Windows, AMD, Epic Mickey or any title.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02) and lavapipe (Mesa 26.0.3), Ubuntu 26.04
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks for the Linux runs and the negative control. Your three failures are the same ones the red in Tested shows, and the whole-block case passes on |
github-actions
Bot
added
unblocks title
Keeps the host-write hook setter next to boykopovar#6184's access observer setter in GuestArena.
github-actions
Bot
added
needs rebase
Keeps the host-write hook next to boykopovar#6234's host-read hook in GuestArena, and adds boykopovar#6234's file-write sentence to this branch's pending storage-image debt entry.
…nding-storage The debt entry keeps this branch's pending storage-image and host-write bullets next to main's updated bullet for results over memory outside the write watch.
|
Windows / NVIDIA data point, another UE4 title: Alone in the Dark (PPSA08240, v1.00). Windows 11 (build 26200), RTX 5070 Ti, driver 617.42, WinLibs GCC 15.2.0 r7, Release. Both sides built from the same tree: Six alternating pairs, each run from launch to the main menu or the crash (
The
The one crash with this PR is the last kind: Earlier, not alternated, on |
The debt file keeps this branch's two entries on pending storage-image results and host writes, and main's new APS5_SKIP_REFUSED_DRAWS entry after them.
|
@LeoFeitosa, thank you, that's a clear pair: the main menu in 1 of 6 runs on The remaining I've added your runs to the description. |
|
@EagleBearerYS I ran it with With the untraced runs, this head has now reached the menu in 13 of 14 runs. The tracing changes the timing, so it may be hiding the remaining |
|
@LeoFeitosa, thanks for the eight traced runs. 13 of 14 with this PR against 1 of 6 on |
|
Second title with the same failure signature, for the record (not a test of this PR). Setup: Code Violet (PPSA26528) 01.000.028 on Linux, RX 6650 XT, RADV (Mesa 26.2.4). Failure: 1 of 8 cold starts ended 8 s after launch with: It was followed by a segmentation fault. The other 7 starts loaded past it to the menus. I have not run this PR or AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@JJenkx, thank you for the Code Violet (PPSA26528) data point. It's the same signature this PR fixed in Epic Mickey: Rebrushed (PPSA15657), where a storage image's write-back landed after an APR read into the same memory. If you have time, could you merge this PR (9f5745d; it merges cleanly with |
|
Another data point, LEGO Horizon Adventures (PPSA14632), Windows 11, RTX 5070 Ti (NVIDIA 591.86), warm shader cache, same input script each run:
|
|
@winstrex thanks for the LEGO Horizon Adventures (PPSA14632) runs, and for keeping the input script the same across them. The |
This branch has not been deployed
9 participants
What
A host write into guest memory doesn't wait for the GPU writes recorded or in flight over the same memory, and doesn't store the storage-image results still pending there.
GuestArena::HostWritescope. That covers the APR read (libkernel/Apr/src/Apr.cpp:159-170), AIO,read/pread/readv,recv/recvfromin libkernel and libSceNet, thefcntl/ioctloutputs, libc's file reads and WebKit'smkstemp.GuestWriteWatchBeginHostWriteis Apple-only.Why that loses data: a storage image's write-back into host-imported guest memory is recorded, not made, while a recorder is active.
StorageTexture::writeBackLayers(Graphics/src/Texture.cpp) decides which 64 KiB blocks to keep for the CPU, records the retile and the copy into the open batch, and notes a pending write. The bytes land when that batch runs.Related: #6636, where the crash below was traced; #6234, the read direction (pending GPU results flushed before file writes), whose hook this one pairs with.
The title. Epic Mickey: Rebrushed (PPSA15657, UE 4.27) crashes now and then while it loads its first level. johnmaia ran it on Linux, RX 7600 (RADV, Mesa 25.2.8), on
main5d9dcc4 + #6236 with no other changes (#6636, comment):APS5_STORE_AT_FLIP=1ObjectSerializationErroronSM_MachineSet_01b(UE's "Serial size mismatch");LowLevelFatalError [Line: 1609]right after the title opens/savedata0/ue4savegame.dpx.sav;main, about 2 runs in 5 crash this way. In those, a 64 KiB block of file data the title had just read is zeroed about 18 ms after the read, with no CPU write path of the driver involved (#6636, comment).APS5_STORE_AT_FLIP=1stores pending results at every flip, which closes that window. This PR closes it at the host write itself.The fix:
GuestArenaBeginHostWrite_nid_postfixnow calls a hook first, on every platform, for every non-empty range. The setter isGuestArenaSetHostWriteHook_nid_no_patch, so everyHostWriteuser above gets it with no change at the call site.GuestMemory::PrepareHostWriteinDriver::Driverand clears it in~Driver.PrepareHostWritecallsFlushGpuWritesunderReadSiteScope(ReadSite::Store), as the driver's own guest stores (GuestMemory::Write,WriteChanged) already do. That call:GuestArena.cpp).Driver::stop()now clears it first, before the worker joins, then sets a stopped flag and waits up to 1 s for calls already inside the hook to leave.PrepareHostWritecounts itself in flight (seq_cst) before it checks the flag and returns at once if it is set, so either a call sees the flag orstop()sees the call. The wait is bounded because on WindowsExitProcesscan end a thread inside the hook before~Driverruns; if the bound expires,stop()logs it and goes on.~Driverstill clears the hook as before.HostWritescope under the tracker lock, so it skips the hook through a thread-local guard and can't re-enter the tracker.g_mutexbefore itsHostWrite.DescriptorControl'slockis the guest record-lock struct, not a mutex.GuestMemory::LockedHostWrites) and logged once with its thread, address and size, then goes on. WithAPS5_ASSERT_GPU_LOCK=1, the switchAssertGpuLockHeldalready reads, it aborts with a[lock] ASSERT:line instead. It doesn't throw in a normal run: with the mutex held, the hook takesFlushForAccess's nested locked wait, which hangs only if completing needs another thread to take the GPU mutex, so a throw would turn a possible hang into a certain crash. The debt file states the rule.docs/dev/debt/libSceAgcDriver.md) is updated. The title's own CPU stores still aren't ordered after a recorded store: they survive only in the 64 KiB blocks the write watch saw written. And its CPU reads after a label still read stale memory.Cost: the hook adds about 75-90 ns to a 64 KiB host write with nothing to wait for (82.5-97.7 ns against 6.9-7.1 ns, the test's
--costmode). That's the flush hook's fast path, well under theread()it precedes.APS5_PROFILE_DRAW=1, a[hostwrite]line every 10 s counts the host writes, those that waited for GPU work over their range, and the total and longest wait, e.g.[hostwrite] 2 host writes, 1 waited for GPU work over their range: 1.34 ms in total, 1.34 ms at most. Without the switch nothing is counted. The title's numbers aren't measured yet.How:
fix(agc): land GPU writes over guest memory before a host write into it: the hook, the driver's registration, the debt entry and the new testagc_driver_host_write_pending_storage(tests/execution/HostWritePendingStorage.cpp)._readFiledoes it,std::ifstream::readinside aGuestArena::HostWritescope. A laterFlushGpuWritesplays the guest's read.refactor(agc): name the host write hook as a pair to the host read hook: names the setter and the driver function after fix(libs): flush pending GPU results before file writes #6234's host read hook (GuestArenaSetHostReadHook_nid_no_patch,PrepareHostRead). The_nid_no_patchsuffix follows CONTRIBUTING.md for helper symbols that must not become NIDs.Tested
Epic Mickey: Rebrushed (PPSA15657), johnmaia's runs: Linux, RX 7600 (RADV, Mesa 25.2.8), short runs to the first level,
APS5_STORE_AT_FLIPunset (#6636, comment). Currentmainrarely shows the crash, so they ran the fix on the base where it fails:main5d9dcc4 + fix(libScePlayGo): let users answer unlisted chunks #6236maine751dd2ObjectSerializationError, theLowLevelFatalError [Line: 1609]or the SIGSEGV.APS5_TRACE_APR=1 APS5_TRACE_FLUSH=1), 23 APR reads were each followed at once by the hook storing a pending image over their destination, before the read's bytes were written:PrepareHostWritestill callsFlushGpuWritesover the written range.Alone in the Dark (PPSA08240, v1.00), LeoFeitosa's runs (comment): Windows 11 (26200), RTX 5070 Ti (driver 617.42), WinLibs GCC 15.2.0, Release.
APS5_PLAYGO_UNLISTED_CHUNKS=installed, and has no input until the main menu.maincd700femain+APS5_STORE_AT_FLIP=1main, all while loading:LowLevelFatalError [Line: 3230], "Serial size mismatch" on UI textures;0xc000001donFAsyncLoadingThread;0xc0000005atAlone in the Dark.exe+0x20ec0bc.0xc0000005at+0x20ec0bc. Whether it shares the cause isn't traced.APS5_TRACE_APR=1 APS5_TRACE_FLUSH=1all reached the main menu, so this PR (2c5b772; the head since only mergesmain) has reached it in 13 of 14 runs (comment). The tracing changes the timing, so those eight runs don't rule the remaining crash out.Linux (lavapipe, Mesa 25.2.8, gcc 13.3, CI's Linux configure), on main 66e290f. Each check ran in all three write-watch modes: userfaultfd user-mode only (unprivileged), full userfaultfd (root), and unwatched (
APS5_NO_WRITE_WATCH=1).ctest -R '^agc' --parallel 4 --timeout 120: 625/625 at the head, 43 skipped as expected.ctest -R 'guest|libc|kernel': the same as main on that machine. The 5 failures are IPv6 tests the container always fails.GitHub runners (run 38952984815), with only the registration reverted for the red:
guest_kqueuefails there with and without the change.Merged with fix(libs): flush pending GPU results before file writes #6234 (0f5651f) and with fix(agc): cache inaccessible guest pages #6184 (4a96406), each on its own, on lavapipe. The new test passes 10/10 on both merges.
agc_driver_storage_file_writepasses 10/10 and^agc626/626.agc_driver_graphicspasses 10/10 and^agc624/625. The one failure isagc_driver_preparation_scope_settle, which passes 5/5 alone.Windows code: MinGW
-fsyntax-onlyofGuestMemory.cpp,Lifecycle.cpp,GuestArena.cppand the test: 0 errors.The review follow-ups (Linux, lavapipe, CI's configuration), each with its case in the new
agc_driver_host_write_hook, in the same commit as its change:Driver::Shutdown, a host write over a range with a recorded store must not reach the driver's flush hook. Red at the previous head: "a host write over a range with a recorded store reached the driver's flush hook 1 time(s) after Driver::Shutdown".APS5_ASSERT_GPU_LOCK=1the test aborts at the[lock] ASSERT:line, as intended.agc_driver_host_write_hookandagc_driver_host_write_pending_storagepass 20/20 in each of the three write-watch modes, and^agcpasses 626/626.^agcsweeps' only failures areagc_driver_preparation_scope_settleon Linux, a timing check under parallel load, andagc_driver_pm4_replay_tool(0xc0000139) on Windows, which fails onmainthere too.AMD Radeon 780M (RDNA 3, AMD driver 25.10.20.01), Windows 11 (WinLibs GCC 15.2.0, Release), run by keravyn-ops at 276f67d (comment):
agc_driver_host_write_hookandagc_driver_host_write_pending_storagepass.NVIDIA GeForce RTX 3080 (NVIDIA 617.42), Windows 10 Pro 22H2 (WinLibs GCC 15.2.0, Release), run by TheSovietWaffle at 276f67d (comment):
agc_driver_host_write_hookandagc_driver_host_write_pending_storagepass 5 of 5 runs each, and the 12guest_socket_host_write_*tests, which go through the changedGuestArenahost-write path, pass.NVIDIA GeForce RTX 3070 (driver 595.99.02) and lavapipe (Mesa 26.0.3, LLVM 21.1.8), Ubuntu 26.04 (GCC 15.2, Release), run by jotitadev at 276f67d merged onto
main12a71f3 (review): both new tests pass, 3/3 on the 3070.^agchas no new failures againstmain: 636 of 638 on the 3070, whereagc_driver_f16_fma_denormal_modesandagc_driver_fma_mix_f16_denormal_modesfail onmaintoo, and 638 of 638 on lavapipe. With only the hook's registration inDriver::Driverremoved, the same three cases fail on both devices as in the red above, and the whole-block case passes.Merged with
maincd700fe and 86b8424 (2c5b772, then 9f5745d, the head). Each conflicted only in the debt file, where both sides' entries are kept. On GitHub's runners at the head (run),agc_driver_host_write_pending_storage,agc_driver_host_write_hook,agc_driver_storage_file_writeandagc_driver_graphicspass 3 runs in a row on Linux. On Windows three of the four skip without Vulkan andagc_driver_graphicspasses. The libc and libkernel suites pass on both.Merged with
main0d078f7 (46d8f39, no conflicts), on GitHub's runners (run): the^agctests pass on Linux (711, 0 failed), and the libc and libkernel suites pass. On Windows the only failures areagc_driver_pm4_replay_toolandagc_shader_replay_tool, which fail the same way on that runner for an unrelated branch (run).LEGO Horizon Adventures (PPSA14632), run by winstrex (comment): Windows 11, NVIDIA GeForce RTX 5070 Ti (NVIDIA 591.86), warm shader cache, the same input script each run.
[hostwrite]line shows host writes landing on pending GPU work over their range in every run (4 to 60 per run).FAsyncLoadingThreadcrash withObjectSerializationError ... Serial size mismatchafter the graphics quality screen came in 2 of 2 runs onmain7ce3b1d, 1 of 6 on this PR's base 86b8424 and 0 of 7 with this PR (9f5745d). winstrex calls that not conclusive at these counts.UObjectHash.cppline 42).tools/check_conventions.py: passed.Overlap. On main 1b1f848, the overlap check finds three open PRs that conflict with this one. 37 others that touch the same files conflict with main themselves, so they couldn't be checked.
GuestArena.hppandGuestArena.cpp, and the debt entry, which both PRs rewrite. Each resolves by keeping both sides.FlushGpuWrites.GuestArenaWriteWatched_nid_postfix.GuestArenaBeginHostWrite, so the two compose independently.Execution/src/Driver/Lifecycle.cpp. Whichever lands second takes a one-line merge.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.mdfiles or images added to the repository (attach them to this PR)main)