Repository navigation
fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does - #7505
Conversation
… watch An import that should stay watched still drops its range from the watch when the range was only partly watched or its import window cannot be resolved. That fallback skipped the unwatch hook, so a storage image with results pending in the range lost the watch without its guest bytes saved, and its next store threw "storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented". Since boykopovar#6060 settled private imports on Windows take this path, and ProsperoPuzzles (PPSA99006) stopped 5 s in (boykopovar#6301). The fallback now releases the tracker lock, runs the hook over the page range, and unwatches it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed range drops it A storage image has results pending over a block whose neighbour block already left the write watch. An import of both blocks that should stay watched cannot vouch for its pages, so the range leaves the watch, and the image's next store has to find the guest bytes the unwatch hook saved. agc_storage_unwalked_import (Linux) runs that flow through HostImportFor. The host import of the first creation is refused through the import budget so that the import under test is the first one over the block. staleGenerationTests runs the same flow for an import that does not stay watched; it now exists for both.
…lready fails An import that should stay watched runs the unwatch hook only when its range leaves the write watch, after the import. When the range is partly outside the watch or the walk before the import fails, the range leaves the watch whatever the import does, so the hook can run first, as it does for the import that does not stay watched. After the import, the walk that tells an import's own page marks from CPU writes (the window compare) is skipped in this case, so the hook collects those marks as CPU writes. A driver that marks the pages of an import written (the NVIDIA driver on Windows does, see Pages under an import window) makes the hook keep the blocks they cover for the CPU unless their bytes are proven unchanged, and those blocks do not receive the GPU results. A failure of the walk after the import still runs the hook after it. The check of boykopovar#6308 also requires the hook to have run before the import.
…m the write watch CheckUnwatchHooks runs Unwatch, an import that does not stay watched, an import whose page walk vouches for its pages and, on Linux, an import whose range loses a block during the import. The hook has to run once, over the dropped range, while the range is still watched: before the import for the first two, after it for the last, and not at all for the vouched import.
|
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.
|
github-actions
Bot
added
build
docs
agc
|
Verified at head |
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, To pick it up: Head is |
|
@blawesom thanks for the rebase handoff. I updated the branch with a merge of
Fork CI: Linux 717/717 (run 39146868385), Windows lab tests green (run 39146868399). AI-assisted: yes (Claude Code) |
|
Tested at
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@AlxcBTW thanks for this. #8172 has merged and #6308 is closed as superseded, so this branch no longer fits main. Main's The gap you closed is still on main, though. The
Could you rework this PR onto current main as the single fix for both? (Please agree with @LeandroLP23 so only one PR carries it.)
As far as we know, no current title reaches these two exits: the reports in #6301 run on main with #8172. So this is a correctness fix for a rare path. If it's reached today, the result is the explicit "results pending ... without the guest bytes saved" refusal, not wrong output. |
…ed range now stays watched
|
Re-checked head It no longer builds on main. Main releases the lock around its own On Windows, though, AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@AlxcBTW this had fallen behind On my earlier request for a regression where the first walk fails before the import: that's no longer required for this PR. We'll treat it as a follow-up, so you don't need to add it here. Next is CI on this head and the game gate. |
|
Re-checked head
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@oneandonlydean, a heads-up on a race this PR brought into In #10026 fixes it: every window walk finishes first, then the failed spans' hooks run under one suspension, before any #10026 waits on #7048, so if you'd rather not leave |
What
ImportWatched(GuestMemory.cpp) runs the unwatch hook for the spans of a stays-watched import that leave the write watch, before the import when the page walk before it already fails and after it when only the walk after it fails, so pending storage-image results over those spans keep saved guest bytes. The branch also tests every exit ofGuestMemorythat has a hook.#6308 (
93df5c42a, LeandroLP23, the hook after the import) was closed unmerged as superseded by #8172, and the branch still carries its commit and its merge commit (48a1d3453, Dean Galvin). It no longer depends on #6308. Against currentmain(a6dcea927;git merge-treeis clean) the net change is 3 files, +92 -3:core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp: inImportWatched, for an import that stays watched, a span whose page walk fails before the import is taken out of the span list and given to the unwatch hook before the import runs; a span whose walk with the import window fails after the import is given to the hook there.mainruns the hook in neither place: both spans leave the watch without it.core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp:CheckUnwatchHooksinagc_write_tracking, forUnwatch(hook once, over the range, while it is watched), an import that does not stay watched (hook before the import), an import whose walk vouches for its pages (no hook, range stays watched) and, on Linux, an import whose range loses a block during the import (hook after it).docs/dev/debt/libSceAgcDriver.md: the pending-results entry names a page walk that fails as an exit that runs the hook, and drops the sentence about an import fallback that unwatched without saving.What
mainalready covers, not repeated here:fix(agc): keep resident DCC targets watched under aligned block imports, merged 2026-10-20) keeps the spans that were watched before a stays-watched import watched through it. That is the case of an import over a partly unwatched range that this PR first wrote a test for (agc_storage_unwalked_import); with fix(agc): keep resident DCC targets watched under aligned block imports #8172 that range stays watched, so the test was dropped from the branch (bae91578f) and the branch no longer asserts that flow.fix(agc): save pending results when a fresh write collect fails, merged) runs the hook when a failed fresh collect takes a range out of the watch (MarkWritten). Nothing in this PR touches it.What the branch still addresses: a span of a stays-watched import that is covered by the watch but whose page walk fails, before the import (the walk's
walkWritesreturns false) or after it (the walk with the import window returns false). Both paths drop the span from the watch onmainwithout the hook, so pending storage-image results over it have no saved guest bytes and storing them throws. This is read from the code; no title run shows it on currentmain. The ProsperoPuzzles data in #6301 shows that, on currentmain, the refusal this chain used to cause no longer appears for that title.Exits that still drop a range without the hook, not changed here:
WatchMapping(DirectMemory.cpp): a second mapping of the same direct memory unregisters both aliases. The hook takesPending().mutexand the GPU mutex, and the driver callsSharedBacking(takesg_directLock) from code that can hold the GPU mutex, so calling the hook from libkernel's map path could invert that lock order. Not shown to deadlock; not touched.Tested
a91b68d56(the PR headbae91578fplus the CI scaffolding commit), ctest regexagc_write_tracking|agc_storage|agc_import|agc_host_import|agc_pending|agc_recorder|agc_driver_host_import|agc_unchanged|agc_collect|agc_partial|agc_resident: 34 of 34 passed, 2 of them skipped (agc_storage_clear_hand_over,agc_host_import_sparse), among themagc_write_trackingandagc_import_watch.agc_write_watch_coverageandagc_write_trackingpass,agc_import_watchis skipped (no Vulkan device).CheckUnwatchHooksfails onmainwithout theGuestMemory.cppchange. By reading, its last section (Linux, a block lost during the import) is the partmaindoes not satisfy; no CI run of the test againstmainwas made.python3 tools/check_conventions.py --base origin/mainon the branch: 0 errors; one warning (debt-length, the pending-results entry indocs/dev/debt/libSceAgcDriver.md, which was already over 1000 characters onmain).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)