fix(tls): actor-side thread-local accessors are #[inline(never)] + fence — LLVM caches TLS addresses across context switches (finding 19)
Without LTO (any downstream `cargo build --release`), LLVM keeps `%fs:0` in a
callee-saved register across `switch_to_scheduler`; after the actor migrates,
`ACTOR_DONE`/`PREEMPTION_ENABLED`/`CURRENT_PID` etc. hit the OLD thread's TLS.
8 multi-thread tests aborted with "scheduler resumed a done actor" (error 5).
Thin LTO only worked because it picked the local-exec model.
- context.rs: module doc "Thread-locals and migration" (the rule), tls_fence()
- every TLS accessor reachable from an actor stack is out of line:
actor::{current_pid, publish_outcome (new)}, preempt::{maybe_preempt,
check_cancelled, current_slot_ptr, note_*, preemption_swap/enabled (new;
replaces direct PREEMPTION_ENABLED.with in NoPreempt/RawMutex/with_runtime/
trace/debug asserts)}, context::{get,set}_scheduler_sp, runtime::{sched_slot,
slot_push, set_yield_intent}, causal, trace
- raw_mutex order checks: out of line only under debug_assertions
- Cargo.toml: [profile.reltest] = release without LTO; 41 test bins link in
~1.5 s each instead of ~12 s (suite 11 min -> 1.5 min) AND it is the
regression oracle for this bug: `cargo test --profile reltest`
Both profiles: 41/41 + doctests green.
This commit is contained in:
@@ -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
|
||||
|
||||
+15
-2
@@ -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<Pid> {
|
||||
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.
|
||||
|
||||
+4
-1
@@ -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;
|
||||
|
||||
@@ -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<usize> = 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))
|
||||
}
|
||||
|
||||
|
||||
+34
-6
@@ -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<AtomicBool>`
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
+12
-4
@@ -75,8 +75,13 @@ thread_local! {
|
||||
static CHANNELS_HELD: std::cell::Cell<u32> = 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<T> RawMutex<T> {
|
||||
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<T> 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+1
-1
@@ -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"
|
||||
);
|
||||
|
||||
+17
-7
@@ -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));
|
||||
}
|
||||
|
||||
|
||||
+10
-7
@@ -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<R>(f: impl FnOnce(&Arc<RuntimeInner>) -> 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<R>(f: impl FnOnce(&Arc<RuntimeInner>) -> 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<R>(f: impl FnOnce(&Arc<RuntimeInner>) -> R) -> Option<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| 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+4
-6
@@ -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);
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user