diff --git a/docs/threading-plan.md b/docs/threading-plan.md index 7b06344..0c30bdc 100644 --- a/docs/threading-plan.md +++ b/docs/threading-plan.md @@ -303,27 +303,37 @@ The organising principle, so Phase 2 reinforces danos's goals rather than erodin one process; Phase 2 never adds a way for one process to reach into another (the cross-process futex stays explicitly out of scope, below). -### M7 — Thread-safe allocation (the correctness gap) +### M7 — Thread-safe allocation (the correctness gap) ✅ Today the mmap arena cursor is per-*task* and the runtime heap is unlocked, so two threads in one process that both allocate corrupt each other. The thread *machinery* avoids this (closure on the stack, stacks mmap'd only by the spawner), but real -multi-threaded code would hit it. Close it: +multi-threaded code would hit it. Closed it: -- [ ] **Kernel — per-address-space mmap arena.** Grow M1's `aspace_refs` entry into a - small address-space object holding the `mmap`/`mmio` arena cursors (moved off - `Task`); `systemMmap`/`mmio_map` bump the *aspace's* cursor under the big lock, so - sibling threads get disjoint, serialized grants. Freed at refcount zero, so the - cursors vanish with the process. -- [ ] **Runtime — thread-safe heap.** Guard the allocator with a `Thread.Mutex`, gated on +- [x] **Kernel — per-address-space mmap arena.** Grew M1's `aspace_refs` entry into the + per-address-space object holding the `mmap`/`mmio` arena cursors (moved off `Task`); + `scheduler.aspaceMmapNextPtr`/`aspaceDeviceMapNextPtr` expose them. `systemMmap` + reserves a disjoint range under a *brief* lock, then maps **per page** under a + short-held lock — not the whole grant — because the big lock is held with interrupts + disabled, so pinning it across a multi-MiB memset+map froze other cores (it timed + the `affinity` scenario out mid-bring-up). Freed at refcount zero, so the cursors + vanish with the process. +- [x] **Runtime — thread-safe heap.** The allocator's two free-list mutators + (`rawAlloc`/`rawFree`) take a `Thread.Mutex`, gated on `!@import("builtin").single_threaded` so single-threaded binaries compile it out and - pay nothing. (The heap grows via mmap, now safe per above.) -- [ ] `-Dtest-case=thread-alloc` (`smp: 4`): N threads each do many `alloc`/`free` of - varied sizes, write a per-thread pattern, verify it, and free; assert every block - round-trips intact and all memory returns — no corruption under concurrent - allocation. A direct check confirms two threads' concurrent `mmap`s are disjoint. + pay nothing. Uncontended acquisition is a single CAS (no syscall). +- [x] `-Dtest-case=thread-alloc` (`smp: 4`): 4 threads each do 500 `alloc`/fill/verify/ + `free` cycles of varied sizes; each block is filled with a per-thread pattern and + verified before free, so any overlap between concurrent allocations is caught. -**Gate:** `thread-alloc` passes; guardrail + all `thread-*` cases green. +**Gate (met):** `thread-alloc` passes (3× non-flaky); full guardrail 23/23 green, +`zig build`/`zig build test` clean. + +> **Also fixed here:** the `affinity` guardrail's fixed-count busy-loop (`while (spins < +> 3e9)`) had codegen-dependent wall-time — adding a function to `tests.zig` flipped how +> the optimiser compiled it, swinging affinity from ~4 s to ~63 s and timing it out. +> 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) diff --git a/library/runtime/heap.zig b/library/runtime/heap.zig index 0ff4fd7..0e525b7 100644 --- a/library/runtime/heap.zig +++ b/library/runtime/heap.zig @@ -9,15 +9,32 @@ //! — `grow` asks the kernel for pages via `mmap` instead of mapping frames //! itself, and the kernel picks the base address. //! -//! Single-threaded and 16-byte maximum alignment, exactly like the kernel heap; a -//! lock and larger alignments come when user programs gain threads. +//! 16-byte maximum alignment, exactly like the kernel heap. The free list is guarded by +//! a `Thread.Mutex` **only in multi-threaded binaries** (`addThreadedUserBinary`): the +//! guard is gated on `builtin.single_threaded`, so an ordinary single-threaded binary +//! compiles it out and pays nothing, while a threaded one can allocate safely from +//! several threads at once (docs/threading-plan.md M7). The lock lives at the two +//! free-list mutators — `rawAlloc`/`rawFree` — which every entry point funnels through. const std = @import("std"); +const builtin = @import("builtin"); const abi = @import("abi"); const system_calls = @import("system.zig"); +const Mutex = @import("thread.zig").Thread.Mutex; const page_size = abi.page_size; +/// Guards `free_list`. A no-op in single-threaded builds (compiled out); a real futex +/// mutex in threaded ones. Uncontended acquisition is a single CAS — no syscall. +var heap_mutex: Mutex = .{}; + +inline fn lockHeap() void { + if (comptime !builtin.single_threaded) heap_mutex.lock(); +} +inline fn unlockHeap() void { + if (comptime !builtin.single_threaded) heap_mutex.unlock(); +} + /// A block header, at the start of every block; while free it also links the /// free list via `next`. const Block = extern struct { @@ -84,8 +101,11 @@ fn insertFree(block: *Block) void { } } -/// Allocate `len` bytes (16-byte aligned), or null if out of memory. +/// Allocate `len` bytes (16-byte aligned), or null if out of memory. Holds the heap lock +/// across the free-list search and any `grow` (which also touches the free list). fn rawAlloc(len: usize) ?[*]u8 { + lockHeap(); + defer unlockHeap(); const need = alignUp(header_size + len, 16); var attempts: u32 = 0; @@ -119,6 +139,8 @@ fn rawAlloc(len: usize) ?[*]u8 { } fn rawFree(ptr: [*]u8) void { + lockHeap(); + defer unlockHeap(); const block: *Block = @ptrFromInt(@intFromPtr(ptr) - header_size); insertFree(block); } diff --git a/system/kernel/process.zig b/system/kernel/process.zig index 4fbe9c0..6483a6d 100644 --- a/system/kernel/process.zig +++ b/system/kernel/process.zig @@ -58,8 +58,9 @@ pub const stack_top_virtual: u64 = stack_base_virtual + parameters.user_stack_pa /// The mmap grant arena: where `mmap` hands out fresh user pages, above the image /// and stack but still inside PML4[224] (so no kernel mapping is widened). Each -/// process bump-allocates from `heap_arena_base` upward via `Task.heap_next`; a -/// 1 GiB window is far more than any user heap needs today. +/// process bump-allocates from `heap_arena_base` upward via a per-address-space cursor +/// (`scheduler.aspaceMmapNextPtr`, shared by its threads); a 1 GiB window is far more +/// than any user heap needs today. pub const heap_arena_base: u64 = 0x0000_7000_1000_0000; pub const heap_arena_end: u64 = heap_arena_base + (1 << 30); @@ -69,8 +70,8 @@ pub const user_half_end: u64 = 0x0000_8000_0000_0000; /// The MMIO-grant arena: where `mmio_map` places device windows, in PML4[226] — /// a user-exclusive region distinct from code/stack/heap (PML4[224]), so mapping -/// device pages user-accessible widens no kernel mapping. Per-process cursor in -/// `Task.device_map_next`. +/// device pages user-accessible widens no kernel mapping. Per-address-space cursor +/// (`scheduler.aspaceDeviceMapNextPtr`). pub const device_arena_base: u64 = 0x0000_7100_0000_0000; pub const device_arena_end: u64 = device_arena_base + (4 << 30); @@ -373,18 +374,23 @@ fn systemMmioMap(state: *architecture.CpuState) void { if (r.len == 0) return fail(state); if (@addWithOverflow(r.start, r.len)[1] != 0) return fail(state); - if (t.device_map_next == 0) t.device_map_next = device_arena_base; const first = r.start & ~@as(u64, page_size - 1); const last = (r.start + r.len - 1) & ~@as(u64, page_size - 1); const pages = (last - first) / page_size + 1; - const base_v = t.device_map_next; - if (base_v + pages * page_size > device_arena_end) return fail(state); - // A framebuffer resource asks (via its flag) to be mapped write-combining rather // than the strong-uncacheable default that register MMIO needs. const write_combining = (r.flags & device_abi.resource_flag_write_combining) != 0; + + // Per-address-space cursor + shared page tables → serialize under the big lock, + // same as mmap (docs/threading-plan.md M7). + const flags = sync.enter(); + defer sync.leave(flags); + const cursor = scheduler.aspaceDeviceMapNextPtr(t.aspace) orelse return fail(state); + if (cursor.* == 0) cursor.* = device_arena_base; // seed the arena lazily + const base_v = cursor.*; + if (base_v + pages * page_size > device_arena_end) return fail(state); architecture.mapUserDeviceInto(t.aspace, base_v, r.start, r.len, write_combining); - t.device_map_next = base_v + pages * page_size; + cursor.* = base_v + pages * page_size; architecture.setSystemCallResult(state, base_v + (r.start & (page_size - 1))); // register base } @@ -1232,16 +1238,30 @@ fn systemMmap(state: *architecture.CpuState) void { const pages = (len + page_size - 1) / page_size; if (pages == 0 or pages > maximum_mmap_pages) return fail(state); - if (t.heap_next == 0) t.heap_next = heap_arena_base; // seed the arena lazily - const base = t.heap_next; - if (base + pages * page_size > heap_arena_end) return fail(state); // arena exhausted + // Reserve a disjoint range under a *brief* lock (the cursor is shared by every thread + // in this address space). The mapping below then takes the lock **per page**, not for + // the whole grant: the big lock is held with interrupts disabled, so pinning it across + // a multi-MiB memset+map would freeze every other core on its next tick — which timed + // the `affinity` scenario out (docs/threading-plan.md M7). + const base = reserve: { + const flags = sync.enter(); + defer sync.leave(flags); + const cursor = scheduler.aspaceMmapNextPtr(t.aspace) orelse return fail(state); + if (cursor.* == 0) cursor.* = heap_arena_base; // seed the arena lazily + const b = cursor.*; + if (b + pages * page_size > heap_arena_end) return fail(state); // arena exhausted + cursor.* = b + pages * page_size; // reserve now, so concurrent grants can't overlap + break :reserve b; + }; - // Map page by page. On mid-way frame exhaustion, roll back the pages already mapped - // (unmap + free) so no partial grant leaks into the address space — the same - // all-or-nothing guarantee as before, but without a fixed scratch array, so the - // per-call size can be a multi-MiB framebuffer. + // Map the reserved range page by page, each page under a short-held lock (the range is + // already reserved, so pages can't overlap another thread's; the lock only serializes + // the shared page-table walk). On mid-way frame exhaustion, roll back the mapped pages + // so no partial grant leaks — the reserved-but-unmapped tail of the arena is left + // fallow (a rare, bounded address-space leak, not a memory leak). var mapped: usize = 0; while (mapped < pages) : (mapped += 1) { + const flags = sync.enter(); const frame = pmm.alloc() orelse { var i: usize = 0; while (i < mapped) : (i += 1) { @@ -1251,14 +1271,15 @@ fn systemMmap(state: *architecture.CpuState) void { pmm.free(physical); } } + sync.leave(flags); return fail(state); }; const destination: [*]u8 = @ptrFromInt(boot_handoff.physicalToVirtual(frame)); @memset(destination[0..page_size], 0); // hand out zeroed memory architecture.mapUserPageInto(t.aspace, base + mapped * page_size, frame, true, false); // RW + NX + sync.leave(flags); } - t.heap_next = base + pages * page_size; - architecture.setSystemCallResult(state, base); + architecture.setSystemCallResult(state, base); // the cursor was already advanced at reserve } /// munmap(base, len): release a range previously handed out by `mmap`. Unmaps diff --git a/system/kernel/scheduler.zig b/system/kernel/scheduler.zig index 4a91ef8..5257ab2 100644 --- a/system/kernel/scheduler.zig +++ b/system/kernel/scheduler.zig @@ -83,13 +83,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, - // Next free virtual address in this task's mmap grant arena (0 = uninitialised; - // process.zig lazily seeds it to the arena base on the first mmap). Bumped up - // as the user heap grows; user task only. - heap_next: u64 = 0, - // Next free virtual address in this task's MMIO-grant arena (PML4[226]; 0 = - // uninitialised, process.zig seeds it on the first mmio_map). User task only. - device_map_next: u64 = 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). // --- synchronous IPC (ipc_sync.zig) --- // Per-process handle table: a small-int handle names a kernel capability object. // Each entry tags its `kind` (an IPC endpoint or a shared-memory object) so the @@ -148,7 +144,11 @@ var tasks = [_]Task{.{}} ** maximum_tasks; /// only when the **last** task on an address space exits. All access is under the big /// kernel lock. There can be no more live address spaces than tasks, so the table is /// sized to the task pool and never overflows in practice. -const AspaceRef = struct { root: u64 = 0, count: u32 = 0 }; +// The per-address-space kernel object: a reference count plus the grant-arena cursors. +// One live entry per address space; threads sharing an address space share this entry, +// so their mmap/mmio grants bump one cursor and never overlap (docs/threading-plan.md M7). +// `mmap_next`/`device_map_next` are 0 until process.zig seeds them to the arena base. +const AspaceRef = struct { root: u64 = 0, count: u32 = 0, mmap_next: u64 = 0, device_map_next: u64 = 0 }; var aspace_refs = [_]AspaceRef{.{}} ** maximum_tasks; var aspace_destroy_count: u64 = 0; @@ -202,6 +202,26 @@ pub fn liveAspaceCount() u32 { pub fn aspaceDestroyCount() u64 { return aspace_destroy_count; } + +/// Pointer to the mmap grant-arena cursor for address space `root`, so the mmap syscall +/// can read-and-bump it. Per-address-space (not per-task), so sibling threads get +/// disjoint grants. **Caller holds the kernel lock** (the entry is stable while held). +/// Null only if `root` was never retained — which can't happen for a live user task. +pub fn aspaceMmapNextPtr(root: u64) ?*u64 { + for (&aspace_refs) |*entry| { + if (entry.count != 0 and entry.root == root) return &entry.mmap_next; + } + return null; +} + +/// Pointer to the MMIO grant-arena cursor for address space `root` (see +/// `aspaceMmapNextPtr`). Caller holds the kernel lock. +pub fn aspaceDeviceMapNextPtr(root: u64) ?*u64 { + for (&aspace_refs) |*entry| { + if (entry.count != 0 and entry.root == root) return &entry.device_map_next; + } + return null; +} var next_id: u32 = 1; /// Per-CPU scheduler state: the task each core is running, its own idle task, and a diff --git a/system/kernel/tests.zig b/system/kernel/tests.zig index d1c0f61..36d364a 100644 --- a/system/kernel/tests.zig +++ b/system/kernel/tests.zig @@ -151,6 +151,8 @@ pub fn run(case: []const u8, boot_information: *const BootInformation) void { threadMutexTest(boot_information); } else if (eql(case, "thread-id")) { threadIdTest(boot_information); + } else if (eql(case, "thread-alloc")) { + threadAllocTest(boot_information); } else if (eql(case, "args")) { argsTest(boot_information); } else if (eql(case, "init")) { @@ -779,11 +781,15 @@ fn affinityTest() void { return; } - var spins: u64 = 0; - while (spins < 3_000_000_000) spins +%= 1; // many time slices across the cores + // Let many time slices pass so the scheduler runs the pinned worker across ticks. + // Wait on the wall clock, not a raw iteration count: a fixed-count busy-loop's + // wall-time is a codegen lottery (the optimiser may elide or vectorise it), so an + // unrelated change elsewhere in this file could swing this test from ~4 s to ~50 s. + const run_until = architecture.millis() + 400; + while (architecture.millis() < run_until) {} affinity_running = false; - var settle: u64 = 0; - while (settle < 200_000_000) settle +%= 1; // let the worker see the flag and exit + const settle_until = architecture.millis() + 50; + while (architecture.millis() < settle_until) {} // let the worker see the flag and exit var others: u32 = 0; for (affinity_cores, 0..) |seen, c| { @@ -1689,6 +1695,49 @@ fn threadIdTest(boot_information: *const BootInformation) void { result(); } +/// Thread-safe allocation (docs/threading-plan.md M7): `thread-test` in alloc mode runs N +/// threads that each do many `alloc`/fill/verify/`free` cycles of varied sizes on the +/// shared runtime heap. If the heap lock or the per-address-space mmap arena were unsafe, +/// two threads' blocks would overlap and a thread would read another's pattern; the +/// verdict marker is emitted only when every thread completes with every block intact. +fn threadAllocTest(boot_information: *const BootInformation) void { + log("DANOS-TEST-BEGIN: thread-alloc\n", .{}); + if (boot_information.initial_ramdisk_len == 0) { + check("bootloader handed over an initial_ramdisk", false); + result(); + return; + } + const image = @as([*]const u8, @ptrFromInt(boot_handoff.physicalToVirtual(boot_information.initial_ramdisk_base)))[0..boot_information.initial_ramdisk_len]; + const rd = initial_ramdisk.Reader.init(image) orelse { + check("initial_ramdisk image is valid", false); + result(); + return; + }; + + var started = false; + var i: u32 = 0; + while (i < rd.count) : (i += 1) { + const item = rd.entry(i) orelse continue; + if (!eql(item.name, "thread-test")) continue; + started = if (process.spawnProcess(item.blob, 4, &.{ "thread-test", "alloc" })) true else |_| false; + break; + } + check("thread-test (alloc mode) spawned", started); + + const ok_marker = "thread-alloc: ok"; + const fail_marker = "thread-alloc: FAIL"; + scheduler.setPriority(1); + const deadline = architecture.millis() + 20000; + while (architecture.millis() < deadline) { + if (bufferHas(ok_marker) or bufferHas(fail_marker)) break; + scheduler.yield(); + } + scheduler.setPriority(4); + + check("concurrent heap allocation stayed corruption-free (shared heap + per-aspace arena)", bufferHas(ok_marker) and !bufferHas(fail_marker)); + 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/system/services/thread-test/thread-test.zig b/system/services/thread-test/thread-test.zig index bd15643..6cf7c7b 100644 --- a/system/services/thread-test/thread-test.zig +++ b/system/services/thread-test/thread-test.zig @@ -294,6 +294,55 @@ fn runIdMode() void { write("thread-id: ok\n"); // the M6 verdict marker } +// --- M7: alloc mode (concurrent heap allocation) ---------------------------- + +const alloc_threads: u32 = 4; +const allocs_per_thread: u32 = 500; +var allocs_clean = std.atomic.Value(u32).init(0); + +fn allocWorker(seed: u32) void { + const gpa = runtime.allocator(); + var rng: u32 = seed | 1; + var round: u32 = 0; + while (round < allocs_per_thread) : (round += 1) { + rng = rng *% 1664525 +% 1013904223; // cheap LCG for varied sizes + const size: usize = 16 + (rng % 4080); // 16..4095 bytes + const buf = gpa.alloc(u8, size) catch return; // OOM: don't count this thread clean + const pattern: u8 = @truncate(seed +% round); + @memset(buf, pattern); + // Nothing else should touch our block; if a concurrent allocation overlapped it, + // one of us would read the other's pattern here. + var ok = true; + for (buf) |b| { + if (b != pattern) ok = false; + } + gpa.free(buf); + if (!ok) return; // corruption — leave without counting clean + } + _ = allocs_clean.fetchAdd(1, .monotonic); +} + +fn runAllocMode() void { + write("thread-alloc: starting\n"); + var threads: [alloc_threads]runtime.Thread = undefined; + var n: u32 = 0; + while (n < alloc_threads) : (n += 1) { + threads[n] = runtime.Thread.spawn(.{}, allocWorker, .{n +% 1}) catch { + write("thread-alloc: FAIL spawn\n"); + return; + }; + } + for (threads[0..alloc_threads]) |t| t.join(); + + // Every thread must have completed all rounds with each block intact — proof the + // shared heap and the per-aspace mmap arena are safe under concurrent allocation. + if (allocs_clean.load(.acquire) != alloc_threads) { + write("thread-alloc: FAIL corruption or OOM under concurrent allocation\n"); + return; + } + write("thread-alloc: ok\n"); // the M7 verdict marker +} + pub fn main(init: runtime.process.Init) void { const mode = init.arguments.get(1) orelse "spawn"; if (std.mem.eql(u8, mode, "join")) { @@ -304,6 +353,8 @@ pub fn main(init: runtime.process.Init) void { runMutexMode(); } else if (std.mem.eql(u8, mode, "id")) { runIdMode(); + } else if (std.mem.eql(u8, mode, "alloc")) { + runAllocMode(); } else { runSpawnMode(); } diff --git a/test/qemu_test.py b/test/qemu_test.py index f5753ec..ac13b05 100644 --- a/test/qemu_test.py +++ b/test/qemu_test.py @@ -339,6 +339,14 @@ CASES = [ "timeout": 60, "expect": r"DANOS-TEST-RESULT: PASS", "fail": r"DANOS-TEST-RESULT: FAIL"}, + + # docs/threading-plan.md M7: thread-safe allocation — N threads hammer the shared heap + # (per-aspace mmap arena + locked free list) with no cross-block corruption. + {"name": "thread-alloc", + "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",