Repository navigation
Hash shared pool pages before opening them for CPU writes - #2136
thehobbit85 wants to merge 1 commit into
Conversation
github-actions
Bot
added
nvn
|
Tested this in Ocarina of Time on an M2 Max (macOS 27.3), and it fixed the strobing ground textures in Goron City for me. Before the patch I was on main That run was about 8 minutes. DM_NVN_DIRTYCHECK was not on in either session, so I have no miss counts to compare. No GPU hangs in either session. |
…ol buffer is made: nvn_dirty_open hashed one pool's protected pages and made them writable before it hashed the next pool's, and adjacent pools (4 KB aligned) share their first / last 16 KB host page, so a guest write to that page between the two went into the second pool's first hash, the close found that pool's page unchanged, and its texture stayed clean with stale content. Every range's protected pages are now hashed and taken (BUSY) before any page is opened, so each hash is of a read-only page; the pages are then opened in runs. A fault or host write on a taken page waits for the open as before, and neither holds a page of its own while it waits. The race reaches only pages two pools share: a write that also touches a texture's unshared pages faults there, so only a texture, or a write, that lies wholly in a shared page was missed. t_dirty_race (open_shared_page: two pools in one host page, a store to the second right after the first opens) fails on the old order (expected 1, got 0) and passes; t_dirty_slots and test_docs pass. regress.py check on OCTOPATH TRAVELER II: PASS (13,980 shaders, 59.3 fps, no new red line, a busy Mac), but that run opened no page for a pool buffer, so the new order is covered by the unit test only. Not run: Ocarina of Time or any game that opens pages, the full gfx suite, lint_c.py, the iPhone. A tester's M2 Max run of 8 minutes saw Ocarina's Goron City ground stop strobing with the PR, with no miss counts. Open: issue #2086's stale terrain map spans about 33 host pages, more than this race reaches by itself, so its cause is not found. The fix and the test are thehobbit85's (PR #2136) Co-authored-by: Eliran Zach <4012041+thehobbit85@users.noreply.github.com>
|
Landed on main as 1e69c18, with you as co-author: your fix and your test as written. Thank you. Changes when landing: the code comment is in the file's one-line style and says why holding the taken pages cannot deadlock (a fault or host write waits for the open and holds no page of its own meanwhile), and the reference entry and the Ocarina note now state the race's reach. On the reach: only a pool's first or last host page can be shared with a neighbour, so only a texture, or a write, that lies wholly in a shared page was missed. A write that also touches a texture's unshared pages faults there and marks it written. The map in #2086 is 260x260x2 RGBA8, about 33 host pages, so this race does not explain it by itself, and #2086 stays open. Checked here (M4 Pro): @bzwink thanks for the Goron City run. If the strobing comes back on main, a log with |
…ol buffer is made: nvn_dirty_open hashed one pool's protected pages and made them writable before it hashed the next pool's, and adjacent pools (4 KB aligned) share their first / last 16 KB host page, so a guest write to that page between the two went into the second pool's first hash, the close found that pool's page unchanged, and its texture stayed clean with stale content. Every range's protected pages are now hashed and taken (BUSY) before any page is opened, so each hash is of a read-only page; the pages are then opened in runs. A fault or host write on a taken page waits for the open as before, and neither holds a page of its own while it waits. The race reaches only pages two pools share: a write that also touches a texture's unshared pages faults there, so only a texture, or a write, that lies wholly in a shared page was missed. t_dirty_race (open_shared_page: two pools in one host page, a store to the second right after the first opens) fails on the old order (expected 1, got 0) and passes; t_dirty_slots and test_docs pass. regress.py check on OCTOPATH TRAVELER II: PASS (13,980 shaders, 59.3 fps, no new red line, a busy Mac), but that run opened no page for a pool buffer, so the new order is covered by the unit test only. Not run: Ocarina of Time or any game that opens pages, the full gfx suite, lint_c.py, the iPhone. A tester's M2 Max run of 8 minutes saw Ocarina's Goron City ground stop strobing with the PR, with no miss counts. Open: issue seedds#2086's stale terrain map spans about 33 host pages, more than this race reaches by itself, so its cause is not found. The fix and the test are thehobbit85's (PR seedds#2136) Co-authored-by: Eliran Zach <4012041+thehobbit85@users.noreply.github.com>
…ol buffer is made: nvn_dirty_open hashed one pool's protected pages and made them writable before it hashed the next pool's, and adjacent pools (4 KB aligned) share their first / last 16 KB host page, so a guest write to that page between the two went into the second pool's first hash, the close found that pool's page unchanged, and its texture stayed clean with stale content. Every range's protected pages are now hashed and taken (BUSY) before any page is opened, so each hash is of a read-only page; the pages are then opened in runs. A fault or host write on a taken page waits for the open as before, and neither holds a page of its own while it waits. The race reaches only pages two pools share: a write that also touches a texture's unshared pages faults there, so only a texture, or a write, that lies wholly in a shared page was missed. t_dirty_race (open_shared_page: two pools in one host page, a store to the second right after the first opens) fails on the old order (expected 1, got 0) and passes; t_dirty_slots and test_docs pass. regress.py check on OCTOPATH TRAVELER II: PASS (13,980 shaders, 59.3 fps, no new red line, a busy Mac), but that run opened no page for a pool buffer, so the new order is covered by the unit test only. Not run: Ocarina of Time or any game that opens pages, the full gfx suite, lint_c.py, the iPhone. A tester's M2 Max run of 8 minutes saw Ocarina's Goron City ground stop strobing with the PR, with no miss counts. Open: issue seedds#2086's stale terrain map spans about 33 host pages, more than this race reaches by itself, so its cause is not found. The fix and the test are thehobbit85's (PR seedds#2136) Co-authored-by: Eliran Zach <4012041+thehobbit85@users.noreply.github.com> (cherry picked from commit 1e69c18)
What and why
Adjacent memory pools can share a host page.
nvn_dirty_openpreviously hashed one pool and made its pages writable before hashing its neighbor. A write to that neighbor in the gap became part of its initial hash, so closing the mapping reported unchanged storage and a texture could stay stale.Hash and claim every affected protected range before making any page writable, then open contiguous runs. The synthetic regression injects a real store immediately after the first protection change and reproduces the missed write on the original implementation.
Related to #2086. This fixes a reproduced tracker defect, but its connection to the reported OoT terrain corruption remains unverified in gameplay. Finding: writes while opening shared pool pages, linked from the OoT note.
Checked
make -j4 -C dm/gfx/tests GAME=0400486024DEC000 run— passed.python3 -m unittest discover -s dm/tools/tests— 1,435 tests; 1,411 passed, 23 skipped, one inherited failure. The inherited ABI-literal-ratchet failure in unchangednvn_mtl.malso reproduces from pristine base revision976fbd57f5cbee1f374453c2450c92ffd9f20dbc.git diff --checkpassed.scripts/regress.py impact BASE..HEADwas run. Usage coverage is unknown; no gameplay regression run was possible without game files. Intended checks: OoT (0400486024DEC000, reported stale terrain), Pokopia (0400A3A01EE4C000, NVN2 pools), Monster Hunter Rise (0100B04011742000, RE Engine), Fire Emblem Engage (0100A6301214E000, Unity), and Super Mario Bros. Wonder (010015100B514000, Nintendo NVN). These cover different clients of the shared pool tracker; none is claimed gameplay-validated by this PR.Docs