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 <noreply@anthropic.com>
This commit is contained in:
chrg6
2026-11-13 17:54:13 -05:00
parent 88a64573a2
commit 168a80f2ed
2 changed files with 23 additions and 8 deletions

View File

@@ -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();

View File

@@ -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 <clang>]
"""
@@ -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')