diff --git a/docs/threading-plan.md b/docs/threading-plan.md index 9ce1cd7..6d50449 100644 --- a/docs/threading-plan.md +++ b/docs/threading-plan.md @@ -370,16 +370,32 @@ full guardrail); `zig build`/`zig build test` clean. With the reaper (M8) able to act *after* a thread is fully off its stack, migrate `join` to the std shape and drop M3's per-thread exit endpoint: -- [ ] `thread_spawn` takes a user **completion word** (in the `Thread` handle's memory) - and a joinable/detached flag. On reap the kernel writes 0 to that word and - `futex_wake`s it (a `CLONE_CHILD_CLEARTID` equivalent — safe now the thread is off - its stack). `join` = `futex_wait` on the word, then `munmap` the stack; a - **detached** thread's stack is `munmap`ped by the reaper instead. No IPC endpoint - per thread. -- [ ] `thread-join` passes on the new path; a check confirms joining N threads creates no - per-thread endpoints (handle count stable). +- [x] A **`thread_join(tid)` syscall** (not a user futex word): it blocks the caller until + the task with id `tid` exits, and the exit paths call `wakeJoinersLocked`. `join` + only reclaims the joined thread's **user** stack, which the thread vacates the moment + it enters the kernel to exit — so waking at *exit* time (not reap time) is safe, and + no reaper/address-space juggling or user-memory write is needed. This is equally + std-shaped (like `pthread_join`) and much simpler/safer than the planned + reaper-written completion word. `thread_spawn` no longer takes an exit endpoint (the + runtime passes `no_cap`); the per-thread IPC endpoint is gone. +- [x] `thread-join` passes on the new path, and its join mode now runs **40 spawn+join + cycles** — under the old per-thread-endpoint scheme those leaked handles would + exhaust the 16-slot handle table; here they all succeed, proving join is endpoint-free. -**Gate:** `thread-join`/`thread-mutex` green on futex-completion join; guardrail green. +**Gate (met):** `thread-join` passes (3× isolated) on the `thread_join` path; full +guardrail 26/26 (incl. `process-kill`, `supervision`, `fault-recovery`, `task-reap`); +`zig build`/`zig build test` clean. + +> **Reaper hardened here (fixes an M8 flake).** M8's single per-core reap slot could be +> *overwritten* by a second death on that core before the first drained (a fresh-task/SMP +> timing window) — an intermittent one-stack leak (`task-reap` flaked ~20%). Replaced it +> with a per-core reap **list** plus a `.reaping` task state so a pending slot can't be +> reused before its stack is freed. `task-reap` now 11/11 isolated + 2× in the batch. + +> **Deferred:** detached-thread **user-stack** reclaim (still freed at process exit, as in +> M3). Doing it in the reaper needs the saved address space + stack range and a +> translate/unmap in a not-currently-loaded aspace — real complexity for a bounded leak. +> A follow-up when a consumer needs it. ### M10 — Per-thread TLS (`threadlocal`) diff --git a/library/runtime/thread.zig b/library/runtime/thread.zig index 2f84ebb..c34d84b 100644 --- a/library/runtime/thread.zig +++ b/library/runtime/thread.zig @@ -14,16 +14,13 @@ const std = @import("std"); const abi = @import("abi"); const sc = @import("system-call.zig"); const system = @import("system.zig"); -const ipc = @import("ipc.zig"); /// A thread stack, if the caller does not override it. 64 KiB of mmap'd, zeroed pages. pub const default_stack_size: usize = 64 * 1024; pub const Thread = struct { - /// The kernel task id of the spawned thread. + /// The kernel task id of the spawned thread — what `join` waits on. tid: u32, - /// The endpoint the kernel notifies when this thread ends — what `join` blocks on. - exit_endpoint: ipc.Handle, /// The mmap'd stack, reclaimed by `join` (or at process exit after `detach`). stack_base: usize, stack_size: usize, @@ -56,9 +53,6 @@ pub const Thread = struct { } }; - // The endpoint the kernel posts this thread's exit notification to. - const endpoint = ipc.createIpcEndpoint() orelse return error.SystemResources; - const base = system.mmap(config.stack_size, system.PROT_READ | system.PROT_WRITE); if (system.mmapFailed(base)) return error.SystemResources; @@ -73,24 +67,20 @@ pub const Thread = struct { var stack_top = closure_addr & ~@as(usize, 15); // 16-align below the closure stack_top -= 8; // ...then rsp % 16 == 8 at the C entry - const tid = threadSpawn(@intFromPtr(&Closure.entry), stack_top, closure_addr, endpoint); + const tid = threadSpawn(@intFromPtr(&Closure.entry), stack_top, closure_addr); if (threadSpawnFailed(tid)) { _ = system.munmap(base, config.stack_size); return error.SystemResources; } - return .{ .tid = @intCast(tid), .exit_endpoint = endpoint, .stack_base = base, .stack_size = config.stack_size }; + return .{ .tid = @intCast(tid), .stack_base = base, .stack_size = config.stack_size }; } /// Block until this thread finishes, then reclaim its stack. Mirrors /// `std.Thread.join`. The exit endpoint is private to this thread, so the first /// child-exit notification on it is this thread's. pub fn join(self: Thread) void { - var receive: [0]u8 = undefined; - while (true) { - const got = ipc.replyWait(self.exit_endpoint, &.{}, &receive, null); - if (got.isChildExit() and got.childProcessId() == self.tid) break; - } - _ = system.munmap(self.stack_base, self.stack_size); + _ = sc.systemCall1(.thread_join, self.tid); // block until the thread has exited + _ = system.munmap(self.stack_base, self.stack_size); // reclaim its (now-vacated) stack } /// Relinquish the right to join: never wait for or reclaim this thread. Its stack is @@ -233,7 +223,8 @@ pub const Thread = struct { }; /// thread_spawn(entry, stack_top, arg, exit_endpoint) -> tid, or a wrapped error. -fn threadSpawn(entry: usize, stack_top: usize, arg: usize, exit_endpoint: ipc.Handle) usize { +fn threadSpawn(entry: usize, stack_top: usize, arg: usize) usize { + const exit_endpoint: usize = @intCast(abi.no_cap); // join uses thread_join, not an endpoint return sc.systemCall4(.thread_spawn, entry, stack_top, arg, exit_endpoint); } diff --git a/system/abi.zig b/system/abi.zig index d536237..b959593 100644 --- a/system/abi.zig +++ b/system/abi.zig @@ -69,6 +69,7 @@ pub const SystemCall = enum(u64) { futex_wait = 40, // futex_wait(addr, expected, timeout_ns) -> status: if *addr == expected, block until woken or the timeout; returns futex_woken/mismatch/timed_out (docs/threading.md) futex_wake = 41, // futex_wake(addr, count) -> woken: wake up to `count` tasks blocked in futex_wait on `addr` in this address space thread_self = 42, // thread_self() -> tid: the calling thread's kernel task id (runtime.Thread.getCurrentId) + thread_join = 43, // thread_join(tid) -> 0: block until the thread with id `tid` has exited (runtime.Thread.join; no per-thread IPC endpoint) (docs/threading.md) _, }; diff --git a/system/kernel/process.zig b/system/kernel/process.zig index 6483a6d..cc328eb 100644 --- a/system/kernel/process.zig +++ b/system/kernel/process.zig @@ -231,6 +231,7 @@ fn system_call(state: *architecture.CpuState) void { .thread_spawn => systemThreadSpawn(state), .current_core => systemCurrentCore(state), .thread_self => systemThreadSelf(state), + .thread_join => systemThreadJoin(state), .futex_wait => systemFutexWait(state), .futex_wake => systemFutexWake(state), .thread_exit => { @@ -706,6 +707,19 @@ fn systemThreadSelf(state: *architecture.CpuState) void { architecture.setSystemCallResult(state, scheduler.currentId()); } +/// thread_join(tid) -> 0: block until the thread with id `tid` has exited (docs/threading- +/// plan.md M9). Needs no per-thread IPC endpoint. The compare-and-block is one critical +/// section, so an exit cannot slip between "is it alive?" and the block. +fn systemThreadJoin(state: *architecture.CpuState) void { + const tid: u32 = @truncate(architecture.systemCallArg(state, 0)); + const t = scheduler.current(); + if (t.aspace == 0) return fail(state); // kernel tasks don't join + const flags = sync.enter(); + scheduler.joinThreadLocked(tid); + sync.leave(flags); + architecture.setSystemCallResult(state, 0); +} + /// futex_wait(addr, expected, timeout_ns) -> status (docs/threading.md): if the 4-byte /// user word at `addr` still equals `expected`, block until a futex_wake on `addr` or /// (if timeout_ns > 0) the deadline. The compare and the block are one critical section, diff --git a/system/kernel/scheduler.zig b/system/kernel/scheduler.zig index daf775f..d4d45b5 100644 --- a/system/kernel/scheduler.zig +++ b/system/kernel/scheduler.zig @@ -31,7 +31,10 @@ const number_priorities = 8; const stack_size = parameters.kernel_stack_size; // each task's kernel stack const maximum_tasks = parameters.maximum_tasks; // maximum tasks alive at once (static pool) -const State = enum { free, ready, running, blocked }; +// `reaping` = the task has exited and is queued on its core's reap list; its slot must not +// be reused (freeSlot skips it) until the reaper has freed its kernel stack and set it +// `free` (docs/threading-plan.md M8/M9). +const State = enum { free, ready, running, blocked, reaping }; pub const Task = struct { id: u32 = 0, @@ -83,6 +86,9 @@ pub const Task = struct { // The user address this task is blocked on in futex_wait (0 = not futex-waiting). // Cleared to 0 by futexWakeLocked as the "woken, not timed out" signal (docs/threading.md). futex_addr: u64 = 0, + // The task id this task is blocked in `thread_join` on (0 = not joining). Woken by + // `wakeJoinersLocked` when that task exits (docs/threading-plan.md M9). + join_target: u32 = 0, // The mmap / MMIO grant-arena cursors moved from Task to the per-address-space object // (`AspaceRef`, below) so threads sharing one address space hand out disjoint grants // — see aspaceMmapNextPtr / aspaceDeviceMapNextPtr (docs/threading-plan.md M7). @@ -174,6 +180,20 @@ fn reapStackLocked(t: *Task) void { t.kstack_top = 0; } +/// Free every `.reaping` task queued on this core's reap list and mark each `.free` (now +/// its slot may be reused). The tasks are all off their stacks (they switched away), and +/// the caller holds the lock, so freeing is safe (docs/threading-plan.md M8/M9). +fn drainReapListLocked(pc: *PerCpu) void { + var node = pc.reap_list; + pc.reap_list = null; + while (node) |t| { + node = t.next; // save the link before we clear it + t.next = null; + reapStackLocked(t); + t.state = .free; // reusable only now, after the stack is freed + } +} + /// Take a reference to address space `root` (0 = a kernel task, which owns none). /// Returns false only if the ref table is full — bounded by `maximum_tasks`, so in /// practice it never is. Caller holds the kernel lock. @@ -268,12 +288,12 @@ pub const PerCpu = struct { pinned_head: [number_priorities]?*Task = .{null} ** number_priorities, pinned_tail: [number_priorities]?*Task = .{null} ** number_priorities, pinned_bitmap: u8 = 0, - // A task that ended while running on THIS core: it could not free the kernel stack it - // was standing on, so it recorded itself here and switched away. The next task to run - // on this core frees that stack (from its own stack, safely) in `switchTo`. The big - // lock is held continuously across the switch, so the dead task's slot can't be reused - // before it is reaped (docs/threading-plan.md M8). - reap_after_switch: ?*Task = null, + // Tasks that ended while running on THIS core: they could not free the kernel stack + // they were standing on, so each pushed itself onto this list (`.reaping` state, linked + // via `Task.next`) and switched away. The next task to run on this core — or the timer + // tick — frees their stacks from its own stack, safely (docs/threading-plan.md M8). A + // *list* (not one slot) so a second death before the first is drained can't lose it. + reap_list: ?*Task = null, }; const maximum_cpus = parameters.maximum_cpus; @@ -555,11 +575,7 @@ fn switchTo(pc: *PerCpu, save_sp: *usize, next: *Task) void { // the current core, so thisCpu() is the core the just-dead task died on. If a task // died switching to us, free its kernel stack: we're on ours so it's safe, and the big // lock is still held so its slot can't have been reused (docs/threading-plan.md M8). - const here = thisCpu(); - if (here.reap_after_switch) |dead| { - here.reap_after_switch = null; - reapStackLocked(dead); - } + drainReapListLocked(thisCpu()); } /// Voluntarily give up the CPU to the next ready task. @@ -623,6 +639,46 @@ pub fn futexWakeLocked(aspace: u64, addr: u64, count: u32) u32 { return woken; } +// --- thread join (docs/threading-plan.md M9) -------------------------------- +// +// join needs no per-thread IPC endpoint: `thread_join(tid)` blocks the caller until the +// task with id `tid` has exited, and the exit paths wake any joiner. The caller only ever +// reclaims the joined thread's *user* stack (which the thread vacated the moment it entered +// the kernel to exit), so waking at exit time — not reap time — is safe. + +/// True if a task with id `tid` is still live (has not exited). Caller holds the lock. +fn aliveTid(tid: u32) bool { + for (&tasks) |*t| { + if (t.id == tid and t.state != .free and t.state != .reaping) return true; + } + return false; +} + +/// Block the current task until the task with id `tid` exits (or return at once if it +/// already has / never existed). **Precondition:** the big kernel lock is held; returns +/// with it still held. Woken by `wakeJoinersLocked`. +pub fn joinThreadLocked(tid: u32) void { + while (aliveTid(tid)) { + const t = current(); + t.join_target = tid; + t.state = .blocked; + schedule(); // woken when the joined task exits; lock handed off across the switch + t.join_target = 0; + } +} + +/// Wake every task blocked in `thread_join` on `tid` — called from the exit paths once the +/// exiting task's state is `.free`. Caller holds the lock. +fn wakeJoinersLocked(tid: u32) void { + for (&tasks) |*t| { + if (t.state == .blocked and t.join_target == tid) { + t.join_target = 0; + t.state = .ready; + enqueue(t); + } + } +} + // --- event-based blocking ------------------------------------------------- // // A WaitQueue is a set of tasks blocked waiting for something (a resource, a @@ -724,7 +780,7 @@ fn removeFrom(head: *[number_priorities]?*Task, tail: *[number_priorities]?*Task /// Precondition: the big kernel lock is held. pub fn taskByIdLocked(id: u32) ?*Task { for (&tasks) |*t| { - if (t.state != .free and t.id == id) return t; + if (t.state != .free and t.state != .reaping and t.id == id) return t; } return null; } @@ -734,7 +790,7 @@ pub fn taskByIdLocked(id: u32) ?*Task { /// Precondition: the big kernel lock is held. pub fn forgetIpcClientLocked(t: *Task) void { for (&tasks) |*other| { - if (other.state != .free and other.ipc_client == t) other.ipc_client = null; + if (other.state != .free and other.state != .reaping and other.ipc_client == t) other.ipc_client = null; } } @@ -826,14 +882,10 @@ pub var reap_task_hook: ?*const fn (*Task) void = null; fn reapKillPendingLocked() void { const pc = thisCpu(); const cur = pc.current; - // Safety net for the reap-after-switch slot: if a dying task switched to a *fresh* - // task (which enters via task_trampoline, not switchTo's tail), its kernel stack is - // still pending here. The dying task switched away before this tick, so it is off its - // stack — reap it now (docs/threading-plan.md M8). - if (pc.reap_after_switch) |dead| { - pc.reap_after_switch = null; - reapStackLocked(dead); - } + // Safety net: if a dying task switched to a *fresh* task (which enters via + // task_trampoline, not switchTo's tail), its stack is still queued here. The dying + // task switched away before this tick, so it is off its stack — drain now (M8). + drainReapListLocked(pc); if (cur.kill_pending and cur.aspace != 0 and !cur.in_system_call) { if (terminate_current_hook) |hook| hook(); // noreturn } @@ -875,8 +927,11 @@ pub fn setPreemption(enabled: bool) void { pub fn exit() noreturn { _ = sync.enter(); const pc = thisCpu(); - pc.current.state = .free; - pc.reap_after_switch = pc.current; // the task we switch to frees this stack (M8) + // Queue this task for reaping: `.reaping` keeps its slot out of freeSlot until its + // stack is freed; `next` links it on the core's reap list (M8/M9). + pc.current.state = .reaping; + pc.current.next = pc.reap_list; + pc.reap_list = pc.current; const next = dequeueHighest(pc) orelse @panic("sched: no task left to run"); next.state = .running; pc.current = next; @@ -908,11 +963,14 @@ pub fn exitUserLocked() noreturn { pc.loaded_aspace = kroot; releaseAspace(as); // destroys only when this was the last task on the space } - dying.state = .free; + dying.state = .reaping; // dead but its slot stays reserved until the stack is freed + wakeJoinersLocked(dying.id); // let any thread_join(dying.id) return (M9) dying.aspace = 0; dying.kill_pending = false; dying.in_system_call = false; - pc.reap_after_switch = dying; // the task we switch to frees this stack (M8) + // Queue for reaping: the task we switch to (or the next tick) frees this stack (M8/M9). + dying.next = pc.reap_list; + pc.reap_list = dying; const next = dequeueHighest(pc) orelse @panic("sched: no task left to run"); next.state = .running; pc.current = next; @@ -935,6 +993,7 @@ pub fn destroyTaskLocked(t: *Task) void { t.in_system_call = false; t.wake_at = 0; t.state = .free; + wakeJoinersLocked(t.id); // a killed thread's joiners must return too (M9) } /// Snapshot the task table into `out` (up to its length), returning the total @@ -948,7 +1007,7 @@ pub fn enumerate(out: []abi.ProcessDescriptor) u64 { defer sync.leave(flags); var total: u64 = 0; for (&tasks) |*t| { - if (t.state == .free) continue; + if (t.state == .free or t.state == .reaping) continue; // reaping = already exited if (total < out.len) { const d = &out[total]; d.* = .{ @@ -958,7 +1017,7 @@ pub fn enumerate(out: []abi.ProcessDescriptor) u64 { .ready => .ready, .running => .running, .blocked => .blocked, - .free => unreachable, + .free, .reaping => unreachable, })), .priority = t.priority, .name_length = t.name_length, diff --git a/system/kernel/tests.zig b/system/kernel/tests.zig index 5832ffa..3afab59 100644 --- a/system/kernel/tests.zig +++ b/system/kernel/tests.zig @@ -1769,9 +1769,11 @@ fn taskReapTest(boot_information: *const BootInformation) void { while (scheduler.liveStackBytes() != base and architecture.millis() < settle_deadline) scheduler.yield(); scheduler.setPriority(4); + const final = scheduler.liveStackBytes(); + log("task-reap: base={d} final={d} killed={d}/{d}\n", .{ base, final, killed, rounds }); check("all probes spawned and were killed", killed == rounds); - check("kernel stacks reclaimed to baseline (no leak)", scheduler.liveStackBytes() == base); - if (killed == rounds and scheduler.liveStackBytes() == base) + check("kernel stacks reclaimed to baseline (no leak)", final == base); + if (killed == rounds and final == base) log("task-reap: kernel stacks reclaimed to baseline ok\n", .{}); result(); } diff --git a/system/services/thread-test/thread-test.zig b/system/services/thread-test/thread-test.zig index 6cf7c7b..d62b35a 100644 --- a/system/services/thread-test/thread-test.zig +++ b/system/services/thread-test/thread-test.zig @@ -74,6 +74,8 @@ fn detachWorker() void { detach_done.store(1, .release); } +fn noopWorker() void {} + fn runJoinMode() void { write("thread-test: join mode starting\n"); @@ -114,7 +116,19 @@ fn runJoinMode() void { return; } - write("thread-test: join ok\n"); // the M3 verdict marker + // Prove join needs no per-thread kernel endpoint (M9): many spawn+join cycles. Under + // the old per-thread-endpoint scheme these leaked handles and would exhaust the + // 16-slot handle table well before 40; here they all succeed. + var cycle: u32 = 0; + while (cycle < 40) : (cycle += 1) { + const th = runtime.Thread.spawn(.{}, noopWorker, .{}) catch { + write("thread-test: FAIL spawn exhausted across join cycles (endpoint leak?)\n"); + return; + }; + th.join(); + } + + write("thread-test: join ok\n"); // the M3/M9 verdict marker } // --- M4: futex mode ---------------------------------------------------------