diff --git a/issues/a-processs-memory-is-a-byte-total-that-reads-zero-under-contention.md b/issues/a-processs-memory-is-a-byte-total-that-reads-zero-under-contention.md index 2ca7892c44..56dbf68bf7 100644 --- a/issues/a-processs-memory-is-a-byte-total-that-reads-zero-under-contention.md +++ b/issues/a-processs-memory-is-a-byte-total-that-reads-zero-under-contention.md @@ -6,11 +6,11 @@ opened: 2026-09-01 # A process's memory is a byte total that reads zero under contention, so no plateau can be trusted -`issues/std-leaks-a-thread-stack-per-spawn.md` is the immediate consumer: -ToyOS std allocates a 2 MiB stack per spawn and `join` performs the join syscall -and nothing else. Deciding whether the mapping and its frames come back means +Deciding whether a process's mappings and their frames come back means reading a *plateau* — a sequence that climbs and returns — and that is what this -tree cannot do honestly today. +tree cannot do honestly today. std's thread stacks did without one: a stack +still allocated cannot share a base with the next, so `std_threads` reads +reuse instead. **Three things already exist, and this track is the difference between them and a plateau.** @@ -38,9 +38,9 @@ number which never returns. `peak_memory` is a high-water mark, which means a *fix* is invisible in it: a leak that stops leaking leaves the peak where it was. `tests/guest/rust-tests/src/bin/abuse_tls_alloc.rs` has the harness shape. It -runs raw `thread_spawn`/`thread_join` over an explicitly `mmap`ed stack, and its -comment at `:57-59` names this exact leak as the reason it avoids -`std::thread::spawn`. What it does not do is read a number. +runs raw `thread_spawn`/`thread_join` over one explicitly `mmap`ed stack, so +the address space holds still while it measures. What it does not do is read a +number. **So what is missing is narrow and worth building.** A test-only per-process *count* of live mappings and resident frames — counts, not bytes — read without @@ -52,7 +52,3 @@ not retain the objects it counts. Both are proved by comparing its total against a quiescent page-table and PMM walk at the start and at the end. Accounting is a pillar of `issues/toyos-explains-itself.md`. - -**Half the defect is not the joined half.** A `JoinHandle` dropped without `join` -detaches, and the record's exit condition currently closes only the joined path. -The plateau must cover both. diff --git a/issues/a-thread-std-detaches-holds-its-place-in-the-process-for-good.md b/issues/a-thread-std-detaches-holds-its-place-in-the-process-for-good.md deleted file mode 100644 index 14744b3939..0000000000 --- a/issues/a-thread-std-detaches-holds-its-place-in-the-process-for-good.md +++ /dev/null @@ -1,23 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-04 ---- - -# A thread std detaches holds its place in the process for good - -The kernel keeps an exited thread in its process's table until -`SYS_THREAD_JOIN` collects it, and counts it against -`toyos_abi::syscall::MAX_THREADS` until then -(`kernel::proclife::spawn::Admit::Full`). std's `Thread` -(`sdk/std/sys/thread.rs`) has no `Drop`, and a `JoinHandle` -dropped without `join` detaches, so nothing ever collects a thread std -detached: a program that detaches threads over its life has `thread::spawn` -refused once those that exited and those still running are -`MAX_THREADS - 1`, though not one of the exited ones runs. libc's -`pthread_detach` joins the thread itself once it has exited -(`sdk/libc/src/pthread.rs`, `reap`); std has no such path. - -Owner: orchestrator. Exit: a thread std detaches is collected once it exits, -and a test that spawns and drops more than `MAX_THREADS` handles, each thread -exiting, is refused none of them. diff --git a/issues/std-leaks-a-thread-stack-per-spawn.md b/issues/std-leaks-a-thread-stack-per-spawn.md deleted file mode 100644 index a44bd39ee4..0000000000 --- a/issues/std-leaks-a-thread-stack-per-spawn.md +++ /dev/null @@ -1,41 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-07-30 ---- - -# Std leaks a whole thread stack on every `thread::spawn` - -`sdk/std/sys/thread.rs` allocates the stack with -`alloc::alloc` (2 MiB minimum), hands its base to `SYS_THREAD_SPAWN`, and never -records the pointer. `Thread` holds only a tid and has no `Drop`, `join` does not -free it, and the trampoline cannot — it is standing on it. So every spawned -thread costs 2 MiB of heap for the life of the process, which dlmalloc serves -from a dedicated `mmap` above its 256 KiB threshold: one leaked 2 MiB kernel -region per spawn, walking the address space downwards. - -Found while testing thread-exit TLS release, where the drift swamped the signal -(the test now drives `SYS_THREAD_SPAWN` directly on a reused stack). It also -makes any per-process memory measurement across a thread-spawning workload wrong. - -**A fix owes both halves, and the obvious one closes only the first.** Putting a -base/layout pair on `Thread` and freeing it in `join` after the tid is reaped -covers the joined thread. It leaves the detached thread exactly as it is: -`Thread::join` takes `self`, `Thread` has no `Drop`, and a `JoinHandle` dropped -without joining detaches — so on that path nobody is left holding the base. - -**The kernel is already told the base and does not own it.** `stack_base` is the -fourth argument of `SYS_THREAD_SPAWN`; `sys_thread_spawn` refuses it above -`stack_ptr` and `spawn_thread` stores it as `user_stack_base`/`user_stack_size` -on the **`ThreadData`** (`kernel/src/process.rs:565`, `:569-570`) — per-thread, -not on the process. The only reader is `SYS_STACK_INFO`, which copies the pair -back out to userland. So the record dies with the thread and the memory does -not: nothing frees the stack, and nothing makes the mapping the thread's to -release. The detached half's answer is therefore a design question — the kernel -reclaiming a thread stack it already knows, or std refusing to detach a thread -whose stack it allocated — and not a missing line in `join`. - -**It cannot be closed without a plateau to measure**, which is -`issues/a-processs-memory-is-a-byte-total-that-reads-zero-under-contention.md`: -a single before/after pair cannot distinguish a leak from a deferred release, and -the plateau has to cover the detached path too. diff --git a/issues/std-says-this-machine-has-one-cpu.md b/issues/std-says-this-machine-has-one-cpu.md deleted file mode 100644 index e3535071b2..0000000000 --- a/issues/std-says-this-machine-has-one-cpu.md +++ /dev/null @@ -1,28 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-23 ---- - -# `std::thread::available_parallelism` answers 1 on every ToyOS machine - -`sdk/std/sys/thread.rs:66` returns a hardcoded -`NonZero::new_unchecked(1)`, and the comment above it says why: "ToyOS runs on -QEMU with a known number of CPUs, but we don't expose a syscall for this yet." -That is no longer true. `SYS_CPU_COUNT` exists, `toyos_abi::syscall::cpu_count` -wraps it, and `sdk/libc/src/misc.rs:222` already answers -`_SC_NPROCESSORS_ONLN` from it — so a C program on this system learns the CPU -count and a Rust program is told there is one CPU. - -The same file's `yield_now` is a `spin_loop` hint for the same stale reason; -there is no yield syscall, which is a separate decision and not this one. - -What it costs: any Rust program that sizes a thread pool, a work-stealing queue -or a shard count from `available_parallelism` runs single-threaded on an eight- -core machine and nothing says so. It bit a test that needed one thread per CPU -(`tests/guest/rust-tests/src/bin/demand_window_race.rs`, which calls -`syscall::cpu_count` and says why); a test that had *not* noticed would have -gone quietly single-threaded and passed while staging nothing. - -`std_threading` asserts only `available_parallelism() > 0`, which a constant 1 -satisfies, so nothing in the estate would catch this changing back either. diff --git a/sdk/std/sys/thread.rs b/sdk/std/sys/thread.rs index b19a3f53cf..7e810716e1 100644 --- a/sdk/std/sys/thread.rs +++ b/sdk/std/sys/thread.rs @@ -1,25 +1,75 @@ use toyos_abi::syscall; +use crate::alloc::Layout; +use crate::cell::UnsafeCell; use crate::ffi::CStr; use crate::io; +use crate::mem::ManuallyDrop; use crate::num::NonZero; +use crate::ptr; +use crate::sync::atomic::{AtomicPtr, AtomicU32, Ordering}; use crate::thread::ThreadInit; use crate::time::Duration; +/// A thread's handle. Joined, it frees the thread's stack; dropped, it +/// detaches the thread, whose stack the next spawn frees once it has ended. pub struct Thread { - tid: u32, + block: *mut Block, } unsafe impl Send for Thread {} unsafe impl Sync for Thread {} +/// What a thread and its handle share. Freed by whoever reaps the thread: +/// its joiner, or for a detached thread the first spawn after it ended. +struct Block { + tid: AtomicU32, + state: AtomicU32, + stack: *mut u8, + layout: Layout, + /// Taken by the thread as it starts. + init: UnsafeCell>>, + /// The next block on [`REAPED`]. + next: *mut Block, +} + +const JOINABLE: u32 = 0; +const DETACHED: u32 = 1; +const EXITED: u32 = 2; + +/// Detached threads that have ended, linked through [`Block::next`]: a thread +/// cannot free the stack it stands on, and pushing allocates nothing. +static REAPED: AtomicPtr = AtomicPtr::new(ptr::null_mut()); + pub const DEFAULT_MIN_STACK_SIZE: usize = 2 * 1024 * 1024; +/// Wait for `block`'s thread to be gone, then free its stack and `block`. A +/// join the kernel refuses frees nothing, since the stack may still be in use. +/// +/// # Safety +/// `block` is a live block whose thread no one else reaps. +unsafe fn reap(block: *mut Block) { + let b = unsafe { &*block }; + if syscall::thread_join(b.tid.load(Ordering::Relaxed) as u64) == 0 { + unsafe { + crate::alloc::dealloc(b.stack, b.layout); + drop(Box::from_raw(block)); + } + } +} + impl Thread { pub unsafe fn new(stack: usize, init: Box) -> io::Result { + let mut ended = REAPED.swap(ptr::null_mut(), Ordering::Acquire); + while !ended.is_null() { + // SAFETY: a block on the list was handed over by its ended thread. + let next = unsafe { (*ended).next }; + unsafe { reap(ended) }; + ended = next; + } + let stack_size = stack.max(DEFAULT_MIN_STACK_SIZE); - // Allocate user stack (page-aligned) - let layout = crate::alloc::Layout::from_size_align(stack_size, 4096).unwrap(); + let layout = Layout::from_size_align(stack_size, 4096).unwrap(); let stack_base = unsafe { crate::alloc::alloc(layout) }; if stack_base.is_null() { return Err(io::const_error!( @@ -28,40 +78,61 @@ impl Thread { )); } let stack_top = stack_base.expose_provenance() as u64 + stack_size as u64; + let block = Box::into_raw(Box::new(Block { + tid: AtomicU32::new(0), + state: AtomicU32::new(JOINABLE), + stack: stack_base, + layout, + init: UnsafeCell::new(Some(init)), + next: ptr::null_mut(), + })); - let data = Box::into_raw(init); - // SAFETY: entry point and stack are valid; data is a valid pointer to the thread init. + // SAFETY: entry point and stack are valid; the block outlives the thread. let tid = unsafe { syscall::thread_spawn( (thread_trampoline as *const ()).expose_provenance() as u64, stack_top, - data.expose_provenance() as u64, + block.expose_provenance() as u64, stack_base.expose_provenance() as u64, ) }; if syscall::SyscallError::from_u64(tid).is_some() { unsafe { - drop(Box::from_raw(data)); - } - unsafe { + drop(Box::from_raw(block)); crate::alloc::dealloc(stack_base, layout); } return Err(io::const_error!(io::ErrorKind::Uncategorized, "thread spawn failed")); } - - Ok(Thread { tid: tid as u32 }) + // Before the handle exists: only a dropped handle detaches the + // thread, so every reaper sees this through the handle or the list. + unsafe { (*block).tid.store(tid as u32, Ordering::Relaxed) }; + Ok(Thread { block }) } pub fn join(self) { - syscall::thread_join(self.tid as u64); + let this = ManuallyDrop::new(self); + // SAFETY: a handle that joins is the only reaper of its thread. + unsafe { reap(this.block) }; + } +} + +impl Drop for Thread { + fn drop(&mut self) { + // SAFETY: the block lives until the thread is reaped, which only this + // handle or, once it is detached, the thread's own exit arranges. + let b = unsafe { &*self.block }; + if b.state.compare_exchange(JOINABLE, DETACHED, Ordering::AcqRel, Ordering::Acquire).is_err() { + // It has ended already, so nothing will hand it to `REAPED`. + unsafe { reap(self.block) }; + } } } extern "C" fn thread_trampoline(data: u64) { - let init = unsafe { - Box::from_raw(crate::ptr::with_exposed_provenance_mut::(data as usize)) - }; + let block = crate::ptr::with_exposed_provenance_mut::(data as usize); + // SAFETY: the spawner put the init here and only this thread takes it. + let init = unsafe { (*(*block).init.get()).take() }.expect("a thread's init is taken once"); let main = init.init(); main(); // Run TLS destructors then clean up the current thread handle. @@ -70,13 +141,23 @@ extern "C" fn thread_trampoline(data: u64) { crate::sys::thread_local::destructors::run(); } crate::rt::thread_cleanup(); + // SAFETY: the block is this thread's until it is handed over below. + if unsafe { &*block }.state.swap(EXITED, Ordering::AcqRel) == DETACHED { + let mut head = REAPED.load(Ordering::Relaxed); + loop { + unsafe { (*block).next = head }; + match REAPED.compare_exchange_weak(head, block, Ordering::Release, Ordering::Relaxed) { + Ok(_) => break, + Err(now) => head = now, + } + } + } syscall::thread_exit(0); } pub fn available_parallelism() -> io::Result> { - // ToyOS runs on QEMU with a known number of CPUs, but we don't expose - // a syscall for this yet. Return 1 for now. - Ok(unsafe { NonZero::new_unchecked(1) }) + NonZero::new(syscall::cpu_count() as usize) + .ok_or(io::const_error!(io::ErrorKind::NotFound, "the kernel counts no CPU")) } pub fn current_os_id() -> Option { diff --git a/tests/guest/rust-tests/src/bin/abuse_tls_alloc.rs b/tests/guest/rust-tests/src/bin/abuse_tls_alloc.rs index d40742c93b..c4e105ca10 100644 --- a/tests/guest/rust-tests/src/bin/abuse_tls_alloc.rs +++ b/tests/guest/rust-tests/src/bin/abuse_tls_alloc.rs @@ -54,9 +54,9 @@ fn map_2m() -> *mut u8 { static THREAD_BLOCK: AtomicU64 = AtomicU64::new(0); -/// Raw thread body. std's `thread::spawn` leaks its 2 MiB stack on every -/// spawn (`sys::thread::toyos` allocates it and `join` never frees it), which -/// walks the address space downwards and would swamp what part 3 measures. +/// Raw thread body, run on one stack this test maps and reuses, so the +/// address space holds still under what part 3 measures; std's +/// `thread::spawn` allocates each thread a stack of its own. extern "C" fn thread_body(_arg: u64) { let r = alloc_block_preserving_dtv(MODULE_EXE); THREAD_BLOCK.store(r.unwrap_or(0), Ordering::Release); diff --git a/tests/guest/rust-tests/src/bin/demand_window_race.rs b/tests/guest/rust-tests/src/bin/demand_window_race.rs index 0049526ff4..5edd5a2068 100644 --- a/tests/guest/rust-tests/src/bin/demand_window_race.rs +++ b/tests/guest/rust-tests/src/bin/demand_window_race.rs @@ -219,10 +219,6 @@ fn worker(mode: Mode, base: usize, t: usize, threads: usize, barrier: &SpinBarri } fn child(mode: Mode) { - // `syscall::cpu_count` and not `std::thread::available_parallelism`, which - // answers 1 on a two-CPU guest — a std gap this file is not the place to - // fix, and a wrong answer here would silently unstage the race. - // // One thread per CPU exactly. Fewer and the window is not contended; more // and a thread spinning at the barrier is holding the CPU another // participant needs, so every round costs a timeslice instead of a fault. diff --git a/tests/guest/rust-tests/src/bin/std_threading.rs b/tests/guest/rust-tests/src/bin/std_threading.rs index 05c7351316..8b264fb1a2 100644 --- a/tests/guest/rust-tests/src/bin/std_threading.rs +++ b/tests/guest/rust-tests/src/bin/std_threading.rs @@ -14,11 +14,6 @@ fn main() { return; } - // Test available_parallelism returns > 0 - let n = thread::available_parallelism().expect("available_parallelism failed"); - assert!(n.get() > 0, "expected parallelism > 0, got {}", n.get()); - println!("available_parallelism = {}", n.get()); - // Test spawning threads that compute partial sums let handles: Vec<_> = (0..4) .map(|i| { diff --git a/tests/guest/rust-tests/src/bin/std_threads.rs b/tests/guest/rust-tests/src/bin/std_threads.rs new file mode 100644 index 0000000000..7238e11633 --- /dev/null +++ b/tests/guest/rust-tests/src/bin/std_threads.rs @@ -0,0 +1,67 @@ +//! What std's threads take from the kernel and give back: the CPU count, and +//! the stack and table place of a thread once it has ended, joined or not. + +use std::collections::BTreeSet; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::thread; + +use toyos_abi::syscall::MAX_THREADS; + +/// Threads spawned and joined one after another. +const JOINS: usize = 64; + +/// The base of the stack the calling thread runs on. +fn stack_base() -> u64 { + toyos_abi::syscall::stack_info().expect("a spawned thread has a stack").0 +} + +fn main() { + // The harness holds the printed count to the CPUs it booted; on a bench, + // the kernel's own count is the only one at hand. + let cpus = thread::available_parallelism().expect("available_parallelism").get(); + let counted = toyos_abi::syscall::cpu_count() as usize; + assert_eq!(cpus, counted, "available_parallelism answers {cpus}, and the kernel counts {counted} CPUs"); + println!("std_threads: {cpus} CPUs"); + + // Stacks still allocated cannot share a base. A freed one can be the next + // spawn's, when the allocator gives the mapping back and the kernel places + // the next at the same address. + let joined: BTreeSet = + (0..JOINS).map(|_| thread::spawn(stack_base).join().expect("an empty thread")).collect(); + assert!( + joined.len() < JOINS, + "{JOINS} threads spawned and joined one at a time had {JOINS} distinct stacks: no joined thread's \ + stack came back" + ); + + // More detached threads than a process may hold, each ended before the + // next spawn: every one is refused nothing only if the spawns collect the + // ones before them, and their stacks share bases only if they are freed. + let detached = MAX_THREADS + 8; + static ENDED: AtomicUsize = AtomicUsize::new(0); + static BASES: std::sync::Mutex> = std::sync::Mutex::new(Vec::new()); + for spawned in 0..detached { + let handle = thread::Builder::new() + .spawn(|| { + BASES.lock().unwrap().push(stack_base()); + ENDED.fetch_add(1, Ordering::Release); + }) + .unwrap_or_else(|e| panic!("detached thread {spawned} of {detached} was refused: {e}")); + drop(handle); + while ENDED.load(Ordering::Acquire) <= spawned { + std::hint::spin_loop(); + } + } + let bases: BTreeSet = BASES.lock().unwrap().iter().copied().collect(); + assert!( + bases.len() < detached, + "{detached} detached threads, each ended before the next spawn, had {detached} distinct stacks: no \ + detached thread's stack came back" + ); + println!( + "std_threads: {JOINS} joined threads ran on {} stacks; {detached} detached threads were all \ + spawned and ran on {} stacks", + joined.len(), + bases.len() + ); +} diff --git a/tests/toyos.rs b/tests/toyos.rs index 3e3ec8bcea..d55c2a3b04 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -272,6 +272,9 @@ const DRIVEN_AND_SHARED: &[&str] = &[ // reads what the kernel said of two of them. "fault_gates", "sched_stress", + // Its shared run is each bench's verdict; + // `std_threads_count_the_cpus_and_give_back_ended_threads` runs it under QEMU. + "std_threads", // Its shared run is each bench's verdict, on the T14's AVX-512 and the AMD // bench's AVX; `vector_state_survives_a_switch` runs it under QEMU. "vector_state", @@ -538,6 +541,10 @@ const MACHINE_TESTS: &[&str] = &[ // run is the benches'; this is TCG's, whose XSAVE is QEMU's own reading of // the SDM, and the KVM runner's CPU. "vector_state_survives_a_switch", + // std's CPU count against the CPUs the guest booted with, and an ended + // thread's stack and table place given back, joined or detached, on the + // two-CPU q35 guest: std's ToyOS layer, which no host builds. + "std_threads_count_the_cpus_and_give_back_ended_threads", // The parity instrument's two builds in one boot, behind ring's known // answers: that every row runs and both builds print the same whole table. // Its timings decide nothing here; the `crypto_instrument` metal row takes @@ -4042,6 +4049,7 @@ fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> { hda_plays_to_the_controller_that_reaches_an_output(test_config) } "vector_state_survives_a_switch" => vector_state_survives_a_switch(test_config), + "std_threads_count_the_cpus_and_give_back_ended_threads" => std_threads_count_the_cpus_and_give_back_ended_threads(test_config), "the_opening_exchange_with_qemus_agent_completes" => the_opening_exchange_with_qemus_agent_completes(), "crypto_instrument_prints_its_table" => crypto_instrument_prints_its_table(test_config), "the_compositor_drives_the_virtio_gpu" => the_compositor_drives_the_virtio_gpu(), @@ -4712,6 +4720,33 @@ fn vector_state_survives_a_switch(test_config: &Path) -> Result<(), String> { Ok(()) } +/// `std_threads` on a guest of [`BootOptions`]' CPUs: it asserts what it +/// frees itself, and the CPU count it prints is held here to the CPUs the +/// guest was booted with. +fn std_threads_count_the_cpus_and_give_back_ended_threads(test_config: &Path) -> Result<(), String> { + const JOB: &str = "std_threads"; + let bin = qemu::build_toyos_bin(qemu::SUITE_ARCH, &compile::repo_root().join("tests/guest/rust-tests"), JOB); + let options = BootOptions::default(); + let booted = options.smp; + let mut qemu = QemuInstance::boot_with_options(test_config, &[], &[(JOB.to_string(), bin)], options); + serial::Serial::boot(&qemu).must_be_clean()?; + let result = qemu.run_test("test_rs_std_threads", Duration::from_secs(300)); + if let Some(why) = &result.error { + return Err(format!("{why}\nthe job said:\n{}", result.stdout)); + } + if result.exit_code != Some(0) { + return Err(format!("the job ended {:?}:\n{}", result.exit_code, result.stdout)); + } + let counted = format!("std_threads: {booted} CPUs"); + if !result.stdout.lines().any(|l| l.trim_end().ends_with(&counted)) { + return Err(format!("the guest was booted with {booted} CPUs and std counted otherwise:\n{}", result.stdout)); + } + for line in result.stdout.lines().filter(|l| l.contains("std_threads: ")) { + eprintln!(" [std] {}", line.trim()); + } + Ok(()) +} + /// Ring's known answers, then both builds of the parity instrument, in one /// boot: each ends cleanly and their lines read back as one whole table. fn crypto_instrument_prints_its_table(test_config: &Path) -> Result<(), String> {