From a8d18ef474321d367e22bb1ee0bb0cf65aa4d453 Mon Sep 17 00:00:00 2001 From: chrg6 Date: Fri, 13 Nov 2026 20:04:59 -0500 Subject: [PATCH] Ask an armed region's page state under the freeze lock when a fault finds its range gone, so that a late write is not taken for a frozen page's and a read is not passed on past an arm. The review found two windows in memory.c lazy_fault, where a fault holding frz_lock and lz_lock finds its address neither armed nor in flight. It returned 0 there, and access_fault then asked for the page's state without a lock. A write whose range a flight gave back while the write waited for frz_lock went on to thaw_fault, whose thaw released every armed range of the 2 MiB region (lazy_drop_region_locked), each fetched under the lock. And after a release that sets no hold (mem_lazy_skip's give-back of another buffer's range), the completion thread could arm the page again between that 0 and access_fault's query, so a read saw an inaccessible page and went to the crash handler; that one predates the ranges in flight. Now a fault that finds a flight there waits for it as before. Otherwise it releases lz_lock, asks for the page's state while it still holds frz_lock, under which no arm runs, and runs the access again when the page allows it: a read on a readable page, a write on a read-write one. A write to a frozen page still goes on to thaw_fault, and an address that is not ours to the crash handler. The new late_write scenario of tests/lazy_fetch_check.py has a thread write a range whose release starts and ends while the write waits for frz_lock: the write runs again and the region's other range stays armed. With the old return of 0 it fails, the other range released. The read's window has no check: the arm would have to land between two instructions. Not reproduced in the game before the change; lazy_fetch_check passed in all its scenarios. Co-Authored-By: Claude Opus 5.5 --- docs/building.md | 3 ++- runtime/memory.c | 45 ++++++++++++++++++++++++++------------- tests/lazy_fetch_check.c | 24 +++++++++++++++++++++ tests/lazy_fetch_check.py | 1 + 4 files changed, 57 insertions(+), 16 deletions(-) diff --git a/docs/building.md b/docs/building.md index 0e4f472b..29d4aa2b 100644 --- a/docs/building.md +++ b/docs/building.md @@ -697,7 +697,8 @@ does not either when a range in flight in its region ends during that test. A fe of its range: freezes, thaws, arms, releases and copies of other ranges return at once meanwhile, also while a thaw fetches the armed ranges of its own regions, two ranges fetch at once, and more touching threads than the flight table holds all end. The touches of a range in flight wait for it and run again, a write among them without releasing the -region's other ranges; thaws, mapping changes, releases of its buffer, arms and freezes see it. The same with +region's other ranges, also when the flight ended while the write waited for the freeze lock; thaws, mapping changes, +releases of its buffer, arms and freezes see it. The same with `wb_lazy_chunk = 1`, and with `wb_lazy_unlocked = 0` the fetch holds the freeze lock again, a thaw's too, and a release fetches one range at a time (the table of ranges in flight stays: a touch of one still waits for it alone). The "mem: lazy write-back" lines give the longest fetch, the longest wait of a fault for the freeze lock and the diff --git a/runtime/memory.c b/runtime/memory.c index ff0a95df..1fc5aba9 100644 --- a/runtime/memory.c +++ b/runtime/memory.c @@ -266,15 +266,17 @@ static volatile uint32_t lz_fetch_ms; // plat_now_ms() | 1 at the end of th // - an arm leaves its pages alone (mem_lazy_arm), a freeze its region (lzr), and mem_lazy_touches counts it. // Every segment that leaves the list with inaccessible pages becomes an entry in the same lz_lock hold, and an entry // ends only once its pages are read-write; lists and entries never overlap. So a fault that finds its address in -// neither, holding frz_lock (no arm runs then), has an address that is not ours or no longer inaccessible. An entry -// starts under frz_lock and lz_lock and ends under lz_lock alone, so a holder of frz_lock may wait for one: its fetch -// takes the queue lock, and no holder of the queue lock takes frz_lock. Every store to lzr is made under lz_lock, from -// the list, the entries and the region an arm is protecting (lz_arm_r). Fetches of different ranges run at once (the -// back end has a slot for each, vk_memory.c lazy_fetch). NX2NAVI_WB_LAZY_UNLOCKED=0 brings back 0.10.1's locking: frz_lock -// is kept from the moment a fault or a release takes a range out until its entry ends, a release takes one segment at a -// time, mem_thaw releases under the lock, and vk_memory.c mirror_evict and hot_check release their victims under -// pool_lock, so one fetch runs at a time. The entries stay with it: a touch of a range in flight waits for that entry -// and runs again, the hold is set at the take-out, and mem_lazy_touches counts the range. +// neither, holding frz_lock (no arm runs then), has an address that is not ours or no longer inaccessible: the page's +// state, asked under that lock, decides whether the access runs again (lazy_fault), and an arm after it only has the +// access fault here once more. An entry starts under frz_lock and lz_lock and ends under lz_lock alone, so a holder of +// frz_lock may wait for one: its fetch takes the queue lock, and no holder of the queue lock takes frz_lock. Every +// store to lzr is made under lz_lock, from the list, the entries and the region an arm is protecting (lz_arm_r). +// Fetches of different ranges run at once (the back end has a slot for each, vk_memory.c lazy_fetch). +// NX2NAVI_WB_LAZY_UNLOCKED=0 brings back 0.10.1's locking: frz_lock is kept from the moment a fault or a release takes +// a range out until its entry ends, a release takes one segment at a time, mem_thaw releases under the lock, and +// vk_memory.c mirror_evict and hot_check release their victims under pool_lock, so one fetch runs at a time. The +// entries stay with it: a touch of a range in flight waits for that entry and runs again, the hold is set at the +// take-out, and mem_lazy_touches counts the range. #define LZ_FLY 64 // more than the threads that touch armed pages at once; when full, a touch waits for room #define LZ_BATCH 16 // segments a release takes out together and fetches in one call (1 with the setting at 0) typedef struct { uint64_t lo, hi, h0, bytes; uint32_t seq, tid; uint8_t used; } LzFly; // bytes: to fetch, 0 none @@ -814,8 +816,9 @@ void mem_lazy_window(int open) { plat_mutex_unlock(&lz_lock); } -// An access fault at a: 1 when a was armed or in flight and its pages are read-write again (the access runs again), 0 -// when a is none of the lazy write-back's. +// An access fault at a: 1 when a was armed or in flight and its pages are read-write again, or when its page allows the +// access by now (another thread gave it back): the access runs again. 0 when a is none of the lazy write-back's and its +// page does not allow the access (a write to a frozen page goes on to thaw_fault). static int lazy_fault(uint64_t a, int write) { if (a >= RT_ADDRESS_LIMIT || !__atomic_load_n(&lzr[a >> FRZ_SHIFT], __ATOMIC_ACQUIRE)) return 0; uint32_t me = plat_thread_id(); @@ -834,11 +837,23 @@ static int lazy_fault(uint64_t a, int write) { plat_mutex_lock(&lz_lock); // the window's test and the segment's leaving the list in one step int i = 0; while (i < lz_n && !(a >= lzs[i].lo && a < lzs[i].hi)) i++; - if (i == lz_n) { // in flight since the look above; else not ours, or given back (no arm runs under frz_lock) - plat_mutex_unlock(&frz_lock); - int joined = lazy_fly_join_locked(a); + if (i == lz_n) { + if (lazy_fly_find_locked(a, a + 1, 0) >= 0) { // in flight since the look above: wait for it alone + plat_mutex_unlock(&frz_lock); + lazy_fly_join_locked(a); + plat_mutex_unlock(&lz_lock); + return 1; + } + // Not ours, or given back since the fault: the page's state decides, asked under frz_lock alone (no system + // call under lz_lock), so that no arm makes the page inaccessible between the answer and the retry. A write + // to a page a flight gave back runs again here instead of going on to thaw_fault, whose thaw would release + // every armed range of the region; and a read is not passed to the crash handler because an arm (after a + // release that sets no hold, mem_lazy_skip's give-back) made the page inaccessible again once this returned. plat_mutex_unlock(&lz_lock); - return joined; + uint64_t pg = a & ~(PAGE - 1), end; + int st = plat_mem_state(pg, pg + PAGE, &end); + plat_mutex_unlock(&frz_lock); + return write ? st == PLAT_PROT_RW : st >= PLAT_PROT_READ; } uint32_t w = __atomic_load_n(&lz_win_tid, __ATOMIC_ACQUIRE); if (w && w != me && tries < 400) { // up to 20 ms: after the hand-over the fetch has that part's writes diff --git a/tests/lazy_fetch_check.c b/tests/lazy_fetch_check.c index 8c8b7d6c..6abbe390 100644 --- a/tests/lazy_fetch_check.c +++ b/tests/lazy_fetch_check.c @@ -268,6 +268,30 @@ int main(int argc, char **argv) { CHECK(armed(s2, K64), "the region's other range was released"); CHECK(lz_other == 0, "%llu other releases", (unsigned long long)lz_other); CHECK(b.took < 250 && b.got == 0x77, "the write waited %.1f ms and read back %#x", b.took, b.got); + } else if (!strcmp(what, "late_write")) { + // B writes a range whose flight starts and ends while B waits for frz_lock in lazy_fault (this thread holds the + // lock and runs a release's flight under it, as a thaw does): B finds the page given back and writes again, and + // the region's other range stays armed (B went on to thaw_fault before, whose thaw released it) + uint64_t R = heap + 128 * M, s1 = R, s2 = R + M; + CHECK(arm_buf(s1, K64, 1) == K64 && arm_buf(s2, K64, 1) == K64, "arms"); + stale(s1, K64, 1), stale(s2, K64, 1); + fetch_ns = 20000000ull; + plat_mutex_lock(&frz_lock); + Touch b; + void *tb = touch(&b, s1 + 0x20, 1); + plat_wait_ns(50000000); // B waits for frz_lock in lazy_fault, past its look for a range in flight + int k[LZ_BATCH]; + LzSeg sg[LZ_BATCH]; + uint64_t rg[1][3] = {{s1, s1 + K64, 0}}; + plat_mutex_lock(&lz_lock); + int nt = lazy_take_batch_locked((const uint64_t (*)[3])rg, 1, k, sg); + plat_mutex_unlock(&lz_lock); + if (nt > 0) lazy_fly_run(k, sg, nt); + plat_mutex_unlock(&frz_lock); + plat_thread_join(tb); + CHECK(nt == 1 && b.got == 0x77, "%d taken out, the write read back %#x", nt, b.got); + CHECK(armed(s2, K64) && lz_other == 0, "the region's other range was released (%llu other releases)", + (unsigned long long)lz_other); } else if (!strcmp(what, "concurrent")) { // two threads touch two ranges at once: their fetches run at the same time uint64_t s1 = heap + 40 * M, s2 = heap + 48 * M; diff --git a/tests/lazy_fetch_check.py b/tests/lazy_fetch_check.py index 89922ef1..9c4d87c6 100644 --- a/tests/lazy_fetch_check.py +++ b/tests/lazy_fetch_check.py @@ -27,6 +27,7 @@ SCENARIOS = { 'no_stall': ('no_stall', {}), 'join': ('join', {}), 'loser': ('loser', {}), + 'late_write': ('late_write', {}), 'concurrent': ('concurrent', {}), 'thaw': ('thaw', {}), 'thaw_unlocked': ('thaw_unlocked', {}),