Repository navigation
fix(agc): host imports write-protect only pages a collect already protects - #1960
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the complete five-file diff. The armed/fresh distinction preserves lazy write protection across imports: imports collect only already-armed pages, and the first GPU write collects fresh 64 KiB blocks before stamping the GPU generation. The added tests cover partial armed collection, first collection of fresh pages, and GPU-versus-CPU write ordering. The head is based on current main f145139; Conventions, Linux and Windows CI pass, including the full Linux CTest run. I found no blocking issue.
1a648ee to
e288a77
Compare
|
Conflicts with main in |
e288a77 to
811d968
Compare
|
@oneandonlydean rebased on current main, conflict in BdaResources.cpp is fixed. While rebasing I also found and fixed a bug in it: MarkWritten was stamping the never-collected pages it picks up as CPU writes, which broke the late label check in agc_driver_recorder_tests. The description has the details and the new tests. |
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
…tects Importing guest memory collected the write watch over the whole import, which write-protected every page of it, so pages nothing had collected yet took a write fault on their first CPU write. The import now collects only the parts a collect already protects (GuestWriteWatchCollectArmed). The rest stays unprotected until something collects it, which reports it as written; a GPU write recorded into such memory (MarkWritten) first collects the never-collected parts of its 64 KiB blocks (GuestWriteWatchCollectFresh), so that report is older than the GPU's results and does not read as a CPU write over them. Those pages are stamped as written and their CPU pages are noted, but the block's CPU stamp is left alone, so a GPU label record or a pending storage image elsewhere in the block does not see a CPU write. StoredOver and UnchangedSinceCollected read the noted pages whenever a block has them, so a store to such a page is still seen by a check over that page. The extra generation step happens only when something was collected; a block whose write protection fails is unwatched and the write returns no generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
811d968 to
c7f4690
Compare
|
Checked on Linux (merged onto current main): scoped build and ctest green, |
What
On Linux the write watch (asynchronous userfaultfd write protection) reports the pages of a range written since it was last collected. A range that was never collected is reported as entirely written on its first collect, and only then write-protected.
GuestMemory::ImportWatchedcollected the whole import before and after importing it (the import window from d8c7c0c): pinning the pages lifts their write protection, so what the window reports is stamped "maybe written", and GPU results pending in the range are kept instead of being dropped as if the CPU had written over them. For a range nothing had collected yet that collect write-protected every page of it: importing a 6 GiB allocation armed 6 GiB, and every first CPU write to a page of it then took a write-protect fault.GuestWriteWatchCollectArmed_nid_postfix: likeGuestWriteWatchCollect_nid_postfix, but it leaves the parts never collected alone (they stay unprotected and are still reported as written on their first collect). The scan loop ofCollectmoved into a helper both use.ImportWatcheduses it for both of its collects. Pages it leaves alone were never protected, so the pin cannot lift their protection.MarkWritten, which every GPU write into imported memory goes through, first collects the never-collected parts of the 64 KiB blocks it stamps (GuestWriteWatchCollectFresh_nid_postfix): their "written" report gets an older generation than the GPU write, as it did when the import collected them, and from then on they are protected and watched as before.StampKind::Fresh): nothing is known to have been stored after the GPU write, so a GPU label record or a pending storage image elsewhere in the block does not read it as a CPU write.StoredOverandUnchangedSinceCollectedread the noted pages whenever a block has them, so a check over such a page still sees a store to it. The second generation step happens only when something was collected, the never-collected pages are looked up under the shared lock first, and a block whose write protection fails is unwatched (the write then returns no generation).watchPrivateMappingalso notes the CPU pages it stamps, sinceUnchangedSinceCollectednow reads them.Registerleaves pages unprotected and reported as written until their first collect, soCollectFreshhas nothing to do there and returns without collecting, andCollectArmedis a plainCollect. I haven't built this on macOS; the write-watch CI job compiles that file.What this leaves different from main, for pages nothing collected before the import:
MarkWrittenreturns (the copy path) call it when the work is recorded, so this only covers stores racing the GPU.On the console a CPU store to direct memory has no hidden cost; this removes a cost the write watch added to the first store to each page of imported memory.
PPSA36082
Its resource loader decompresses into the 512 MiB upload pool and the 6 GiB heap, which are imported when they are mapped (#1961). It took about 372,000 minor faults per 400 ms of loading (170 ms of it in the kernel), and the thread that submits the loader's fences waits for it, so the fences came every ~12 ms instead of ~8 ms and the pool overflowed. With this change: about 1,800 faults per 400 ms (70 ms in the kernel).
On main f145139 with #861, #1367, #1249, #1905, #1961 (import at map) and #1959 (formatted buffer accesses) it reaches the main menu in 10/10 runs (40 s each, pool peak 340-391 MiB of 512); without this change 1/5 (the others overflow at 3.7 to 4.0 s).
Tested
Linux (CachyOS), x86-64, GCC, 7800X3D / RTX 4090.
guest_memory(new check): after a range is half collected and two pages are written,CollectArmedreports only the write in the collected half; the next collect reports the uncollected half as written, then nothing. MakingCollectArmedbehave likeCollect, or scan nothing, fails it.guest_memory(new check):CollectFreshreports and protects only the never-collected part of a range, leaves writes to protected pages for the next collect, and reports nothing the second time.agc_driver_graphics(new checks): importing a watched range nothing has collected leaves it to be reported whole by the next collect; after a collect, a write to a protected page is taken by the import. A GPU write (MarkWritten) into a never-collected imported range followed by a collect does not read as a CPU write after it, and a CPU write after it does. The range starts 4 KiB into a 64 KiB block, so the block reaches past the registration.agc_driver_graphics(new checks): a GPU write next to never-collected pages does not make a collected page failUnchangedSinceCollected, keeps the block's CPU flag inChangedBlocksclear, still reports a collected neighbour toStoredOver, and a CPU write afterwards is seen; a CPU store to a never-collected page that a GPU write elsewhere in its block collected is seen byUnchangedSinceCollected; a GPU write into a block with nothing left to collect advances the generation once; with a real host import, a CPU write to a page protected after the import is seen.MarkWrittencollect;MarkWrittencollects only the written pages instead of whole blocks;CollectFreshrequires the whole block to be registered;CollectFreshcollects nothing;CollectArmedbehaves likeCollect;CollectArmedscans nothing; the collected pages stamped as CPU writes or as driver writes; an unconditional second generation step;StoredOverorUnchangedSinceCollectedreading the noted pages only when the block's CPU stamp moved; the shared-lock lookup finding nothing.agc_driver_recorder_tests(not in ctest), run by hand: the late label checks pass, and withstorageRefreshTests,staleGenerationTestsandimportWindowTestsskipped (they fail on main) it passes, as main does. The unwatch after a failed write protection is not tested.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)