Repository navigation
fix(agc): stamp the watched blocks of a store into partly watched memory - #6455
oneandonlydean merged 5 commits into
Conversation
MarkWritten and StoreOwnBytes return 0 for a range that is not wholly watched, and they stamped none of its blocks. The watched blocks such a store wrote kept their old stamps, so UnchangedSince and StoredOver over them still read as unchanged after a GPU write that crossed into an unwatched import. A vouched shader table column or a cached range then kept bytes that had been replaced. Both now stamp each block whose part of the store starts or ends in watched memory, as the wholly watched path stamps every block. They still return 0 and note the store as unstamped. This costs one or two coverage lookups per block, and only on this path.
A driver store and an own store across a watched block and an unwatched one must stamp the watched block and no other. On Linux, a store across the halves of a split block must stamp it, and a store into its unwatched half alone must not.
GuestWriteWatchCovers says whether a range is watched in full. The Linux and macOS watches unregister page ranges, so a range can hold watched pages between unwatched ones, and code that must act on every watched byte of a store has had no way to find them short of asking page by page. GuestWriteWatchOverlaps answers with one lookup in the registered ranges, like Covers. The macOS write-watch test and agc_write_tracking on Linux check it around watched pages between unwatched ones, at both edges and for an empty range.
…ed store stampWatchedPart stamped a block when the store's part of it started or ended in watched memory. That is exact on Windows, where the watch excludes whole 64 KiB blocks and the arena's host regions are 64 KiB holes. On Linux and macOS the watch unregisters page ranges, so a block can be watched only between two unwatched ranges, and a store across all three left its watched pages with their old stamps. Off Windows the block is now stamped when the store's part of it overlaps any watched page (GuestWriteWatchOverlaps). A store that overlaps no watched page at all returns after that one lookup, where it made two coverage lookups per block before. The test watches only the middle pages of a block: a driver store and an own store around them must stamp it, a store into an unwatched end must not.
…e watch Since boykopovar#6060 a settled Windows host import of private write-watched memory stays watched, and its bytes are vouched by the write tracker's stamps instead of compared. A GPU write that runs from such an import into a neighbouring import that left the watch reaches the tracker as one MarkWritten over a partly watched range. The test imports two blocks that stay watched next to one that leaves the watch, and checks that a driver store and an own store across the boundary stamp the watched block they wrote, are seen over exactly its stored bytes, and leave the other watched block 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.
|
|
A title that hits this: MXGP 26 (PPSA30839) on Linux. Ubuntu 24.04 (kernel 6.8), GCC 13.3, Release, RTX 4070 Ti (NVIDIA 595.99.02), warm shader cache, pad input, 120 s runs. Every build has
Just before the stop, the log has Not checked: whether the stored bytes match the console, Windows. AI-assisted: yes |
|
@milescoviello thank you for finding a title for this, and for the pair with and without it. I've added MXGP 26 (PPSA30839) and your runs to the description, including that #6308 is in both builds and doesn't prevent the stop on Linux. |
|
An update to my MXGP 26 (PPSA30839) data above. On current
With only #6184, AI-assisted: yes (Claude Code) |
|
Thanks @milescoviello for re-running on current |
What
A hole in
main's write tracker, found by review: a driver store or an own store over a range that is only partly write-watched stamps none of its blocks. milescoviello found a title that hit it on an oldermain: MXGP 26 (PPSA30839) on Linux. Onmain84b0d2f it no longer stops there without this PR (their runs under Tested). And since #6060 the compare-free vouch for settled Windows host imports relies on these stamps. cc @parl0vThe hole. Every GPU write path into guest memory stamps the blocks it wrote through
MarkWritten: packet stores and labels, DMA fills and copies, CMASK and DCC metadata, the write-backs of address-based builds, storage-image results and BDA stores. Own stores go throughstoreOwn(StoreOwnBytes). For a range that isn't wholly watched, both return 0 and only note the store as unstamped, and onlyNewestStampandStoredAllSinceread that note. The watched blocks such a store wrote kept their old stamps, soUnchangedSince,StoredOverandChangedBlocksstill read them as unchanged after a GPU write that crossed into unwatched memory. Whatever relies on those answers then keeps bytes that were replaced:StorageTexture::blocksKeptandevictStale, so a write-back can land over newer driver bytes;When it happens: a GPU write whose range runs from watched memory into memory that has left the watch.
APS5_WRITE_WATCH_IMPORTS=unwatch, or the import probe's verdict), or any other unregistered page range.The fix:
StoredOver. They still return 0 and note the store as unstamped, so their callers keep treating the store as one the tracker doesn't vouch for.GuestWriteWatchOverlaps, which is one lookup in the registered ranges, likeGuestWriteWatchCovers. A store that overlaps no watched page at all returns after that one lookup.Cost: the wholly watched path is unchanged. On the other path:
The other write paths:
MarkWrittenorStoreOwnBytes, so the fix covers them all at the source.GetWriteWatch, userfaultfd,mprotect). A collect over a partly watched range fails and returns no generation, so nothing can be vouched over it.CopyBuffer, whose order is deliberate. A reader on another queue that doesn't wait can see the old bytes at the new generation.The commits:
fix(agc): stamp the watched blocks of a partly watched store.test(agc): check the stamps of a store into partly watched memory.feat(libc): ask whether any part of a range is write-watched (GuestWriteWatchOverlaps, with its tests).fix(agc): stamp a block whose watched pages lie inside a partly watched store.test(agc): check a store across a watched import and one that left the watch (perf(agc): keep Windows host imports of write-watched private memory watched #6060's case).Overlaps: #6266 and #6308 add tests next to these in
tests/WriteTracking.cpp, and the conflict with each is in its list of checks only. #6266's coarse runs are raised inWriteTracker::stamp, which these stamps go through, so they stay consistent with it. #7048 (AlxcBTW) handles another way to reach the sameTexture.cpprefusal: a unit with a write generation that becomes pending after its block left the watch. It shares no file with this PR. Its new kept-block check reads the per-block stamps (ChangedBlocks) that this PR adds for stores over partly watched ranges, so the two compose. #7233 (MarcZX100) adds acompleteflag toWriteTracker::noteDriverStore, andMarkPossiblyWrittenfor ranges that may have been written (page-granular BDA reports, writable descriptor ranges). This PR'sstampWatchedPartcallsnoteDriverStorefrom the partly watched paths ofstoreOwnandMarkWritten, so whichever lands second passes the flag through:truefromstoreOwn, andmarkWritten's own flag from there. That way a possible write into partly watched memory stays a possible write.Tested
agc_write_tracking:CheckPartlyWatchedStore: a driver store and an own store across a watched block and an unwatched one stamp the watched block and no other;StoredOversees the watched bytes stored and no others; on Linux and macOS, a store across the halves of a split block stamps it, and a store into its unwatched half doesn't.CheckWatchedMiddleStore(Linux and macOS): with only a block's middle pages watched, a driver store and an own store around them stamp the block, and a store into an unwatched end doesn't.CheckStoreAcrossImports: two blocks imported withImportWatchedthat stay watched, next to one imported so that it leaves the watch. A driver store and an own store across the boundary stamp the watched block they wrote, are seen over exactly its stored bytes, and leave the other watched block unchanged.CheckWatchOverlaps(Linux):GuestWriteWatchOverlapsaround watched pages between unwatched ones, at both edges, and for an empty range.write_watch_overlaps(macOS): the same checks on the macOS watch.main0c4816f: withmain'sGuestMemory.cpp,agc_write_trackingfails on both at "a driver store into partly watched memory left its watched block unchanged"; with the fix it passes 20 of 20 on both (run). In that run's fix arm, Linux's libkernel suite hit a bus error inguest_dynamic_loader, which links only libkernel and libc, both unchanged at that commit; it passed in the run's other arm and at the head.GuestMemory.cpp: the test fails on Linux at "a driver store around the watched middle of a block left it unchanged" (Windows has no such block, since its watch excludes whole blocks); with commit 4 it passes 20 of 20 on both (run).main0029e62): fails on both at "a GPU write across a watched import and an unwatched one left the watched import's last block unchanged" (run).guest_kqueue, which fails onmainin the same runs.macos-15-intelandmacos-15): the write-watch suite at the head, each of its 10 tests until-fail:20, passes on both,write_watch_overlapsincluded (run).^agcsweeps at the head: Linux 604 of 604 (42 skip; run). Windows 598 of 599 (466 skip without a Vulkan device; run); the one failure,agc_driver_pm4_replay_tool(0xc0000139), and libkernel'sguest_kqueuealso fail onmainon that runner.maindoesn't build in full with MinGW since fix(agc): keep the gate run going past indirect dispatches whose arguments stay on the GPU #5747 (alignas(65536)intests/execution/GpuIndirectArgumentsMisalignedWriterAhead.cpp), so the Windows sweep ran with test(agc): align the indirect arguments block at run time #6452's fix for that test applied, for this check only.python3 tools/check_conventions.py --base origin/main: passed.MXGP 26 (PPSA30839), run by milescoviello on Linux (Ubuntu 24.04, GCC 13.3, Release, RTX 4070 Ti with NVIDIA 595.99.02, warm shader cache, 120 s runs), on
main4524944 with #6814, #6094, #6160, #6308 and #6184 in both builds (comment):storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented, just after[gpu] storage image 0x1227740000+0xf00000: memory no longer registered; 240 pending ranges droppedfor four neighbouring 15 MiB storage images;fix(agc): run the unwatch hook when a watched import leaves the write watch #6308 fixes the same throw's Windows path in ProsperoPuzzles (AGC: since #6060, ProsperoPuzzles (PPSA99006) stops at 5 s with the storage-image write-back throw on Windows #6301). It is in both builds here, and on Linux it doesn't prevent the stop.
Update on current
main(milescoviello, Linux, RTX 4070 Ti, warm cache, pad input, 120 s, 2 runs per build, comment): onmain84b0d2f with #6184 and #6160, the stop no longer reproduces. Without this PR, with it, and with #7048 instead, 2 of 2 runs each reach the 120 s limit, and none of the six logs the refusal. So MXGP 26 no longer needs this PR on currentmain, and neither this PR nor #7048 regresses it. #6160 is in every build because with only #6184,mainnow stops at 48 s on a computeds_swappc_b64target. Two runs per build are too few to compare frame rates.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)main: MXGP 26 (PPSA30839) got past 30 s on Linux with it onmain4524944, and gets there without it onmain84b0d2f (milescoviello's runs). It closes the hole that perf(agc): keep Windows host imports of write-watched private memory watched #6060's compare-free vouch for settled Windows imports rests on. perf(agc): settle Windows imports under 1 MiB without waiting #6678 depends on it because it returns settled imports' 64 KiB blocks to the write watch, so a store across such a block and a neighbour still outside the watch is a partly watched store.