Hash shared pool pages before opening them for CPU writes

dm/games/0400486024DEC000/README.md

@@ -184,6 +184,7 @@ From an empty save: at about 45 s `A` (title), `A` (New Game), then the movie. F
 - [A cold start leaves a file per timer flush, and the next start compacts them all](../../../docs/reference/rendering.md#a-cold-start-leaves-a-file-per-timer-flush-and-the-next-start-compacts-them-all): a cold start wrote 80 run files and the next start captured 8,859 pipelines again, 762 s before the first frame on an M4 with 16 GB; with the change 5 files, and 3.3 s (issue #1747).
 - [Archive lookups asked every file, so a few files made a start compile every base again](../../../docs/reference/rendering.md#archive-lookups-asked-every-file-so-a-few-files-made-a-start-compile-every-base-again): 9 files of play beside a compaction's 5 made the next start capture 8,543 bases again, 713 s before the first frame (PR #1880); an index beside each file makes a lookup ask one file whatever the number of files.
 - [Texture IDs of one storage share one packaged texture](../../../docs/reference/nvn-objects.md#texture-ids-of-one-storage-share-one-packaged-texture)
+- [A write missed while opening shared pool pages](../../../docs/reference/nvn-objects.md#a-write-missed-while-opening-shared-pool-pages): a tracker race reproduced with synthetic memory while investigating issue #2086. The race is fixed; its connection to the reported terrain corruption is still unverified in play.
 - [A texture shared by its packaged texture's serials left the residency set with one of them](../../../docs/reference/nvn-objects.md#a-texture-shared-by-its-packaged-textures-serials-left-the-residency-set-with-one-of-them): GPU page faults 15-45 minutes into play (issue #1326), on 260x260 terrain tiles made GPU textures at the GPU's first write.
 - [Finalizing a copy of a texture object](../../../docs/reference/nvn-objects.md#finalizing-a-copy-of-a-texture-object)
 - [Dynamic resolution that never moves, and draw passes that wait for each other](../../../docs/reference/frame-rate.md#dynamic-resolution-that-never-moves-and-draw-passes-that-wait-for-each-other)

dm/gfx/nvn/nvn_dirty.c

@@ -215,26 +215,28 @@ void nvn_dirty_open(const void *addr, size_t len) {
     pthread_mutex_lock(&g_slot_mu);
     g_nopen = 0;
     uintptr_t a = (uintptr_t)addr, e = a + len; int n = atomic_load(&g_nr);
+    // Take and hash every range before opening any page: adjacent pools can share
+    // a host page. Opening one first let a write reach its neighbour before that
+    // neighbour's hash, so the neighbour's before/after hashes missed the write.
     for (int k = 0; len && k < n; k++) {
         range_t *r = rg(k);
         if (!live_at(k) || e <= r->base || a >= r->end) continue;
-        uintptr_t lo = a > r->base ? a : r->base, hi = e < r->end ? e : r->end, p = (lo - r->base) >> g_shift, q = (hi - 1 - r->base) >> g_shift, run = 0; int open = 0;
-        for (uintptr_t i = p; i <= q + 1; i++) {
-            // A protected page is read-only and, with g_slot_mu held, stays protected until someone unprotects it: hashed first, then taken; one unprotected meanwhile is not ours
-            uint8_t st = i <= q ? atomic_load(&r->state[i]) : PG_NONE; int took = 0;
+        uintptr_t lo = a > r->base ? a : r->base, hi = e < r->end ? e : r->end, p = (lo - r->base) >> g_shift, q = (hi - 1 - r->base) >> g_shift;
+        for (uintptr_t i = p; i <= q; i++) {
+            uint8_t st = atomic_load(&r->state[i]);
             if (st == PG_PROT) {
                 uint64_t h = page_hash((const void *)(r->base + (i << g_shift)));
-                if ((took = atomic_compare_exchange_strong(&r->state[i], &st, PG_BUSY))) open_add(r, i, h);
+                if (atomic_compare_exchange_strong(&r->state[i], &st, PG_BUSY)) open_add(r, i, h);
             }
-            if (took) { if (!open) { run = i; open = 1; } continue; }
-            if (open) {
-                mp(r->base + (run << g_shift), (i - run) << g_shift, PROT_READ | PROT_WRITE);
-                for (uintptr_t j = run; j < i; j++) atomic_store(&r->state[j], PG_OPEN);
-                open = 0;
-            }
-            if (st == PG_BUSY) busy_wait(r, i);   // being unprotected: writable once it is done (with g_slot_mu held nothing protects it), and the buffer must not be made before
+            if (st == PG_BUSY) busy_wait(r, i);   // another thread's unprotect: it finishes without taking g_slot_mu or waiting for another page
         }
     }
+    for (size_t j = 0; j < g_nopen; ) {
+        range_t *r = g_open[j].r; size_t s = j++;
+        while (j < g_nopen && g_open[j].r == r && g_open[j].p == g_open[s].p + (j - s)) j++;
+        mp(r->base + (g_open[s].p << g_shift), (j - s) << g_shift, PROT_READ | PROT_WRITE);
+        for (size_t k = s; k < j; k++) atomic_store(&r->state[g_open[k].p], PG_OPEN);
+    }
     atomic_fetch_add(&g_opened, g_nopen);
 }
 void nvn_dirty_close(void) {

dm/gfx/tests/t_dirty_race.c

@@ -35,7 +35,7 @@ static int g_fail;
     if (want_ != got_) g_fail++; \
 } while (0)
 
-static uint8_t *g_page; static _Atomic int g_armed, g_go, g_b_done;
+static uint8_t *g_page, *g_shared_write; static _Atomic int g_armed, g_go, g_b_done;
 static int g_publish_armed, g_observed_clean;
 static uint8_t *g_publish_page;
 static const volatile void *g_publish_gen;
@@ -50,7 +50,13 @@ static void t_before_store(const volatile void *object) {
 static void *thread_b(void *arg) { (void)arg; while (!atomic_load(&g_go)) usleep(200); prep_write(g_page, 16); atomic_store(&g_b_done, 1); return NULL; }
 int t_mprotect(void *a, size_t n, int prot) {
     if (prot == PROT_READ && atomic_exchange(&g_armed, 0)) { atomic_store(&g_go, 1); usleep(100000); }   // Before A's mprotect lands: let B in and wait until it runs to the end or gets stuck waiting
-    return mprotect(a, n, prot);
+    int result = mprotect(a, n, prot);
+    // A guest store after the first range opens a shared page, before the next range hashes it.
+    if (!result && prot == (PROT_READ | PROT_WRITE) && g_shared_write &&
+        (uintptr_t)g_shared_write >= (uintptr_t)a && (uintptr_t)g_shared_write < (uintptr_t)a + n) {
+        *g_shared_write = 9; g_shared_write = NULL;
+    }
+    return result;
 }
 
 // A page another thread is unprotecting (its mprotect to writable done and the new generation stored, the state still BUSY) must be waited out by nvn_dirty_protect and
@@ -149,6 +155,26 @@ static void open_close(size_t pg) {
     EXPECT("unwritten page: a later write is seen", 1, nvn_dirty_since(m + 2 * pg, pg, later));
     nvn_dirty_remove_range(m); munmap(m, 3 * pg);
 }
+// Adjacent 4 KB-aligned pools share a 16 KB host page. Opening A makes B's bytes
+// writable too, before B takes its own hash. A write in that window must dirty B
+// even though B's own before/after hashes match. No game data or GPU is involved.
+static void open_shared_page(size_t pg) {
+    if (pg <= 4096) return;   // no shared host page for 4 KB-aligned pools
+    uint8_t *m = mmap(NULL, 3 * pg, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
+    if (m == MAP_FAILED) { perror("mmap"); g_fail++; return; }
+    size_t na = pg - 4096, nb = pg + 4096;
+    uint8_t *b = m + na;
+    nvn_dirty_add_range(m, na); nvn_dirty_add_range(b, nb);
+    uint64_t stamp = nvn_dirty_now();
+    nvn_dirty_protect(m, na); nvn_dirty_protect(b, nb);
+    nvn_dirty_open(m, 2 * pg); nvn_dirty_close();
+    EXPECT("shared page with no writes: B stays clean", 0, nvn_dirty_since(b, nb, stamp));
+    g_shared_write = b + 16;
+    nvn_dirty_open(m, 2 * pg); nvn_dirty_close();
+    EXPECT("the guest store reached B's shared-page bytes", 9, b[16]);
+    EXPECT("write while A opens: B's texture is dirty", 1, nvn_dirty_since(b, nb, stamp));
+    nvn_dirty_remove_range(b); nvn_dirty_remove_range(m); munmap(m, 3 * pg);
+}
 // A page another thread is unprotecting (BUSY, still read-only) when nvn_dirty_open looks: the buffer must not be made over it while it is read-only, so the open waits for it
 static void open_waits_busy(size_t pg) {
     uint8_t *m = mmap(NULL, pg, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
@@ -193,5 +219,6 @@ int main(void) {
     generation_publish(pg, 1);
     open_close(pg);
     open_waits_busy(pg);
+    open_shared_page(pg);
     printf("t_dirty_race: %s\n", g_fail ? "FAILED" : "all passed"); return g_fail != 0;
 }

docs/agents/symptoms.md

@@ -603,6 +603,7 @@ One line per symptom in the [subsystem references](../reference/): what you see
 - textures that should hold CPU-written data come out magenta (ASTC's error colour) or black. → [Sampled textures in a sparse pool uploaded through the page table](../reference/nvn-objects.md#sampled-textures-in-a-sparse-pool-uploaded-through-the-page-table)
 - the submit thread spends most of its time in `nvn_dirty_since` under `nvn_sparse_since`, called from `tex_mtl` (render-target and texture lookups of textures in a sparse pool). → [A sparse texture's CPU-write check at every lookup](../reference/nvn-objects.md#a-sparse-textures-cpu-write-check-at-every-lookup)
 - some textures are re-uploaded every frame (several MB per frame). → [Dirty-page ranges of pools that share a host page](../reference/nvn-objects.md#dirty-page-ranges-of-pools-that-share-a-host-page)
+- a CPU-written texture stays stale although its storage changed while the host made a zero-copy pool buffer. → [A write missed while opening shared pool pages](../reference/nvn-objects.md#a-write-missed-while-opening-shared-pool-pages)
 - packaged textures re-uploaded at every use with unchanged content; upload staging and frame times rising. → [Writes beside a packaged texture in a shared host page](../reference/nvn-objects.md#writes-beside-a-packaged-texture-in-a-shared-host-page)
 - with `tex_dirty_upload`, textures the CPU never writes (window textures, render targets) are logged as `CPU-written texture …` or `registered texture written by the CPU after its first use: uploaded from its storage` soon after boot. The present thread then re-uploads a window texture from its storage, and the host aborts: `[RED] upload batches belong to the submit thread only: up_begin called on thread 'nvn-present'`, with `pkg_upload` ← `tex_mtl_` ← `present_now` on the stack. → [Writes beside a registered texture in a shared host page](../reference/nvn-objects.md#writes-beside-a-registered-texture-in-a-shared-host-page)
 - about one run in four stalled early (around the opening movie or the title), with no log output, not even the time-limit report. → [Dirty-page tracker race and early stalls](../reference/nvn-objects.md#dirty-page-tracker-race-and-early-stalls)

docs/reference/nvn-objects.md

@@ -664,6 +664,14 @@ Games read this count as a position in their own command memory and size their h
 - **Found on:** the source names no game.
 - **Check:** `t_dirty_slots` covers it.
 
+### A write missed while opening shared pool pages
+
+- **Symptom:** a CPU-written texture stays stale although its storage changed while the host made a zero-copy pool buffer.
+- **Cause:** `nvn_dirty_open` hashed and opened each pool range in turn. Adjacent 4 KB-aligned pools can share a 16 KB host page. Opening the first pool made the second pool's bytes writable before the second pool's hash was taken. A CPU write between those steps changed the first range's hash, but the second range's before/after hashes matched, leaving its texture clean with stale content.
+- **Fix:** take and hash all protected ranges before making any of their pages writable. Closing still re-protects the pages and stamps only changed content; unchanged pages do not force texture uploads.
+- **Found on:** a synthetic reproduction while investigating [Ocarina of Time](../../dm/games/0400486024DEC000/README.md) [issue #2086](https://github.com/seedds/nxnative/issues/2086). Whether this race caused that game's reported terrain corruption remains open; no game files or gameplay run were used.
+- **Check:** `t_dirty_race` places two adjacent pools in one host page and makes a real store to the second pool immediately after the first becomes writable. Without the fix its texture is incorrectly clean; with the fix it is dirty. Opening and closing without a write stays clean. The existing fault, host-write, busy-page and pool-retirement tests also run without game files.
+
 ### Writes beside a packaged texture in a shared host page
 
 - **Symptom:** packaged textures re-uploaded at every use with unchanged content; upload staging and frame times rising.