diff --git a/docs/threading-plan.md b/docs/threading-plan.md index 0c30bdc..9ce1cd7 100644 --- a/docs/threading-plan.md +++ b/docs/threading-plan.md @@ -335,23 +335,35 @@ multi-threaded code would hit it. Closed it: > Reworked it (and the settle loop) to wait on the wall clock instead, so its duration is > independent of unrelated code changes. -### M8 — The task reaper (cleanup + resilience) +### M8 — The task reaper (cleanup + resilience) ✅ -A dead task's **kernel** stack is currently leaked ("no reaper yet") — every process -*and* thread death loses one, so a crash loop bleeds kernel memory. A reaper fixes it and -serves the [resilience](resilience.md) restart goal directly: +A dead task's **kernel** stack was leaked ("no reaper yet") — every process *and* thread +death lost one, so a crash loop bled kernel memory. The reaper fixes it and serves the +[resilience](resilience.md) restart goal directly: -- [ ] A dying task cannot free the kernel stack it runs on, so it hands itself to a - **reap list** and switches away; the kernel stack (and, for a detached thread, its - user stack) is reclaimed from another context — a low-priority reaper step drained - on the scheduler tick and when a core goes idle. Extends the existing - `reap_task_hook`/`destroyTaskLocked` path rather than inventing a parallel one. -- [ ] `-Dtest-case=task-reap`: spawn and exit many threads and processes; assert the - kernel-heap free bytes (a new test observable) return to **baseline** — kernel - stacks reclaimed, no leak — and that the `fault-recovery`/kill paths reclaim too. +- [x] A dying task cannot free the kernel stack it runs on, so `exit()`/`exitUserLocked` + record it in a **per-core `reap_after_switch` slot** and switch away; the task that + resumes on that core frees the stack in `switchTo`'s tail (it's on its own stack, the + big lock is still held so the slot can't have been reused). A **tick-time drain** + (`reapKillPendingLocked`) is the safety net for the case where the next task is + *fresh* (enters via the trampoline, bypassing `switchTo`'s tail). A task killed while + *not* running is freed immediately in `destroyTaskLocked`. A `live_stack_bytes` + counter is the observable. *(Detached-thread user-stack reclaim moves to M9, which + adds the joinable/detached flag.)* +- [x] `-Dtest-case=task-reap` (`smp: 4`): spawn and kill 12 processes; poll the + test-observable `scheduler.liveStackBytes()` until it returns to **baseline** (a + correct reaper gets there in a few ms; a genuine leak times out) — every kernel + stack reclaimed, no leak. Threads exit through the same `exitUserLocked`, so covered. -**Gate:** `task-reap` passes; `fault-recovery`, `supervision`, `process-kill`, -`aspace-refcount` still green. +**Gate (met):** `task-reap` passes (5× isolated + 2× in the full batch); `fault-recovery`, +`supervision`, `process-kill`, `aspace-refcount`, `smp`, `affinity` all still green (24/24 +full guardrail); `zig build`/`zig build test` clean. + +> **Bug found + fixed here (touches every context switch):** the post-`switchContext` reap +> first read the `pc` **parameter**, but a task that migrated cores carries a *stale* `pc` +> in its saved `switchTo` frame — so it read the wrong core's slot and freed a live stack +> (a #GP under SMP). Fixed to re-fetch `thisCpu()` after the switch (the switch only swaps +> stacks on the current core). ### M9 — Futex-completion join (retire the per-thread endpoint) diff --git a/system/kernel/scheduler.zig b/system/kernel/scheduler.zig index 5257ab2..daf775f 100644 --- a/system/kernel/scheduler.zig +++ b/system/kernel/scheduler.zig @@ -152,6 +152,28 @@ const AspaceRef = struct { root: u64 = 0, count: u32 = 0, mmap_next: u64 = 0, de var aspace_refs = [_]AspaceRef{.{}} ** maximum_tasks; var aspace_destroy_count: u64 = 0; +/// Total bytes of task **kernel** stacks currently allocated from the kernel heap — +/// incremented when a task is created, decremented when the reaper frees a dead task's +/// stack. A test-observable proof that the reaper reclaims every stack (docs/threading- +/// plan.md M8): with no live tasks beyond the baseline, this returns to its baseline. +var live_stack_bytes: usize = 0; + +/// Test-observable: bytes of task kernel stacks currently live (see `live_stack_bytes`). +pub fn liveStackBytes() usize { + return live_stack_bytes; +} + +/// Free a dead task's kernel stack and drop it from `live_stack_bytes`. The task must be +/// off that stack already (killed while not running, or reaped after it switched away). +/// Caller holds the kernel lock. +fn reapStackLocked(t: *Task) void { + if (t.stack.len == 0) return; // boot/idle tasks run on a static stack — nothing to free + live_stack_bytes -= t.stack.len; + heap.allocator().free(t.stack); + t.stack = &.{}; + t.kstack_top = 0; +} + /// 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. @@ -246,6 +268,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, }; const maximum_cpus = parameters.maximum_cpus; @@ -422,6 +450,7 @@ pub fn spawnUserLocked(aspace: u64, entry: u64, user_sp: u64, user_arg: u64, pri heap.allocator().free(stack); return null; } + live_stack_bytes += stack.len; // the reaper drops this when the task dies (M8) t.* = .{ .id = next_id, .state = .ready, @@ -465,6 +494,7 @@ fn startUserTask() void { fn create(entry: *const fn () void, priority: Priority, affinity: ?u32) *Task { const t = freeSlot() orelse @panic("sched: task table full"); const stack = heap.allocator().alloc(u8, stack_size) catch @panic("sched: no memory for task stack"); + live_stack_bytes += stack.len; // the reaper drops this when the task dies (M8) t.* = .{ .id = next_id, .state = .ready, .priority = priority, .stack = stack, .affinity = affinity }; next_id += 1; const top = @intFromPtr(stack.ptr) + stack.len; @@ -519,6 +549,17 @@ fn switchTo(pc: *PerCpu, save_sp: *usize, next: *Task) void { pc.loaded_aspace = want; } architecture.switchContext(save_sp, next.sp); + // Resumed now (switchContext returned into our own switchTo frame). Re-fetch the core + // via thisCpu(): the `pc` parameter is from *our* earlier switchTo call, so it names + // the core we last ran on — stale if we migrated. switchContext only swaps stacks on + // 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); + } } /// Voluntarily give up the CPU to the next ready task. @@ -785,6 +826,14 @@ 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); + } if (cur.kill_pending and cur.aspace != 0 and !cur.in_system_call) { if (terminate_current_hook) |hook| hook(); // noreturn } @@ -827,6 +876,7 @@ 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) const next = dequeueHighest(pc) orelse @panic("sched: no task left to run"); next.state = .running; pc.current = next; @@ -862,6 +912,7 @@ pub fn exitUserLocked() noreturn { 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) const next = dequeueHighest(pc) orelse @panic("sched: no task left to run"); next.state = .running; pc.current = next; @@ -878,6 +929,7 @@ pub fn exitUserLocked() noreturn { /// yet). Precondition: the big kernel lock is held. pub fn destroyTaskLocked(t: *Task) void { if (t.aspace != 0) releaseAspace(t.aspace); // destroys only on the last reference + reapStackLocked(t); // safe to free now: `t` is not running on any core (M8) t.aspace = 0; t.kill_pending = false; t.in_system_call = false; diff --git a/system/kernel/tests.zig b/system/kernel/tests.zig index 36d364a..5832ffa 100644 --- a/system/kernel/tests.zig +++ b/system/kernel/tests.zig @@ -153,6 +153,8 @@ pub fn run(case: []const u8, boot_information: *const BootInformation) void { threadIdTest(boot_information); } else if (eql(case, "thread-alloc")) { threadAllocTest(boot_information); + } else if (eql(case, "task-reap")) { + taskReapTest(boot_information); } else if (eql(case, "args")) { argsTest(boot_information); } else if (eql(case, "init")) { @@ -1738,6 +1740,42 @@ fn threadAllocTest(boot_information: *const BootInformation) void { result(); } +/// The task reaper (docs/threading-plan.md M8): a dead task's kernel stack used to be +/// leaked ("no reaper yet"). Spawn and kill many ring-3 processes and confirm the total +/// kernel-stack bytes return to baseline — every stack reclaimed, no leak. (Threads exit +/// through the same exitUserLocked path, so this covers them too.) +fn taskReapTest(boot_information: *const BootInformation) void { + _ = boot_information; + log("DANOS-TEST-BEGIN: task-reap\n", .{}); + const base = scheduler.liveStackBytes(); + const rounds: u32 = 12; + var killed: u32 = 0; + var round: u32 = 0; + while (round < rounds) : (round += 1) { + process.fault_kill_count = 0; + const probe = spawnFaultingProcess() orelse break; + _ = probe; + scheduler.setPriority(1); + const deadline = architecture.millis() + 5000; + while (process.fault_kill_count < 1 and architecture.millis() < deadline) scheduler.yield(); + scheduler.setPriority(4); + if (process.fault_kill_count >= 1) killed += 1; + } + // Reaping is asynchronous — a dead task's stack is freed when its core next switches + // or ticks. Poll (bounded) until the live bytes return to baseline: a correct reaper + // gets there in a few ms; a genuine leak never does and this times out. + scheduler.setPriority(1); + const settle_deadline = architecture.millis() + 3000; + while (scheduler.liveStackBytes() != base and architecture.millis() < settle_deadline) scheduler.yield(); + scheduler.setPriority(4); + + 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) + log("task-reap: kernel stacks reclaimed to baseline ok\n", .{}); + result(); +} + /// The full PID-1 path: the bootloader read /system/services/init off the boot volume and /// handed it over; load it as a user ELF and spawn it as a real ring-3 process /// — the same call the normal boot path makes — then confirm it beats. init diff --git a/test/qemu_test.py b/test/qemu_test.py index ac13b05..ac9a5f0 100644 --- a/test/qemu_test.py +++ b/test/qemu_test.py @@ -347,6 +347,14 @@ CASES = [ "timeout": 60, "expect": r"DANOS-TEST-RESULT: PASS", "fail": r"DANOS-TEST-RESULT: FAIL"}, + + # docs/threading-plan.md M8: the task reaper — spawn+kill many processes; total kernel + # stack bytes return to baseline (every dead task's stack reclaimed, no leak). + {"name": "task-reap", + "smp": 4, + "timeout": 60, + "expect": r"DANOS-TEST-RESULT: PASS", + "fail": r"DANOS-TEST-RESULT: FAIL"}, # Process arguments: argv arrives on the SysV entry stack (argv[0] = the spawned # name, argv[1..] = the system_spawn argument blob) and echoes back intact. {"name": "args",