From 168a80f2ed0d35d1fc88eabbe514e616a061b672 Mon Sep 17 00:00:00 2001 From: chrg6 Date: Fri, 13 Nov 2026 17:54:13 -0500 Subject: [PATCH] Run the copy sweep at normal priority, so a preempted sweep cannot hold the recording and submitting threads. The review of the NX2NAVI_COPY_RETIRE count found a priority inversion. The "nvn copy sweep" thread ran at the lowest priority while it held locks the drawing threads take: copy_lock shared for 64 lookups at a time (retire_pass), which copy_add, release_copies and the submission take exclusive, and later shared takers such as copy_touch queue behind such a waiter; retire_lock for the queue swap, which retire_queue takes on the recording thread; memory.c's lock through mem_read_hold and lz_lock through mem_lazy_touches (ctl_fingerprint), which the replay's per-draw check takes. A sweep preempted inside one of those holds by any thread above the lowest level (the pipeline workers at BELOW_NORMAL are enough) kept the recording and submitting threads waiting until the scheduler's starvation boost ran it again, which can take seconds: a new wait on the drawing threads, in the mode the play-test turns on to measure the copies. retire_main now sets normal priority, the level of the threads it shares the locks with, and keeps it: nearly every step of a pass holds one of those locks, and a pass is at most 4096 lookups and 16-byte reads every 5 seconds. The block comment says why. At the default copy_retire = 0 the thread never starts, so nothing changes for players. tests/copy_sweep_check.py also fails when retire_main lowers the thread below normal priority; against the previous nvn_api_cmd.c it fails with that message. The check passes, and the compile check of nvn_api_cmd.c (also with -Wall -Wextra) adds no warnings. Co-Authored-By: Claude Opus 5.5 --- runtime/nvn_api_cmd.c | 21 ++++++++++++++------- tests/copy_sweep_check.py | 10 +++++++++- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/runtime/nvn_api_cmd.c b/runtime/nvn_api_cmd.c index 5ff7ec8e..1ba31c6e 100644 --- a/runtime/nvn_api_cmd.c +++ b/runtime/nvn_api_cmd.c @@ -557,12 +557,13 @@ static uint8_t *flatten_recording(const uint8_t *p, size_t n) { // memory (cmd_t.ctl is accounting only), so the 16 bytes at a copy's control position are whatever the game left // there, and a fingerprint of them that no longer matches says the game wrote there since. A reuse that writes the // same bytes (zeros, often) is not seen: the counts are a lower bound. -// The "nvn copy sweep" thread does every read of guest memory, at its lowest priority: a recording thread taking the -// fingerprint at EndRecording could fault into the lazy heap (an untouched control block) and wait there. Every -// RETIRE_PERIOD_MS it fingerprints the copies made since its last pass (a queue copy_add fills, under retire_lock), -// then visits up to RETIRE_BATCH of the copies it knows, from a cursor, and counts those no submission used for -// COPY_IDLE_MINS whose bytes changed or whose memory is gone. It takes copy_lock shared only, RETIRE_CHUNK lookups at a -// time; a copy that is no longer live (retired, or its handle reused: made_at and bytes differ) leaves its records. +// The "nvn copy sweep" thread does every read of guest memory, at normal priority (retire_main says why): a recording +// thread taking the fingerprint at EndRecording could fault into the lazy heap (an untouched control block) and wait +// there. Every RETIRE_PERIOD_MS it fingerprints the copies made since its last pass (a queue copy_add fills, under +// retire_lock), then visits up to RETIRE_BATCH of the copies it knows, from a cursor, and counts those no submission +// used for COPY_IDLE_MINS whose bytes changed or whose memory is gone. It takes copy_lock shared only, RETIRE_CHUNK +// lookups at a time; a copy that is no longer live (retired, or its handle reused: made_at and bytes differ) leaves its +// records. #define RETIRE_PERIOD_MS 5000 #define RETIRE_BATCH 4096 #define RETIRE_CHUNK 64 @@ -677,9 +678,15 @@ static void retire_pass(void) { } } +// The sweep runs at normal priority, as the threads it shares locks with: nearly every step of a pass holds one they +// take (copy_lock shared, which copy_add, release_copies and the submission take exclusive; retire_lock, which +// retire_queue takes on the recording thread; memory.c's lock through mem_read_hold and lz_lock through +// mem_lazy_touches). A holder below their priority, preempted by any thread above its own (the pipeline workers at +// BELOW_NORMAL are enough), would keep the recording and submitting threads waiting until the scheduler's starvation +// boost ran it again, seconds later. A pass is at most RETIRE_BATCH lookups and 16-byte reads every RETIRE_PERIOD_MS. static void retire_main(void *arg) { (void)arg; - plat_thread_priority(PLAT_PRIO_LOWEST); + plat_thread_priority(PLAT_PRIO_NORMAL); // set, not inherited from the recording thread that started it for (;;) { plat_sleep_ms(RETIRE_PERIOD_MS); retire_pass(); diff --git a/tests/copy_sweep_check.py b/tests/copy_sweep_check.py index db234b13..1d30ecb3 100644 --- a/tests/copy_sweep_check.py +++ b/tests/copy_sweep_check.py @@ -9,7 +9,9 @@ table of live copies and stub guest memory, and checks that without a read; - a retired copy, or one whose handle a new copy took, leaves its records and the counts; - the visits go on where the last pass stopped: two passes over 5000 copies look each up at least once and none - more than twice. + more than twice; + - the sweep thread does not lower its priority below normal (retire_main), as nearly every step of a pass holds a lock + the recording and submitting threads take. No game files. Usage: python tests/copy_sweep_check.py [--cc ] """ @@ -151,6 +153,12 @@ def main(): env['PATH'] = str(MSYS_BIN) + os.pathsep + env.get('PATH', '') src = (ROOT / 'runtime' / 'nvn_api_cmd.c').read_text(encoding='utf-8') body = cut(src, '// ---------- recording copies whose control memory the game reused', '\nstatic void retire_main(') + sweep = cut(src, '\nstatic void retire_main(', '\n}\n') + lowered = [p for p in ('PLAT_PRIO_IDLE', 'PLAT_PRIO_LOWEST', 'PLAT_PRIO_BELOW_NORMAL') if p in sweep] + if lowered: + print(f'FAIL: retire_main lowers the sweep thread\'s priority ({", ".join(lowered)}); it holds locks the ' + 'recording and submitting threads take') + return 1 with tempfile.TemporaryDirectory(prefix='nx2navi-copy-sweep-') as tmp: c = Path(tmp) / 'check.c' c.write_text(HARNESS % body, encoding='utf-8')