Repository navigation
Conversation
|
#650 and #652 have landed, and each conflicts with this on one line. Both resolutions keep both sides:
#650 also added a paragraph to |
|
Tested ctest: Release, PPSA26344 v1.008 on our integration branch, with and without this PR. It allocates 13.5 GiB of direct memory and makes 835 maps (4.7 GiB). No page is mapped twice and nothing is unmapped.
Problems. Each has a standalone repro against this branch's libc/libkernel. Main's libs pass all three.
For the |
|
Do you plan to address Celegans12's Windows report from 10-05 here? It found three problems, each with a repro:
The branch also conflicts with main in |
|
Tested
No regression here; in the level the queue 0 worker is CPU-bound on draws either way, so the frame rate there doesn't change. Tested with AI assistance (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the work. Not repeating @oneandonlydean's question: the three problems @Celegans12 reproduced on d6bb6c5b are still open, and the diff confirms them. They need to be fixed before this can merge:
- Stale GPU import after a second mapping:
share()/cut()(WindowsMappings.hpp:124,:405,:429) replace the pages of the whole run, but the existing import keeps its registered range. Either invalidate imports of every page in the run, or avoid touching siblings. - Lost writes:
cut()/share()copy pages while still writable (:414,:437) andcut()recommits read-write before copying back (:417-:419). A write from another thread in those windows never faults, so it is lost. Keep the pages protected (or otherwise fenced) until the copy is done. - Double commit charge after remap: pages stored in the
SEC_RESERVEsection stay committed once mapped again as resident copies (:105-:108). Decommit the section page after copying it out.
Also:
- Rebase: conflicts with
maininGuestArena.cppandDirectMemory.cpp, anddocs/user/USAGE.mdneeds the commit-charge wording updated (as @oneandonlydean noted). CollectcallsGetWriteWatchfor resident runs even whenwatchResidentis false (:281); that memory isn'tMEM_WRITE_WATCH. Please guard it or explain why it can't happen.- The new test is single-threaded and never imports into the GPU or remaps a large run, so it can't catch any of the three problems above. Please add a test per problem (the repros in @Celegans12's comment are a good start; each must fail without the fix).
The test and the Windows paths can't run on Linux CI; once pushed, a Windows run of guest_memory linked in the PR would help.
|
@DotDebian are you still planning to work on this? It matters a lot for Ghost of Yōtei: the process now peaks at about 37 GiB private on a 32 GiB machine and pages heavily, and with this branch its commit charge dropped from 15.3 to about 7 GiB in my test on 10-05. If you don't have time, I'd be glad to continue it: rebase onto |
… on Windows Write tracking of a section view write-protects it and resumes the write fault on the guest thread. Windows builds the exception frame directly below rsp, over the System V red zone, so the guest resumes with its leaf locals overwritten when its AVX state is live (boykopovar#361). A direct memory page mapped at one guest address now lives in private MEM_WRITE_WATCH memory at that address, and its writes are collected with GetWriteWatch like flexible memory: no fault is raised. A page mapped at a second address is moved into the section and both addresses become views of it, tracked by write protection as before. Pages are stored back into the section when they are unmapped. The section is reserved rather than committed, so only shared or stored pages are charged twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… out of the section Review of boykopovar#637 found three problems in the resident direct memory path. A second mapping or a partial unmap split a resident run by copying it out while it was still writable and committing it again, so a write from another thread in that window was lost. A run that has to be broken is now made read-only, copied into the section and replaced by views of it: there is no copy back into live memory. A write that hits the fence, and a read or write that hits the placeholder between the release and the views, is retried by the fault handler once the views exist. The pages of the whole run are replaced, so a GPU import of the first mapping kept pinning the old pages. Mapping a physical page a second time and unmapping part of a run now wait for the leases and imports of every range that shares the run, as an unmap of those ranges does. A page stored in the SEC_RESERVE section stayed committed after it was copied back out, and Windows cannot decommit it. Unmapped resident pages are now kept in private memory that is released when they are mapped again or when their backing is destroyed; a page that was once in the section stays a view. Collect refuses resident pages when the arena has no write watch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d6bb6c5 to
dcffc09
Compare
|
Sorry for the silence. Rebased onto
Cost: pages that share a run (up to 2 MiB) with a page mapped twice or partly unmapped become views and are back on the fault path. Three existing tests assumed a collect write-protects singly mapped direct memory and were adapted; details in the description. Four new tests, each failing without its fix (a lease wait, a GPU import replaced after a second mapping, concurrent writes and reads over 20 rounds, a 256 MiB remap charged once). Windows @Celegans12 thanks for the offer and for the repros. A rerun of your three and of the Yōtei commit numbers on this head would help. Written with AI assistance (Claude Code). |
|
Windows run of Revert-pin (non-test files back to
With the fix: full ctest 720 of 720 pass (6 skipped: the SDL-offscreen Head vs reviewed commit: Merges: clean onto Not checked: a title run (no dump here) and Linux, where the maintainers build. Thanks for the quick turnaround on the three problems and the rebase, @DotDebian. AI-assisted: yes (Claude Code Fable 5.1 xhigh) |
|
Thanks for the rebase and the fixes; @orishamir's Windows run of This one overlaps #2054, which makes the same change (singly mapped direct memory as private
Open points on this PR whichever way it goes:
|
|
Linux evidence for
AI-assisted: yes (Claude Code Fable 5.1 xhigh) |
|
#637: it's rebased, each of the three problems has a fix and a test, and it has Windows and Linux runs; #2054 still has open asks and conflicts. Two things before it can go in: it now conflicts with main in |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase and the fixes; @orishamir's Windows and Linux runs of dcffc098 are a good confirmation. It now conflicts with main, and one of the conflicts needs a change in your code:
GuestBufferMemory.cpp(importAllocation):mainnow makesWindowsMappings::MapAliasthrowstd::runtime_erroron every refusal, and the caller catches it, adds the base tostate.unaliasedSpansand falls back to copies.MapAliasstill doesreturn nullptrwhen a resident run extends outside the range (WindowsMappings.hpp:259). Please make it throw through the samerefuse(...)helper, keep the caller as onmain, and drop theentry.alias == nullptrbranch. Without that,entry.aliasstays null after the merge.tests/Recorder.cpp: keep both--unit-shadow-only/--cmask-onlyand--host-import-second-mapping-only.- As @oneandonlydean asked:
CheckBreakingAResidentRunKeepsConcurrentAccessesandCheckRemappedDirectMemoryIsChargedOncepass onmain. Make them fail there, or say in the description that they guard the earlierd6bb6c5bbehaviour.
We can't build or run the Windows paths here, so we didn't resolve the MapAlias change ourselves. This overlaps #2054 (same change, conflicting too), so only one of them can land; once this one is rebased and green on Windows it is the one with runs on several titles.
|
Windows real-title result for head Demon's Souls PPSA01342 v1.04 normally stops after approximately 14 s on this host with an audio DSP access violation at No main menu or presented frame verified. This is one bounded real-game run, not a claim that all remaining #361 paths are fixed. The full |
|
@DotDebian GTA V (PPSA04264) still needs this on current
AI-assisted (Claude Code). |
|
@tibzejoker yes, please port it onto current main and keep DotDebian as the author. The points from the 10-11 review still apply: |
|
@oneandonlydean @DotDebian the port is up as #5596, on AI-assisted (Claude Code). |
|
Closing in favour of #5596, the port of this onto current main with DotDebian's commit kept as the author's; follow-ups go there. |
What
Part of #361: on Windows, writes to direct memory mapped at a single guest address are now tracked without raising a fault on the guest thread.
Problem. Direct memory is mapped as section views, and
WindowsMappings::Collectwrite-protects the views it collects. The next guest write faults and is resumed by the vectored handler. Windows builds the exception frame directly belowrsp, over the System V red zone, so a leaf function resumes with its locals overwritten when its AVX state is live (mechanism and standalone reproduction in #361). The bytes are gone before any handler runs, so the fault has to be avoided.Change.
MEM_WRITE_WATCHmemory at that address ("resident").Collectreads its writes withGetWriteWatch, as it already does for flexible memory. No page is write-protected, no fault is raised.GetWriteWatchcall per run.MapAliasneeds views of it. The run is made read-only, copied into the section and replaced by views of the section, tracked by write protection as before. Nothing is copied back into live memory. A write that hits the fence, and a read or write that hits the placeholder between the release and the views, is retried by the fault handler once the views exist.sceKernelMapDirectMemoryand the unmap paths wait for the leases and host imports of every registered range that shares the run (RequireUnpinned), as an unmap of those ranges does, so no import keeps the replaced pages.SEC_RESERVE: it commits only the pages of broken runs.Not changed, still open in #361.
HandleSse4astill resumes an exception on the guest stack.Behaviour to know about.
MapAliasreturns null when a resident run of the range extends outside it, and the import falls back to copies.SEC_RESERVEpage cannot be decommitted, which is why unmapped pages are not stored in the section.CheckPinnedSharedPagesand the shared block ofagc_driver_dcc_keys_first_writenow map their memory twice, andCheckFailedCollectKeepsWritesexpects one 4 KiB page.Tested
Windows 11 Pro 10.0.26200, Intel Core Ultra 9 285K (no AVX-512, so the red zone overwrite from #361 reproduces here), RTX 5080, MinGW-w64 GCC 15.2.0. Head
dcffc098, onmain5edae353.ctest: 710 of 710 run pass, 5 skipped (fourvideo_out_flip_*,guest_video_out_wqhd_detection_no_vulkan).guest_memory,agc_host_import_second_mappingandagc_driver_dcc_keys_first_writepass 10 of 10 repeats.CheckSingleDirectMappingTracksWritesWithoutFaults(guest_memory): two writes to a singly mapped range raise no access violation and are collected as two pages; contents and tracking survive a partial unmap, a second mapping and a full unmap and remap.CheckSecondDirectMappingWaitsForLeases(guest_memory): a second mapping of a page returns only after a lease on the first mapping is dropped. Without the wait it returns at once.agc_host_import_second_mapping(agc_driver_recorder_tests --host-import-second-mapping-only): a host import of singly mapped direct memory is replaced after its first page is mapped again. Without the wait it fails withthe import of direct memory whose pages were replaced was kept.CheckBreakingAResidentRunKeepsConcurrentAccesses(guest_memory): 20 rounds; one thread writes every byte of four runs once and another reads them while the main thread maps a page of each run a second time, or unmaps it. On the previous head the reader faults; with the reader off, bytes are lost in 3 of 3 runs.CheckRemappedDirectMemoryIsChargedOnce(guest_memory): mapping, filling, unmapping and remapping 256 MiB raises the system commit by less than 384 MiB. On the previous head it fails.d6bb6c5bonly, see below) and Linux. The change is inside#ifdef _WIN32code, apart from the new tests, which are compiled out on Linux.Measured on the first version (
d6bb6c5b) on top of theastrobotbranch (b58df66c, discussion #357), not repeated since:SceSndzAudioOutMainat 12 s (eboot+0xec64b5), the crash described in Windows: resuming a write fault overwrites the guest red zone when AVX state is live #361;APS5_NO_WRITE_WATCH=1workaround.Not measured: titles that map most of their direct memory twice, and the cost of unmapping large resident ranges.
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)🤖 Generated with Claude Code