diff --git a/Cargo.toml b/Cargo.toml index b80e1e0..2ba0dd8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,14 @@ panic = "unwind" lto = "thin" codegen-units = 1 +# `cargo test --profile reltest`: release codegen for the crate (same opt-level, +# same panic strategy) but no LTO at the final link. Thin LTO is what makes each +# of the ~40 test binaries cost ~12 s to link instead of ~2 s; the tests don't +# need cross-crate LTO, the benches do (they keep using `release`). +[profile.reltest] +inherits = "release" +lto = false + [[bench]] name = "primes" harness = false diff --git a/src/actor.rs b/src/actor.rs index f845a2c..f320a9a 100644 --- a/src/actor.rs +++ b/src/actor.rs @@ -72,7 +72,10 @@ pub fn clear_current_pid() { CURRENT_PID.with(|c| c.set(None)); } +/// Actor-side TLS accessor: `#[inline(never)]` + fence, see `context` docs. +#[inline(never)] pub fn current_pid() -> Option { + crate::context::tls_fence(); CURRENT_PID.with(|c| c.get()) } @@ -109,8 +112,7 @@ pub extern "C-unwind" fn trampoline() { } }; - LAST_OUTCOME.with(|r| *r.borrow_mut() = Some(outcome)); - ACTOR_DONE.with(|c| c.set(true)); + publish_outcome(outcome); // Hand control back. The scheduler will tear down our slot and never // resume us again. @@ -119,6 +121,17 @@ pub extern "C-unwind" fn trampoline() { unreachable!("scheduler resumed a done actor"); } +/// Record the outcome for the scheduler that is about to be switched to. Kept +/// out of line: the actor may have migrated threads while its closure ran, so +/// the TLS base `trampoline` computed at entry must not be reused here (see +/// `context` module docs on thread-locals and migration). +#[inline(never)] +fn publish_outcome(outcome: Outcome) { + crate::context::tls_fence(); + LAST_OUTCOME.with(|r| *r.borrow_mut() = Some(outcome)); + ACTOR_DONE.with(|c| c.set(true)); +} + /// One actor's worth of state. Owned by the scheduler's slot table. pub struct Actor { /// The PID this actor was assigned at spawn time. diff --git a/src/causal.rs b/src/causal.rs index 89f3155..a27331e 100644 --- a/src/causal.rs +++ b/src/causal.rs @@ -274,7 +274,9 @@ mod inner { /// The experiment-active path, kept out of the inlined fast path. #[cold] + #[inline(never)] fn cold_check(exp: u64) { + crate::context::tls_fence(); let slot = preempt::current_slot_ptr(); if slot.is_null() { return; @@ -367,8 +369,9 @@ mod inner { /// - Entering the target site: re-arm the sample clock, so time spent /// *before* the site can never be attributed to it by the first /// in-site check (the symmetric over-attribution). - #[inline] + #[inline(never)] fn site_transition(slot: *const crate::runtime::Slot, old: u32, new: u32) { + crate::context::tls_fence(); let exp = EXPERIMENT.load(Ordering::Relaxed); if exp == 0 || old == new { return; diff --git a/src/context.rs b/src/context.rs index f8a6d4d..de5e962 100644 --- a/src/context.rs +++ b/src/context.rs @@ -13,17 +13,55 @@ //! the initial stack so that the first `switch_to_actor` lands inside the //! entry function with `rsp % 16 == 8` (the x86-64 ABI requirement at //! function entry). +//! +//! # Thread-locals and migration (read before touching any `thread_local!`) +//! +//! An actor may park on scheduler thread A and be resumed on thread B. LLVM +//! treats the address of a thread-local as a loop-invariant, side-effect-free +//! value: it computes `%fs:0 + offset` once per function and happily keeps it +//! in a callee-saved register across calls — including across +//! `switch_to_scheduler`. Any function that touches a scheduler thread-local +//! both before and after a switch (or that gets *inlined* into one that does) +//! therefore reads and writes the *old thread's* TLS after migration. Nothing +//! at the switch can prevent this: it is not a memory clobber problem, the +//! address is not memory-derived in LLVM's model. Under thin LTO the code +//! happened to use the local-exec model (`%fs:imm` operands, nothing to +//! cache) so it worked by luck; a plain `cargo build --release` of a +//! downstream crate broke multi-thread runs (`ACTOR_DONE` written to the wrong +//! thread → "scheduler resumed a done actor"). +//! +//! Rule: every function that touches a thread-local and can execute on an +//! actor stack must be `#[inline(never)]` and call [`tls_fence`] first, so the +//! TLS base is recomputed inside a callee that cannot be inlined into a frame +//! spanning a switch, and so LLVM cannot infer the accessor is pure and merge +//! two calls to it. Scheduler-side code (`schedule_loop` and what it calls +//! before/after `switch_to_actor`) never migrates and is exempt. Do not return +//! `&Cell`/pointers into TLS from these accessors; return values. +//! `cargo test --profile reltest` (no LTO) is the regression oracle. use std::cell::Cell; +/// Compiler barrier for TLS accessors, see the module docs. Emits no code; a +/// side-effecting empty asm keeps LLVM from marking the enclosing +/// `#[inline(never)]` accessor `memory(none)` and merging calls to it. +#[inline(always)] +pub(crate) fn tls_fence() { + // SAFETY: empty asm, no operands, no stack, no flags. + unsafe { core::arch::asm!("", options(nostack, preserves_flags)) } +} + thread_local! { static SCHEDULER_SP: Cell = const { Cell::new(0) }; } +#[inline(never)] fn get_scheduler_sp() -> usize { + tls_fence(); SCHEDULER_SP.with(|c| c.get()) } +#[inline(never)] fn set_scheduler_sp(v: usize) { + tls_fence(); SCHEDULER_SP.with(|c| c.set(v)) } diff --git a/src/preempt.rs b/src/preempt.rs index d11d0db..193fc65 100644 --- a/src/preempt.rs +++ b/src/preempt.rs @@ -105,18 +105,36 @@ pub(crate) fn clear_current_slot() { /// (RFC 019 §7), which additionally relies on this being a plain load of a /// const-initialized TLS Cell (no lazy init, no allocation, no dtor): safe /// from a signal handler. -#[inline] +#[inline(never)] pub(crate) fn current_slot_ptr() -> *const crate::runtime::Slot { + crate::context::tls_fence(); CURRENT_SLOT.with(|c| c.get()) } +/// Swap the preemption gate, returning the previous value. The one accessor +/// for `PREEMPTION_ENABLED` from actor context (`NoPreempt`, `RawMutex`, +/// `with_runtime`, trace): `#[inline(never)]` + fence, see `context` docs. +#[inline(never)] +pub(crate) fn preemption_swap(enabled: bool) -> bool { + crate::context::tls_fence(); + PREEMPTION_ENABLED.with(|c| c.replace(enabled)) +} + +/// Read the preemption gate (debug assertions on the queue paths). +#[inline(never)] +pub(crate) fn preemption_enabled() -> bool { + crate::context::tls_fence(); + PREEMPTION_ENABLED.with(|c| c.get()) +} + /// RFC 007 (`smarm-causal`) — push the slice start forward by `cycles`, so /// virtually-injected delay spun inside `maybe_preempt` does not count against /// the actor's timeslice (the clock-correction half of the RFC: the runtime /// owns this clock, so it can subtract its own perturbation). #[cfg(feature = "smarm-causal")] -#[inline] +#[inline(never)] pub(crate) fn extend_timeslice(cycles: u64) { + crate::context::tls_fence(); TIMESLICE_START.with(|c| c.set(c.get().wrapping_add(cycles))); } @@ -124,8 +142,9 @@ pub(crate) fn extend_timeslice(cycles: u64) { /// no-op if no actor is bound (the scheduler's own stack). Reached only from /// the slice-expiry branch, which is already the yield path, so its cost is /// irrelevant. -#[inline] +#[inline(never)] fn note_overrun() { + crate::context::tls_fence(); let p = CURRENT_SLOT.with(|c| c.get()); // SAFETY: `p` is null (no actor on-CPU) or a pointer to the on-CPU actor's // slot in the fixed slab. The slot is not reclaimed while the actor runs @@ -141,8 +160,9 @@ fn note_overrun() { /// outside an actor (null slot). One TLS load + one Relaxed load/store on a /// cache line the receiving thread already owns — no atomic RMW, no lock. Same /// slot-lifetime safety argument as `note_overrun`. -#[inline] +#[inline(never)] pub(crate) fn note_message_received() { + crate::context::tls_fence(); let p = CURRENT_SLOT.with(|c| c.get()); if !p.is_null() { unsafe { (*p).record_message() }; @@ -158,8 +178,9 @@ pub(crate) fn note_message_received() { /// Called from `maybe_preempt` (amortised, the `check!()`/alloc path) and from /// the wakeup side of every blocking park (`park_current`/`yield_now`), which /// is past the prep-to-park window — so it can never lose a wakeup. -#[inline] +#[inline(never)] pub fn check_cancelled() { + crate::context::tls_fence(); let p = CURRENT_STOP.with(|c| c.get()); // SAFETY: `p` is either null (no actor on-CPU — the scheduler clears it on // every return) or a pointer into the on-CPU actor's `Arc` @@ -266,8 +287,13 @@ unsafe impl GlobalAlloc for PreemptingAllocator { /// the actor would then park, and the wakeup would be lost. Library /// code that touches the parking primitives must keep its prep-to-park /// regions allocation-free and check!()-free. -#[inline(always)] +/// +/// `#[inline(never)]`: this touches thread-locals and can switch threads in +/// the middle; inlined into a caller's loop the TLS base would be hoisted +/// across the switch (see `context` module docs). The call is the price. +#[inline(never)] pub fn maybe_preempt() { + crate::context::tls_fence(); ALLOC_COUNT.with(|c| { let n = c.get(); if n == 0 { @@ -316,7 +342,9 @@ pub fn maybe_preempt() { // --------------------------------------------------------------------------- /// Force-expire the timeslice so the next RDTSC check preempts. +#[inline(never)] pub fn expire_timeslice_for_test() { + crate::context::tls_fence(); TIMESLICE_START.with(|c| c.set(0)); ALLOC_COUNT.with(|c| c.set(0)); } diff --git a/src/raw_mutex.rs b/src/raw_mutex.rs index 3dafe9b..3c14eba 100644 --- a/src/raw_mutex.rs +++ b/src/raw_mutex.rs @@ -75,8 +75,13 @@ thread_local! { static CHANNELS_HELD: std::cell::Cell = const { std::cell::Cell::new(0) }; } -#[inline] +// Debug-only TLS bookkeeping: out of line only when it has a body (release +// builds must not pay a call for an empty function). +#[cfg_attr(debug_assertions, inline(never))] +#[cfg_attr(not(debug_assertions), inline(always))] fn order_check_acquire(class: LockClass) { + #[cfg(debug_assertions)] + crate::context::tls_fence(); #[cfg(debug_assertions)] match class { LockClass::Leaf => LEAVES_HELD.with(|l| { @@ -111,8 +116,11 @@ fn order_check_acquire(class: LockClass) { let _ = class; } -#[inline] +#[cfg_attr(debug_assertions, inline(never))] +#[cfg_attr(not(debug_assertions), inline(always))] fn order_check_release(class: LockClass) { + #[cfg(debug_assertions)] + crate::context::tls_fence(); #[cfg(debug_assertions)] match class { LockClass::Leaf => LEAVES_HELD.with(|c| c.set(c.get() - 1)), @@ -157,7 +165,7 @@ impl RawMutex { pub(crate) fn lock(&self) -> RawMutexGuard<'_, T> { // Enter NoPreempt *before* acquiring, so a preemption can't fire // between acquisition and guard construction. - let prev_preempt = crate::preempt::PREEMPTION_ENABLED.with(|c| c.replace(false)); + let prev_preempt = crate::preempt::preemption_swap(false); order_check_acquire(self.class); if self .state @@ -237,7 +245,7 @@ impl Drop for RawMutexGuard<'_, T> { self.m.unlock(); order_check_release(self.m.class); // Restore preemption only after the lock is released. - crate::preempt::PREEMPTION_ENABLED.with(|c| c.set(self.prev_preempt)); + crate::preempt::preemption_swap(self.prev_preempt); } } diff --git a/src/run_queue.rs b/src/run_queue.rs index 0ff932c..e2c652f 100644 --- a/src/run_queue.rs +++ b/src/run_queue.rs @@ -86,7 +86,7 @@ pub(crate) type RunQueue = StripedRing; #[inline] fn assert_no_preempt() { debug_assert!( - !crate::preempt::PREEMPTION_ENABLED.with(|c| c.get()), + !crate::preempt::preemption_enabled(), "run-queue op with preemption enabled — a switch mid-op stalls or \ corrupts the queue; route through with_runtime or scheduler context" ); diff --git a/src/runtime.rs b/src/runtime.rs index 0970a77..5200001 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -418,7 +418,7 @@ impl SchedulerStats { /// Bump a per-thread diagnostic counter on the calling scheduler thread. macro_rules! diag { ($inner:expr, $field:ident) => { - SCHED_SLOT.with(|s| $inner.stats[s.get()].$field.fetch_add(1, Ordering::Relaxed)) + $inner.stats[sched_slot()].$field.fetch_add(1, Ordering::Relaxed) }; } @@ -1239,9 +1239,11 @@ impl RuntimeInner { /// Displacement: the NEW wake takes the slot (newest is hottest; the old /// occupant was about to lose its locality window anyway) and the old /// occupant is pushed to the shared queue. + #[inline(never)] fn slot_push(&self, pid: Pid) { + crate::context::tls_fence(); debug_assert!( - !crate::preempt::PREEMPTION_ENABLED.with(|c| c.get()), + !crate::preempt::preemption_enabled(), "slot_push with preemption enabled — a switch mid-op could \ migrate the actor and split the slot access across threads" ); @@ -1255,11 +1257,9 @@ impl RuntimeInner { let displaced = WAKE_SLOT.with(|s| s.replace(Some(pid))); crate::te!(crate::trace::Event::SlotPush(pid)); if let Some(old) = displaced { - SCHED_SLOT.with(|s| { - self.stats[s.get()] - .slot_displacements - .fetch_add(1, Ordering::Relaxed) - }); + self.stats[sched_slot()] + .slot_displacements + .fetch_add(1, Ordering::Relaxed); self.enqueue(old); } } @@ -1640,7 +1640,17 @@ pub(crate) enum YieldIntent { Park, } +/// This scheduler thread's stats index. Read from actor context by every +/// `stat!()` on the enqueue/wake paths, hence out of line (`context` docs). +#[inline(never)] +fn sched_slot() -> usize { + crate::context::tls_fence(); + SCHED_SLOT.with(|s| s.get()) +} + +#[inline(never)] pub(crate) fn set_yield_intent(i: YieldIntent) { + crate::context::tls_fence(); YIELD_INTENT.with(|c| c.set(i)); } diff --git a/src/scheduler.rs b/src/scheduler.rs index 25f15de..e543f33 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -85,8 +85,10 @@ use std::sync::{Arc, Weak}; // released on the wrong thread's copy of the thread-local, corrupting its // borrow count. `f` is also always runtime bookkeeping that should run to // completion without the actor being suspended or unwound partway through. +#[inline(never)] pub(crate) fn with_runtime(f: impl FnOnce(&Arc) -> R) -> R { - let prev = crate::preempt::PREEMPTION_ENABLED.with(|c| c.replace(false)); + crate::context::tls_fence(); + let prev = crate::preempt::preemption_swap(false); let result = RUNTIME.with(|r| { let b = r.borrow(); let inner = match b.as_ref() { @@ -95,17 +97,19 @@ pub(crate) fn with_runtime(f: impl FnOnce(&Arc) -> R) -> R { }; f(inner) }); - crate::preempt::PREEMPTION_ENABLED.with(|c| c.set(prev)); + crate::preempt::preemption_swap(prev); result } // Borrow the runtime if present, otherwise `None`. Used on cleanup paths // (e.g. a channel's Drop impl during teardown) that may run after the // runtime has already gone away. Same preemption gate as `with_runtime`. +#[inline(never)] pub(crate) fn try_with_runtime(f: impl FnOnce(&Arc) -> R) -> Option { - let prev = crate::preempt::PREEMPTION_ENABLED.with(|c| c.replace(false)); + crate::context::tls_fence(); + let prev = crate::preempt::preemption_swap(false); let result = RUNTIME.with(|r| r.borrow().as_ref().map(f)); - crate::preempt::PREEMPTION_ENABLED.with(|c| c.set(prev)); + crate::preempt::preemption_swap(prev); result } @@ -758,14 +762,13 @@ pub struct NoPreempt(bool); impl NoPreempt { pub fn enter() -> Self { - let prev = crate::preempt::PREEMPTION_ENABLED.with(|c| c.replace(false)); - NoPreempt(prev) + NoPreempt(crate::preempt::preemption_swap(false)) } } impl Drop for NoPreempt { fn drop(&mut self) { - crate::preempt::PREEMPTION_ENABLED.with(|c| c.set(self.0)); + crate::preempt::preemption_swap(self.0); } } diff --git a/src/trace.rs b/src/trace.rs index fbe4b5c..6a48270 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -176,18 +176,16 @@ mod inner { // Hot path // ----------------------------------------------------------------------- + #[inline(never)] pub fn record(event: Event) { + crate::context::tls_fence(); // Disable preemption for the entire duration of record(). Any // allocation here (mutex internals, channel send, lazy init) would // trigger PreemptingAllocator -> maybe_preempt -> switch_to_scheduler, // which would try to re-acquire inner.shared (already held at many // te!() call sites) -> deadlock. Guard at the very top, before any // allocation-capable call. - let was_enabled = crate::preempt::PREEMPTION_ENABLED.with(|e| { - let v = e.get(); - e.set(false); - v - }); + let was_enabled = crate::preempt::preemption_swap(false); LOCAL_STATE.with(|cell| { let mut opt = cell.borrow_mut(); @@ -209,7 +207,7 @@ mod inner { } }); - crate::preempt::PREEMPTION_ENABLED.with(|e| e.set(was_enabled)); + crate::preempt::preemption_swap(was_enabled); } // -----------------------------------------------------------------------