-
Notifications
You must be signed in to change notification settings - Fork 5k
Startup snapshots (1/4): dependency pins and build wiring #37259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| --- a/crypto/rand/fork_detect.cc | ||
| +++ b/crypto/rand/fork_detect.cc | ||
| @@ -171,6 +171,17 @@ | ||
| return current_generation; | ||
| } | ||
|
|
||
| +// Bun: a process resumed from a startup snapshot inherits these statics from the process that built it, including a | ||
| +// page address that is not mapped here. Restore is single-threaded; installing this process's own page is enough, | ||
| +// since the once flag above already reads as done. | ||
| +extern "C" void CRYPTO_fork_detect_reinit_for_startup_snapshot(void) { | ||
| + if (g_fork_detect_addr == nullptr) return; // never initialized (this process's once will), or WIPEONFORK was unavailable there (stays in the always-reseed fallback) | ||
| + uint64_t generation_in_builder = g_fork_generation; | ||
| + g_fork_detect_addr = nullptr; | ||
| + init_fork_detect(); | ||
| + g_fork_generation = generation_in_builder + 1; // a restore duplicates the address space like a fork: anything cached against the builder's value must reseed | ||
| +} | ||
| + | ||
|
claude[bot] marked this conversation as resolved.
|
||
| void bssl::CRYPTO_fork_detect_force_madv_wipeonfork_for_testing(int on) { | ||
| g_force_madv_wipeonfork = 1; | ||
| g_force_madv_wipeonfork_enabled = on; | ||
| @@ -197,6 +208,14 @@ | ||
| g_atfork_fork_generation = 1; | ||
| } | ||
|
|
||
| +// Bun: see the WIPEONFORK variant; here the build process's atfork registration does not exist in this process. | ||
| +extern "C" void CRYPTO_fork_detect_reinit_for_startup_snapshot(void) { | ||
| + if (g_atfork_fork_generation == 0) return; // as above: nothing to redo if the build process never initialized it | ||
| + uint64_t generation_in_builder = g_atfork_fork_generation; | ||
| + init_pthread_fork_detection(); | ||
| + g_atfork_fork_generation = generation_in_builder + 1; // as above | ||
| +} | ||
| + | ||
| uint64_t bssl::CRYPTO_get_fork_generation() { | ||
| CRYPTO_once(&g_pthread_fork_detection_once, init_pthread_fork_detection); | ||
|
|
||
| @@ -210,6 +229,7 @@ | ||
| // assume address space duplication is not a concern and adding entropy to | ||
| // every RAND_bytes call is not needed. | ||
| uint64_t bssl::CRYPTO_get_fork_generation() { return 0xc0ffee; } | ||
| +extern "C" void CRYPTO_fork_detect_reinit_for_startup_snapshot(void) {} | ||
|
|
||
| #else | ||
|
|
||
| @@ -218,5 +238,6 @@ | ||
| // space duplication could have occurred on any call entropy must be added to | ||
| // every RAND_bytes call. | ||
| uint64_t bssl::CRYPTO_get_fork_generation() { return 0; } | ||
| +extern "C" void CRYPTO_fork_detect_reinit_for_startup_snapshot(void) {} | ||
|
|
||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1432,7 +1432,7 @@ macro_rules! bss_singleton { | |
| fn slow() -> *mut $ty { | ||
| let p = $crate::bss_heap_init::<$ty>(<$ty>::init_at).as_ptr(); | ||
| // Race: two threads may both reach here. The mmap'd region is | ||
| // process-lifetime and never freed, so the loser is leaked | ||
| // process-lifetime and never freed, so the loser is leaked (its arena bytes too: first touch is single-threaded, so unlike the arena mapping this need not be claim-first) | ||
| // (≤ one per declare site, which in practice is single-threaded | ||
| // — `FileSystem::init` runs once on the main thread). The CAS | ||
| // is the publication barrier. | ||
|
|
@@ -1533,26 +1533,32 @@ fn bss_arena_bump(size: usize, align: usize) -> *mut u8 { | |
| static CURSOR: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| // Resolve the arena base. Fast path is one Acquire load; the cold path | ||
| // maps the 4 MiB region once and publishes via CAS. A losing racer's | ||
| // mapping is leaked (≤ one per process; `MAP_NORESERVE` so it costs no | ||
| // committed memory) — same race policy as `bss_singleton!`. | ||
| // maps the 4 MiB region exactly once: a racer claims the right to map before mapping, and the others wait for the | ||
| // result. (Map-then-race would let a loser consume a placement hint too, and the arena's address has to be the same in | ||
| // every process that may build or restore a snapshot.) | ||
|
Comment on lines
1535
to
+1538
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Extended reasoning...What the finding isThe 06:49 fix (responding to earlier comment #17) rewrote If two threads raced a singleton's first touch, both would call Step-by-step
Addressing the refutationOne reviewer's counter-argument is worth engaging directly:
Why nit, not normalThe accommodated race cannot fire in practice: line 1436-1437 says "in practice is single-threaded — Suggested fixEither of:
The first is proportionate to a race that can't fire; the second fully re-couples the two sites the deleted comment said were the same.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this one as it is: the singleton's own comment already records that first touch is single-threaded, and unlike the arena its address is not something a snapshot depends on directly (the arena's is), which is why only that site was made claim-first. |
||
| let mut base = BASE.load(Ordering::Acquire); | ||
| if base.is_null() { | ||
| static CLAIMED: core::sync::atomic::AtomicBool = core::sync::atomic::AtomicBool::new(false); | ||
| #[cold] | ||
| #[inline(never)] | ||
| fn map_arena() -> *mut u8 { | ||
| bss_mmap_noreserve(BSS_ARENA_SIZE) | ||
| fn map_arena_once() -> *mut u8 { | ||
| if CLAIMED | ||
| .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) | ||
| .is_ok() | ||
| { | ||
| let fresh = bss_mmap_noreserve(BSS_ARENA_SIZE); | ||
| BASE.store(fresh, Ordering::Release); | ||
| return fresh; | ||
| } | ||
| loop { | ||
| let b = BASE.load(Ordering::Acquire); | ||
| if !b.is_null() { | ||
| return b; | ||
| } | ||
| core::hint::spin_loop(); | ||
| } | ||
| } | ||
| let fresh = map_arena(); | ||
| base = match BASE.compare_exchange( | ||
| core::ptr::null_mut(), | ||
| fresh, | ||
| Ordering::AcqRel, | ||
| Ordering::Acquire, | ||
| ) { | ||
| Ok(_) => fresh, | ||
| Err(winner) => winner, // leak `fresh` (untouched MAP_NORESERVE) | ||
| }; | ||
| base = map_arena_once(); | ||
| } | ||
|
|
||
| // Bump the cursor: round up to `align`, reserve `size`. CAS loop because | ||
|
|
@@ -1579,6 +1585,37 @@ fn bss_arena_bump(size: usize, align: usize) -> *mut u8 { | |
| } | ||
| } | ||
|
|
||
| /// Where a snapshot may be built or mapped (the targets deps/mimalloc.ts builds the hint machinery for), this reservation | ||
| /// has to land at the same address in every process; the allocator decides that once and hands out the same kind of bump | ||
| /// hint it uses for its own reservations. Null = no preference. | ||
| #[cfg(any(target_os = "macos", target_os = "linux", target_os = "android"))] | ||
| fn snapshot_reserve_hint(len: usize) -> *mut libc::c_void { | ||
| // Bottom of the address window StartupSnapshot.cpp captures as ours (0x1f0'0000'0000..); mimalloc's own hinted arenas start above it. | ||
| const SNAPSHOT_RESERVE_BASE: usize = 0x1f0_0000_0000; | ||
| const SNAPSHOT_RESERVE_ALIGN: usize = 4 << 20; | ||
| static SNAPSHOT_HINT: core::sync::atomic::AtomicUsize = core::sync::atomic::AtomicUsize::new(0); | ||
| let mut hint: *mut libc::c_void = core::ptr::null_mut(); | ||
| if mimalloc::mi_startup_snapshot_hints_enabled() { | ||
| let _ = SNAPSHOT_HINT.compare_exchange( | ||
| 0, | ||
| SNAPSHOT_RESERVE_BASE, | ||
| core::sync::atomic::Ordering::AcqRel, | ||
| core::sync::atomic::Ordering::Acquire, | ||
| ); | ||
| let aligned = (len + SNAPSHOT_RESERVE_ALIGN - 1) & !(SNAPSHOT_RESERVE_ALIGN - 1); | ||
| hint = SNAPSHOT_HINT.fetch_add(aligned, core::sync::atomic::Ordering::AcqRel) | ||
| as *mut libc::c_void; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| } | ||
| hint | ||
| } | ||
| #[cfg(all( | ||
| unix, | ||
| not(any(target_os = "macos", target_os = "linux", target_os = "android")) | ||
| ))] | ||
| fn snapshot_reserve_hint(_len: usize) -> *mut libc::c_void { | ||
| core::ptr::null_mut() | ||
| } | ||
|
|
||
| /// One `mmap(MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE)` of `len` RW bytes. | ||
| /// Aborts on `MAP_FAILED`. Returned pointer is page-aligned and the region | ||
| /// reads as all-zeros until written. | ||
|
|
@@ -1595,11 +1632,12 @@ fn bss_mmap_noreserve(len: usize) -> *mut u8 { | |
| const MAP_FLAGS: libc::c_int = libc::MAP_PRIVATE | libc::MAP_ANONYMOUS | libc::MAP_NORESERVE; | ||
| #[cfg(not(any(target_os = "linux", target_os = "android")))] | ||
| const MAP_FLAGS: libc::c_int = libc::MAP_PRIVATE | libc::MAP_ANONYMOUS; | ||
| let hint = snapshot_reserve_hint(len); | ||
| // SAFETY: anonymous private mapping — fd/offset ignored, `len` is non-zero | ||
| // (callers pass `size_of` of a non-ZST); failure handled below. | ||
| // (callers pass `size_of` of a non-ZST); the hint is advisory; failure handled below. | ||
| let p = unsafe { | ||
| libc::mmap( | ||
| core::ptr::null_mut(), | ||
| hint, | ||
| len, | ||
| libc::PROT_READ | libc::PROT_WRITE, | ||
| MAP_FLAGS, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.