From 322fd8352a89f7ed4d063c486a8a4742d0cb2c37 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 23 Oct 2026 00:01:32 -0700 Subject: [PATCH 1/2] fix: purge-holes secure guard page, C++ test scope, and scavenger-polluted pacing test (#618) - page-holes.c: mi_page_discard_slack ran its range to the end of the slices, over the MI_SECURE>=5 trailing PROT_NONE guard page; the scavenger's eager-zero memset there segfaulted (idle-thread-releases- large-page, deterministic in a secure C++ build). Stop before the guard, as arena.c's page_noguard_size does. - test-purge-holes.c: scope test_owner_sweep_pacing's measurement locals so its early goto done is valid C++ (MI_USE_CXX); reads unchanged. - new-thread-does-not-sweep-at-once counted process-wide discards, so a background scavenger sweep of thread A's page during C's life was charged to C (loaded: 33/144 fail at 227eef06; 0/144 with the scavenger off). Run it in its own process with no scanner (test-new-thread-pacing, static builds), as test-reserved-page-reuse does. Split out of the rejected V7 draft #616 so the fixes land on their own. --- CMakeLists.txt | 5 ++ src/page-holes.c | 3 ++ test/test-purge-holes.c | 116 +++++++++++++++++++++++----------------- 3 files changed, 74 insertions(+), 50 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a41921ec..1fb8cea1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1574,6 +1574,11 @@ if (MI_BUILD_TESTS) add_test(NAME test-reserved-page-reuse-off COMMAND mimalloc-test-purge-holes --serial-reserve-reuse) set_tests_properties(test-reserved-page-reuse-off PROPERTIES TIMEOUT 600 ENVIRONMENT "MIMALLOC_SCAVENGER=0;MIMALLOC_PURGE_HOLES=0") + # #618: the #478 owner-side pacing case counts process-wide discards, so a background + # scavenger sweep during the measured thread's life polluted it. Fresh process, no scanner. + add_test(NAME test-new-thread-pacing COMMAND mimalloc-test-purge-holes --new-thread-pacing) + set_tests_properties(test-new-thread-pacing PROPERTIES TIMEOUT 600 + ENVIRONMENT "MIMALLOC_SCAVENGER=0;MIMALLOC_PURGE_HOLES=1") endif() # test-purge-zero: imported from oven-sh/mimalloc @ 942b8342, MIT (issue #272 / diff --git a/src/page-holes.c b/src/page-holes.c index 7b429ba0..1a58b414 100644 --- a/src/page-holes.c +++ b/src/page-holes.c @@ -551,6 +551,9 @@ static void mi_page_discard_slack(mi_page_t* page) { const size_t os_size = _mi_os_page_size(); uint8_t* const pstart = mi_page_start(page); uint8_t* hi = mi_page_slice_start(page) + page->memid.mem.arena.slice_count * MI_ARENA_SLICE_SIZE; + #if MI_SECURE>=5 + hi -= _mi_os_secure_guard_page_size(); // #618: the trailing guard page is PROT_NONE, never slack + #endif const size_t committed = mi_page_slice_committed(page); // 0: the whole page is committed if (committed > 0 && hi > mi_page_slice_start(page) + committed) { hi = mi_page_slice_start(page) + committed; } // never beyond what is committed const uintptr_t lo = _mi_align_up((uintptr_t)pstart + (size_t)page->reserved * mi_page_block_size(page), os_size); diff --git a/test/test-purge-holes.c b/test/test-purge-holes.c index 75591091..bc90ce93 100644 --- a/test/test-purge-holes.c +++ b/test/test-purge-holes.c @@ -1884,57 +1884,61 @@ static bool test_owner_sweep_pacing(void) { bool ok_all = true; const long saved = mi_option_get(mi_option_purge_holes_min_interval); - // (a) baseline: an unpaced sweep, which stamps this thread's `holes_sweep_last`. - mi_option_set(mi_option_purge_holes_min_interval, 0); - if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } - mi_on_thread_idle(); - const mi_msecs_t stamped_at = _mi_clock_now(); + // Scope the measurement locals so the early `goto done`s skip their initialisation as a + // whole: valid C++ too, where this file is built under MI_USE_CXX. + { + // (a) baseline: an unpaced sweep, which stamps this thread's `holes_sweep_last`. + mi_option_set(mi_option_purge_holes_min_interval, 0); + if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } + mi_on_thread_idle(); + const mi_msecs_t stamped_at = _mi_clock_now(); - // (b) inside the window: a second `mi_on_thread_idle()` must not sweep at all. - mi_option_set(mi_option_purge_holes_min_interval, PACE_INTERVAL_MS); - if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } - const int64_t before_in = pace_sweep_work(); - mi_on_thread_idle(); - const int64_t after_in = pace_sweep_work(); - const mi_msecs_t elapsed = _mi_clock_now() - stamped_at; - if (elapsed >= PACE_INTERVAL_MS) { - // the churn itself outran the window (a very loaded box): the negative half is not - // decidable, say so rather than fail on the machine's timing - fprintf(stderr, "\n SKIP in-window half: churn took %ldms, window is %dms\n", - (long)elapsed, PACE_INTERVAL_MS); - } - else if (after_in != before_in) { - fprintf(stderr, "\n swept %ldms into a %dms window: sweep work %lld -> %lld\n", - (long)elapsed, PACE_INTERVAL_MS, (long long)before_in, (long long)after_in); - ok_all = false; - } + // (b) inside the window: a second `mi_on_thread_idle()` must not sweep at all. + mi_option_set(mi_option_purge_holes_min_interval, PACE_INTERVAL_MS); + if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } + const int64_t before_in = pace_sweep_work(); + mi_on_thread_idle(); + const int64_t after_in = pace_sweep_work(); + const mi_msecs_t elapsed = _mi_clock_now() - stamped_at; + if (elapsed >= PACE_INTERVAL_MS) { + // the churn itself outran the window (a very loaded box): the negative half is not + // decidable, say so rather than fail on the machine's timing + fprintf(stderr, "\n SKIP in-window half: churn took %ldms, window is %dms\n", + (long)elapsed, PACE_INTERVAL_MS); + } + else if (after_in != before_in) { + fprintf(stderr, "\n swept %ldms into a %dms window: sweep work %lld -> %lld\n", + (long)elapsed, PACE_INTERVAL_MS, (long long)before_in, (long long)after_in); + ok_all = false; + } - // (c) past the window: the very next `mi_on_thread_idle()` sweeps again. The deadline has - // to EXPIRE, not merely be disabled -- so wait it out rather than setting the option to 0. - #if MI_OWNER_GATE - // #366: in a gated build this thread is PARKED for the whole wait, so the scavenger sweeps it - // as soon as the window expires -- which re-stamps `holes_sweep_last`, and the owner's own - // `mi_on_thread_idle()` below then lands INSIDE a fresh window and (correctly) does nothing. - // The contract "a sweep happens past the window" still holds; count from before the wait. - const int64_t before_out = pace_sweep_work(); - pace_sleep_ms(PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2)); - if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } - #else - pace_sleep_ms(PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2)); - if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } - const int64_t before_out = pace_sweep_work(); - #endif - mi_on_thread_idle(); - const int64_t after_out = pace_sweep_work(); - if (purging_enabled && after_out <= before_out) { - fprintf(stderr, "\n did not sweep %dms past a %dms window: sweep work %lld -> %lld\n", - PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2), PACE_INTERVAL_MS, - (long long)before_out, (long long)after_out); - ok_all = false; + // (c) past the window: the very next `mi_on_thread_idle()` sweeps again. The deadline has + // to EXPIRE, not merely be disabled -- so wait it out rather than setting the option to 0. + #if MI_OWNER_GATE + // #366: in a gated build this thread is PARKED for the whole wait, so the scavenger sweeps it + // as soon as the window expires -- which re-stamps `holes_sweep_last`, and the owner's own + // `mi_on_thread_idle()` below then lands INSIDE a fresh window and (correctly) does nothing. + // The contract "a sweep happens past the window" still holds; count from before the wait. + const int64_t before_out = pace_sweep_work(); + pace_sleep_ms(PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2)); + if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } + #else + pace_sleep_ms(PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2)); + if (!pace_churn(ptrs, N, SZ)) { ok_all = false; goto done; } + const int64_t before_out = pace_sweep_work(); + #endif + mi_on_thread_idle(); + const int64_t after_out = pace_sweep_work(); + if (purging_enabled && after_out <= before_out) { + fprintf(stderr, "\n did not sweep %dms past a %dms window: sweep work %lld -> %lld\n", + PACE_INTERVAL_MS + (PACE_INTERVAL_MS / 2), PACE_INTERVAL_MS, + (long long)before_out, (long long)after_out); + ok_all = false; + } + // with `purge_holes=0` there is no hole phase to count, so (c) has no observable -- the + // stamp still runs (see `_mi_purge_holes_of`), which is what keeps the scavenger's own + // pacing identical in the two builds. } - // with `purge_holes=0` there is no hole phase to count, so (c) has no observable -- the - // stamp still runs (see `_mi_purge_holes_of`), which is what keeps the scavenger's own - // pacing identical in the two builds. done: for (size_t i = 0; i < N; i++) { if (ptrs[i] != NULL) mi_free(ptrs[i]); } @@ -2239,8 +2243,21 @@ int main(int argc, char** argv) { CHECK("dead-thread-page-is-reused", test_dead_thread_page_is_reused()); return print_test_summary(); } + if (argc == 2 && strcmp(argv[1], "--new-thread-pacing") == 0) { + // #618: the case counts PROCESS-wide discards around thread C, so a background sweep + // of A's abandoned page during C's life was charged to C (reproduced under load on the + // frozen baseline too; 0 of 144 loaded runs failed with the scavenger off). #478 is about + // the OWNER's first tick, so measure it in its own process with no scanner started; the + // background release cases keep running with it enabled in the default invocation. + CHECK("new-thread-pacing-scavenger-disabled", + !mi_option_is_enabled(mi_option_scavenger) && !_mi_scavenger_is_running()); + if (failed != 0) return print_test_summary(); + purging_enabled = mi_option_is_enabled(mi_option_purge_holes); + CHECK("new-thread-does-not-sweep-at-once", test_new_thread_does_not_sweep_at_once()); + return print_test_summary(); + } if (argc != 1) { - fprintf(stderr, "usage: %s [--serial-reserve-reuse]\n", argv[0]); + fprintf(stderr, "usage: %s [--serial-reserve-reuse | --new-thread-pacing]\n", argv[0]); return EXIT_FAILURE; } purging_enabled = mi_option_is_enabled(mi_option_purge_holes); @@ -2310,7 +2327,6 @@ int main(int argc, char** argv) { CHECK("sweep-does-not-unpurge-on-collect", test_sweep_no_unpurge_on_collect()); CHECK("sweep-full-every-bounds-a-missed-hole", test_sweep_full_every()); CHECK("owner-sweeps-are-paced", test_owner_sweep_pacing()); - CHECK("new-thread-does-not-sweep-at-once", test_new_thread_does_not_sweep_at_once()); #if !defined(_WIN32) CHECK("idle-thread-releases-large-page", test_idle_thread_releases_large_page()); CHECK("reserved-page-is-released", test_reserved_page_is_released()); From 6f16f0f6f8550c8b96304521e89ab1647bdc6860 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 23 Oct 2026 00:01:45 -0700 Subject: [PATCH 2/2] build: regenerate vendor C for #618 fixes Local-Gate: v1 tree=4e8686a912585756840b5a6d1bb1f4a6993c6a78 secs=73 lanes=lint:run Ci-Attestation: {"at":1792738979,"gate":"python/all/lint","host":"linux-x86_64","key":"a205b85527b6c381cb0d7b1b8f84a8462b6b50492a47b48b07c583ea0879eb00","lane":"lint","parents":["322fd8352a89f7ed4d063c486a8a4742d0cb2c37"],"secs":73,"stamp":"8bfbb21af4ce30960798d94dba866ac8","tree":"4e8686a912585756840b5a6d1bb1f4a6993c6a78","v":1,"via":"run"} --- rust/mimalloc-pprof/vendor/mimalloc-pprof-amalgamated.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rust/mimalloc-pprof/vendor/mimalloc-pprof-amalgamated.c b/rust/mimalloc-pprof/vendor/mimalloc-pprof-amalgamated.c index e769e8f6..f3888245 100644 --- a/rust/mimalloc-pprof/vendor/mimalloc-pprof-amalgamated.c +++ b/rust/mimalloc-pprof/vendor/mimalloc-pprof-amalgamated.c @@ -1,4 +1,4 @@ -/* GENERATED FILE -- DO NOT EDIT. Produced by rust/xtask from commit 52eab5b3 of src/static.c. Regenerate with: cargo run -p xtask -- amalgamate-c */ +/* GENERATED FILE -- DO NOT EDIT. Produced by rust/xtask from commit 322fd835 of src/static.c. Regenerate with: cargo run -p xtask -- amalgamate-c */ /* ---- begin inlined: src/static.c ---- */ /* ---------------------------------------------------------------------------- @@ -27774,6 +27774,9 @@ static void mi_page_discard_slack(mi_page_t* page) { const size_t os_size = _mi_os_page_size(); uint8_t* const pstart = mi_page_start(page); uint8_t* hi = mi_page_slice_start(page) + page->memid.mem.arena.slice_count * MI_ARENA_SLICE_SIZE; + #if MI_SECURE>=5 + hi -= _mi_os_secure_guard_page_size(); // #618: the trailing guard page is PROT_NONE, never slack + #endif const size_t committed = mi_page_slice_committed(page); // 0: the whole page is committed if (committed > 0 && hi > mi_page_slice_start(page) + committed) { hi = mi_page_slice_start(page) + committed; } // never beyond what is committed const uintptr_t lo = _mi_align_up((uintptr_t)pstart + (size_t)page->reserved * mi_page_block_size(page), os_size);