From 244ad0abc8a97b333147d15cf0e8045c6ffaa2f7 Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 29 Sep 2026 22:58:30 +0200 Subject: [PATCH 01/14] Stage 6 step 1: a device's handler posts its watch; irq_ring's UserDev and Audio go The small-kernel track's stage 6 was one paragraph. It is now five steps in the track issue: interrupts post (this commit), the interrupts-off and preemption-off windows measured, the i8042 leaving with ps2server (#592's stage), xHCI leaving with usbd (storage step 10), and `drain_irqs` gone. For each device whose handler does work, the thread that does it is named, and no step makes a kernel thread. A post is now legal in an interrupt handler, and three kinds of handler make one: a claimed PCI function's vector, the IOMMU's refusal of a claimed function, and both audio backends. The per-CPU relay loses `UserDev` and `Audio`, and `drain_irqs` loses both arms, so the scheduler pass no longer posts a claim's or the audio watch. What makes the post legal in a handler, which may interrupt any holder of a preempt-only lock, the allocator's included: - toyos-sched's watch allocates and frees nothing under its list lock. A registration grows the list with the lock let go and swaps the room in; the sweep counts the dead, allocates outside, moves them out, and drops them outside. A post fires every ring entry where it stands and frees nothing: a fired entry is dead, and the next registration sweeps it. The lock order this makes is list lock, then a ring's own lock, then the watch its submitters park on, which holds threads and no ring. - The kernel's `KernelLock`, the leaf every watch list is, is held with interrupts off, as `LeafLock`'s own contract already said it was. It raises the preempt count before closing the interrupt bracket, so the inner lock's release never reaches depth zero, and a pass, with interrupts masked. - A poll ring's completions move behind a `KernelLock` of their own. The submission side and the poll list stay behind the thread-only `Lock`, which no post reaches, and teardown clears the completions before the page can go. `pcidev`'s `drain_pending` and the record's `pending` flag existed only for the relay and go. The first-message line moves off the handler to the holder's first read of a record with a count, which a fault's refusal never returns. `issues/kernel/every-wait-in-this-kernel-is-a-spin.md` lists "posting from interrupt context" among rejected decisions; the track, which supersedes that file's ordering, makes it stage 6's first step. Tests: - toyos-sched/tests/watch_list.rs: a counting global allocator sees no allocation or free under the list lock and none inside a post. Red on the base watch.rs ("registering allocated or freed under the list lock"), and red with only `post` reverted to take-and-drop ("posting: a post allocated or freed"). - loom_watch: two posts on two watches, each firing a poll of one ring under its list lock with the ring's lock and watch nested beneath, while the ring's submitter waits for both: no deadlock, one completion each, no lost wake. - `handler-post` and `handler_post_without_a_pass`: on a CPU holding preemption off, claim slot 0's vector is raised inside a post of that slot's own watch, and each of four holds must see the handler's post once the outer post lets go. On the base the handler posts nothing; with a `KernelLock` that leaves interrupts open, the handler's post spins on the lock its own CPU holds. - kernel-loom's device_irq loses its two `pending` models; `device-irq-lossy` still reds on `every_message_is_counted_once`. Measured: `kernel/src` 67,060 lines to 67,112, which is the shipped kernel 34 lines smaller and the `handler-post` actuator, compiled only with `boot-actuators`, 86 lines. `toyos-sched/src` 8,540 to 8,595. Co-Authored-By: Claude Opus 5.5 --- ...-small-interrupts-post-and-threads-wait.md | 39 +++- kernel-loom/Cargo.toml | 5 +- kernel-loom/tests/device_irq.rs | 81 ++------ kernel/Cargo.toml | 8 +- kernel/src/actuator.rs | 4 + kernel/src/arch/x86_64/idt/hda.rs | 2 +- kernel/src/arch/x86_64/idt/user_dev.rs | 10 +- kernel/src/arch/x86_64/idt/virtio_sound.rs | 2 +- kernel/src/drivers/hda.rs | 2 +- kernel/src/drivers/virtio_sound.rs | 4 +- kernel/src/inbox/mod.rs | 103 ++++++---- kernel/src/irq_ring.rs | 7 +- kernel/src/pcidev/mod.rs | 42 ++--- kernel/src/pcidev/record.rs | 70 ++----- kernel/src/sched/driver.rs | 16 +- kernel/src/sched/payload.rs | 17 +- kernel/src/watch.rs | 85 ++++++++- src/ci.rs | 1 - tests/toyos.rs | 41 ++++ toyos-sched/loom/tests/loom_watch.rs | 118 ++++++++++++ toyos-sched/src/sync.rs | 6 +- toyos-sched/src/watch.rs | 139 +++++++++----- toyos-sched/tests/watch_list.rs | 177 ++++++++++++++++++ 23 files changed, 696 insertions(+), 283 deletions(-) create mode 100644 toyos-sched/tests/watch_list.rs diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 696e5048872..69869726f79 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -144,10 +144,41 @@ times: written once as straight-line code. **Exit**: no interrupts-off window longer than a register access, and keyboard input keeps flowing while a stick misbehaves. -6. **The scheduler knows nothing about devices.** Interrupt handlers only post - to their device's `Watch`, and the device's thread does the work. The - per-CPU IRQ relay, the driver list in the scheduler pass and the idle - special cases are deleted. +6. **The scheduler knows nothing about devices.** A handler posts its + device's `Watch` and ends its interrupt; the thread waiting on that watch + does the work, and no step creates a kernel thread. `irq_ring`, the driver + list in `drain_irqs` and the idle loop's device checks are gone by step 5. + Each step measures the kernel's lines, and from step 2 the longest + interrupts-off and preemption-off windows, against stage 6's first commit. + 1. **Interrupts post.** A post is legal in a handler: a watch's list and a + poll ring's completions sit behind interrupts-off leaf locks nothing + allocates or frees under, and a post frees nothing, since the next + registration sweeps what it fired. A claimed function's vector, the + IOMMU's refusal and both audio backends post from the handler, and + `irq_ring`'s `UserDev` and `Audio` and their arms in `drain_irqs` go. + The thread is the holder's: netd's, blockd's, soundd's mix thread, and + the `isa` claim's when #592 lands. **Exit**: `handler_post_without_a_pass`, + a vector taken inside a post of its own watch on a CPU holding + preemption off posting once the outer post lets go and before any pass, + red on the base; the watch's loom models over the new post. + 2. **The windows, measured**: the longest interrupts-off and preemption-off + windows per CPU, reported beside the IRQ census and fed by each + architecture's masking primitives and entries, the number the ARM + track's stage 4 owes as well. After step 1 because the ARM track is + reshaping those primitives now; applied to stage 6's first commit for + the baseline. **Exit**: both windows read off the T14 at stage 6's start + and at step 1's head. + 3. **The i8042's thread is ps2server's** (#592's i8042 stage): `irq_ring`'s + `I8042`, `keyboard_controller::service` and the idle loop's + `verdict_due` go with the kernel's driver. **Exit**: that stage's. + 4. **xHCI's thread is usbd's** (step 10 above): `Xhci`, `poll_if_pending` + and `port_work_pending` go with the kernel's driver, and `irq_ring` with + them. **Exit**: step 10's. + 5. **The pass is the scheduler's.** `drain_irqs` goes: the blocked-task + dump with its panel hold, and the heartbeat, become `pass`'s own, and + the TCO feed stays, since what it proves is that passes run. **Exit**: + `pass` and the idle loop call no driver, and both windows are measured + against stage 6's start. ## Standing diff --git a/kernel-loom/Cargo.toml b/kernel-loom/Cargo.toml index 51a00715256..271dd8a0a76 100644 --- a/kernel-loom/Cargo.toml +++ b/kernel-loom/Cargo.toml @@ -147,9 +147,8 @@ sleeplock-acquire-off = [] # # Never on by default, and no kernel build can reach it. durability-settle-blind = [] -# The negative control for a claimed PCI function's interrupt record. Its two -# read-modify-writes — the reader's `swap` of the count and the scheduler pass's -# `swap` of the wake flag — become a load and a store, which is the whole of +# The negative control for a claimed PCI function's interrupt record. The +# reader's `swap` of the count becomes a load and a store, which is the whole of # what this record's design is, and `device_irq.rs` must red: # # cargo test --manifest-path kernel-loom/Cargo.toml --features device-irq-lossy \ diff --git a/kernel-loom/tests/device_irq.rs b/kernel-loom/tests/device_irq.rs index 450046d45eb..d5370bf87fb 100644 --- a/kernel-loom/tests/device_irq.rs +++ b/kernel-loom/tests/device_irq.rs @@ -2,30 +2,25 @@ //! //! The kernel programs one MSI-X vector per claimed function and accumulates //! what arrives into a record its holder reads through a syscall. So there are -//! two parties on two CPUs and two words between them: an ISR that bumps a -//! count and arms a wake, a reader that takes the count, and a scheduler pass -//! that takes the wake. +//! two parties on two CPUs and one word between them: an ISR that bumps a +//! count, and a reader that takes it. //! -//! **The invariant is that every message is counted exactly once and owes -//! exactly one wake.** Nothing here orders anything else — each word is the -//! whole of what it says, so the orderings are `Relaxed` and every property is -//! an interleaving. What makes them hold is that the taking side of both words -//! is a read-modify-write: a reader that loaded a count and then cleared it -//! drops every message the ISR recorded in between, and two scheduler passes -//! that both loaded a wake flag both wake one message's watchers. +//! **The invariant is that every message is counted exactly once.** Nothing +//! here orders anything else — the word is the whole of what it says, so the +//! orderings are `Relaxed` and the property is an interleaving. What makes it +//! hold is that the taking side is a read-modify-write: a reader that loaded a +//! count and then cleared it drops every message the ISR recorded in between. //! -//! That pair is the record's whole design, so the negative control is the pair -//! turned off — a cargo feature rather than a comment: +//! That is the record's whole design, so the negative control is it turned off +//! — a cargo feature rather than a comment: //! //! ```text //! cargo test --manifest-path kernel-loom/Cargo.toml --features device-irq-lossy \ //! --test device_irq //! ``` //! -//! makes both `swap`s a load and a store and the ISR's `fetch_add` a load, an -//! add and a store, and this file must red — at -//! [`every_message_is_counted_once`] and at [`one_message_is_one_wake`], which -//! are the two defects stated exactly. +//! makes the `swap` a load and a store and the ISR's `fetch_add` a load, an +//! add and a store, and [`every_message_is_counted_once`] must red. use kernel_loom::device_irq::Interrupt; use loom::sync::Arc; @@ -62,32 +57,6 @@ fn every_message_is_counted_once() { }); } -/// A wake is owed exactly once per message, and the pass that owes it is the -/// one that takes it. -/// -/// The wake and the ISR are the *same* CPU in the kernel — the scheduler pass -/// that drains runs after the handler that armed it, with interrupts off in -/// between — so this models the weaker thing that must also hold: two passes -/// racing each other never both wake, and never both decline. -#[test] -fn one_message_is_one_wake() { - loom::model(|| { - let irq = Arc::new(Interrupt::new()); - irq.took(); - - let other = { - let irq = irq.clone(); - loom::thread::spawn(move || irq.take_pending()) - }; - - let mine = irq.take_pending(); - let theirs = other.join().unwrap(); - - assert!(!(mine && theirs), "two passes both woke one message's watchers"); - assert!(mine || theirs, "neither pass woke a message that had already arrived"); - }); -} - /// A reader that finds nothing answers nothing, and leaves nothing behind. /// /// Single-threaded and deliberately so: this is the answer on the path a @@ -106,31 +75,3 @@ fn an_idle_record_answers_nothing() { assert!(!irq.armed(), "a drained record still reads ready"); }); } - -/// A fault owes its holder a wake, and the pass that takes it reads the fault. -/// -/// The fault handler and the scheduler pass that turns the wake into a wake-up -/// may be different CPUs, and what the woken holder reads next is the refusal: -/// a pass that took the wake and still read the claim unfaulted would wake a -/// holder into reading "no interrupt" and parking again, for a function that -/// can no longer send one. A `Relaxed` wake fails the first assertion, and a -/// fault that posts no wake the last. -#[test] -fn a_faults_wake_carries_the_fault() { - loom::model(|| { - let irq = Arc::new(Interrupt::new()); - - let handler = { - let irq = irq.clone(); - loom::thread::spawn(move || irq.fault()) - }; - - let taken_before = irq.take_pending(); - if taken_before { - assert!(irq.faulted(), "a pass took a fault's wake and read the claim unfaulted"); - } - handler.join().unwrap(); - assert!(irq.faulted()); - assert!(taken_before || irq.take_pending(), "a fault owed its holder a wake and posted none"); - }); -} diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index 81e0a9b3152..6474d87d5c3 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -56,10 +56,10 @@ shard-publish-relaxed = [] # `src/durability.rs`'s `settle` becomes the pre-generation blind clear, and # `durability` reds. durability-settle-blind = [] -# `src/pcidev/record.rs`'s two read-modify-writes become a load and a store, so -# a message the ISR records between a reader's two halves is lost and a wake can -# be owed twice, and `device_irq` reds. That pair *is* the record's design, so -# turning it off reverts the whole of what the model claims. +# `src/pcidev/record.rs`'s read-modify-writes become a load and a store, so a +# message the ISR records between a reader's two halves is lost, and +# `device_irq` reds. They *are* the record's design, so turning them off reverts +# the whole of what the model claims. device-irq-lossy = [] # `src/sched/dump_request.rs`'s `take` and `end_report` go `Relaxed`, so the next report # is unordered against the last one's writes, and `dump_request` reds. diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 7e65a02bbf9..cb4dda3c2bd 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -281,6 +281,10 @@ actuators! { /// parking, so a post lands in the window its commit must refuse the park over. watch_window = "watch-window"; + /// Raise claim slot 0's vector inside a post of its own watch while the CPU + /// holds preemption off, and count whether the handler posted it there. + handler_post = "handler-post"; + /// Starve the four xHCI bring-up register waits in `init_one`. xhci_deaf_controller = "xhci-deaf-controller"; diff --git a/kernel/src/arch/x86_64/idt/hda.rs b/kernel/src/arch/x86_64/idt/hda.rs index 579bebedfc6..d75e99ef709 100644 --- a/kernel/src/arch/x86_64/idt/hda.rs +++ b/kernel/src/arch/x86_64/idt/hda.rs @@ -1,6 +1,6 @@ use super::device_irq::device_irq_entry; -// Lock-free, heap-free: may interrupt a CPU holding the controller lock (preemption disabled, not interrupts). +// Heap-free, and takes only interrupts-off locks: may interrupt a CPU holding the controller lock (preemption disabled, not interrupts). extern "sysv64" fn hda_handler() { crate::arch::percpu::irq_took!(Hda); crate::drivers::hda::isr_complete(); diff --git a/kernel/src/arch/x86_64/idt/user_dev.rs b/kernel/src/arch/x86_64/idt/user_dev.rs index 18142f94df5..ec24a26cc77 100644 --- a/kernel/src/arch/x86_64/idt/user_dev.rs +++ b/kernel/src/arch/x86_64/idt/user_dev.rs @@ -6,18 +6,16 @@ //! wake every user driver in the machine on any of their interrupts, which is //! one process learning when another's device is busy. //! -//! Lock-free and heap-free like every other device entry here: the record is -//! atomics and the wake happens on the next scheduler pass. +//! Heap-free like every other device entry here: the record is atomics, and +//! the claim's watch is posted from the handler. use super::device_irq::device_irq_entry; -use crate::irq_ring::IrqSource; fn took(slot: usize) { crate::arch::percpu::irq_took!(UserDev); crate::pcidev::isr(slot); - crate::irq_ring::isr_publish(IrqSource::UserDev, crate::clock::nanos_since_boot()); - // Force resched now, so `drain_irqs` turns the record into a wake before - // the next quantum tick rather than after it. + // A holder the post woke on this CPU runs at the interrupt's exit, not at + // the next quantum tick. crate::preempt::set_need_resched(); crate::arch::apic::eoi(); } diff --git a/kernel/src/arch/x86_64/idt/virtio_sound.rs b/kernel/src/arch/x86_64/idt/virtio_sound.rs index 98060824d62..6e3744d624a 100644 --- a/kernel/src/arch/x86_64/idt/virtio_sound.rs +++ b/kernel/src/arch/x86_64/idt/virtio_sound.rs @@ -1,6 +1,6 @@ use super::device_irq::device_irq_entry; -// Lock-free and heap-free: may interrupt a CPU holding the controller lock, which disables preemption but not interrupts. +// Heap-free, and takes only interrupts-off locks: may interrupt a CPU holding the controller lock, which disables preemption but not interrupts. extern "sysv64" fn virtio_sound_handler() { crate::arch::percpu::irq_took!(Sound); crate::drivers::virtio_sound::isr_complete(); diff --git a/kernel/src/drivers/hda.rs b/kernel/src/drivers/hda.rs index 579c341cbed..00d26ed8f8b 100644 --- a/kernel/src/drivers/hda.rs +++ b/kernel/src/drivers/hda.rs @@ -166,7 +166,7 @@ pub fn isr_complete() { } ISR.timestamp.store(timestamp, Ordering::Relaxed); ISR.mask.fetch_or(mask, Ordering::Release); - crate::irq_ring::isr_publish(crate::irq_ring::IrqSource::Audio, timestamp); + super::AUDIO_WATCH.post(); crate::preempt::set_need_resched(); } diff --git a/kernel/src/drivers/virtio_sound.rs b/kernel/src/drivers/virtio_sound.rs index f4787219c49..7f170f13d41 100644 --- a/kernel/src/drivers/virtio_sound.rs +++ b/kernel/src/drivers/virtio_sound.rs @@ -108,8 +108,8 @@ pub fn isr_complete() { return; } isr_push_completion(mask, timestamp); - crate::irq_ring::isr_publish(crate::irq_ring::IrqSource::Audio, timestamp); - // Force a scheduler entry on IRQ return so the record becomes wakes now, not at next tick. + super::AUDIO_WATCH.post(); + // soundd, woken on this CPU, runs at the interrupt's exit, not at the next tick. crate::preempt::set_need_resched(); } diff --git a/kernel/src/inbox/mod.rs b/kernel/src/inbox/mod.rs index d7c69cc7906..a98fecae3d2 100644 --- a/kernel/src/inbox/mod.rs +++ b/kernel/src/inbox/mod.rs @@ -19,20 +19,25 @@ //! process's ring or say something no object said. A post writes at most one //! entry, and a ring holds at most [`MAX_PENDING_WATCHES`] polls. //! -//! **Locks.** A ring's own lock takes nothing under it and is never taken under -//! a watch's: a post fires its polls with its list let go. A ring's own watch -//! holds only threads, because no handle names a ring as a thing to watch. +//! **Locks.** What a completion writes sits behind a `KernelLock` of its own, +//! held with interrupts off, because a post fires its polls under its list lock +//! and a device's handler posts; nothing is taken under it. The rest of a ring, +//! its submissions and its polls, is its `Lock`'s, which no post reaches. A +//! ring's own watch holds only threads, because no handle names a ring as a +//! thing to watch. use alloc::sync::Arc; use alloc::vec::Vec; use core::sync::atomic::Ordering; +use toyos_sched::sync::LeafLock; use toyos_sched::task::WaitClass; use toyos_sched::watch::{Fire, Ring}; use crate::object::shm::SharedMemObject; use crate::object::{ops, KObjectRef}; use crate::process::{self, Pid}; +use crate::sched::payload::KernelLock; use crate::scheduler; use crate::sync::Lock; use crate::time::{Deadline, Duration}; @@ -62,6 +67,8 @@ impl InboxRef { impl Drop for InboxRef { fn drop(&mut self) { + // First, so no post writes into the page this drop lets go of. + self.0.completions.with(|c| *c = None); // Taken out under the lock and let go of outside it: the unmap flushes. let Some(mut state) = self.0.state.lock().take() else { unreachable!("an inbox is torn down by its one reference, once"); @@ -193,6 +200,8 @@ const MAX_PENDING_WATCHES: usize = 1024; pub struct Inbox { /// `None` once the ring's one reference let go of it. state: Lock>, + /// `None` from the moment that reference starts letting go of it. + completions: KernelLock>, /// Threads parked in `submit`; never a poll — see the module header. watch: Watch, } @@ -202,59 +211,68 @@ struct RingState { /// A ring's page has no lifetime of its own; it goes with the last handle to the ring. shm: Arc, submission_size: u32, - completion_size: u32, - /// The kernel's own copy of the completion tail, the only one it reads. - completion_tail: u32, /// Polls still armed as of the last registration, which sweeps the rest. pending: Vec>, owner_pid: Pid, } -impl RingState { - // No accessor below returns a Rust reference into this page — the process - // maps it writable, so only atomics or `read_volatile` are sound here. +// No accessor below returns a Rust reference into a ring's page — the process +// maps it writable, so only atomics or `read_volatile` are sound here. - /// One atomic word of one ring header; never `&RingHeader` — see the block above. - fn ring_word(&self, ring_off: u64, field_off: usize) -> &core::sync::atomic::AtomicU32 { - let ptr = self.shm_phys.as_mut_ptr::(); - // SAFETY: offset is in-bounds and 4-aligned within the 2 MiB page; `AtomicU32` is sound over memory the process also writes. - unsafe { - core::sync::atomic::AtomicU32::from_ptr( - ptr.add(ring_off as usize + field_off) as *mut u32, - ) - } +/// One atomic word of one ring header; never `&RingHeader` — see the block above. +fn ring_word(page: &DirectMap, ring_off: u64, field_off: usize) -> &core::sync::atomic::AtomicU32 { + let ptr = page.as_mut_ptr::(); + // SAFETY: offset is in-bounds and 4-aligned within the 2 MiB page, which lives as long as the borrow of its holder; `AtomicU32` is sound over memory the process also writes. + unsafe { + core::sync::atomic::AtomicU32::from_ptr( + ptr.add(ring_off as usize + field_off) as *mut u32, + ) } +} +impl RingState { fn submission_head(&self) -> &core::sync::atomic::AtomicU32 { - self.ring_word(SUBMISSION_RING_OFF, core::mem::offset_of!(RingHeader, head)) + ring_word(&self.shm_phys, SUBMISSION_RING_OFF, core::mem::offset_of!(RingHeader, head)) } fn submission_tail(&self) -> &core::sync::atomic::AtomicU32 { - self.ring_word(SUBMISSION_RING_OFF, core::mem::offset_of!(RingHeader, tail)) + ring_word(&self.shm_phys, SUBMISSION_RING_OFF, core::mem::offset_of!(RingHeader, tail)) } + /// One submission entry, copied out by value via `read_volatile` — never a `&Submission`. + fn submission_at(&self, index: u32) -> Submission { + let ptr = self.shm_phys.as_mut_ptr::(); + // SAFETY: `index` is masked by `submission_size` (≤256), keeping the read in-bounds and aligned within the page. + unsafe { (ptr.add(SUBMISSIONS_OFF as usize + index as usize * core::mem::size_of::()) as *const Submission).read_volatile() } + } +} + +/// What a poll's completion writes, which a post from an interrupt handler +/// reaches. Its page lives while it is `Some`: the teardown takes it to `None` +/// before it lets the page go. +struct Completions { + page: DirectMap, + completion_size: u32, + /// The kernel's own copy of the completion tail, the only one it reads. + completion_tail: u32, +} + +impl Completions { fn completion_head(&self) -> &core::sync::atomic::AtomicU32 { - self.ring_word(COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, head)) + ring_word(&self.page, COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, head)) } fn completion_tail_word(&self) -> &core::sync::atomic::AtomicU32 { - self.ring_word(COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, tail)) + ring_word(&self.page, COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, tail)) } fn completion_dropped(&self) -> &core::sync::atomic::AtomicU32 { - self.ring_word(COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, dropped)) - } - - /// One submission entry, copied out by value via `read_volatile` — never a `&Submission`. - fn submission_at(&self, index: u32) -> Submission { - let ptr = self.shm_phys.as_mut_ptr::(); - // SAFETY: `index` is masked by `submission_size` (≤256), keeping the read in-bounds and aligned within the page. - unsafe { (ptr.add(SUBMISSIONS_OFF as usize + index as usize * core::mem::size_of::()) as *const Submission).read_volatile() } + ring_word(&self.page, COMPLETION_RING_OFF, core::mem::offset_of!(RingHeader, dropped)) } /// The address of one completion entry — a pointer, never a `&mut` minted from a shared borrow. fn completion_at(&self, index: u32) -> *mut Completion { - let ptr = self.shm_phys.as_mut_ptr::(); + let ptr = self.page.as_mut_ptr::(); // SAFETY: `index` is masked by `completion_size` (≤512), keeping the offset inside the page. unsafe { ptr.add(COMPLETION_RING_OFF as usize + core::mem::size_of::() + index as usize * core::mem::size_of::()) as *mut Completion } } @@ -269,7 +287,7 @@ impl RingState { return; } let idx = tail & (self.completion_size - 1); - // SAFETY: `idx` is masked to ring size; the ring's lock serializes kernel writers. + // SAFETY: `idx` is masked to ring size; the completions' lock serializes kernel writers. unsafe { self.completion_at(idx).write(Completion { token: user_data, result, flags }) }; self.completion_tail = tail.wrapping_add(1); self.completion_tail_word().store(tail.wrapping_add(1), Ordering::Release); @@ -292,8 +310,10 @@ impl Inbox { /// Post one completion and wake whoever waits in `submit`. A ring already /// torn down takes nothing and wakes nobody. fn complete(&self, user_data: u64, result: i32) { - let posted = self.with_state(|state| state.post_completion(user_data, result, 0)); - if posted.is_ok() { + let posted = self + .completions + .with(|c| c.as_mut().map(|c| c.post_completion(user_data, result, 0))); + if posted.is_some() { self.watch.post(); } } @@ -301,6 +321,10 @@ impl Inbox { fn with_state(&self, f: impl FnOnce(&mut RingState) -> R) -> Result { self.state.lock().as_mut().map(f).ok_or(SyscallError::NotFound) } + + fn with_completions(&self, f: impl FnOnce(&Completions) -> R) -> Result { + self.completions.with(|c| c.as_ref().map(f)).ok_or(SyscallError::NotFound) + } } /// Largest submission ring a process may ask for. @@ -359,11 +383,14 @@ pub fn create(depth: u32) -> Result<(InboxRef, u64), SyscallError> { shm_phys, shm, submission_size, - completion_size, - completion_tail: 0, pending: Vec::new(), owner_pid: pid, })), + completions: KernelLock::new(Some(Completions { + page: shm_phys, + completion_size, + completion_tail: 0, + })), watch: Watch::new(), }); Ok((InboxRef(inbox), shm_vaddr)) @@ -391,7 +418,7 @@ pub fn submit( } loop { - let (count, dropped) = inbox.with_state(|s| (s.completion_count(), s.dropped()))?; + let (count, dropped) = inbox.with_completions(|c| (c.completion_count(), c.dropped()))?; if count >= min_complete || min_complete == 0 { return Ok(count); @@ -418,7 +445,7 @@ pub fn submit( 0, WaitClass::Io, deadline, - || inbox.with_state(|s| s.completion_count()).map_or(true, |n| n >= min_complete), + || inbox.with_completions(|c| c.completion_count()).map_or(true, |n| n >= min_complete), ) .is_err() { diff --git a/kernel/src/irq_ring.rs b/kernel/src/irq_ring.rs index 6d2c7216522..4101b18bb67 100644 --- a/kernel/src/irq_ring.rs +++ b/kernel/src/irq_ring.rs @@ -12,17 +12,12 @@ use crate::scheduler::MAX_CPUS; /// Interrupt sources that drive scheduling; exhaustive, so a new variant requires updating every `match`. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum IrqSource { - Audio, - /// Every vector a claimed PCI function delivers on, coalesced into one - /// slot: the record only says a pass is owed, and `pcidev` keeps the - /// per-slot flag that says whose. - UserDev, Xhci, I8042, } impl IrqSource { - pub const COUNT: usize = 4; + pub const COUNT: usize = 2; } /// 64-byte aligned so two CPUs' slots never share a cache line. diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 0fc518dadb6..6d201122067 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1725,7 +1725,11 @@ pub fn take_record(slot: usize) -> Result, SyscallError> if IRQ[slot].faulted() { return Err(SyscallError::Io); } - Ok(IRQ[slot].take().map(|count| DeviceIrqRecord { count })) + let taken = IRQ[slot].take(); + if taken.is_some() && IRQ[slot].take_unannounced() { + log!("pcidev: slot {slot} took its first message on vector {:#x}", VECTORS[slot]); + } + Ok(taken.map(|count| DeviceIrqRecord { count })) } /// Whether a read of the claim answers at once: a message is waiting, or the @@ -1734,42 +1738,22 @@ pub fn has_irq(slot: usize) -> bool { IRQ[slot].armed() || IRQ[slot].faulted() } -/// Records one message. Called from the vector's ISR, so it takes no lock and -/// allocates nothing; `record.rs` owns the counting, and `kernel-loom` models -/// it against a concurrent reader. +/// Records one message and posts the claim's watch. Called from the vector's +/// handler, so it allocates nothing; `record.rs` owns the counting, and +/// `kernel-loom` models it against a concurrent reader. pub fn isr(slot: usize) { IRQ[slot].took(); -} - -/// Turn every message taken since the last pass into a wake. -/// -/// On the scheduler pass rather than in the ISR, like every other device in -/// this kernel: a wake takes the inbox lock and an ISR may not. -pub fn drain_pending() { - for (slot, irq) in IRQ.iter().enumerate() { - if !irq.take_pending() { - continue; - } - // A fault's wake is no message. - if !irq.faulted() && irq.take_unannounced() { - log!( - "pcidev: slot {slot} took its first message on vector {:#x}", - VECTORS[slot] - ); - } - WATCHES[slot].post(); - } + WATCHES[slot].post(); } /// The unit refused this function an access. /// -/// Called from the fault handler, which takes no lock: every call the claim -/// answers refuses from here on, its interrupt read included, and this CPU's -/// next scheduler pass wakes whoever waits on the claim to read that refusal — -/// the pass a message earns, posted the way its ISR posts it. +/// Called from the fault handler: every call the claim answers refuses from +/// here on, its interrupt read included, and the post wakes whoever waits on +/// the claim to read that refusal, as a message's does. pub fn note_fault(slot: usize) { IRQ[slot].fault(); - crate::irq_ring::isr_publish(crate::irq_ring::IrqSource::UserDev, crate::clock::nanos_since_boot()); + WATCHES[slot].post(); crate::preempt::set_need_resched(); } diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 51f27139a2d..aa3e2e21af8 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -6,20 +6,16 @@ //! not an ordering, and no guest test in this suite lands on it. //! //! **Two parties race**: the ISR, on whichever CPU the unit routed the message -//! to, and the holder reading its record through a syscall on any CPU. The -//! scheduler pass that turns a message into a wake runs on the ISR's own CPU -//! after it, so those two do not interleave. +//! to, and the holder reading its record through a syscall on any CPU. //! -//! **The invariant is that every message is counted exactly once, and owes -//! exactly one wake.** Both are read-modify-writes and neither is a load -//! followed by a store: a reader that loaded a count and then cleared it drops -//! every message the ISR recorded in between, and a driver that misses one -//! waits for a device that has already spoken. No ordering carries anything -//! across these words — each is the whole of what it says — so the orderings -//! here are `Relaxed` and the model is about the interleaving, **but for one -//! edge**: a fault arms the same wake a message does, and the pass that takes -//! that wake has to read the fault, so `pending` is released by [`Interrupt::fault`] -//! and acquired by [`Interrupt::take_pending`]. +//! **The invariant is that every message is counted exactly once.** The count +//! is a read-modify-write on both sides and never a load followed by a store: +//! a reader that loaded a count and then cleared it drops every message the ISR +//! recorded in between, and a driver that misses one waits for a device that +//! has already spoken. No ordering carries anything across these words — each +//! is the whole of what it says — so the orderings here are `Relaxed` and the +//! model is about the interleaving; the holder reads them after the wake the +//! claim's watch post owes it, which orders them. #[cfg(not(feature = "loom"))] use core::sync::atomic::{AtomicBool, AtomicU32, Ordering}; @@ -32,16 +28,13 @@ use loom::sync::atomic::{AtomicBool, AtomicU32, Ordering}; /// control for and `kernel-loom` is the model of. const ORDER: Ordering = Ordering::Relaxed; -/// The negative control: the two read-modify-writes become a load and a store, +/// The negative control: the read-modify-writes become a load and a store, /// which is the whole of what this record's design is. Never on in a kernel /// build. #[cfg(feature = "device-irq-lossy")] macro_rules! take_word { - ($word:expr, $empty:expr) => { - take_word!($word, $empty, ORDER) - }; - ($word:expr, $empty:expr, $order:expr) => {{ - let held = $word.load($order); + ($word:expr, $empty:expr) => {{ + let held = $word.load(ORDER); $word.store($empty, ORDER); held }}; @@ -49,10 +42,7 @@ macro_rules! take_word { #[cfg(not(feature = "device-irq-lossy"))] macro_rules! take_word { ($word:expr, $empty:expr) => { - take_word!($word, $empty, ORDER) - }; - ($word:expr, $empty:expr, $order:expr) => { - $word.swap($empty, $order) + $word.swap($empty, ORDER) }; } @@ -76,8 +66,6 @@ macro_rules! bump { pub struct Interrupt { /// Messages since the holder's last read. count: AtomicU32, - /// Set by the ISR, cleared by the scheduler pass that turns it into a wake. - pending: AtomicBool, /// The unit refused this function an access. Every call the claim answers /// refuses from here on: its bus mastering is gone, so a driver that kept /// going would be driving nothing. @@ -97,7 +85,6 @@ impl Interrupt { pub const fn new() -> Self { Self { count: AtomicU32::new(0), - pending: AtomicBool::new(false), faulted: AtomicBool::new(false), unannounced: AtomicBool::new(true), } @@ -109,7 +96,6 @@ impl Interrupt { pub fn new() -> Self { Self { count: AtomicU32::new(0), - pending: AtomicBool::new(false), faulted: AtomicBool::new(false), unannounced: AtomicBool::new(true), } @@ -122,12 +108,6 @@ impl Interrupt { /// two of these, and what it took plus what is left has to be what arrived. pub fn took(&self) { bump!(self.count); - // `swap` and not a store: [`Self::fault`] releases through this same - // word, and a plain write landing after that release in `pending`'s - // modification order ends the release sequence there — the pass that - // later takes the fault's wake would then synchronize with nothing. - // An RMW extends the sequence instead, whichever order it lands in. - self.pending.swap(true, ORDER); } /// The messages since the last read, or `None` for none. @@ -147,36 +127,19 @@ impl Interrupt { self.count.load(ORDER) != 0 } - /// Whether a wake is owed, taken at most once per message. Answers `true` - /// for the pass that owes it and `false` for every pass after. - /// - /// `swap` for the same reason as [`Self::take`]: two passes that both - /// loaded `true` would both wake one message's watchers. `Acquire`, so - /// the pass that takes a fault's wake reads [`Self::faulted`] set. - pub fn take_pending(&self) -> bool { - take_word!(self.pending, false, Ordering::Acquire) - } - /// Whether this is the first message this slot has taken. Answers `true` /// once per claim and `false` ever after, so a caller may log on it. /// - /// `swap` for [`Self::take_pending`]'s reason: two passes that both loaded - /// `true` would both announce one message. + /// `swap` for [`Self::take`]'s reason: two reads that both loaded `true` + /// would both announce one message. pub fn take_unannounced(&self) -> bool { take_word!(self.unannounced, false) } /// The unit refused this function an access. Called from the fault handler, - /// which takes no lock: every call the claim answers refuses from here on, - /// and a wake is owed as for a message, because a holder waiting on the - /// claim would otherwise wait for a function that can no longer speak. - /// - /// The wake is a `swap` and not a store: this one races a pass on another - /// CPU, and loom 0.7 lets a plain store be lost to a concurrent `swap`, which - /// C11 forbids, so a store here is a wake the model cannot show is owed. + /// which takes no lock: every call the claim answers refuses from here on. pub fn fault(&self) { self.faulted.store(true, ORDER); - self.pending.swap(true, Ordering::Release); } pub fn faulted(&self) -> bool { @@ -187,7 +150,6 @@ impl Interrupt { /// up. The holder is not running at either point. pub fn clear(&self) { self.count.store(0, ORDER); - self.pending.store(false, ORDER); self.faulted.store(false, ORDER); self.unannounced.store(true, ORDER); } diff --git a/kernel/src/sched/driver.rs b/kernel/src/sched/driver.rs index 7b9797324a3..b560db87dd6 100644 --- a/kernel/src/sched/driver.rs +++ b/kernel/src/sched/driver.rs @@ -671,18 +671,6 @@ fn drain_irqs(entered: super::dump::Entered) { super::dump::serve_if_owed(); // Repaints the panel if whoever owns the screen has drawn over the report. crate::drivers::panic_console::hold_report(); - - if crate::irq_ring::take(crate::irq_ring::IrqSource::UserDev).is_some() { - // Which claim it was is the per-slot flag `pcidev` keeps; the record - // here says only that a pass is owed, so one function's interrupt does - // not wake every user driver in the machine. - crate::pcidev::drain_pending(); - } - if crate::irq_ring::take(crate::irq_ring::IrqSource::Audio).is_some() { - // Both backends share one watch, so a second would need the parking side - // to know which driver bound, which it doesn't. - crate::drivers::AUDIO_WATCH.post(); - } } /// Leave the current stack for this CPU's idle stack and never come back. @@ -715,6 +703,10 @@ extern "C" fn idle_loop() -> ! { if crate::drivers::panic_console::probe_due() { panic!("metal-panic-probe: a fatal report over a desktop that owns the screen"); } + #[cfg(feature = "boot-actuators")] + if crate::actuator::handler_post() { + crate::watch::handler_post::run(); + } crate::scheduler::log_health(); crate::scheduler::reap_finished(); // `pass` below covers this too; here as well so a CPU that diff --git a/kernel/src/sched/payload.rs b/kernel/src/sched/payload.rs index 523e2a6ede0..83664b1ef60 100644 --- a/kernel/src/sched/payload.rs +++ b/kernel/src/sched/payload.rs @@ -19,7 +19,10 @@ use crate::process::{OwnedAlloc, PageTables, ProcessAccounting, TaskId}; use crate::symbols::SymbolTable; use crate::sync::Lock; -/// The environment's leaf lock; holding it raises the preempt count, making a wake path a legal mailbox producer. +/// The environment's leaf lock, held with interrupts off, so an interrupt +/// handler may take one and never finds it held by the context it interrupted; +/// holding it raises the preempt count, making a wake path a legal mailbox +/// producer. pub struct KernelLock(Lock); impl KernelLock { @@ -30,7 +33,17 @@ impl KernelLock { impl LeafLock for KernelLock { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { - f(&mut self.0.lock()) + // Raised first, so the lock's own release never reaches depth zero, and + // a pass, with interrupts masked. + crate::sched::driver::preempt_off(|_| { + let _irq = crate::arch::IrqGuard::close(); + let mut held = self.0.lock(); + #[cfg(feature = "boot-actuators")] + crate::watch::handler_post::raise_if_staged(); + let out = f(&mut held); + drop(held); + out + }) } } diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index f8bf0aee262..bab1d6f6ccd 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -8,10 +8,10 @@ //! thread's state word, which its own next commit reads. Nothing here keeps a //! record of what was posted — a waiter re-reads the object, never the post. //! -//! **A post allocates nothing and may be made under any lock but a poll -//! ring's** (`crate::inbox`), so a driver posts from its scheduler-pass drain -//! and never from its ISR, which publishes to `irq_ring` and nothing else. -//! Registration allocates, in the syscall that registers. +//! **A post allocates and frees nothing and may be made under any lock but a +//! poll ring's** (`crate::inbox`), so a device's interrupt handler posts its +//! watch itself: every lock a post takes is a `KernelLock`, held with +//! interrupts off. Registration allocates, in the syscall that registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] //! holds it, so an object cannot be freed under a thread waiting on it, and @@ -54,6 +54,8 @@ impl Watch { } fn post_as(&self, cause: WakeCause) { + #[cfg(feature = "boot-actuators")] + handler_post::note_post(); preempt_off(|p| { let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; self.0.post(cause, &env); @@ -247,6 +249,81 @@ mod window { } } +/// `handler-post`: claim slot 0's vector, raised on this CPU inside a post of +/// that slot's own watch while the CPU holds preemption off, posts the watch +/// once the outer post lets go of it and before any pass can run. A hold counts +/// the posts made on its CPU: the outer one and the handler's are two, and a +/// hold that saw fewer by its budget lapsed. One run, on whichever idle loop +/// reaches it first; its verdict is one [`handler_post::SAID`] line. +#[cfg(feature = "boot-actuators")] +pub mod handler_post { + use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering::Relaxed}; + + use crate::time::{Budget, Deadline, Duration}; + + /// The verdict line's words; the counts follow them. + pub const SAID: &str = "handler-post:"; + const HOLDS: u32 = 4; + const WINDOW: Budget = Budget::of( + Duration::from_secs(1), + "the hold is counted as lapsed, and the verdict line says so", + ); + + const NOBODY: u32 = u32::MAX; + static RAN: AtomicBool = AtomicBool::new(false); + /// The CPU whose next leaf lock raises the vector inside itself. + static RAISE_INSIDE: AtomicU32 = AtomicU32::new(NOBODY); + static HOLDING: AtomicU32 = AtomicU32::new(NOBODY); + static POSTS: AtomicU64 = AtomicU64::new(0); + + pub fn run() { + if RAN.load(Relaxed) || RAN.swap(true, Relaxed) { + return; + } + let me = crate::arch::percpu::cpu_id(); + let (mut posted, mut lapsed) = (0, 0); + crate::sched::driver::preempt_off(|_| { + HOLDING.store(me, Relaxed); + for _ in 0..HOLDS { + let before = POSTS.load(Relaxed); + RAISE_INSIDE.store(me, Relaxed); + crate::pcidev::watch(0).post(); + let deadline = Deadline::at(crate::clock::now() + WINDOW.duration()); + loop { + if POSTS.load(Relaxed) >= before + 2 { + posted += 1; + break; + } + if deadline.reached(crate::clock::now()) { + lapsed += 1; + break; + } + core::hint::spin_loop(); + } + } + HOLDING.store(NOBODY, Relaxed); + }); + crate::log!("{SAID} {HOLDS} holds, {posted} posted into by a handler, {lapsed} lapsed"); + } + + /// From inside every `KernelLock`: the staged CPU's next one raises the + /// vector while it holds. + pub fn raise_if_staged() { + let staged = RAISE_INSIDE.load(Relaxed); + if staged != NOBODY && staged == crate::arch::percpu::cpu_id() { + RAISE_INSIDE.store(NOBODY, Relaxed); + crate::arch::irqchip::send_self(crate::pcidev::VECTORS[0]); + } + } + + pub fn note_post() { + let holding = HOLDING.load(Relaxed); + if holding != NOBODY && holding == crate::arch::percpu::cpu_id() { + POSTS.fetch_add(1, Relaxed); + } + } +} + /// Register, then park until `ready()` holds, for a wait a kill may not end /// and no deadline bounds. #[track_caller] diff --git a/src/ci.rs b/src/ci.rs index 362947a895c..08526614892 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -278,7 +278,6 @@ pub(crate) const CONTROLS: &[Control] = &[ ]), red(KERNEL_LOOM, "device-irq-lossy", Some("device_irq"), &[ "every_message_is_counted_once ... FAILED", - "one_message_is_one_wake ... FAILED", ]), red(KERNEL_LOOM, "dump-report-relaxed", Some("dump_request"), &[ "a_request_filed_during_a_report_is_reported ... FAILED", diff --git a/tests/toyos.rs b/tests/toyos.rs index b86007fbf0f..3aa3be06b26 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -1279,6 +1279,11 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // waiter between reading its condition and parking, so the peer's post lands where // only the notified bit carries it to the commit. ("blocking_read_window", Sched::Parallel, Tier::Nightly), + // A claimed function's vector posts its watch from the handler: raised + // inside a post of that watch on a CPU holding preemption off, it posts + // once the outer post lets go and before any pass. One boot; the verdict + // is counts. + ("handler_post_without_a_pass", Sched::Parallel, Tier::Fast), // A sibling's munmap and mmap staged between a typed copy's translation // and its store (`copy-meets-a-remap`): the store never reaches the region // mapped after it. @@ -12335,6 +12340,19 @@ fn run_machine_test( "smp_failed_ap_leaves_no_hole" => { smp_failed_ap_leaves_no_hole(test_config, c_bins, rust_bins) } + "handler_post_without_a_pass" => { + let mut qemu = QemuInstance::boot_with_options( + test_config, + c_bins, + rust_bins, + BootOptions { kernel_params: &["handler-post"], ..Default::default() }, + ); + let mut log = qemu.boot_log().to_string(); + if !log.lines().any(handler_post_said) { + log += &qemu.drain_until(Duration::from_secs(30), handler_post_said); + } + handler_post(&log) + } "input_merge" => { // The check runs in the kernel and panics on mismatch, so a // failure arrives as a dead boot; the marker is the only proof it @@ -14835,6 +14853,29 @@ fn sysret_ss(log: &str) -> Result<(), String> { Ok(()) } +/// `kernel/src/watch.rs`'s `handler_post::SAID`, and the counts every hold +/// posted into gives it. +const HANDLER_POST_SAID: &str = "handler-post:"; +const HANDLER_POST_POSTED: &str = "handler-post: 4 holds, 4 posted into by a handler, 0 lapsed"; + +fn handler_post_said(line: &str) -> bool { + line.contains(HANDLER_POST_SAID) +} + +/// Every hold was posted into by the handler of the vector raised inside it. +fn handler_post(log: &str) -> Result<(), String> { + let Some(said) = log.lines().find(|line| handler_post_said(line)) else { + return Err(format!("`handler-post` never said its verdict:\n{log}")); + }; + if !said.contains(HANDLER_POST_POSTED) { + return Err(format!( + "a hold lapsed with no handler's post in it — the wake waited for a pass:\n{said}\n{log}" + )); + } + eprintln!(" [handler-post] {}", said.trim()); + Ok(()) +} + /// The input core merged what it was handed. /// /// Text in, a verdict out: every line it reads is a kernel record, so the diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index 2b09968f570..529e375402f 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -49,6 +49,7 @@ use toyos_sched_loom::park::{prepare, Cancel, Commit, CurrentTask}; use toyos_sched_loom::task::{ Claim, Refused, TaskKey, TaskShared, TaskState, WaitClass, WakeCause, WakeReason, }; +use toyos_sched_loom::sync::LeafLock; use toyos_sched_loom::watch::{Fire, Gate, Poster, Ring, Waiters, Watch}; #[path = "../../../kernel/src/inbox/once.rs"] @@ -594,3 +595,120 @@ fn a_poll_on_two_watches_racing_both_posts_completes_exactly_once() { ); }); } + +/// A poll ring as the kernel's is: its completions behind a lock of its own, +/// and a watch its submitter parks on, which holds threads and no ring. +struct PollRing { + cpus: CpuHandles, + kicks: Kicks, + written: LoomLock, + parked: RingWatch, +} + +/// One of that ring's polls, registered on one device's watch. +struct RingPoll { + ring: Arc, + state: once::Once, +} + +struct RingEntry(Arc); + +type RingWatch = Watch>>; + +impl Ring for RingEntry { + fn fire(&self, _how: Fire) { + if self.0.state.fire() { + let ring = &self.0.ring; + ring.written.with(|n| *n += 1); + let env = Poster { cpus: &ring.cpus, kicker: &ring.kicks, preempt: &RemoteGuard }; + ring.parked.post(WakeCause::new(WakeReason::Woken), &env); + } + } + + fn live(&self) -> bool { + self.0.state.armed() + } +} + +/// **A post fires its rings under its own list lock**, so beneath it are the +/// ring's lock and the ring's watch. Two devices' watches each hold a poll of +/// one ring and are posted at once, while the ring's submitter waits for both +/// completions: the three locks nest in one order, so no schedule deadlocks, +/// each poll completes once, and a submitter parked with both completions +/// written was owed the wake the second one posted. +#[test] +fn two_posts_through_one_rings_lock_complete_it_once_each_and_lose_no_wake() { + model(|| { + let (tx, mut rx) = mailbox::(); + let ring = Arc::new(PollRing { + cpus: CpuHandles::new(vec![CpuHandle::new(CPU0, tx)]), + kicks: Kicks::new(), + written: LoomLock::new(0), + parked: Watch::new(watch_list()), + }); + let devices: Vec> = + (0..2).map(|_| Arc::new(Watch::new(watch_list()))).collect(); + for device in &devices { + device.add_ring(RingEntry(Arc::new(RingPoll { + ring: ring.clone(), + state: once::Once::new(), + }))); + } + let submitter = task(1); + + let waiting = { + let ring = ring.clone(); + let submitter = submitter.clone(); + loom::thread::spawn(move || { + ring.parked.register(&submitter, 0); + // Each post ends at most one iteration. + for _ in 0..4 { + if ring.written.with(|n| *n) == 2 { + return false; + } + let Ok(ticket) = + prepare(&CurrentTask::new(&submitter, CPU0), Cancel::Answers, WaitClass::Io) + else { + continue; + }; + match ticket.commit() { + Commit::Parked(_) => return true, + Commit::AlreadyWoken => continue, + Commit::Killed => unreachable!("nothing retires in this model"), + } + } + unreachable!("two posts ended more than three iterations") + }) + }; + let posters: Vec<_> = devices + .iter() + .map(|device| { + let (device, ring) = (device.clone(), ring.clone()); + loom::thread::spawn(move || { + let env = + Poster { cpus: &ring.cpus, kicker: &ring.kicks, preempt: &RemoteGuard }; + device.post(WakeCause::new(WakeReason::Woken), &env); + }) + }) + .collect(); + + let parked = waiting.join().unwrap(); + for poster in posters { + poster.join().unwrap(); + } + let guard = PreemptModel::new(); + let msgs = drain(&mut rx, &guard); + + assert_eq!(ring.written.with(|n| *n), 2, "a poll was completed other than once"); + if parked { + assert_eq!( + msgs, + [Msg::Wake(TaskKey(1), WakeReason::Woken)], + "parked with both completions written and no wake owed: a ring's post was lost", + ); + } else { + assert!(msgs.is_empty(), "a submitter that never parked is owed nothing: {msgs:?}"); + } + ring.parked.unregister(&submitter); + }); +} diff --git a/toyos-sched/src/sync.rs b/toyos-sched/src/sync.rs index b0e1a982716..41cd9564048 100644 --- a/toyos-sched/src/sync.rs +++ b/toyos-sched/src/sync.rs @@ -20,9 +20,9 @@ pub use loom::sync::Arc; /// Interior mutability for a small shared cell, supplied by the environment: /// a watch's waiter list and the per-process fair share. The kernel's -/// implementor is a few-instruction, IRQ-off leaf lock that acquires nothing -/// beneath it and is never held across a pass or a switch; the simulator and -/// the loom models supply their own. +/// implementor is a few-instruction, IRQ-off leaf lock that is never held +/// across a pass or a switch; the simulator and the loom models supply their +/// own. /// /// It lives here rather than in one of its users because the core crate may /// not implement a lock itself — that would need `unsafe`, which only diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index 96867851337..85da25652df 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -15,16 +15,17 @@ //! waiting side does between its registration and its park can be skipped: //! `park::prepare` consumes the flag, and the commit consumes the claim. //! -//! **A post allocates nothing.** Every ring entry is one-shot, so a post takes -//! them all out of the list and fires them with the list lock let go; what it -//! frees is those entries. A registration sweeps the entries a withdrawal left -//! behind, so the list never holds more than the polls live at its last post or -//! registration, plus that one. +//! **A post allocates and frees nothing, so an interrupt handler may make +//! one.** It fires every ring entry where it stands; an entry is one-shot, so a +//! fired one is dead, and the next registration sweeps the dead out. Nothing +//! allocates or frees under the list lock at all: a registration grows the list +//! and drops what it swept with the lock let go. //! -//! **Lock order.** The list lock is a leaf the environment supplies: -//! [`Ring::fire`] is always called with it let go, and so is the drop of every -//! entry the list lets go of, because an entry's last reference may own another -//! watch. So a post may be made under any lock but a ring's own. +//! **Lock order.** A post fires its rings under the list lock, so beneath it +//! are each ring's own lock and the watch that ring's submitters park on, which +//! holds threads and no ring and so nests nothing. The drop of every entry the +//! list lets go of runs with it let go, because an entry's last reference may +//! own another watch. So a post may be made under any lock but a ring's own. use alloc::vec::Vec; use core::marker::PhantomData; @@ -50,8 +51,10 @@ pub enum Fire { pub trait Ring { /// Post this poll's completion. One-shot across every watch the poll is /// registered on: an entry that already fired, or whose poll was withdrawn, - /// posts nothing. Called with no watch's list lock held; may take only its - /// ring's own lock and post only the watch its ring's submitters park on. + /// posts nothing. Called with at most the posting watch's list lock held, + /// from wherever a post is made, an interrupt handler included: may take + /// only its ring's own lock and post only the watch its ring's submitters + /// park on, and allocates and frees nothing. fn fire(&self, how: Fire); /// Whether a fire would still post anything. `false` is permanent. fn live(&self) -> bool; @@ -119,24 +122,18 @@ impl>> Watch { pub fn register(&self, task: &Arc>, token: u64) { assert!(task.set_waiting(), "a task waits on at most one watch"); task.forget_posts(); - let dead = self.list.with(|w| { - w.threads.push(Waiter { - task: task.clone(), - token, - }); - sweep(&mut w.rings) - }); - drop(dead); + self.sweep(); + self.push(Waiter { task: task.clone(), token }, |w| &mut w.threads); } /// End one wait. Idempotent against [`Self::revoke`], which may have taken /// the registration out already. pub fn unregister(&self, task: &Arc>) { - self.list.with(|w| { - if let Some(at) = w.threads.iter().position(|t| Arc::ptr_eq(&t.task, task)) { - w.threads.remove(at); - } + let gone = self.list.with(|w| { + let at = w.threads.iter().position(|t| Arc::ptr_eq(&t.task, task))?; + Some(w.threads.remove(at)) }); + drop(gone); task.clear_waiting(); } @@ -144,28 +141,74 @@ impl>> Watch { /// after this returns and fires the entry itself if the object is already /// ready — the ring's half of the same order a thread keeps. pub fn add_ring(&self, entry: R) { - let dead = self.list.with(|w| { - let dead = sweep(&mut w.rings); - w.rings.push(entry); - dead - }); - drop(dead); + self.sweep(); + self.push(entry, |w| &mut w.rings); } - /// Something changed: wake every registered thread, and fire and let go of - /// every ring entry. + /// Something changed: wake every registered thread, and fire every ring + /// entry where it stands. pub fn post(&self, cause: WakeCause, env: &Poster<'_, M, K, P>) { - let fired = self.list.with(|w| { + self.list.with(|w| { for waiter in &w.threads { notify(&waiter.task, cause, env.cpus, env.kicker, env.preempt); } - core::mem::take(&mut w.rings) + for ring in &w.rings { + ring.fire(Fire::Ready); + } }); - for ring in &fired { - ring.fire(Fire::Ready); + } + + /// Put `item` on the list `list` picks, growing it with the lock let go + /// when it is full. + fn push(&self, item: T, list: fn(&mut Waiters) -> &mut Vec) { + let mut item = item; + loop { + let full = self.list.with(|w| { + let v = list(w); + if v.len() < v.capacity() { + v.push(item); + return None; + } + Some((v.capacity(), item)) + }); + let Some((seen, back)) = full else { return }; + item = back; + // Swapped in unless another registration grew it first; whichever + // buffer loses is dropped out here. + let mut room = Vec::with_capacity((seen * 2).max(4)); + self.list.with(|w| { + let v = list(w); + if v.capacity() == seen { + room.append(v); + core::mem::swap(v, &mut room); + } + }); + drop(room); } } + /// Take out the ring entries that can no longer fire, and drop them — and + /// allocate the room they are taken into — with the lock let go. + fn sweep(&self) { + let dead = self.list.with(|w| w.rings.iter().filter(|r| !r.live()).count()); + if dead == 0 { + return; + } + let mut out = Vec::with_capacity(dead); + self.list.with(|w| { + let mut at = 0; + // Order is nothing to a ring entry: every post fires them all. + while at < w.rings.len() && out.len() < out.capacity() { + if w.rings[at].live() { + at += 1; + } else { + out.push(w.rings.swap_remove(at)); + } + } + }); + drop(out); + } + /// Wake at most `limit` threads registered with `token`, in registration /// order, and answer how many this post woke. A thread that was not parked /// is flagged and spends nothing: its own commit rechecks, and the post @@ -297,12 +340,6 @@ fn gate_fence() { } } -/// Take out the entries that can no longer fire, for the caller to drop once -/// the list lock is let go. -fn sweep(rings: &mut Vec) -> Vec { - rings.extract_if(.., |r| !r.live()).collect() -} - /// An object that ends with polls still registered answers them: dropping a /// watch fires every live ring entry as [`Fire::Gone`]. impl>> Drop for Watch { @@ -639,11 +676,29 @@ mod tests { withdrawn.withdraw(); let fresh = Arc::new(Poll::default()); w.add_ring(fresh); - assert_eq!(Arc::strong_count(&fired), 1, "a post lets go of what it fired"); + assert_eq!(Arc::strong_count(&fired), 1, "a fired entry is swept"); assert_eq!(Arc::strong_count(&withdrawn), 1, "a withdrawn entry is swept"); assert_eq!(w.live_rings(), 1); } + /// A post fires where the entry stands and lets go of nothing: the entry + /// it fired is still the list's until a registration sweeps it. + #[test] + fn a_post_fires_in_place_and_drops_nothing() { + let (handles, _rx) = cpus(); + let env = Poster { cpus: &handles, kicker: &NoKick, preempt: &NoPreempt }; + let w = watch(); + let poll = Arc::new(Poll::default()); + w.add_ring(poll.clone()); + w.post(woken(), &env); + assert_eq!(poll.posts.load(Ordering::Acquire), 1); + assert_eq!(Arc::strong_count(&poll), 2, "the post let go of the entry it fired"); + let t = task(1); + w.register(&t, 0); + assert_eq!(Arc::strong_count(&poll), 1, "the registration did not sweep it"); + w.unregister(&t); + } + #[test] fn cancel_and_drop_answer_every_live_poll_as_gone() { let w = watch(); diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs new file mode 100644 index 00000000000..9a0f23d5d18 --- /dev/null +++ b/toyos-sched/tests/watch_list.rs @@ -0,0 +1,177 @@ +//! What makes a post legal in an interrupt handler, which interrupts whoever +//! holds the allocator's lock: nothing allocates or frees under a watch's list +//! lock, and a post frees nothing at all. +//! +//! The allocator below counts every allocation and free this thread makes +//! while it holds a list lock, and every one it makes inside a post. + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::cell::Cell; +use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering::{AcqRel, Acquire, Relaxed}}; +use std::sync::{Arc, Mutex}; + +use toyos_sched::cpu::{CpuHandle, CpuHandles}; +use toyos_sched::hw::{CpuId, Kicker}; +use toyos_sched::mailbox::{mailbox, PreemptGuard, SchedMsg}; +use toyos_sched::park::{prepare, Cancel, Commit, CurrentTask}; +use toyos_sched::sync::LeafLock; +use toyos_sched::task::{TaskKey, TaskShared, TaskState, WaitClass, WakeCause, WakeReason}; +use toyos_sched::watch::{Fire, Poster, Ring, Waiters, Watch}; + +thread_local! { + static HELD: Cell = const { Cell::new(false) }; + static POSTING: Cell = const { Cell::new(false) }; +} + +static UNDER_LOCK: AtomicUsize = AtomicUsize::new(0); +static IN_POST: AtomicUsize = AtomicUsize::new(0); + +struct Counting; + +impl Counting { + fn note(&self) { + // `try_with`: a thread allocates while its locals are torn down. + if HELD.try_with(Cell::get).unwrap_or(false) { + UNDER_LOCK.fetch_add(1, Relaxed); + } + if POSTING.try_with(Cell::get).unwrap_or(false) { + IN_POST.fetch_add(1, Relaxed); + } + } +} + +// SAFETY: every method forwards to `System` unchanged after a count. +unsafe impl GlobalAlloc for Counting { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + self.note(); + // SAFETY: the caller's contract, passed on. + unsafe { System.alloc(layout) } + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + self.note(); + // SAFETY: the caller's contract, passed on. + unsafe { System.dealloc(ptr, layout) } + } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 { + self.note(); + // SAFETY: the caller's contract, passed on. + unsafe { System.realloc(ptr, layout, size) } + } +} + +#[global_allocator] +static ALLOCATOR: Counting = Counting; + +/// The list lock, marking this thread as holding it. +struct Watched(Mutex); + +impl LeafLock for Watched { + fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { + let mut guard = self.0.lock().unwrap(); + let was = HELD.replace(true); + let out = f(&mut guard); + HELD.set(was); + out + } +} + +#[derive(Debug)] +enum Msg { + Wake, + Retire, +} + +impl SchedMsg for Msg { + fn wake(_key: TaskKey, _cause: WakeCause) -> Self { + Msg::Wake + } + fn retire(_shared: Arc>) -> Self { + Msg::Retire + } +} + +struct NoPreempt; + +// SAFETY: one thread and no scheduler, so nothing can deschedule a push. +unsafe impl PreemptGuard for NoPreempt {} + +struct NoKick; + +impl Kicker for NoKick { + fn kick(&self, _target: CpuId) {} +} + +/// 0 armed, 1 fired, 2 withdrawn. +#[derive(Default)] +struct Poll(AtomicU32); + +struct Entry(Arc); + +impl Ring for Entry { + fn fire(&self, _how: Fire) { + let _ = self.0 .0.compare_exchange(0, 1, AcqRel, Acquire); + } + fn live(&self) -> bool { + self.0 .0.load(Acquire) == 0 + } +} + +const C0: CpuId = CpuId(0); + +fn clean(phase: &str) { + assert_eq!(UNDER_LOCK.load(Relaxed), 0, "{phase} allocated or freed under the list lock"); + assert_eq!(IN_POST.load(Relaxed), 0, "{phase}: a post allocated or freed"); +} + +#[test] +fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_frees_nothing() { + let (tx, mut rx) = mailbox::(); + let cpus = CpuHandles::new(vec![CpuHandle::new(C0, tx)]); + let env = Poster { cpus: &cpus, kicker: &NoKick, preempt: &NoPreempt }; + let w: Watch>> = + Watch::new(Watched(Mutex::new(Waiters::new()))); + + // Nine of each grows both lists from nothing three times over. + let tasks: Vec<_> = + (0..9).map(|k| Arc::new(TaskShared::new(TaskKey(k), TaskState::Running(C0)))).collect(); + for (token, t) in tasks.iter().enumerate() { + w.register(t, token as u64); + let ticket = prepare(&CurrentTask::new(t, C0), Cancel::Answers, WaitClass::Other) + .expect("nothing has posted yet"); + assert!(matches!(ticket.commit(), Commit::Parked(_))); + } + let polls: Vec<_> = (0..9).map(|_| Arc::new(Poll::default())).collect(); + for poll in &polls { + w.add_ring(Entry(poll.clone())); + } + clean("registering"); + + POSTING.set(true); + w.post(WakeCause::new(WakeReason::Woken), &env); + POSTING.set(false); + clean("posting"); + assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post fired every entry"); + + // Every entry is dead now, and a withdrawn one joins them: these two + // registrations sweep all ten. + let withdrawn = Arc::new(Poll::default()); + w.add_ring(Entry(withdrawn.clone())); + withdrawn.0.store(2, Relaxed); + w.add_ring(Entry(Arc::new(Poll::default()))); + clean("sweeping"); + assert!(polls.iter().all(|p| Arc::strong_count(p) == 1), "the sweep let go of every fired entry"); + + assert_eq!(w.post_n(3, 1, WakeCause::new(WakeReason::Woken), &env), 0, "every waiter is claimed"); + assert_eq!(w.revoke(|token| token % 2 == 0, WakeCause::new(WakeReason::Woken), &env), 5); + for t in &tasks { + w.unregister(t); + } + w.cancel_rings(); + clean("ending"); + + while rx.pop(&NoPreempt).is_some() {} + drop(w); + clean("dropping"); +} From 44fe28316df017d7e7970e1bb74b4fa0f74850c2 Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 29 Sep 2026 23:15:05 +0200 Subject: [PATCH 02/14] handler-post holds only on a CPU that takes interrupts A hold claims that a vector taken while its CPU holds preemption off posts before any pass. That claim is about a CPU with interrupts open: on one with them masked the raised vector is not delivered at all, and every hold would lapse on a correct kernel too. So `run` starts on the first idle trip whose interrupts are open, rather than on the first idle trip. Co-Authored-By: Claude Opus 5.5 --- kernel/src/watch.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index bab1d6f6ccd..16765a83db2 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -254,7 +254,8 @@ mod window { /// once the outer post lets go of it and before any pass can run. A hold counts /// the posts made on its CPU: the outer one and the handler's are two, and a /// hold that saw fewer by its budget lapsed. One run, on whichever idle loop -/// reaches it first; its verdict is one [`handler_post::SAID`] line. +/// reaches it first with interrupts open; its verdict is one +/// [`handler_post::SAID`] line. #[cfg(feature = "boot-actuators")] pub mod handler_post { use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering::Relaxed}; @@ -277,7 +278,8 @@ pub mod handler_post { static POSTS: AtomicU64 = AtomicU64::new(0); pub fn run() { - if RAN.load(Relaxed) || RAN.swap(true, Relaxed) { + // A hold is a CPU that takes interrupts while it holds. + if RAN.load(Relaxed) || !crate::arch::cpu::interrupts_enabled() || RAN.swap(true, Relaxed) { return; } let me = crate::arch::percpu::cpu_id(); From be715c7b5e74795d83d4adacb8401a5e24e98dae Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 13:14:09 +0200 Subject: [PATCH 03/14] A thread's post fires its polls with the list let go; only a handler posts in place Firing every ring entry under the list lock made each post an interrupts-off walk over that watch's polls, now that the list lock masks interrupts. A process controls how many polls sit on its own pipe's watch, so it controlled that window for the whole CPU. Before this branch the walk ran after the lock was let go, preemptible between fires. So there are two posts again, each legal where the other is not: - `Watch::post`, from a thread: wakes the threads and takes the ring entries out under the lock, then fires and frees them with it let go, as on main. - `Watch::post_in_place`, from an interrupt handler, which may not free: fires every entry where it stands under the lock and frees nothing, and the next registration sweeps what it fired. Its cost is every registration on the watch, so a handler posts only a watch whose registrations are its device's holder's own: a claim's, whose handle has no DUP, and the audio watch. The claim, fault and audio handlers post in place, and so does a ring's completion, which a handler's post reaches through the poll it fires. That ring's watch holds threads and no ring. `handler-post` now counts only slot 0's watch. Counting every post on the CPU let a poll the outer post completed, whose ring's watch it posts, stand in for the handler's post on a kernel whose handler posts nothing. `toyos-sched/tests/watch_list.rs` holds `post_in_place` to freeing nothing and every path to allocating and freeing nothing under the lock, the thread post included. loom runs the registration race over both posts, the two-ring nesting model is `post_in_place`'s, and the host job's `poll-fire-load-store` and `commit-ignores-notify` controls now also expect the red of each new model. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- kernel/src/drivers/hda.rs | 2 +- kernel/src/drivers/virtio_sound.rs | 2 +- kernel/src/inbox/mod.rs | 14 ++--- kernel/src/pcidev/mod.rs | 4 +- kernel/src/watch.rs | 37 +++++++++---- src/ci.rs | 2 + toyos-sched/loom/tests/loom_watch.rs | 79 +++++++++++++++++----------- toyos-sched/src/watch.rs | 62 +++++++++++++++------- toyos-sched/tests/watch_list.rs | 21 +++++--- 9 files changed, 145 insertions(+), 78 deletions(-) diff --git a/kernel/src/drivers/hda.rs b/kernel/src/drivers/hda.rs index 00d26ed8f8b..7a73ceaaec8 100644 --- a/kernel/src/drivers/hda.rs +++ b/kernel/src/drivers/hda.rs @@ -166,7 +166,7 @@ pub fn isr_complete() { } ISR.timestamp.store(timestamp, Ordering::Relaxed); ISR.mask.fetch_or(mask, Ordering::Release); - super::AUDIO_WATCH.post(); + super::AUDIO_WATCH.post_in_place(); crate::preempt::set_need_resched(); } diff --git a/kernel/src/drivers/virtio_sound.rs b/kernel/src/drivers/virtio_sound.rs index 7f170f13d41..b8470ebba36 100644 --- a/kernel/src/drivers/virtio_sound.rs +++ b/kernel/src/drivers/virtio_sound.rs @@ -108,7 +108,7 @@ pub fn isr_complete() { return; } isr_push_completion(mask, timestamp); - super::AUDIO_WATCH.post(); + super::AUDIO_WATCH.post_in_place(); // soundd, woken on this CPU, runs at the interrupt's exit, not at the next tick. crate::preempt::set_need_resched(); } diff --git a/kernel/src/inbox/mod.rs b/kernel/src/inbox/mod.rs index a98fecae3d2..7efb1e0e995 100644 --- a/kernel/src/inbox/mod.rs +++ b/kernel/src/inbox/mod.rs @@ -20,11 +20,11 @@ //! entry, and a ring holds at most [`MAX_PENDING_WATCHES`] polls. //! //! **Locks.** What a completion writes sits behind a `KernelLock` of its own, -//! held with interrupts off, because a post fires its polls under its list lock -//! and a device's handler posts; nothing is taken under it. The rest of a ring, -//! its submissions and its polls, is its `Lock`'s, which no post reaches. A -//! ring's own watch holds only threads, because no handle names a ring as a -//! thing to watch. +//! held with interrupts off, because a device's interrupt handler posts in +//! place, firing its polls under its list lock; nothing is taken under it. The +//! rest of a ring, its submissions and its polls, is its `Lock`'s, which no +//! post reaches. A ring's own watch holds only threads, because no handle names +//! a ring as a thing to watch. use alloc::sync::Arc; use alloc::vec::Vec; @@ -314,7 +314,9 @@ impl Inbox { .completions .with(|c| c.as_mut().map(|c| c.post_completion(user_data, result, 0))); if posted.is_some() { - self.watch.post(); + // In place: an interrupt handler's post reaches here through the + // poll it fires. + self.watch.post_in_place(); } } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 6d201122067..49a7b89d397 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1743,7 +1743,7 @@ pub fn has_irq(slot: usize) -> bool { /// `kernel-loom` models it against a concurrent reader. pub fn isr(slot: usize) { IRQ[slot].took(); - WATCHES[slot].post(); + WATCHES[slot].post_in_place(); } /// The unit refused this function an access. @@ -1753,7 +1753,7 @@ pub fn isr(slot: usize) { /// the claim to read that refusal, as a message's does. pub fn note_fault(slot: usize) { IRQ[slot].fault(); - WATCHES[slot].post(); + WATCHES[slot].post_in_place(); crate::preempt::set_need_resched(); } diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index 16765a83db2..b048c3cd546 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -8,10 +8,11 @@ //! thread's state word, which its own next commit reads. Nothing here keeps a //! record of what was posted — a waiter re-reads the object, never the post. //! -//! **A post allocates and frees nothing and may be made under any lock but a -//! poll ring's** (`crate::inbox`), so a device's interrupt handler posts its -//! watch itself: every lock a post takes is a `KernelLock`, held with -//! interrupts off. Registration allocates, in the syscall that registers. +//! **A post allocates nothing and may be made under any lock but a poll +//! ring's** (`crate::inbox`); a post in place frees nothing either, so a +//! device's interrupt handler makes one: every lock it takes is a `KernelLock`, +//! held with interrupts off. Registration allocates, in the syscall that +//! registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] //! holds it, so an object cannot be freed under a thread waiting on it, and @@ -55,13 +56,25 @@ impl Watch { fn post_as(&self, cause: WakeCause) { #[cfg(feature = "boot-actuators")] - handler_post::note_post(); + handler_post::note_post(self); preempt_off(|p| { let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; self.0.post(cause, &env); }); } + /// [`Self::post`] for an interrupt handler, which may not free: every poll + /// is completed where it stands, with interrupts off, so a handler posts + /// only a watch whose registrations are its device's holder's own. + pub fn post_in_place(&self) { + #[cfg(feature = "boot-actuators")] + handler_post::note_post(self); + preempt_off(|p| { + let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; + self.0.post_in_place(WakeCause::new(WakeReason::Woken), &env); + }); + } + /// Wake at most `limit` threads waiting with `token`, and answer how many /// were parked; one that was not is told anyway and spends nothing. pub fn post_n(&self, token: u64, limit: usize) -> usize { @@ -252,9 +265,10 @@ mod window { /// `handler-post`: claim slot 0's vector, raised on this CPU inside a post of /// that slot's own watch while the CPU holds preemption off, posts the watch /// once the outer post lets go of it and before any pass can run. A hold counts -/// the posts made on its CPU: the outer one and the handler's are two, and a -/// hold that saw fewer by its budget lapsed. One run, on whichever idle loop -/// reaches it first with interrupts open; its verdict is one +/// the posts of that watch made on its CPU, and no other watch's, which a poll +/// the outer post completes also posts: the outer one and the handler's are +/// two, and a hold that saw fewer by its budget lapsed. One run, on whichever +/// idle loop reaches it first with interrupts open; its verdict is one /// [`handler_post::SAID`] line. #[cfg(feature = "boot-actuators")] pub mod handler_post { @@ -318,9 +332,12 @@ pub mod handler_post { } } - pub fn note_post() { + pub fn note_post(watch: &super::Watch) { let holding = HOLDING.load(Relaxed); - if holding != NOBODY && holding == crate::arch::percpu::cpu_id() { + if holding != NOBODY + && holding == crate::arch::percpu::cpu_id() + && core::ptr::eq(watch, crate::pcidev::watch(0)) + { POSTS.fetch_add(1, Relaxed); } } diff --git a/src/ci.rs b/src/ci.rs index 08526614892..a9ef44aa202 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -299,6 +299,7 @@ pub(crate) const CONTROLS: &[Control] = &[ // with. A double panic, so the verdict is the first one's message. red(SCHED_LOOM, "commit-ignores-notify", Some("loom_watch"), &[ "parked with the condition true and no wake owed: the post was lost", + "parked with both completions written and no wake owed: a ring's post was lost", ]), // The notify's flagged arm answering off a load: a second post reads the // word from before the waiter consumed the first flag. @@ -313,6 +314,7 @@ pub(crate) const CONTROLS: &[Control] = &[ // ring entry. red(SCHED_LOOM, "poll-fire-load-store", Some("loom_watch"), &[ "a_poll_registered_racing_a_post_completes_exactly_once ... FAILED", + "a_poll_registered_racing_a_post_in_place_completes_exactly_once ... FAILED", "a_poll_on_two_watches_racing_both_posts_completes_exactly_once ... FAILED", ]), // Reproduces an open defect diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index 529e375402f..1444e54dc1c 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -24,7 +24,7 @@ //! `gate-fence-off` removes the [`Gate`]'s two fences, and //! `a_transition_racing_an_opening_gate_is_never_missed` must red; and //! `poll-fire-load-store`, the kernel's own control for the poll's one-shot -//! answer, which the ring models below compile, must red both poll models here. +//! answer, which the ring models below compile, must red every poll model here. //! //! **The ring entry is the kernel's [`Once`], compiled from //! `kernel/src/inbox/once.rs`**, the decision a `PollEntry` makes; what else a @@ -110,6 +110,15 @@ impl World { self.watch.post(WakeCause::new(WakeReason::Woken), &env); } + fn post_in_place(&self) { + let env = Poster { + cpus: &self.cpus, + kicker: &self.kicks, + preempt: &RemoteGuard, + }; + self.watch.post_in_place(WakeCause::new(WakeReason::Woken), &env); + } + fn post_one(&self, token: u64) -> usize { let env = Poster { cpus: &self.cpus, @@ -275,31 +284,39 @@ fn a_bounded_post_racing_a_timeout_reaches_a_live_waiter() { /// and never by both, which is a completion the process did not ask for. #[test] fn a_poll_registered_racing_a_post_completes_exactly_once() { - model(|| { - let (world, _rx) = world(); - let ready = Arc::new(AtomicBool::new(false)); - let poll = Entry::new(); + model(|| poll_racing(World::post)); +} - let registrant = { - let world = world.clone(); - let ready = ready.clone(); - let poll = poll.clone(); - loom::thread::spawn(move || { - world.watch.add_ring(poll.clone()); - if ready.load(Ordering::Acquire) { - poll.fire(Fire::Ready); - } - }) - }; - let producer = loom::thread::spawn(move || { - ready.store(true, Ordering::Release); - world.post(); - }); - registrant.join().unwrap(); - producer.join().unwrap(); +/// The same, against the post an interrupt handler makes. +#[test] +fn a_poll_registered_racing_a_post_in_place_completes_exactly_once() { + model(|| poll_racing(World::post_in_place)); +} - assert_eq!(poll.posts(), 1, "a poll over a ready object completes once"); +fn poll_racing(post: fn(&World)) { + let (world, _rx) = world(); + let ready = Arc::new(AtomicBool::new(false)); + let poll = Entry::new(); + + let registrant = { + let world = world.clone(); + let ready = ready.clone(); + let poll = poll.clone(); + loom::thread::spawn(move || { + world.watch.add_ring(poll.clone()); + if ready.load(Ordering::Acquire) { + poll.fire(Fire::Ready); + } + }) + }; + let producer = loom::thread::spawn(move || { + ready.store(true, Ordering::Release); + post(&world); }); + registrant.join().unwrap(); + producer.join().unwrap(); + + assert_eq!(poll.posts(), 1, "a poll over a ready object completes once"); } /// The object's end racing its readiness: the poll is answered once, as ready @@ -621,7 +638,7 @@ impl Ring for RingEntry { let ring = &self.0.ring; ring.written.with(|n| *n += 1); let env = Poster { cpus: &ring.cpus, kicker: &ring.kicks, preempt: &RemoteGuard }; - ring.parked.post(WakeCause::new(WakeReason::Woken), &env); + ring.parked.post_in_place(WakeCause::new(WakeReason::Woken), &env); } } @@ -630,12 +647,12 @@ impl Ring for RingEntry { } } -/// **A post fires its rings under its own list lock**, so beneath it are the -/// ring's lock and the ring's watch. Two devices' watches each hold a poll of -/// one ring and are posted at once, while the ring's submitter waits for both -/// completions: the three locks nest in one order, so no schedule deadlocks, -/// each poll completes once, and a submitter parked with both completions -/// written was owed the wake the second one posted. +/// **A post in place fires its rings under its own list lock**, so beneath it +/// are the ring's lock and the ring's watch. Two devices' watches each hold a +/// poll of one ring and are posted at once, while the ring's submitter waits +/// for both completions: the three locks nest in one order, so no schedule +/// deadlocks, each poll completes once, and a submitter parked with both +/// completions written was owed the wake the second one posted. #[test] fn two_posts_through_one_rings_lock_complete_it_once_each_and_lose_no_wake() { model(|| { @@ -687,7 +704,7 @@ fn two_posts_through_one_rings_lock_complete_it_once_each_and_lose_no_wake() { loom::thread::spawn(move || { let env = Poster { cpus: &ring.cpus, kicker: &ring.kicks, preempt: &RemoteGuard }; - device.post(WakeCause::new(WakeReason::Woken), &env); + device.post_in_place(WakeCause::new(WakeReason::Woken), &env); }) }) .collect(); diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index 85da25652df..10014578ddf 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -15,17 +15,20 @@ //! waiting side does between its registration and its park can be skipped: //! `park::prepare` consumes the flag, and the commit consumes the claim. //! -//! **A post allocates and frees nothing, so an interrupt handler may make -//! one.** It fires every ring entry where it stands; an entry is one-shot, so a -//! fired one is dead, and the next registration sweeps the dead out. Nothing -//! allocates or frees under the list lock at all: a registration grows the list -//! and drops what it swept with the lock let go. +//! **Nothing allocates or frees under the list lock**, because an interrupt +//! handler's post can interrupt the allocator's holder while another CPU holds +//! the list waiting for the allocator. A registration grows the list and drops +//! what it swept with the lock let go. [`Watch::post`] takes every ring entry +//! out and fires and frees them with the lock let go; [`Watch::post_in_place`], +//! for a handler, which may not free at all, fires them where they stand, and +//! the next registration sweeps the dead out, since an entry is one-shot. //! -//! **Lock order.** A post fires its rings under the list lock, so beneath it -//! are each ring's own lock and the watch that ring's submitters park on, which -//! holds threads and no ring and so nests nothing. The drop of every entry the -//! list lets go of runs with it let go, because an entry's last reference may -//! own another watch. So a post may be made under any lock but a ring's own. +//! **Lock order.** A post in place fires its rings under the list lock, so +//! beneath it are each ring's own lock and the watch that ring's submitters +//! park on, which holds threads and no ring and so nests nothing. The drop of +//! every entry the list lets go of runs with it let go, because an entry's last +//! reference may own another watch. So a post may be made under any lock but a +//! ring's own. use alloc::vec::Vec; use core::marker::PhantomData; @@ -52,9 +55,9 @@ pub trait Ring { /// Post this poll's completion. One-shot across every watch the poll is /// registered on: an entry that already fired, or whose poll was withdrawn, /// posts nothing. Called with at most the posting watch's list lock held, - /// from wherever a post is made, an interrupt handler included: may take - /// only its ring's own lock and post only the watch its ring's submitters - /// park on, and allocates and frees nothing. + /// and from an interrupt handler by a post in place: may take only its + /// ring's own lock and post only the watch its ring's submitters park on, + /// and allocates and frees nothing. fn fire(&self, how: Fire); /// Whether a fire would still post anything. `false` is permanent. fn live(&self) -> bool; @@ -145,9 +148,28 @@ impl>> Watch { self.push(entry, |w| &mut w.rings); } - /// Something changed: wake every registered thread, and fire every ring - /// entry where it stands. + /// Something changed: wake every registered thread, and fire and let go of + /// every ring entry, with the list lock let go. pub fn post(&self, cause: WakeCause, env: &Poster<'_, M, K, P>) { + let fired = self.list.with(|w| { + for waiter in &w.threads { + notify(&waiter.task, cause, env.cpus, env.kicker, env.preempt); + } + core::mem::take(&mut w.rings) + }); + for ring in &fired { + ring.fire(Fire::Ready); + } + } + + /// [`Self::post`] for a context that may not free, an interrupt handler: + /// every ring entry is fired where it stands, under the list lock, so the + /// cost is every registration on the watch with that lock held. + pub fn post_in_place( + &self, + cause: WakeCause, + env: &Poster<'_, M, K, P>, + ) { self.list.with(|w| { for waiter in &w.threads { notify(&waiter.task, cause, env.cpus, env.kicker, env.preempt); @@ -676,21 +698,21 @@ mod tests { withdrawn.withdraw(); let fresh = Arc::new(Poll::default()); w.add_ring(fresh); - assert_eq!(Arc::strong_count(&fired), 1, "a fired entry is swept"); + assert_eq!(Arc::strong_count(&fired), 1, "a post lets go of what it fired"); assert_eq!(Arc::strong_count(&withdrawn), 1, "a withdrawn entry is swept"); assert_eq!(w.live_rings(), 1); } - /// A post fires where the entry stands and lets go of nothing: the entry - /// it fired is still the list's until a registration sweeps it. + /// A post in place fires where the entry stands and lets go of nothing: + /// the entry it fired is still the list's until a registration sweeps it. #[test] - fn a_post_fires_in_place_and_drops_nothing() { + fn a_post_in_place_fires_and_drops_nothing() { let (handles, _rx) = cpus(); let env = Poster { cpus: &handles, kicker: &NoKick, preempt: &NoPreempt }; let w = watch(); let poll = Arc::new(Poll::default()); w.add_ring(poll.clone()); - w.post(woken(), &env); + w.post_in_place(woken(), &env); assert_eq!(poll.posts.load(Ordering::Acquire), 1); assert_eq!(Arc::strong_count(&poll), 2, "the post let go of the entry it fired"); let t = task(1); diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs index 9a0f23d5d18..d797c5fd590 100644 --- a/toyos-sched/tests/watch_list.rs +++ b/toyos-sched/tests/watch_list.rs @@ -1,9 +1,9 @@ //! What makes a post legal in an interrupt handler, which interrupts whoever //! holds the allocator's lock: nothing allocates or frees under a watch's list -//! lock, and a post frees nothing at all. +//! lock, and a post in place frees nothing at all. //! //! The allocator below counts every allocation and free this thread makes -//! while it holds a list lock, and every one it makes inside a post. +//! while it holds a list lock, and every one it makes inside a post in place. use std::alloc::{GlobalAlloc, Layout, System}; use std::cell::Cell; @@ -122,11 +122,11 @@ const C0: CpuId = CpuId(0); fn clean(phase: &str) { assert_eq!(UNDER_LOCK.load(Relaxed), 0, "{phase} allocated or freed under the list lock"); - assert_eq!(IN_POST.load(Relaxed), 0, "{phase}: a post allocated or freed"); + assert_eq!(IN_POST.load(Relaxed), 0, "{phase}: a post in place allocated or freed"); } #[test] -fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_frees_nothing() { +fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_nothing() { let (tx, mut rx) = mailbox::(); let cpus = CpuHandles::new(vec![CpuHandle::new(C0, tx)]); let env = Poster { cpus: &cpus, kicker: &NoKick, preempt: &NoPreempt }; @@ -149,10 +149,10 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_frees_nothing() { clean("registering"); POSTING.set(true); - w.post(WakeCause::new(WakeReason::Woken), &env); + w.post_in_place(WakeCause::new(WakeReason::Woken), &env); POSTING.set(false); - clean("posting"); - assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post fired every entry"); + clean("posting in place"); + assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post in place fired every entry"); // Every entry is dead now, and a withdrawn one joins them: these two // registrations sweep all ten. @@ -163,6 +163,13 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_frees_nothing() { clean("sweeping"); assert!(polls.iter().all(|p| Arc::strong_count(p) == 1), "the sweep let go of every fired entry"); + // A thread's post frees what it fired, with the lock let go. + let later = Arc::new(Poll::default()); + w.add_ring(Entry(later.clone())); + w.post(WakeCause::new(WakeReason::Woken), &env); + clean("posting"); + assert_eq!(Arc::strong_count(&later), 1, "the post let go of the entry it fired"); + assert_eq!(w.post_n(3, 1, WakeCause::new(WakeReason::Woken), &env), 0, "every waiter is claimed"); assert_eq!(w.revoke(|token| token % 2 == 0, WakeCause::new(WakeReason::Woken), &env), 5); for t in &tasks { From 8cd2948df8ca8e3cc5a3d9c44de304b48e39ce71 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 13:18:20 +0200 Subject: [PATCH 04/14] Stage 6 step 1's plan line loses its claim that a post frees nothing Only a post in place frees nothing now; a thread's post frees what it fired, with the list let go. The clause is deleted, not reworded: the module header of `toyos-sched/src/watch.rs` is where the two posts are stated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- .../the-kernel-is-small-interrupts-post-and-threads-wait.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 69869726f79..facc4c6e52d 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -152,8 +152,7 @@ times: interrupts-off and preemption-off windows, against stage 6's first commit. 1. **Interrupts post.** A post is legal in a handler: a watch's list and a poll ring's completions sit behind interrupts-off leaf locks nothing - allocates or frees under, and a post frees nothing, since the next - registration sweeps what it fired. A claimed function's vector, the + allocates or frees under. A claimed function's vector, the IOMMU's refusal and both audio backends post from the handler, and `irq_ring`'s `UserDev` and `Audio` and their arms in `drain_irqs` go. The thread is the holder's: netd's, blockd's, soundd's mix thread, and From bee8c9d16fc979ebe4432dcd29b6801822b15ddf Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 13:27:50 +0200 Subject: [PATCH 05/14] Stage 6 step 5's exit is what the step can reach Its exit said `pass` and the idle loop call no driver, while the same step keeps the TCO feed in the pass and makes the blocked-task dump the pass's own, and the dump paints and holds its report on the panel. The exit now names what the stage's own sentence deletes, `drain_irqs` and the idle loop's device checks, and whether the dump's panel stays with the pass is recorded as the owner's ruling. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...he-kernel-is-small-interrupts-post-and-threads-wait.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index facc4c6e52d..9412a0554fc 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -174,9 +174,11 @@ times: and `port_work_pending` go with the kernel's driver, and `irq_ring` with them. **Exit**: step 10's. 5. **The pass is the scheduler's.** `drain_irqs` goes: the blocked-task - dump with its panel hold, and the heartbeat, become `pass`'s own, and - the TCO feed stays, since what it proves is that passes run. **Exit**: - `pass` and the idle loop call no driver, and both windows are measured + dump and the heartbeat become `pass`'s own, and the TCO feed stays, + since what it proves is that passes run. The dump still paints its + report on the panel and holds it there, a device the pass reaches; + whether that stays is the owner's ruling. **Exit**: `drain_irqs` and the + idle loop's device checks are gone, and both windows are measured against stage 6's start. ## Standing From a50015bd7c6220d65b25431e48403952771b61ab Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 16:00:57 +0200 Subject: [PATCH 06/14] Only the watches a handler posts mask interrupts, and they cannot free Answers the first review of #634 (issuecomment-5910966142), its code half. Masking is a type, not every lock. `KernelLock` is main's again, a plain preempt-raising lock with interrupts open, for every futex bucket, pipe, port, join, keyboard and log watch and for the fair share. A watch an interrupt handler reaches is `watch::IrqWatch`, whose list sits behind the new `watch::IrqLock` (preempt count raised, then `IrqGuard`, then the lock): the claim watches, `AUDIO_WATCH`, and a ring's own watch and its completions, which a handler's post reaches through the poll it fires. `Watch` and `IrqWatch` are one generic `Waitable`, so arming and waiting are written once. A handler cannot free: `IrqWatch` has `post_in_place` and no `post`, `post_n` or `revoke_range`, so a `.post()` written into a handler (the `isa::drain_pending` move #592 owes) is a compile error, E0599, rather than a same-CPU deadlock on the allocator's preempt-only lock. A ring's page is its completions'. The `Arc` moves from `RingState` into `Completions`, and the teardown unmaps through what it took out of the completions' lock, so the page cannot be freed while a post can still reach it: dropping that take, or moving it below the unmap, no longer compiles. `toyos-sched`'s registration is one section in the common case: it takes out up to `FEW` dead ring entries into a stack array (nothing to allocate), and pushes when there is room; a full list is grown in a buffer allocated with the lock let go, swapped in only if it still holds the whole list. A thread's `post` leaves the list's buffer in place when it fires `FEW` or fewer entries, so a re-arm after it does not regrow. `tests/watch_list.rs`'s lock counts its sections and can run a stage between two of them, and asserts the one-section re-arm after both posts, a sweep of more dead entries than one section takes, and a list that outgrew the buffer sized for it before that buffer came back. The rest of the review's notes: - `LeafLock` is `CellLock`: under `IrqLock` a post in place takes a ring's completions lock and its watch's. - `unregister` is main's shape again. - `IrqSource` pins `Xhci = 2, I8042 = 3`, the `IrqDrain` top byte traces already carry; the slots index through `slot()`. - `handler-post`'s words are `toyos_sched::watch::handler_post`'s, which the kernel prints and the harness compares whole, as `watch-window`'s are. It gains a second arm: the vector raised inside a completion written into a kernel-owned ring that polls the claim, so completions behind an interrupts-open lock deadlock the handler's post. - The two-ring loom model keeps only its lost-wake check, which `commit-ignores-notify` reds; its "once each" assertion could not fail and is deleted, as is the header's "no deadlock". - `poll_racing`'s readiness flag is `Relaxed` on both sides, as a claim's `faulted` is, so the model checks the order the kernel has: the list lock alone. - Deleted: "takes no lock" in `vtd/fault.rs` and `pcidev/record.rs`, the list of `IrqGuard` holders in `arch/x86_64/mod.rs`, "a few-instruction, IRQ-off leaf lock" in `toyos-sched/src/sync.rs`, and "runs at the interrupt's exit" in `user_dev.rs` and `virtio_sound.rs`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- kernel/src/actuator.rs | 5 +- kernel/src/arch/x86_64/idt/user_dev.rs | 2 - kernel/src/arch/x86_64/mod.rs | 6 +- kernel/src/arch/x86_64/vtd/fault.rs | 5 +- kernel/src/drivers/mod.rs | 2 +- kernel/src/drivers/virtio_sound.rs | 1 - kernel/src/inbox/mod.rs | 101 ++++++++---- kernel/src/irq_ring.rs | 19 ++- kernel/src/object/ops.rs | 27 +++- kernel/src/pcidev/mod.rs | 6 +- kernel/src/pcidev/record.rs | 6 +- kernel/src/sched/payload.rs | 21 +-- kernel/src/watch.rs | 204 +++++++++++++++++-------- tests/toyos.rs | 15 +- toyos-sched/loom/src/model.rs | 8 +- toyos-sched/loom/tests/loom_watch.rs | 19 +-- toyos-sched/sim/src/payload.rs | 4 +- toyos-sched/src/cpu.rs | 4 +- toyos-sched/src/fair.rs | 6 +- toyos-sched/src/sync.rs | 7 +- toyos-sched/src/task.rs | 6 +- toyos-sched/src/watch.rs | 157 +++++++++++-------- toyos-sched/tests/watch_list.rs | 120 ++++++++++++--- 23 files changed, 489 insertions(+), 262 deletions(-) diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index cb4dda3c2bd..951a39cea74 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -281,8 +281,9 @@ actuators! { /// parking, so a post lands in the window its commit must refuse the park over. watch_window = "watch-window"; - /// Raise claim slot 0's vector inside a post of its own watch while the CPU - /// holds preemption off, and count whether the handler posted it there. + /// Raise claim slot 0's vector inside a post of its own watch, and inside a + /// completion into a ring polling it, while the CPU holds preemption off, + /// and count whether the handler posted it there. handler_post = "handler-post"; /// Starve the four xHCI bring-up register waits in `init_one`. diff --git a/kernel/src/arch/x86_64/idt/user_dev.rs b/kernel/src/arch/x86_64/idt/user_dev.rs index ec24a26cc77..9ad2dccbc32 100644 --- a/kernel/src/arch/x86_64/idt/user_dev.rs +++ b/kernel/src/arch/x86_64/idt/user_dev.rs @@ -14,8 +14,6 @@ use super::device_irq::device_irq_entry; fn took(slot: usize) { crate::arch::percpu::irq_took!(UserDev); crate::pcidev::isr(slot); - // A holder the post woke on this CPU runs at the interrupt's exit, not at - // the next quantum tick. crate::preempt::set_need_resched(); crate::arch::apic::eoi(); } diff --git a/kernel/src/arch/x86_64/mod.rs b/kernel/src/arch/x86_64/mod.rs index 41ec8a0f805..1e565f53f79 100644 --- a/kernel/src/arch/x86_64/mod.rs +++ b/kernel/src/arch/x86_64/mod.rs @@ -52,10 +52,8 @@ pub const ELF_MACHINE: toyos_elf::Machine = toyos_elf::Machine::X86_64; /// Interrupts masked on this CPU for as long as the guard lives, and then put /// back as they were — restored, not enabled — so a guard nests inside a region -/// that is already masked. The one way this kernel masks and restores: the -/// scheduler's pass, a log record's reservation and publication, and the -/// console backend each hold one. `TF` is always clear in Ring 0, so the guard -/// leaves it alone. +/// that is already masked. The one way this kernel masks and restores. `TF` is +/// always clear in Ring 0, so the guard leaves it alone. /// /// Both edges are compiler barriers (no `nomem`): a memory access written /// inside the region is emitted inside it. diff --git a/kernel/src/arch/x86_64/vtd/fault.rs b/kernel/src/arch/x86_64/vtd/fault.rs index c0188b87c1e..3de98c0f382 100644 --- a/kernel/src/arch/x86_64/vtd/fault.rs +++ b/kernel/src/arch/x86_64/vtd/fault.rs @@ -1,6 +1,6 @@ //! Vt-d fault interrupt handling: MSI-delivered, never polled. //! -//! The handler is bounded, allocates nothing and takes no lock; unit and +//! The handler is bounded, allocates nothing; unit and //! function state lives in fixed arrays of atomics, written once before the //! mask comes off. Whatever the stream, the same things happen first: Bus //! Master Enable cleared on the function that faulted, the first record latched @@ -36,8 +36,7 @@ const FSTS_OVERFLOW: u32 = 1 << 0; // One fault recording register's F bit, in the 32-bit word that carries it. const RECORD_FAULT: u32 = 1 << 31; -// Atomics only: the handler takes no lock; each field is written once before -// the unit's mask comes off. +// Atomics only: each field is written once before the unit's mask comes off. struct FaultUnit { // Physical base of the register window; 0 means this slot is unused. // vtd::window refuses a base of 0, so no armed unit can collide with the sentinel. diff --git a/kernel/src/drivers/mod.rs b/kernel/src/drivers/mod.rs index 586cf961a4e..5069a5836fe 100644 --- a/kernel/src/drivers/mod.rs +++ b/kernel/src/drivers/mod.rs @@ -24,4 +24,4 @@ pub use crate::mm::DmaPool; /// What an audio read and an audio poll wait on: one for both backends, since /// at most one binds and the claim's reader cannot know which. -pub static AUDIO_WATCH: crate::watch::Watch = crate::watch::Watch::new(); +pub static AUDIO_WATCH: crate::watch::IrqWatch = crate::watch::IrqWatch::new(); diff --git a/kernel/src/drivers/virtio_sound.rs b/kernel/src/drivers/virtio_sound.rs index b8470ebba36..1268d095de7 100644 --- a/kernel/src/drivers/virtio_sound.rs +++ b/kernel/src/drivers/virtio_sound.rs @@ -109,7 +109,6 @@ pub fn isr_complete() { } isr_push_completion(mask, timestamp); super::AUDIO_WATCH.post_in_place(); - // soundd, woken on this CPU, runs at the interrupt's exit, not at the next tick. crate::preempt::set_need_resched(); } diff --git a/kernel/src/inbox/mod.rs b/kernel/src/inbox/mod.rs index 7efb1e0e995..7370f15e36b 100644 --- a/kernel/src/inbox/mod.rs +++ b/kernel/src/inbox/mod.rs @@ -19,29 +19,28 @@ //! process's ring or say something no object said. A post writes at most one //! entry, and a ring holds at most [`MAX_PENDING_WATCHES`] polls. //! -//! **Locks.** What a completion writes sits behind a `KernelLock` of its own, -//! held with interrupts off, because a device's interrupt handler posts in -//! place, firing its polls under its list lock; nothing is taken under it. The -//! rest of a ring, its submissions and its polls, is its `Lock`'s, which no -//! post reaches. A ring's own watch holds only threads, because no handle names -//! a ring as a thing to watch. +//! **Locks.** What a completion writes, and the page it is written into, sit +//! behind an [`IrqLock`] of their own, because a device's interrupt handler +//! posts in place, firing its polls under its list lock; nothing is taken +//! under it. The rest of a ring, its submissions and its polls, is its +//! `Lock`'s, which no post reaches. A ring's own watch is an [`IrqWatch`] and +//! holds only threads, because no handle names a ring as a thing to watch. use alloc::sync::Arc; use alloc::vec::Vec; use core::sync::atomic::Ordering; -use toyos_sched::sync::LeafLock; +use toyos_sched::sync::CellLock; use toyos_sched::task::WaitClass; use toyos_sched::watch::{Fire, Ring}; use crate::object::shm::SharedMemObject; use crate::object::{ops, KObjectRef}; use crate::process::{self, Pid}; -use crate::sched::payload::KernelLock; use crate::scheduler; use crate::sync::Lock; use crate::time::{Deadline, Duration}; -use crate::watch::Watch; +use crate::watch::{IrqLock, IrqWatch}; use crate::DirectMap; use toyos_abi::inbox::{ @@ -67,9 +66,12 @@ impl InboxRef { impl Drop for InboxRef { fn drop(&mut self) { - // First, so no post writes into the page this drop lets go of. - self.0.completions.with(|c| *c = None); - // Taken out under the lock and let go of outside it: the unmap flushes. + // The page is the completions', so it goes only once no post can reach + // them. Both halves are taken out under their locks and let go of + // outside them: the unmap flushes. + let Some(completions) = self.0.completions.with(Option::take) else { + unreachable!("an inbox is torn down by its one reference, once"); + }; let Some(mut state) = self.0.state.lock().take() else { unreachable!("an inbox is torn down by its one reference, once"); }; @@ -77,7 +79,7 @@ impl Drop for InboxRef { poll.withdraw(); } // `Unmapped`'s drop flushes; the `Arc` drop after it frees the pages. - drop(state.shm.unmap_from(state.owner_pid)); + drop(completions.shm.unmap_from(state.owner_pid)); } } @@ -201,15 +203,15 @@ pub struct Inbox { /// `None` once the ring's one reference let go of it. state: Lock>, /// `None` from the moment that reference starts letting go of it. - completions: KernelLock>, + completions: IrqLock>, /// Threads parked in `submit`; never a poll — see the module header. - watch: Watch, + watch: IrqWatch, } struct RingState { + /// The page's address. The page is [`Completions`]'s, which the teardown + /// lets go of only after it has taken this. shm_phys: DirectMap, - /// A ring's page has no lifetime of its own; it goes with the last handle to the ring. - shm: Arc, submission_size: u32, /// Polls still armed as of the last registration, which sweeps the rest. pending: Vec>, @@ -222,7 +224,7 @@ struct RingState { /// One atomic word of one ring header; never `&RingHeader` — see the block above. fn ring_word(page: &DirectMap, ring_off: u64, field_off: usize) -> &core::sync::atomic::AtomicU32 { let ptr = page.as_mut_ptr::(); - // SAFETY: offset is in-bounds and 4-aligned within the 2 MiB page, which lives as long as the borrow of its holder; `AtomicU32` is sound over memory the process also writes. + // SAFETY: offset is in-bounds and 4-aligned within the 2 MiB page, which outlives both of the ring's halves that name it; `AtomicU32` is sound over memory the process also writes. unsafe { core::sync::atomic::AtomicU32::from_ptr( ptr.add(ring_off as usize + field_off) as *mut u32, @@ -248,9 +250,10 @@ impl RingState { } /// What a poll's completion writes, which a post from an interrupt handler -/// reaches. Its page lives while it is `Some`: the teardown takes it to `None` -/// before it lets the page go. +/// reaches, and the ring's page, which goes only with these. struct Completions { + /// A ring's page has no lifetime of its own; it goes with the last handle to the ring. + shm: Arc, page: DirectMap, completion_size: u32, /// The kernel's own copy of the completion tail, the only one it reads. @@ -310,9 +313,13 @@ impl Inbox { /// Post one completion and wake whoever waits in `submit`. A ring already /// torn down takes nothing and wakes nobody. fn complete(&self, user_data: u64, result: i32) { - let posted = self - .completions - .with(|c| c.as_mut().map(|c| c.post_completion(user_data, result, 0))); + let posted = self.completions.with(|c| { + // Inside the section whatever lock it is, so `handler-post` reds + // on one that leaves interrupts open. + #[cfg(feature = "boot-actuators")] + crate::watch::handler_post::raise_if_staged(); + c.as_mut().map(|c| c.post_completion(user_data, result, 0)) + }); if posted.is_some() { // In place: an interrupt handler's post reaches here through the // poll it fires. @@ -383,21 +390,63 @@ pub fn create(depth: u32) -> Result<(InboxRef, u64), SyscallError> { let inbox = Arc::new(Inbox { state: Lock::new(Some(RingState { shm_phys, - shm, submission_size, pending: Vec::new(), owner_pid: pid, })), - completions: KernelLock::new(Some(Completions { + completions: IrqLock::new(Some(Completions { + shm, page: shm_phys, completion_size, completion_tail: 0, })), - watch: Watch::new(), + watch: IrqWatch::new(), }); Ok((InboxRef(inbox), shm_vaddr)) } +/// `handler-post`'s ring: the kernel's own, mapped into no process and +/// submitted to by nobody, which polls a watch and completes as a submission +/// does. +#[cfg(feature = "boot-actuators")] +pub(crate) struct Staged(Arc); + +#[cfg(feature = "boot-actuators")] +impl Staged { + pub(crate) fn new() -> Self { + // Room for every completion the actuator's holds write. + let depth = toyos_sched::watch::handler_post::HOLDS; + let shm = SharedMemObject::create(crate::mm::PAGE_2M).expect("handler-post: a ring's page"); + let page = shm.phys_before_mapping(); + write_ring_page(page, depth, depth * 2); + Self(Arc::new(Inbox { + state: Lock::new(None), + completions: IrqLock::new(Some(Completions { + shm, + page, + completion_size: depth * 2, + completion_tail: 0, + })), + watch: IrqWatch::new(), + })) + } + + /// A poll of this ring on `watch`, which that watch's next post completes. + pub(crate) fn poll(&self, watch: &IrqWatch) { + let poll = Arc::new(Poll { + inbox: self.0.clone(), + user_data: 0, + handle: RawHandle(0), + state: Once::new(), + }); + watch.add_poll(PollEntry { poll, direction: Readiness { readable: true, writable: false } }); + } + + pub(crate) fn complete(&self) { + self.0.complete(0, 0); + } +} + /// Processes submissions and waits for completions; called from the syscall handler. pub fn submit( inbox: &Arc, diff --git a/kernel/src/irq_ring.rs b/kernel/src/irq_ring.rs index 4101b18bb67..a71e5204c1c 100644 --- a/kernel/src/irq_ring.rs +++ b/kernel/src/irq_ring.rs @@ -10,14 +10,23 @@ use crate::arch::percpu; use crate::scheduler::MAX_CPUS; /// Interrupt sources that drive scheduling; exhaustive, so a new variant requires updating every `match`. +/// The discriminant is `trace::Kind::IrqDrain`'s top byte, pinned for the reason `Kind` is. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum IrqSource { - Xhci, - I8042, + Xhci = 2, + I8042 = 3, } impl IrqSource { pub const COUNT: usize = 2; + + /// This source's record in a CPU's slots. + const fn slot(self) -> usize { + match self { + Self::Xhci => 0, + Self::I8042 => 1, + } + } } /// 64-byte aligned so two CPUs' slots never share a cache line. @@ -31,7 +40,7 @@ static SLOTS: [CpuSlots; MAX_CPUS] = pub fn isr_publish(source: IrqSource, timestamp_nanos: u64) { // MSI-X vectors are configured after clock calibration, so a real IRQ never stamps 0. assert!(timestamp_nanos != 0, "irq_ring: zero IRQ timestamp"); - let slot = &SLOTS[percpu::cpu_id() as usize].0[source as usize]; + let slot = &SLOTS[percpu::cpu_id() as usize].0[source.slot()]; // ISRs run with IF=0, so this load-then-store can't interleave with a same-CPU `take`. if slot.load(Ordering::Relaxed) == 0 { slot.store(timestamp_nanos, Ordering::Relaxed); @@ -40,7 +49,7 @@ pub fn isr_publish(source: IrqSource, timestamp_nanos: u64) { /// Consumes the current CPU's pending record for `source`, returning its IRQ-time timestamp. pub fn take(source: IrqSource) -> Option { - let slot = &SLOTS[percpu::cpu_id() as usize].0[source as usize]; + let slot = &SLOTS[percpu::cpu_id() as usize].0[source.slot()]; // Atomic swap: an interrupting ISR sees either the old record or the cleared slot, never a torn value. match slot.swap(0, Ordering::Relaxed) { 0 => None, @@ -54,7 +63,7 @@ pub fn take(source: IrqSource) -> Option { /// True if `source` has an undrained record on this CPU; non-consuming, unlike [`take`]. pub fn pending(source: IrqSource) -> bool { - SLOTS[percpu::cpu_id() as usize].0[source as usize].load(Ordering::Relaxed) != 0 + SLOTS[percpu::cpu_id() as usize].0[source.slot()].load(Ordering::Relaxed) != 0 } /// True if any IRQ record is undrained on the current CPU; non-consuming. diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index 40f6143e77b..ce4065ff2ac 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -16,7 +16,8 @@ use crate::time::Deadline; use crate::pipe::{self, PipeId}; use crate::process::PipeMap; use crate::user_ptr::{UserBytes, UserBytesMut}; -use crate::watch::Watch; +use crate::inbox::PollEntry; +use crate::watch::{IrqWatch, Watch}; use crate::{device as device_registry, keyboard, mouse}; use super::device::DeviceClaim; @@ -231,14 +232,24 @@ pub fn pipe_id_write(object: &KObjectRef) -> Option { pub enum WatchRef { Static(&'static Watch), Shared(Arc), + /// A device's, which its interrupt handler posts. + Irq(&'static IrqWatch), } -impl core::ops::Deref for WatchRef { - type Target = Watch; - fn deref(&self) -> &Watch { +impl WatchRef { + pub(crate) fn add_poll(&self, entry: PollEntry) { match self { - Self::Static(watch) => watch, - Self::Shared(watch) => watch, + Self::Static(watch) => watch.add_poll(entry), + Self::Shared(watch) => watch.add_poll(entry), + Self::Irq(watch) => watch.add_poll(entry), + } + } + + pub fn cancel_polls(&self) { + match self { + Self::Static(watch) => watch.cancel_polls(), + Self::Shared(watch) => watch.cancel_polls(), + Self::Irq(watch) => watch.cancel_polls(), } } } @@ -255,10 +266,10 @@ pub fn read_watch(object: &KObjectRef) -> Option { device_registry::DeviceType::Keyboard => Some(WatchRef::Static(&keyboard::WATCH)), device_registry::DeviceType::Mouse => Some(WatchRef::Static(&mouse::WATCH)), device_registry::DeviceType::PciFunction => { - d.pci_slot().map(|slot| WatchRef::Static(crate::pcidev::watch(slot))) + d.pci_slot().map(|slot| WatchRef::Irq(crate::pcidev::watch(slot))) } device_registry::DeviceType::HdaAudio | device_registry::DeviceType::VirtioSound => { - Some(WatchRef::Static(&crate::drivers::AUDIO_WATCH)) + Some(WatchRef::Irq(&crate::drivers::AUDIO_WATCH)) } device_registry::DeviceType::Framebuffer => None, // A partition answers its description and has nothing to wait for. diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 49a7b89d397..79c3411b0d3 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -117,7 +117,7 @@ use crate::mm::policy::{CachePolicy, MmioPolicy}; use crate::mm::{align_2m, DirectMap, Mmio, PAGE_2M}; use crate::object::shm::{Region, SharedMemObject}; use crate::sync::Lock; -use crate::watch::Watch; +use crate::watch::IrqWatch; /// How many functions this machine can hand out at once. /// @@ -280,7 +280,7 @@ static BOUND: [Lock>; MAX_FUNCTIONS] = /// What a claimed function's poll waits on, one per slot: two processes each driving a /// function must not learn when the other's device is busy. -static WATCHES: [Watch; MAX_FUNCTIONS] = [const { Watch::new() }; MAX_FUNCTIONS]; +static WATCHES: [IrqWatch; MAX_FUNCTIONS] = [const { IrqWatch::new() }; MAX_FUNCTIONS]; /// Every function this machine enumerated, and the two windows a BAR may be /// moved into. @@ -1758,6 +1758,6 @@ pub fn note_fault(slot: usize) { } /// The watch of the function a claim holds at `slot`. -pub fn watch(slot: usize) -> &'static Watch { +pub fn watch(slot: usize) -> &'static IrqWatch { &WATCHES[slot] } diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index aa3e2e21af8..72ff55f33b7 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -62,7 +62,7 @@ macro_rules! bump { /// What the ISR writes and the claim reads back. /// -/// Atomics only: the handler takes no lock and allocates nothing. +/// Atomics only: the handler allocates nothing. pub struct Interrupt { /// Messages since the holder's last read. count: AtomicU32, @@ -136,8 +136,8 @@ impl Interrupt { take_word!(self.unannounced, false) } - /// The unit refused this function an access. Called from the fault handler, - /// which takes no lock: every call the claim answers refuses from here on. + /// The unit refused this function an access. Called from the fault handler: + /// every call the claim answers refuses from here on. pub fn fault(&self) { self.faulted.store(true, ORDER); } diff --git a/kernel/src/sched/payload.rs b/kernel/src/sched/payload.rs index 83664b1ef60..34d737df6bb 100644 --- a/kernel/src/sched/payload.rs +++ b/kernel/src/sched/payload.rs @@ -8,7 +8,7 @@ use core::sync::atomic::{AtomicU32, AtomicU64, Ordering}; use toyos_sched::fair::{FairShare, ShareState}; use toyos_sched::hw::Nanos; use toyos_sched::msg::Msg; -use toyos_sched::sync::LeafLock; +use toyos_sched::sync::CellLock; use toyos_sched::task::{SchedPayload, TaskAccounting, TaskShared, WaitClass}; use toyos_sched::park::WaitTicket; @@ -19,10 +19,7 @@ use crate::process::{OwnedAlloc, PageTables, ProcessAccounting, TaskId}; use crate::symbols::SymbolTable; use crate::sync::Lock; -/// The environment's leaf lock, held with interrupts off, so an interrupt -/// handler may take one and never finds it held by the context it interrupted; -/// holding it raises the preempt count, making a wake path a legal mailbox -/// producer. +/// The environment's leaf lock; holding it raises the preempt count, making a wake path a legal mailbox producer. pub struct KernelLock(Lock); impl KernelLock { @@ -31,19 +28,9 @@ impl KernelLock { } } -impl LeafLock for KernelLock { +impl CellLock for KernelLock { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { - // Raised first, so the lock's own release never reaches depth zero, and - // a pass, with interrupts masked. - crate::sched::driver::preempt_off(|_| { - let _irq = crate::arch::IrqGuard::close(); - let mut held = self.0.lock(); - #[cfg(feature = "boot-actuators")] - crate::watch::handler_post::raise_if_staged(); - let out = f(&mut held); - drop(held); - out - }) + f(&mut self.0.lock()) } } diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index b048c3cd546..adde29e8237 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -9,9 +9,10 @@ //! record of what was posted — a waiter re-reads the object, never the post. //! //! **A post allocates nothing and may be made under any lock but a poll -//! ring's** (`crate::inbox`); a post in place frees nothing either, so a -//! device's interrupt handler makes one: every lock it takes is a `KernelLock`, -//! held with interrupts off. Registration allocates, in the syscall that +//! ring's** (`crate::inbox`). A watch an interrupt handler posts is an +//! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its one post is made +//! in place and frees nothing. Every other watch's list lock leaves interrupts +//! open, and no handler takes it. Registration allocates, in the syscall that //! registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] @@ -21,6 +22,7 @@ use alloc::sync::Arc; use toyos_sched::hw::Nanos; +use toyos_sched::sync::CellLock; use toyos_sched::task::{Refused, WaitClass, WakeCause, WakeReason}; use toyos_sched::watch::{Poster, Waiters}; @@ -31,16 +33,52 @@ use crate::inbox::PollEntry; use crate::sched::driver::{cpus, preempt_off}; use crate::sched::payload::{KMsg, KShared, KernelLock, TaskHandle}; use crate::scheduler::Parkable; +use crate::sync::Lock; use crate::time::Deadline; -type Inner = toyos_sched::watch::Watch>>; +type List = Waiters; -/// What an object holds to be waitable. -pub struct Watch(Inner); +/// What an object holds to be waitable, its list behind `L`. +pub struct Waitable>(toyos_sched::watch::Watch); + +/// A watch no interrupt handler reaches. +pub type Watch = Waitable>; + +/// A watch an interrupt handler posts, and the watch of a ring such a post +/// completes into. It has no `post`, which frees: only +/// [`IrqWatch::post_in_place`]. +pub type IrqWatch = Waitable>; + +/// What an interrupt handler's post takes: an [`IrqWatch`]'s list and a poll +/// ring's completions. Held with interrupts off, so a handler never finds one +/// held by the context it interrupted; nothing allocates or frees under it. +pub struct IrqLock(Lock); + +impl IrqLock { + pub const fn new(value: T) -> Self { + Self(Lock::new(value)) + } +} + +impl CellLock for IrqLock { + fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { + // Raised first, so the lock's own release never reaches depth zero, and + // a pass, with interrupts masked. + preempt_off(|_| { + let _irq = crate::arch::IrqGuard::close(); + let mut held = self.0.lock(); + #[cfg(feature = "boot-actuators")] + handler_post::raise_if_staged(); + let out = f(&mut held); + drop(held); + out + }) + } +} impl Watch { pub const fn new() -> Self { - Self(Inner::new(KernelLock::new(Waiters::new()))) + Self(toyos_sched::watch::Watch::new(KernelLock::new(Waiters::new()))) } /// Something about the object changed: wake every thread waiting on it and @@ -55,26 +93,12 @@ impl Watch { } fn post_as(&self, cause: WakeCause) { - #[cfg(feature = "boot-actuators")] - handler_post::note_post(self); preempt_off(|p| { let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; self.0.post(cause, &env); }); } - /// [`Self::post`] for an interrupt handler, which may not free: every poll - /// is completed where it stands, with interrupts off, so a handler posts - /// only a watch whose registrations are its device's holder's own. - pub fn post_in_place(&self) { - #[cfg(feature = "boot-actuators")] - handler_post::note_post(self); - preempt_off(|p| { - let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; - self.0.post_in_place(WakeCause::new(WakeReason::Woken), &env); - }); - } - /// Wake at most `limit` threads waiting with `token`, and answer how many /// were parked; one that was not is told anyway and spends nothing. pub fn post_n(&self, token: u64, limit: usize) -> usize { @@ -97,7 +121,28 @@ impl Watch { ) }) } +} + +impl IrqWatch { + pub const fn new() -> Self { + Self(toyos_sched::watch::Watch::new(IrqLock::new(Waiters::new()))) + } + /// Something about the object changed: wake every thread waiting on it and + /// complete every poll where it stands, freeing nothing. A handler makes it + /// with its CPU's preempt count raised, as `device_irq_entry` holds it, so + /// this post's own never reaches zero, and a pass, inside the interrupt. + pub fn post_in_place(&self) { + #[cfg(feature = "boot-actuators")] + handler_post::note_post(self); + preempt_off(|p| { + let env = Poster { cpus: cpus(), kicker: &HW, preempt: p }; + self.0.post_in_place(WakeCause::new(WakeReason::Woken), &env); + }); + } +} + +impl> Waitable { /// A poll ring's entry, from `inbox`'s registration and nowhere else. pub(crate) fn add_poll(&self, entry: PollEntry) { self.0.add_ring(entry); @@ -112,8 +157,8 @@ impl Watch { /// A thread's registration on one watch, held across its wait and ended by /// its drop. #[must_use = "a registration must outlive the park it was made for"] -pub struct Armed<'a> { - watch: &'a Watch, +pub struct Armed<'a, L: CellLock = KernelLock> { + watch: &'a Waitable, shared: Arc, task: Arc, /// Wait class for the blocked-time breakdown; the park carries no subject @@ -121,7 +166,7 @@ pub struct Armed<'a> { class: WaitClass, } -impl Drop for Armed<'_> { +impl> Drop for Armed<'_, L> { fn drop(&mut self) { self.watch.0.unregister(&self.shared); } @@ -129,7 +174,11 @@ impl Drop for Armed<'_> { /// Register the running task on `watch`; `None` when there is no current task. /// Call before reading the condition the wait is for. -pub fn arm(watch: &Watch, token: u64, class: WaitClass) -> Option> { +pub fn arm>( + watch: &Waitable, + token: u64, + class: WaitClass, +) -> Option> { let task = crate::sched::driver::current_handle()?; let shared = crate::sched::driver::current_shared()?; watch.0.register(&shared, token); @@ -159,7 +208,11 @@ fn not_revocable() -> ! { /// this thread is cancelled. A return is not an answer — the caller re-reads /// its condition. #[track_caller] -pub fn wait(p: &Parkable, armed: &Armed<'_>, deadline: Deadline) -> Result<(), Cancelled> { +pub fn wait>( + p: &Parkable, + armed: &Armed<'_, L>, + deadline: Deadline, +) -> Result<(), Cancelled> { match wait_inner(p, armed, deadline, Cancel::Answers) { Ok(()) => Ok(()), Err(Ended::Cancelled) => Err(Cancelled(())), @@ -169,7 +222,7 @@ pub fn wait(p: &Parkable, armed: &Armed<'_>, deadline: Deadline) -> Result<(), C /// The same as [`wait`], for a wait a kill may not end. #[track_caller] -pub fn wait_uncancellable(p: &Parkable, armed: &Armed<'_>, deadline: Deadline) { +pub fn wait_uncancellable>(p: &Parkable, armed: &Armed<'_, L>, deadline: Deadline) { match wait_inner(p, armed, deadline, Cancel::Ignores) { Ok(()) => {} Err(Ended::Cancelled) => unreachable!("an uncancellable wait never reports a cancel"), @@ -183,9 +236,9 @@ pub fn wait_uncancellable(p: &Parkable, armed: &Armed<'_>, deadline: Deadline) { /// be waited on again safely, and the caller reads its own condition again to /// tell them apart. #[track_caller] -pub fn wait_until( +pub fn wait_until>( p: &Parkable, - watch: &Watch, + watch: &Waitable, token: u64, class: WaitClass, deadline: Deadline, @@ -225,7 +278,7 @@ mod window { use toyos_sched::task::WaitClass; use toyos_sched::watch::window::{HELD, STEP}; - use super::Armed; + use super::{Armed, CellLock, List}; use crate::time::{Budget, Deadline, Duration}; /// A pipe wait nothing posts — a reader whose writer is idle — ends its @@ -238,7 +291,7 @@ mod window { static POSTED: AtomicU64 = AtomicU64::new(0); static LAPSED: AtomicU64 = AtomicU64::new(0); - pub(super) fn hold(armed: &Armed<'_>) { + pub(super) fn hold>(armed: &Armed<'_, L>) { if !crate::actuator::watch_window() || armed.class != WaitClass::Pipe { return; } @@ -262,23 +315,23 @@ mod window { } } -/// `handler-post`: claim slot 0's vector, raised on this CPU inside a post of -/// that slot's own watch while the CPU holds preemption off, posts the watch -/// once the outer post lets go of it and before any pass can run. A hold counts -/// the posts of that watch made on its CPU, and no other watch's, which a poll -/// the outer post completes also posts: the outer one and the handler's are -/// two, and a hold that saw fewer by its budget lapsed. One run, on whichever -/// idle loop reaches it first with interrupts open; its verdict is one -/// [`handler_post::SAID`] line. +/// `handler-post`: claim slot 0's vector, raised on this CPU while it holds +/// preemption off, posts that slot's watch from the handler before any pass +/// can run. Raised inside a post of the watch itself, the handler's post +/// follows once the outer one lets go; raised inside a completion written into +/// a ring that polls the watch, the handler's post completes that poll once the +/// writer lets go. A hold counts the posts of the watch made on its CPU, and no +/// other watch's, and lapses at its budget. One run of [`HOLDS`] holds per arm, +/// on whichever idle loop reaches it first with interrupts open; its verdict is +/// one [`Verdict`] line. #[cfg(feature = "boot-actuators")] pub mod handler_post { use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering::Relaxed}; + use toyos_sched::watch::handler_post::{Verdict, HOLDS}; + use crate::time::{Budget, Deadline, Duration}; - /// The verdict line's words; the counts follow them. - pub const SAID: &str = "handler-post:"; - const HOLDS: u32 = 4; const WINDOW: Budget = Budget::of( Duration::from_secs(1), "the hold is counted as lapsed, and the verdict line says so", @@ -286,7 +339,7 @@ pub mod handler_post { const NOBODY: u32 = u32::MAX; static RAN: AtomicBool = AtomicBool::new(false); - /// The CPU whose next leaf lock raises the vector inside itself. + /// The CPU whose next interrupts-off section raises the vector inside itself. static RAISE_INSIDE: AtomicU32 = AtomicU32::new(NOBODY); static HOLDING: AtomicU32 = AtomicU32::new(NOBODY); static POSTS: AtomicU64 = AtomicU64::new(0); @@ -297,33 +350,50 @@ pub mod handler_post { return; } let me = crate::arch::percpu::cpu_id(); - let (mut posted, mut lapsed) = (0, 0); - crate::sched::driver::preempt_off(|_| { + let claim = crate::pcidev::watch(0); + let ring = crate::inbox::Staged::new(); + let verdict = crate::sched::driver::preempt_off(|_| { HOLDING.store(me, Relaxed); - for _ in 0..HOLDS { - let before = POSTS.load(Relaxed); + // The outer post is one of the two a hold waits for. + let in_a_list = holds(2, || { RAISE_INSIDE.store(me, Relaxed); - crate::pcidev::watch(0).post(); - let deadline = Deadline::at(crate::clock::now() + WINDOW.duration()); - loop { - if POSTS.load(Relaxed) >= before + 2 { - posted += 1; - break; - } - if deadline.reached(crate::clock::now()) { - lapsed += 1; - break; - } - core::hint::spin_loop(); - } - } + claim.post_in_place(); + }); + let in_a_ring = holds(1, || { + ring.poll(claim); + RAISE_INSIDE.store(me, Relaxed); + ring.complete(); + }); HOLDING.store(NOBODY, Relaxed); + Verdict { in_a_list, in_a_ring } }); - crate::log!("{SAID} {HOLDS} holds, {posted} posted into by a handler, {lapsed} lapsed"); + crate::log!("{verdict}"); + } + + /// [`HOLDS`] holds of `stage`, answering how many saw `owed` posts of the + /// claim's watch before their budget. + fn holds(owed: u64, stage: impl Fn()) -> u32 { + let mut posted = 0; + for _ in 0..HOLDS { + let before = POSTS.load(Relaxed); + stage(); + let deadline = Deadline::at(crate::clock::now() + WINDOW.duration()); + loop { + if POSTS.load(Relaxed) >= before + owed { + posted += 1; + break; + } + if deadline.reached(crate::clock::now()) { + break; + } + core::hint::spin_loop(); + } + } + posted } - /// From inside every `KernelLock`: the staged CPU's next one raises the - /// vector while it holds. + /// From inside an interrupts-off section: the staged CPU's next one raises + /// the vector while it holds. pub fn raise_if_staged() { let staged = RAISE_INSIDE.load(Relaxed); if staged != NOBODY && staged == crate::arch::percpu::cpu_id() { @@ -332,7 +402,7 @@ pub mod handler_post { } } - pub fn note_post(watch: &super::Watch) { + pub fn note_post(watch: &super::IrqWatch) { let holding = HOLDING.load(Relaxed); if holding != NOBODY && holding == crate::arch::percpu::cpu_id() @@ -363,9 +433,9 @@ pub fn wait_uncancellable_until(p: &Parkable, watch: &Watch, token: u64, ready: } #[track_caller] -fn wait_inner( +fn wait_inner>( _p: &Parkable, - armed: &Armed<'_>, + armed: &Armed<'_, L>, deadline: Deadline, cancel: Cancel, ) -> Result<(), Ended> { diff --git a/tests/toyos.rs b/tests/toyos.rs index 02f68e0d3ea..84f354b7560 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -1270,10 +1270,10 @@ const MACHINE_TESTS: &[(&str, Sched)] = &[ // waiter between reading its condition and parking, so the peer's post lands where // only the notified bit carries it to the commit. ("blocking_read_window", Sched::Parallel), - // A claimed function's vector posts its watch from the handler: raised - // inside a post of that watch on a CPU holding preemption off, it posts - // once the outer post lets go and before any pass. One boot; the verdict - // is counts. + // A claimed function's vector posts its watch from the handler: raised on + // a CPU holding preemption off, inside a post of that watch or inside a + // completion into a ring polling it, it posts once that section lets go + // and before any pass. One boot; the verdict is counts. ("handler_post_without_a_pass", Sched::Parallel), // A sibling's munmap and mmap staged between a typed copy's translation // and its store (`copy-meets-a-remap`): the store never reaches the region @@ -14882,10 +14882,7 @@ fn sysret_ss(log: &str) -> Result<(), String> { Ok(()) } -/// `kernel/src/watch.rs`'s `handler_post::SAID`, and the counts every hold -/// posted into gives it. -const HANDLER_POST_SAID: &str = "handler-post:"; -const HANDLER_POST_POSTED: &str = "handler-post: 4 holds, 4 posted into by a handler, 0 lapsed"; +use toyos_sched::watch::handler_post::{Verdict as HandlerPost, SAID as HANDLER_POST_SAID}; fn handler_post_said(line: &str) -> bool { line.contains(HANDLER_POST_SAID) @@ -14896,7 +14893,7 @@ fn handler_post(log: &str) -> Result<(), String> { let Some(said) = log.lines().find(|line| handler_post_said(line)) else { return Err(format!("`handler-post` never said its verdict:\n{log}")); }; - if !said.contains(HANDLER_POST_POSTED) { + if !said.contains(&HandlerPost::GREEN.to_string()) { return Err(format!( "a hold lapsed with no handler's post in it — the wake waited for a pass:\n{said}\n{log}" )); diff --git a/toyos-sched/loom/src/model.rs b/toyos-sched/loom/src/model.rs index 4970c1bfe7f..f58573bb9fd 100644 --- a/toyos-sched/loom/src/model.rs +++ b/toyos-sched/loom/src/model.rs @@ -1,5 +1,5 @@ //! Scaffolding shared by the loom models: the message type, the modelled -//! preempt count, the leaf lock and the kick recorder. +//! preempt count, the cell lock and the kick recorder. use loom::sync::atomic::{AtomicUsize, Ordering}; use loom::sync::{Mutex, MutexGuard}; @@ -8,7 +8,7 @@ use crate::hw::{CpuId, Kicker}; use crate::mailbox::{PreemptGuard, SchedMsg}; use crate::sync::Arc; use crate::task::{TaskKey, TaskShared, WakeCause, WakeReason}; -use crate::sync::LeafLock; +use crate::sync::CellLock; use crate::watch::Waiters; pub const CPU0: CpuId = CpuId(0); @@ -113,7 +113,7 @@ pub struct RemoteGuard; #[allow(unsafe_code)] unsafe impl PreemptGuard for RemoteGuard {} -/// `LeafLock` over loom's mutex, so the watch models exercise the real +/// `CellLock` over loom's mutex, so the watch models exercise the real /// critical sections. pub struct LoomLock(Mutex); @@ -123,7 +123,7 @@ impl LoomLock { } } -impl LeafLock for LoomLock { +impl CellLock for LoomLock { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { f(&mut self.0.lock().unwrap()) } diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index 1444e54dc1c..a7684096770 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -24,7 +24,8 @@ //! `gate-fence-off` removes the [`Gate`]'s two fences, and //! `a_transition_racing_an_opening_gate_is_never_missed` must red; and //! `poll-fire-load-store`, the kernel's own control for the poll's one-shot -//! answer, which the ring models below compile, must red every poll model here. +//! answer, which the ring models below compile, must red every +//! `*_completes_exactly_once` model here. //! //! **The ring entry is the kernel's [`Once`], compiled from //! `kernel/src/inbox/once.rs`**, the decision a `PollEntry` makes; what else a @@ -49,7 +50,7 @@ use toyos_sched_loom::park::{prepare, Cancel, Commit, CurrentTask}; use toyos_sched_loom::task::{ Claim, Refused, TaskKey, TaskShared, TaskState, WaitClass, WakeCause, WakeReason, }; -use toyos_sched_loom::sync::LeafLock; +use toyos_sched_loom::sync::CellLock; use toyos_sched_loom::watch::{Fire, Gate, Poster, Ring, Waiters, Watch}; #[path = "../../../kernel/src/inbox/once.rs"] @@ -293,6 +294,8 @@ fn a_poll_registered_racing_a_post_in_place_completes_exactly_once() { model(|| poll_racing(World::post_in_place)); } +/// `ready` is `Relaxed` on both sides, as a claim's `faulted` is: the list lock +/// alone orders it against the registrant's recheck. fn poll_racing(post: fn(&World)) { let (world, _rx) = world(); let ready = Arc::new(AtomicBool::new(false)); @@ -304,13 +307,13 @@ fn poll_racing(post: fn(&World)) { let poll = poll.clone(); loom::thread::spawn(move || { world.watch.add_ring(poll.clone()); - if ready.load(Ordering::Acquire) { + if ready.load(Ordering::Relaxed) { poll.fire(Fire::Ready); } }) }; let producer = loom::thread::spawn(move || { - ready.store(true, Ordering::Release); + ready.store(true, Ordering::Relaxed); post(&world); }); registrant.join().unwrap(); @@ -650,11 +653,10 @@ impl Ring for RingEntry { /// **A post in place fires its rings under its own list lock**, so beneath it /// are the ring's lock and the ring's watch. Two devices' watches each hold a /// poll of one ring and are posted at once, while the ring's submitter waits -/// for both completions: the three locks nest in one order, so no schedule -/// deadlocks, each poll completes once, and a submitter parked with both -/// completions written was owed the wake the second one posted. +/// for both completions: a submitter parked with both completions written was +/// owed the wake the second one posted. #[test] -fn two_posts_through_one_rings_lock_complete_it_once_each_and_lose_no_wake() { +fn two_posts_through_one_rings_lock_lose_no_wake() { model(|| { let (tx, mut rx) = mailbox::(); let ring = Arc::new(PollRing { @@ -716,7 +718,6 @@ fn two_posts_through_one_rings_lock_complete_it_once_each_and_lose_no_wake() { let guard = PreemptModel::new(); let msgs = drain(&mut rx, &guard); - assert_eq!(ring.written.with(|n| *n), 2, "a poll was completed other than once"); if parked { assert_eq!( msgs, diff --git a/toyos-sched/sim/src/payload.rs b/toyos-sched/sim/src/payload.rs index b0676b36c7d..c67b3871412 100644 --- a/toyos-sched/sim/src/payload.rs +++ b/toyos-sched/sim/src/payload.rs @@ -11,7 +11,7 @@ use std::sync::{Arc, Mutex}; use toyos_sched::fair::ShareState; use toyos_sched::mailbox::PreemptGuard; -use toyos_sched::sync::LeafLock; +use toyos_sched::sync::CellLock; use toyos_sched::task::{SchedPayload, TaskKey}; use toyos_sched::watch::{Fire, Ring, Waiters}; @@ -29,7 +29,7 @@ impl StdLock { } } -impl LeafLock for StdLock { +impl CellLock for StdLock { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { f(&mut self.0.lock().expect("the simulator never poisons a lock")) } diff --git a/toyos-sched/src/cpu.rs b/toyos-sched/src/cpu.rs index 0cba829f61c..7db1c4d9e5b 100644 --- a/toyos-sched/src/cpu.rs +++ b/toyos-sched/src/cpu.rs @@ -2438,13 +2438,13 @@ mod tests { use crate::fair::{FairShare, ShareState}; use crate::hw::{Kicker, Machine}; use crate::mailbox::{mailbox, NoPreempt}; - use crate::sync::LeafLock; + use crate::sync::CellLock; use crate::task::{RtState, TaskAccounting, TaskBuilder}; use std::sync::Mutex; struct TestLock(Mutex); - impl LeafLock for TestLock { + impl CellLock for TestLock { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { f(&mut self.0.lock().expect("a test never poisons a lock")) } diff --git a/toyos-sched/src/fair.rs b/toyos-sched/src/fair.rs index 95aa1256a3b..cbcd4603002 100644 --- a/toyos-sched/src/fair.rs +++ b/toyos-sched/src/fair.rs @@ -5,7 +5,7 @@ use core::num::NonZeroU32; use core::sync::atomic::{AtomicU64, Ordering}; -use crate::sync::LeafLock; +use crate::sync::CellLock; /// Clamp for stored lag at the Runnable→NonRunnable transition: how far /// behind (entitled catch-up) or ahead (throttled on wake) of the frontier a @@ -206,13 +206,13 @@ fn vrt_from_lag(frontier: u64, lag: i64) -> u64 { } /// One process's fair-share pot, reached through any thread that owns it. The -/// cell is supplied by the environment for the reason stated on [`LeafLock`]: +/// cell is supplied by the environment for the reason stated on [`CellLock`]: /// the kernel's is a word-sized spin, the simulator's a mutex. pub struct FairShare { state: L, } -impl> FairShare { +impl> FairShare { pub fn new(state: L) -> Self { Self { state } } diff --git a/toyos-sched/src/sync.rs b/toyos-sched/src/sync.rs index 41cd9564048..1a08ca17168 100644 --- a/toyos-sched/src/sync.rs +++ b/toyos-sched/src/sync.rs @@ -20,13 +20,12 @@ pub use loom::sync::Arc; /// Interior mutability for a small shared cell, supplied by the environment: /// a watch's waiter list and the per-process fair share. The kernel's -/// implementor is a few-instruction, IRQ-off leaf lock that is never held -/// across a pass or a switch; the simulator and the loom models supply their -/// own. +/// implementor is never held across a pass or a switch; the simulator and the +/// loom models supply their own. /// /// It lives here rather than in one of its users because the core crate may /// not implement a lock itself — that would need `unsafe`, which only /// `mailbox.rs` is allowed to write. -pub trait LeafLock: Sync { +pub trait CellLock: Sync { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R; } diff --git a/toyos-sched/src/task.rs b/toyos-sched/src/task.rs index 419de23ae15..9ee5564649c 100644 --- a/toyos-sched/src/task.rs +++ b/toyos-sched/src/task.rs @@ -13,7 +13,7 @@ use crate::fair::{FairShare, ShareState, QUANTUM_NS}; use crate::hw::{CpuId, Nanos}; use crate::mailbox::MailboxNode; use crate::msg::Msg; -use crate::sync::{Arc, AtomicBool, AtomicU64, LeafLock, Ordering}; +use crate::sync::{Arc, AtomicBool, AtomicU64, CellLock, Ordering}; use crate::park::CommittedTicket; /// Monotonic, never reused. Stale messages keyed by `TaskKey` are provably @@ -31,8 +31,8 @@ pub trait SchedPayload: Sized + Send + 'static { /// The cell the per-process [`FairShare`] lives in. Supplied by the /// environment because the core crate may not implement a lock itself - /// (see [`LeafLock`]). - type ShareLock: LeafLock + Send; + /// (see [`CellLock`]). + type ShareLock: CellLock + Send; } /// Shorthand for the share type a payload implies. diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index 10014578ddf..f45e3154805 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -17,11 +17,13 @@ //! //! **Nothing allocates or frees under the list lock**, because an interrupt //! handler's post can interrupt the allocator's holder while another CPU holds -//! the list waiting for the allocator. A registration grows the list and drops -//! what it swept with the lock let go. [`Watch::post`] takes every ring entry -//! out and fires and frees them with the lock let go; [`Watch::post_in_place`], -//! for a handler, which may not free at all, fires them where they stand, and -//! the next registration sweeps the dead out, since an entry is one-shot. +//! the list waiting for the allocator. A registration takes the entries that +//! can no longer fire out [`FEW`] at a time, grows a full list in a buffer +//! allocated with the lock let go, and drops both with it let go. +//! [`Watch::post`] takes every ring entry out and fires and frees them with the +//! lock let go; [`Watch::post_in_place`], for a handler, which may not free at +//! all, fires them where they stand, and later registrations sweep the dead +//! out, since an entry is one-shot. //! //! **Lock order.** A post in place fires its rings under the list lock, so //! beneath it are each ring's own lock and the watch that ring's submitters @@ -37,7 +39,7 @@ use crate::cpu::CpuHandles; use crate::hw::Kicker; use crate::mailbox::{PreemptGuard, SchedMsg}; use crate::park::{notify, revoke}; -use crate::sync::{fence, Arc, AtomicU32, LeafLock, Ordering}; +use crate::sync::{fence, Arc, AtomicU32, CellLock, Ordering}; use crate::task::{TaskShared, WakeCause}; /// Why a ring entry is posted. @@ -63,7 +65,7 @@ pub trait Ring { fn live(&self) -> bool; } -/// Everything a watch holds, behind the environment's leaf lock. +/// Everything a watch holds. pub struct Waiters { /// In registration order, so a bounded post reaches the longest waiter /// first. @@ -103,12 +105,16 @@ pub struct Poster<'a, M, K, P> { pub preempt: &'a P, } -pub struct Watch>> { +/// How many ring entries one section takes out of the list to drop with the +/// lock let go, on the stack and so allocating nothing. +const FEW: usize = 4; + +pub struct Watch>> { list: L, _msg: PhantomData (M, R)>, } -impl>> Watch { +impl>> Watch { pub const fn new(list: L) -> Self { Self { list, @@ -117,7 +123,7 @@ impl>> Watch { } } -impl>> Watch { +impl>> Watch { /// Register the running task for one wait. After this, and before its /// condition is read: that order is the whole lost-wake argument. A post /// that reached this task during an earlier wait is forgotten here, before @@ -125,18 +131,17 @@ impl>> Watch { pub fn register(&self, task: &Arc>, token: u64) { assert!(task.set_waiting(), "a task waits on at most one watch"); task.forget_posts(); - self.sweep(); - self.push(Waiter { task: task.clone(), token }, |w| &mut w.threads); + self.admit(Waiter { task: task.clone(), token }, |w| &mut w.threads); } /// End one wait. Idempotent against [`Self::revoke`], which may have taken /// the registration out already. pub fn unregister(&self, task: &Arc>) { - let gone = self.list.with(|w| { - let at = w.threads.iter().position(|t| Arc::ptr_eq(&t.task, task))?; - Some(w.threads.remove(at)) + self.list.with(|w| { + if let Some(at) = w.threads.iter().position(|t| Arc::ptr_eq(&t.task, task)) { + w.threads.remove(at); + } }); - drop(gone); task.clear_waiting(); } @@ -144,20 +149,27 @@ impl>> Watch { /// after this returns and fires the entry itself if the object is already /// ready — the ring's half of the same order a thread keeps. pub fn add_ring(&self, entry: R) { - self.sweep(); - self.push(entry, |w| &mut w.rings); + self.admit(entry, |w| &mut w.rings); } /// Something changed: wake every registered thread, and fire and let go of /// every ring entry, with the list lock let go. pub fn post(&self, cause: WakeCause, env: &Poster<'_, M, K, P>) { - let fired = self.list.with(|w| { + let mut few: [Option; FEW] = [const { None }; FEW]; + let many = self.list.with(|w| { for waiter in &w.threads { notify(&waiter.task, cause, env.cpus, env.kicker, env.preempt); } - core::mem::take(&mut w.rings) + // [`FEW`] leave the list's buffer to the next registration. + if w.rings.len() > FEW { + return core::mem::take(&mut w.rings); + } + for (slot, ring) in few.iter_mut().zip(w.rings.drain(..)) { + *slot = Some(ring); + } + Vec::new() }); - for ring in &fired { + for ring in few.iter().flatten().chain(&many) { ring.fire(Fire::Ready); } } @@ -180,57 +192,46 @@ impl>> Watch { }); } - /// Put `item` on the list `list` picks, growing it with the lock let go - /// when it is full. - fn push(&self, item: T, list: fn(&mut Waiters) -> &mut Vec) { + /// Put `item` on the list `list` picks, taking out up to [`FEW`] ring + /// entries that can no longer fire first. One section while the list has + /// room; a full one grows in a buffer allocated with the lock let go, and + /// what a section took out is dropped with it let go. + fn admit(&self, item: T, list: fn(&mut Waiters) -> &mut Vec) { let mut item = item; + let mut room = Vec::new(); loop { + let mut dead: [Option; FEW] = [const { None }; FEW]; let full = self.list.with(|w| { + let (mut at, mut taken) = (0, 0); + // Order is nothing to a ring entry: every post fires them all. + while at < w.rings.len() && taken < FEW { + if w.rings[at].live() { + at += 1; + } else { + dead[taken] = Some(w.rings.swap_remove(at)); + taken += 1; + } + } let v = list(w); + // Only a buffer that holds the whole list and `item`: another + // registration may have outgrown it since it was sized. + if v.len() == v.capacity() && room.capacity() > v.len() { + room.append(v); + core::mem::swap(v, &mut room); + } if v.len() < v.capacity() { v.push(item); return None; } Some((v.capacity(), item)) }); + drop(dead); let Some((seen, back)) = full else { return }; item = back; - // Swapped in unless another registration grew it first; whichever - // buffer loses is dropped out here. - let mut room = Vec::with_capacity((seen * 2).max(4)); - self.list.with(|w| { - let v = list(w); - if v.capacity() == seen { - room.append(v); - core::mem::swap(v, &mut room); - } - }); - drop(room); + room = Vec::with_capacity((seen * 2).max(FEW)); } } - /// Take out the ring entries that can no longer fire, and drop them — and - /// allocate the room they are taken into — with the lock let go. - fn sweep(&self) { - let dead = self.list.with(|w| w.rings.iter().filter(|r| !r.live()).count()); - if dead == 0 { - return; - } - let mut out = Vec::with_capacity(dead); - self.list.with(|w| { - let mut at = 0; - // Order is nothing to a ring entry: every post fires them all. - while at < w.rings.len() && out.len() < out.capacity() { - if w.rings[at].live() { - at += 1; - } else { - out.push(w.rings.swap_remove(at)); - } - } - }); - drop(out); - } - /// Wake at most `limit` threads registered with `token`, in registration /// order, and answer how many this post woke. A thread that was not parked /// is flagged and spends nothing: its own commit rechecks, and the post @@ -364,7 +365,7 @@ fn gate_fence() { /// An object that ends with polls still registered answers them: dropping a /// watch fires every live ring entry as [`Fire::Gone`]. -impl>> Drop for Watch { +impl>> Drop for Watch { fn drop(&mut self) { let ended = self.list.with(|w| core::mem::take(&mut w.rings)); for ring in &ended { @@ -385,6 +386,42 @@ pub mod window { pub const STEP: u64 = 64; } +/// The `handler-post` actuator's line: the kernel writes it once, and the +/// harness compares it whole against [`Verdict::GREEN`]. +/// +/// [`Verdict::GREEN`]: handler_post::Verdict::GREEN +pub mod handler_post { + use core::fmt; + + /// The line's first word, which the harness waits for. + pub const SAID: &str = "handler-post:"; + /// Holds staged per arm. + pub const HOLDS: u32 = 4; + + /// The holds a handler's post ended, per arm: its vector raised inside a + /// watch's list lock, and inside a ring's completions. + #[derive(Clone, Copy)] + pub struct Verdict { + pub in_a_list: u32, + pub in_a_ring: u32, + } + + impl Verdict { + pub const GREEN: Self = Self { in_a_list: HOLDS, in_a_ring: HOLDS }; + } + + impl fmt::Display for Verdict { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{SAID} of {HOLDS} holds per arm, a handler posted into {} inside a list lock \ + and {} inside a ring's completions", + self.in_a_list, self.in_a_ring, + ) + } + } +} + #[cfg(test)] mod tests { extern crate std; @@ -415,7 +452,7 @@ mod tests { } struct StdLock(Mutex); - impl LeafLock for StdLock { + impl CellLock for StdLock { fn with(&self, f: impl FnOnce(&mut T) -> U) -> U { f(&mut self.0.lock().unwrap()) } @@ -666,7 +703,7 @@ mod tests { } struct SharedLock(std::sync::Arc>>); - impl LeafLock> for SharedLock { + impl CellLock> for SharedLock { fn with(&self, f: impl FnOnce(&mut Waiters) -> U) -> U { f(&mut self.0.lock().unwrap()) } diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs index d797c5fd590..8e70b32ca2d 100644 --- a/toyos-sched/tests/watch_list.rs +++ b/toyos-sched/tests/watch_list.rs @@ -4,39 +4,45 @@ //! //! The allocator below counts every allocation and free this thread makes //! while it holds a list lock, and every one it makes inside a post in place. +//! The list lock counts its sections, and can run a stage between two of them. use std::alloc::{GlobalAlloc, Layout, System}; -use std::cell::Cell; -use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering::{AcqRel, Acquire, Relaxed}}; +use std::cell::{Cell, RefCell}; +use std::sync::atomic::{AtomicU32, Ordering::{AcqRel, Acquire, Relaxed}}; use std::sync::{Arc, Mutex}; use toyos_sched::cpu::{CpuHandle, CpuHandles}; use toyos_sched::hw::{CpuId, Kicker}; use toyos_sched::mailbox::{mailbox, PreemptGuard, SchedMsg}; use toyos_sched::park::{prepare, Cancel, Commit, CurrentTask}; -use toyos_sched::sync::LeafLock; +use toyos_sched::sync::CellLock; use toyos_sched::task::{TaskKey, TaskShared, TaskState, WaitClass, WakeCause, WakeReason}; use toyos_sched::watch::{Fire, Poster, Ring, Waiters, Watch}; thread_local! { static HELD: Cell = const { Cell::new(false) }; static POSTING: Cell = const { Cell::new(false) }; + static UNDER_LOCK: Cell = const { Cell::new(0) }; + static IN_POST: Cell = const { Cell::new(0) }; + static SECTIONS: Cell = const { Cell::new(0) }; + /// Sections left to start before [`BETWEEN`] runs, ahead of the last one. + static COUNTDOWN: Cell = const { Cell::new(0) }; + static BETWEEN: RefCell>> = const { RefCell::new(None) }; } -static UNDER_LOCK: AtomicUsize = AtomicUsize::new(0); -static IN_POST: AtomicUsize = AtomicUsize::new(0); - struct Counting; impl Counting { fn note(&self) { // `try_with`: a thread allocates while its locals are torn down. - if HELD.try_with(Cell::get).unwrap_or(false) { - UNDER_LOCK.fetch_add(1, Relaxed); - } - if POSTING.try_with(Cell::get).unwrap_or(false) { - IN_POST.fetch_add(1, Relaxed); - } + let count = |flag: &'static std::thread::LocalKey>, + counter: &'static std::thread::LocalKey>| { + if flag.try_with(Cell::get).unwrap_or(false) { + let _ = counter.try_with(|n| n.set(n.get() + 1)); + } + }; + count(&HELD, &UNDER_LOCK); + count(&POSTING, &IN_POST); } } @@ -67,8 +73,18 @@ static ALLOCATOR: Counting = Counting; /// The list lock, marking this thread as holding it. struct Watched(Mutex); -impl LeafLock for Watched { +impl CellLock for Watched { fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { + // Before the lock is taken, so a stage lands between two sections. + let due = COUNTDOWN.get(); + if due > 0 { + COUNTDOWN.set(due - 1); + if due == 1 { + let between = BETWEEN.take().expect("a countdown is staged with its stage"); + between(); + } + } + SECTIONS.set(SECTIONS.get() + 1); let mut guard = self.0.lock().unwrap(); let was = HELD.replace(true); let out = f(&mut guard); @@ -77,6 +93,19 @@ impl LeafLock for Watched { } } +/// Run `between` with no list lock held, just before the `nth` section from +/// here takes its lock. +fn stage(nth: usize, between: impl FnOnce() + 'static) { + BETWEEN.set(Some(Box::new(between))); + COUNTDOWN.set(nth); +} + +fn sections(f: impl FnOnce()) -> usize { + let before = SECTIONS.get(); + f(); + SECTIONS.get() - before +} + #[derive(Debug)] enum Msg { Wake, @@ -118,11 +147,21 @@ impl Ring for Entry { } } +type TestWatch = Watch>>; + const C0: CpuId = CpuId(0); +fn watch() -> TestWatch { + Watch::new(Watched(Mutex::new(Waiters::new()))) +} + +fn task(key: u64) -> Arc> { + Arc::new(TaskShared::new(TaskKey(key), TaskState::Running(C0))) +} + fn clean(phase: &str) { - assert_eq!(UNDER_LOCK.load(Relaxed), 0, "{phase} allocated or freed under the list lock"); - assert_eq!(IN_POST.load(Relaxed), 0, "{phase}: a post in place allocated or freed"); + assert_eq!(UNDER_LOCK.get(), 0, "{phase} allocated or freed under the list lock"); + assert_eq!(IN_POST.get(), 0, "{phase}: a post in place allocated or freed"); } #[test] @@ -130,12 +169,10 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth let (tx, mut rx) = mailbox::(); let cpus = CpuHandles::new(vec![CpuHandle::new(C0, tx)]); let env = Poster { cpus: &cpus, kicker: &NoKick, preempt: &NoPreempt }; - let w: Watch>> = - Watch::new(Watched(Mutex::new(Waiters::new()))); + let w = watch(); // Nine of each grows both lists from nothing three times over. - let tasks: Vec<_> = - (0..9).map(|k| Arc::new(TaskShared::new(TaskKey(k), TaskState::Running(C0)))).collect(); + let tasks: Vec<_> = (0..9).map(task).collect(); for (token, t) in tasks.iter().enumerate() { w.register(t, token as u64); let ticket = prepare(&CurrentTask::new(t, C0), Cancel::Answers, WaitClass::Other) @@ -154,21 +191,29 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth clean("posting in place"); assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post in place fired every entry"); - // Every entry is dead now, and a withdrawn one joins them: these two - // registrations sweep all ten. + // Every entry is dead now, and a withdrawn one joins them: these three + // registrations sweep all ten, more than one section takes out. let withdrawn = Arc::new(Poll::default()); - w.add_ring(Entry(withdrawn.clone())); + let first = Entry(withdrawn.clone()); + assert_eq!(sections(|| w.add_ring(first)), 1, "a re-arm after a post in place took the lock twice"); withdrawn.0.store(2, Relaxed); - w.add_ring(Entry(Arc::new(Poll::default()))); + let fresh: Vec<_> = (0..2).map(|_| Arc::new(Poll::default())).collect(); + for poll in &fresh { + w.add_ring(Entry(poll.clone())); + } clean("sweeping"); assert!(polls.iter().all(|p| Arc::strong_count(p) == 1), "the sweep let go of every fired entry"); + assert_eq!(Arc::strong_count(&withdrawn), 1, "the sweep let go of the withdrawn entry"); - // A thread's post frees what it fired, with the lock let go. + // A thread's post frees what it fired, with the lock let go, and leaves + // the list's buffer to the next registration. let later = Arc::new(Poll::default()); w.add_ring(Entry(later.clone())); w.post(WakeCause::new(WakeReason::Woken), &env); clean("posting"); assert_eq!(Arc::strong_count(&later), 1, "the post let go of the entry it fired"); + let rearmed = Entry(Arc::new(Poll::default())); + assert_eq!(sections(|| w.add_ring(rearmed)), 1, "a re-arm after a post regrew the list"); assert_eq!(w.post_n(3, 1, WakeCause::new(WakeReason::Woken), &env), 0, "every waiter is claimed"); assert_eq!(w.revoke(|token| token % 2 == 0, WakeCause::new(WakeReason::Woken), &env), 5); @@ -182,3 +227,30 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth drop(w); clean("dropping"); } + +/// A registration that finds the list full sizes a bigger buffer with the lock +/// let go. Registrations that fill the list past that buffer before it comes +/// back leave it too small to take the list, and moving the list into it then +/// would grow it under the lock. +#[test] +fn a_list_outgrowing_the_buffer_sized_for_it_is_not_moved_into_it() { + let w = Arc::new(watch()); + let tasks: Vec<_> = (0..17).map(task).collect(); + // Four fill the list's first buffer, so the fifth grows it. + for t in &tasks[..4] { + w.register(t, 0); + } + // Twelve more fill it to sixteen, the buffer the fifth sized holds eight. + let (others, rest) = (w.clone(), tasks[5..].to_vec()); + stage(2, move || { + for t in &rest { + others.register(t, 0); + } + }); + w.register(&tasks[4], 0); + clean("growing past a buffer sized before"); + assert_eq!(w.threads(), 17, "a registration was lost"); + for t in &tasks { + w.unregister(t); + } +} From 5ed2f6e066d9435bc35dc8920d287b16665d4afb Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 16:01:07 +0200 Subject: [PATCH 07/14] Stage 6 stays open for the panel ruling, and step 2's exit can fail Answers the first review of #634, its issue half. - Step 1 says what this branch now does: only the watches a handler posts, and the completions of a ring they complete into, mask interrupts; every other watch's lock leaves them open. Its exit names both of `handler-post`'s arms. - Step 2's exit is a bound: the windows are read on the T14, which is x86 metal, and neither may be longer at step 1's head than at stage 6's start. Its reason to follow step 1, the ARM track, is deleted: the reading it names is x86. - Steps 3 and 4 land with #592's i8042 stage and with usbd, not alone, and stage 6 stays open past step 5 until the owner rules on the panel the dump paints. - `every-wait-in-this-kernel-is-a-spin.md` no longer lists "posting from interrupt context" among its rejected decisions: the track has said handlers post since it opened, and this branch makes them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- .../every-wait-in-this-kernel-is-a-spin.md | 2 +- ...-small-interrupts-post-and-threads-wait.md | 25 +++++++++++-------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/issues/kernel/every-wait-in-this-kernel-is-a-spin.md b/issues/kernel/every-wait-in-this-kernel-is-a-spin.md index 2178d4a4684..dd6c01cb031 100644 --- a/issues/kernel/every-wait-in-this-kernel-is-a-spin.md +++ b/issues/kernel/every-wait-in-this-kernel-is-a-spin.md @@ -119,7 +119,7 @@ both before any lock conversion; the order is forced, not preferred. - A watch is a node the waiter lends to the object, and the subject is a borrowed reference, never an id. **Rejected:** a global registry, a slot - arena, two park channels, posting from interrupt context, multishot polls, + arena, two park channels, multishot polls, userspace-only blocking wrappers, a sleep lock that spins where it cannot park, poisoning, and shootdown-as-completion. A freed object cannot be named. - The park token proves the *context* may park and never encodes which locks are diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 9412a0554fc..63a16dfd657 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -150,23 +150,28 @@ times: list in `drain_irqs` and the idle loop's device checks are gone by step 5. Each step measures the kernel's lines, and from step 2 the longest interrupts-off and preemption-off windows, against stage 6's first commit. - 1. **Interrupts post.** A post is legal in a handler: a watch's list and a - poll ring's completions sit behind interrupts-off leaf locks nothing - allocates or frees under. A claimed function's vector, the + Steps 3 and 4 do not land alone: they land with #592's i8042 stage and + with usbd. Stage 6 stays open past step 5 until the owner rules on the + panel the dump paints. + 1. **Interrupts post.** A post is legal in a handler: the watches a handler + posts, and the completions of a ring they complete into, sit behind + interrupts-off locks nothing allocates or frees under, and every other + watch's lock leaves interrupts open. A claimed function's vector, the IOMMU's refusal and both audio backends post from the handler, and `irq_ring`'s `UserDev` and `Audio` and their arms in `drain_irqs` go. The thread is the holder's: netd's, blockd's, soundd's mix thread, and the `isa` claim's when #592 lands. **Exit**: `handler_post_without_a_pass`, - a vector taken inside a post of its own watch on a CPU holding - preemption off posting once the outer post lets go and before any pass, - red on the base; the watch's loom models over the new post. + a vector taken on a CPU holding preemption off, inside a post of its own + watch or inside a completion into a ring polling it, posting once that + section lets go and before any pass, red on the base; the watch's loom + models over the new post. 2. **The windows, measured**: the longest interrupts-off and preemption-off windows per CPU, reported beside the IRQ census and fed by each architecture's masking primitives and entries, the number the ARM - track's stage 4 owes as well. After step 1 because the ARM track is - reshaping those primitives now; applied to stage 6's first commit for - the baseline. **Exit**: both windows read off the T14 at stage 6's start - and at step 1's head. + track's stage 4 owes as well. Applied to stage 6's first commit for + the baseline. **Exit**: both windows read on the T14, which is x86 + metal, at stage 6's start and at step 1's head, and neither is longer + at step 1's head than at the start. 3. **The i8042's thread is ps2server's** (#592's i8042 stage): `irq_ring`'s `I8042`, `keyboard_controller::service` and the idle loop's `verdict_due` go with the kernel's driver. **Exit**: that stage's. From fb0fc7b56bfe5d2e5f53aae9dcb193f8152d9205 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 16:05:18 +0200 Subject: [PATCH 08/14] The poll race model holds its watch past its assertion `poll_racing` moved its world into the producer thread, so the watch dropped when that thread ended, and a watch's drop answers every live entry as `Fire::Gone`, which the model's `Entry` counts as a post: a poll that neither the post nor the recheck completed read as completed once. With the producer posting before it sets `ready`, both `a_poll_registered_racing_a_post*_completes_exactly_once` stayed green (EXIT=0). The world now outlives the assertion, so the model checks the fault's order it claims to: `Relaxed` on both sides, the list lock alone between them. Main's `a_poll_on_two_watches_racing_both_posts_completes_exactly_once` has the same shape and is outside this change; filed as `issues/build/the-two-watch-poll-model-counts-a-dropped-watchs-answer-as-its-completion.md`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...dropped-watchs-answer-as-its-completion.md | 23 +++++++++++++++++++ toyos-sched/loom/tests/loom_watch.rs | 12 ++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 issues/build/the-two-watch-poll-model-counts-a-dropped-watchs-answer-as-its-completion.md diff --git a/issues/build/the-two-watch-poll-model-counts-a-dropped-watchs-answer-as-its-completion.md b/issues/build/the-two-watch-poll-model-counts-a-dropped-watchs-answer-as-its-completion.md new file mode 100644 index 00000000000..899c9c18517 --- /dev/null +++ b/issues/build/the-two-watch-poll-model-counts-a-dropped-watchs-answer-as-its-completion.md @@ -0,0 +1,23 @@ +--- +status: open +kind: tooling +opened: 2026-09-30 +--- + +# The two-watch poll model counts a dropped watch's answer as its completion + +`toyos-sched/loom/tests/loom_watch.rs`'s +`a_poll_on_two_watches_racing_both_posts_completes_exactly_once` moves both +worlds into their producer threads, so both watches drop before its assertion, +and a watch's drop fires every live entry as `Fire::Gone`, which the model's +`Entry` counts as a post. Its "never by neither" half cannot fail: a poll no +post and no recheck completed is completed by the drop. + +**Evidence:** with each producer posting before it makes its condition true (a +lost completion by construction), `cargo test -p toyos-sched-loom --test +loom_watch a_poll_on_two_watches` is EXIT=0, 1 passed. `poll_racing`, the +single-watch model beside it, had the same shape and holds its world past the +assertion since #634. + +**Exit:** the model keeps both worlds alive past its assertions, and the +mutation above reds it. diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index a7684096770..e4c5c32fb79 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -312,13 +312,17 @@ fn poll_racing(post: fn(&World)) { } }) }; - let producer = loom::thread::spawn(move || { - ready.store(true, Ordering::Relaxed); - post(&world); - }); + let producer = { + let world = world.clone(); + loom::thread::spawn(move || { + ready.store(true, Ordering::Relaxed); + post(&world); + }) + }; registrant.join().unwrap(); producer.join().unwrap(); + // While `world` lives: its watch's drop answers a live entry as gone. assert_eq!(poll.posts(), 1, "a poll over a ready object completes once"); } From 309153b6de337a139f0698273c5da5d82f2c7b08 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 16:43:52 +0200 Subject: [PATCH 09/14] The poll race model drops its world after its assertion, by name CI's `host` clippy step, with `redundant_clone` adopted, refused the producer's `world.clone()` in `poll_racing`: the original was only ever dropped, at the function's end. That drop's place is the point, since a watch's drop answers a live entry as gone, so it is now written where it happens, after the assertion, and the clone has a use. `cargo clippy -p toyos-sched-loom --all-targets` with the adopted lints and warnings denied: EXIT=101 before, EXIT=0 after. `cargo test -p toyos-sched-loom --test loom_watch`: EXIT=0, 10 passed; with the producer posting before it sets `ready`, EXIT=101, both `a_poll_registered_racing_a_post*_completes_exactly_once` failing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- toyos-sched/loom/tests/loom_watch.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index e4c5c32fb79..9b25c203439 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -324,6 +324,7 @@ fn poll_racing(post: fn(&World)) { // While `world` lives: its watch's drop answers a live entry as gone. assert_eq!(poll.posts(), 1, "a poll over a ready object completes once"); + drop(world); } /// The object's end racing its readiness: the poll is answered once, as ready From d6a04fafb9760d578fb35bc3245564cfc7b58c80 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 18:02:19 +0200 Subject: [PATCH 10/14] Answer the fourth review of #634: no post drops an entry, no handler frees, the ring's watch is tested, and the mask comes before the lock by type - `Watch::post` indexes its stack slots, so an entry past them panics rather than being dropped unfired under the list lock. `watch_list`'s new test posts FEW + 1 live entries, each the last owner of a heap cell, and asserts each fired once and none was freed under the lock; `FEW` is public for it. - `cancel_polls` is `Watch`'s alone. `IrqWatch` gets `cancel_polls_in_place`, over toyos-sched's `cancel_rings_in_place`, which fires every entry as gone where it stands and frees nothing; the sweep frees. The claim's release and `WatchRef::Irq`'s close answer through it, so a `cancel_polls()` written in `note_fault` is E0599. Unit test, `watch_list`'s no-free phase, and a loom model of an end in place racing a post in place. - `IrqLock` holds a `Masked` lock whose only way in is a borrow of a closed `IrqGuard`, so the lock can be taken neither before the mask nor held past it. - `handler-post` gains a third arm: the vector raised while the CPU holds the staged ring's own watch's list lock (toyos-sched's `holding`), with a live poll of that ring on the claim. It raises the last claim slot, which a claim takes only when every slot is held, and asserts it unheld; since #536 slot 0 is blockd's NVMe claim. - `fault-posted-before-it-is-set` is a standing `toyos-sched-loom` control: the ring models' producer posts before it stores the readiness, and both `a_poll_registered_racing_*` models red with a poll completed by neither. - Filed: a process lengthens an interrupts-off walk by the threads it parks on one ring, owned by stage 6 step 2. The aarch64 dispatcher rule goes into the ARM track's stage 6. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...alk-by-the-threads-it-parks-on-one-ring.md | 41 +++++++ ...-small-interrupts-post-and-threads-wait.md | 10 +- issues/kernel/toyos-runs-on-arm64.md | 10 ++ kernel/src/actuator.rs | 7 +- kernel/src/inbox/mod.rs | 11 +- kernel/src/object/ops.rs | 2 +- kernel/src/pcidev/mod.rs | 2 +- kernel/src/watch.rs | 105 +++++++++++++----- src/ci.rs | 7 ++ tests/toyos.rs | 8 +- toyos-sched/loom/Cargo.toml | 9 ++ toyos-sched/loom/tests/loom_watch.rs | 60 ++++++---- toyos-sched/src/watch.rs | 60 ++++++++-- toyos-sched/tests/watch_list.rs | 43 ++++++- 14 files changed, 298 insertions(+), 77 deletions(-) create mode 100644 issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md diff --git a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md new file mode 100644 index 00000000000..f23ef3e6b9f --- /dev/null +++ b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md @@ -0,0 +1,41 @@ +--- +status: assigned +kind: defect +opened: 2026-09-30 +--- + +# A process lengthens an interrupts-off walk by the threads it parks on one ring + +Held by the small-kernel track's stage 6 step 2 +(`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md`), +whose instrument is the only thing that can read it. + +Every poll ring's own watch and its completions sit behind an `IrqLock` +(`kernel/src/inbox/mod.rs:206`, `:208`), because a device handler's post +reaches them through the polls it fires. So any process, not only a device's +holder, decides how long a CPU runs with interrupts masked: + +- **N threads parked in `submit` on one ring** (`kernel/src/inbox/mod.rs:500`) + are N registrations on its watch. Every completion into that ring posts the + watch in place (`:326`), which notifies all N under the list lock + (`toyos-sched/src/watch.rs:188`), each a word exchange and, for a parked + thread, a mailbox push and perhaps an IPI (`toyos-sched/src/park.rs:117-126`). + Each woken thread's unregister is a `position` and a + `remove` over the N (`toyos-sched/src/watch.rs:139-143`), and a registration + that finds the list full copies it (`:221`), all with interrupts masked. +- **A claim's holder polling its claim from R rings, P polls each** (up to + `MAX_PENDING_WATCHES`, 1024, `kernel/src/inbox/mod.rs:199`) makes its device's + handler fire R × P entries under the claim's list lock, each taking that + ring's completions lock and posting that ring's watch, whose own N threads + it notifies. Entries a post or a cancel in place fired stay in the list until + registrations sweep them four at a time. + +Nothing caps N: a thread costs its process a 128 KiB kernel stack +(`kernel/src/process.rs:236`) and no count. Before #634 every one of these +walks ran with interrupts open, under preemption off. By reading, not +measured: no instrument in the tree reads an interrupts-off window. + +**Exit**: step 2's interrupts-off window, read on the T14 while one process +parks 256 threads in `submit` on one ring and a sibling thread completes into +it, is no longer at step 1's head than at stage 6's first commit under the +same load. diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 63a16dfd657..55933cb1a97 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -162,16 +162,18 @@ times: The thread is the holder's: netd's, blockd's, soundd's mix thread, and the `isa` claim's when #592 lands. **Exit**: `handler_post_without_a_pass`, a vector taken on a CPU holding preemption off, inside a post of its own - watch or inside a completion into a ring polling it, posting once that - section lets go and before any pass, red on the base; the watch's loom - models over the new post. + watch, inside a completion into a ring polling it, or inside that ring's + own watch, posting once that section lets go and before any pass, red on + the base; the watch's loom models over the new post. 2. **The windows, measured**: the longest interrupts-off and preemption-off windows per CPU, reported beside the IRQ census and fed by each architecture's masking primitives and entries, the number the ARM track's stage 4 owes as well. Applied to stage 6's first commit for the baseline. **Exit**: both windows read on the T14, which is x86 metal, at stage 6's start and at step 1's head, and neither is longer - at step 1's head than at the start. + at step 1's head than at the start, under the load + `issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md` + names as well. 3. **The i8042's thread is ps2server's** (#592's i8042 stage): `irq_ring`'s `I8042`, `keyboard_controller::service` and the idle loop's `verdict_due` go with the kernel's driver. **Exit**: that stage's. diff --git a/issues/kernel/toyos-runs-on-arm64.md b/issues/kernel/toyos-runs-on-arm64.md index 1e2566457de..c8776bef3ea 100644 --- a/issues/kernel/toyos-runs-on-arm64.md +++ b/issues/kernel/toyos-runs-on-arm64.md @@ -304,6 +304,16 @@ Each stage names its exit; "measured" means a number from a run. before its body) get a test here that reds with `put` written back as one `self.buf.write(off, trb)`; x86's TSO hides all three from every guest test until then. + **The claim's handler is the first arm of `irq()` + (`kernel/src/arch/aarch64/trap.rs:135`) that posts a watch or lets go of a + `Lock`**, and either runs `preempt::enable`, whose pass at depth zero + (`kernel/src/preempt.rs:67`) reads nothing of `DAIF`; `do_preempt`'s + `assert_baseline(BASELINE_IRQ_EXIT)` (`kernel/src/scheduler.rs:358`) passes + at depth zero, so that pass would run inside the handler, before + `irqchip::end`. Owed before that arm lands: `irq()` holds the preempt count + across every device arm, as x86-64's `device_irq_entry` does, or + `preempt::enable` refuses a pass with interrupts masked, which `IrqOff`'s + SAFETY (`kernel/src/sched/driver.rs:49`) already assumes. 7. **Userland boots.** `init`, `logd`, the compositor, netd, soundd and sshd, built for `aarch64-unknown-toyos`. C programs stay x86-only until this diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 7f96914184a..62cd6337455 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -254,9 +254,10 @@ actuators! { /// parking, so a post lands in the window its commit must refuse the park over. watch_window = "watch-window"; - /// Raise claim slot 0's vector inside a post of its own watch, and inside a - /// completion into a ring polling it, while the CPU holds preemption off, - /// and count whether the handler posted it there. + /// Raise an unheld claim slot's vector inside a post of its own watch, + /// inside a completion into a ring polling it, and inside that ring's own + /// watch, while the CPU holds preemption off, and count whether the + /// handler posted it there. handler_post = "handler-post"; /// Starve the four xHCI bring-up register waits in `init_one`. diff --git a/kernel/src/inbox/mod.rs b/kernel/src/inbox/mod.rs index 7370f15e36b..512d82ee0b7 100644 --- a/kernel/src/inbox/mod.rs +++ b/kernel/src/inbox/mod.rs @@ -414,8 +414,9 @@ pub(crate) struct Staged(Arc); #[cfg(feature = "boot-actuators")] impl Staged { pub(crate) fn new() -> Self { - // Room for every completion the actuator's holds write. - let depth = toyos_sched::watch::handler_post::HOLDS; + // Room for every completion the actuator's holds write: two per hold + // inside the completions, one per hold inside the watch. + let depth = 2 * toyos_sched::watch::handler_post::HOLDS; let shm = SharedMemObject::create(crate::mm::PAGE_2M).expect("handler-post: a ring's page"); let page = shm.phys_before_mapping(); write_ring_page(page, depth, depth * 2); @@ -445,6 +446,12 @@ impl Staged { pub(crate) fn complete(&self) { self.0.complete(0, 0); } + + /// Run `f` holding this ring's own watch's list lock, as a registration + /// in `submit` holds it. + pub(crate) fn holding_its_watch(&self, f: impl FnOnce()) { + self.0.watch.holding(f); + } } /// Processes submissions and waits for completions; called from the syscall handler. diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index 1d4265044b9..4ad96db3eea 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -260,7 +260,7 @@ impl WatchRef { match self { Self::Static(watch) => watch.cancel_polls(), Self::Shared(watch) => watch.cancel_polls(), - Self::Irq(watch) => watch.cancel_polls(), + Self::Irq(watch) => watch.cancel_polls_in_place(), } } } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 35a4e3afaec..ad133bf7cf8 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1486,7 +1486,7 @@ fn tear_down(slot: usize, mut bound: Bound) { } IRQ[slot].clear(); // The function is gone from this slot, so a poll on it is answered rather than left for the next holder's interrupts. - WATCHES[slot].cancel_polls(); + WATCHES[slot].cancel_polls_in_place(); log!( "pcidev: PCI {:02x}:{:02x}.{} [{:04x}:{:04x}] released from slot {slot}; reset by {how}", bound.pci.bus, diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index adde29e8237..fc350c6daa5 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -10,10 +10,10 @@ //! //! **A post allocates nothing and may be made under any lock but a poll //! ring's** (`crate::inbox`). A watch an interrupt handler posts is an -//! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its one post is made -//! in place and frees nothing. Every other watch's list lock leaves interrupts -//! open, and no handler takes it. Registration allocates, in the syscall that -//! registers. +//! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its post and its +//! cancel are made in place and free nothing. Every other watch's list lock +//! leaves interrupts open, and no handler takes it. Registration allocates, in +//! the syscall that registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] //! holds it, so an object cannot be freed under a thread waiting on it, and @@ -33,7 +33,6 @@ use crate::inbox::PollEntry; use crate::sched::driver::{cpus, preempt_off}; use crate::sched::payload::{KMsg, KShared, KernelLock, TaskHandle}; use crate::scheduler::Parkable; -use crate::sync::Lock; use crate::time::Deadline; type List = Waiters; @@ -45,18 +44,18 @@ pub struct Waitable>(toyos_sched::watch::Watch>; /// A watch an interrupt handler posts, and the watch of a ring such a post -/// completes into. It has no `post`, which frees: only -/// [`IrqWatch::post_in_place`]. +/// completes into. It has no `post` and no `cancel_polls`, which free: only +/// [`IrqWatch::post_in_place`] and [`IrqWatch::cancel_polls_in_place`]. pub type IrqWatch = Waitable>; /// What an interrupt handler's post takes: an [`IrqWatch`]'s list and a poll /// ring's completions. Held with interrupts off, so a handler never finds one /// held by the context it interrupted; nothing allocates or frees under it. -pub struct IrqLock(Lock); +pub struct IrqLock(masked::Masked); impl IrqLock { pub const fn new(value: T) -> Self { - Self(Lock::new(value)) + Self(masked::Masked::new(value)) } } @@ -65,17 +64,34 @@ impl CellLock for IrqLock { // Raised first, so the lock's own release never reaches depth zero, and // a pass, with interrupts masked. preempt_off(|_| { - let _irq = crate::arch::IrqGuard::close(); - let mut held = self.0.lock(); + let irq = crate::arch::IrqGuard::close(); + let mut held = self.0.lock(&irq); #[cfg(feature = "boot-actuators")] handler_post::raise_if_staged(); - let out = f(&mut held); - drop(held); - out + f(&mut held) }) } } +mod masked { + use crate::arch::IrqGuard; + use crate::sync::{Lock, LockGuard}; + + /// A lock taken only through a borrow of a closed [`IrqGuard`]: never + /// before the mask, and never held past it. + pub struct Masked(Lock); + + impl Masked { + pub const fn new(value: T) -> Self { + Self(Lock::new(value)) + } + + pub fn lock<'a>(&'a self, _closed: &'a IrqGuard) -> LockGuard<'a, T> { + self.0.lock() + } + } +} + impl Watch { pub const fn new() -> Self { Self(toyos_sched::watch::Watch::new(KernelLock::new(Waiters::new()))) @@ -121,6 +137,11 @@ impl Watch { ) }) } + + /// Answer every poll registered here as gone: the source it watched ended. + pub fn cancel_polls(&self) { + self.0.cancel_rings(); + } } impl IrqWatch { @@ -140,6 +161,12 @@ impl IrqWatch { self.0.post_in_place(WakeCause::new(WakeReason::Woken), &env); }); } + + /// Answer every poll registered here as gone, where it stands, freeing + /// nothing: the source it watched ended. + pub fn cancel_polls_in_place(&self) { + self.0.cancel_rings_in_place(); + } } impl> Waitable { @@ -148,9 +175,10 @@ impl> Waitable { self.0.add_ring(entry); } - /// Answer every poll registered here as gone: the source it watched ended. - pub fn cancel_polls(&self) { - self.0.cancel_rings(); + /// `handler-post`'s stand where a registration holds the list lock. + #[cfg(feature = "boot-actuators")] + pub(crate) fn holding(&self, f: impl FnOnce()) { + self.0.holding(f); } } @@ -315,23 +343,29 @@ mod window { } } -/// `handler-post`: claim slot 0's vector, raised on this CPU while it holds -/// preemption off, posts that slot's watch from the handler before any pass -/// can run. Raised inside a post of the watch itself, the handler's post -/// follows once the outer one lets go; raised inside a completion written into -/// a ring that polls the watch, the handler's post completes that poll once the -/// writer lets go. A hold counts the posts of the watch made on its CPU, and no -/// other watch's, and lapses at its budget. One run of [`HOLDS`] holds per arm, -/// on whichever idle loop reaches it first with interrupts open; its verdict is -/// one [`Verdict`] line. +/// `handler-post`: the last claim slot's vector, which no claim holds, raised +/// on this CPU while it holds preemption off, posts that slot's watch from the +/// handler before any pass can run. Raised inside a post of the watch itself, +/// the handler's post follows once the outer one lets go; raised inside a +/// completion written into a ring that polls the watch, or inside that ring's +/// own watch's list lock, the handler's post completes that poll once the +/// section lets go. A hold counts the posts of the watch made on its CPU, and +/// no other watch's, and lapses at its budget. One run of [`HOLDS`] holds per +/// arm, on whichever idle loop reaches it first with interrupts open; its +/// verdict is one [`Verdict`] line. #[cfg(feature = "boot-actuators")] pub mod handler_post { use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering::Relaxed}; use toyos_sched::watch::handler_post::{Verdict, HOLDS}; + use crate::pcidev::{MAX_FUNCTIONS, VECTORS}; use crate::time::{Budget, Deadline, Duration}; + /// A claim takes the first free slot, so the last is held only when every + /// slot is. + const SLOT: usize = MAX_FUNCTIONS - 1; + const WINDOW: Budget = Budget::of( Duration::from_secs(1), "the hold is counted as lapsed, and the verdict line says so", @@ -349,8 +383,10 @@ pub mod handler_post { if RAN.load(Relaxed) || !crate::arch::cpu::interrupts_enabled() || RAN.swap(true, Relaxed) { return; } + // A held slot's driver would take counts its device never raised. + assert!(crate::pcidev::held_at(SLOT).is_none(), "handler-post: claim slot {SLOT} is held"); let me = crate::arch::percpu::cpu_id(); - let claim = crate::pcidev::watch(0); + let claim = crate::pcidev::watch(SLOT); let ring = crate::inbox::Staged::new(); let verdict = crate::sched::driver::preempt_off(|_| { HOLDING.store(me, Relaxed); @@ -364,8 +400,13 @@ pub mod handler_post { RAISE_INSIDE.store(me, Relaxed); ring.complete(); }); + // Where a submitter's registration holds it. + let in_a_rings_watch = holds(1, || { + ring.poll(claim); + ring.holding_its_watch(raise); + }); HOLDING.store(NOBODY, Relaxed); - Verdict { in_a_list, in_a_ring } + Verdict { in_a_list, in_a_ring, in_a_rings_watch } }); crate::log!("{verdict}"); } @@ -392,13 +433,17 @@ pub mod handler_post { posted } + fn raise() { + crate::arch::irqchip::send_self(VECTORS[SLOT]); + } + /// From inside an interrupts-off section: the staged CPU's next one raises /// the vector while it holds. pub fn raise_if_staged() { let staged = RAISE_INSIDE.load(Relaxed); if staged != NOBODY && staged == crate::arch::percpu::cpu_id() { RAISE_INSIDE.store(NOBODY, Relaxed); - crate::arch::irqchip::send_self(crate::pcidev::VECTORS[0]); + raise(); } } @@ -406,7 +451,7 @@ pub mod handler_post { let holding = HOLDING.load(Relaxed); if holding != NOBODY && holding == crate::arch::percpu::cpu_id() - && core::ptr::eq(watch, crate::pcidev::watch(0)) + && core::ptr::eq(watch, crate::pcidev::watch(SLOT)) { POSTS.fetch_add(1, Relaxed); } diff --git a/src/ci.rs b/src/ci.rs index 2fe5987d4e7..08e533522d4 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -307,6 +307,13 @@ pub(crate) const CONTROLS: &[Control] = &[ "a_poll_registered_racing_a_post_in_place_completes_exactly_once ... FAILED", "a_poll_on_two_watches_racing_both_posts_completes_exactly_once ... FAILED", ]), + // The ring models' lost-completion half: the producer posts before it + // stores the readiness its registrant rechecks. + red(SCHED_LOOM, "fault-posted-before-it-is-set", Some("loom_watch"), &[ + "a poll over a ready object was completed by neither", + "a_poll_registered_racing_a_post_completes_exactly_once ... FAILED", + "a_poll_registered_racing_a_post_in_place_completes_exactly_once ... FAILED", + ]), // Reproduces an open defect // (`issues/kernel/steal-probe-node-dies-with-its-victim.md`) rather than // proving a lie is caught, and goes with its fix. diff --git a/tests/toyos.rs b/tests/toyos.rs index a45ab6a1742..971a78faf95 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -1256,10 +1256,10 @@ const MACHINE_TESTS: &[(&str, Sched)] = &[ // waiter between reading its condition and parking, so the peer's post lands where // only the notified bit carries it to the commit. ("blocking_read_window", Sched::Parallel), - // A claimed function's vector posts its watch from the handler: raised on - // a CPU holding preemption off, inside a post of that watch or inside a - // completion into a ring polling it, it posts once that section lets go - // and before any pass. One boot; the verdict is counts. + // A claim slot's vector posts its watch from the handler: raised on a CPU + // holding preemption off, inside a post of that watch, inside a completion + // into a ring polling it or inside that ring's own watch, it posts once + // that section lets go and before any pass. One boot; the verdict is counts. ("handler_post_without_a_pass", Sched::Parallel), // A sibling's munmap and mmap staged between a typed copy's translation // and its store (`copy-meets-a-remap`): the store never reaches the region diff --git a/toyos-sched/loom/Cargo.toml b/toyos-sched/loom/Cargo.toml index e6a978c80a9..1c9dcfe234c 100644 --- a/toyos-sched/loom/Cargo.toml +++ b/toyos-sched/loom/Cargo.toml @@ -101,6 +101,15 @@ gate-fence-off = [] # cargo test -p toyos-sched-loom --release --features poll-fire-load-store --test loom_watch # poll-fire-load-store = [] +# The control for `poll_racing`'s lost-completion half: its producer posts, then +# stores the readiness, the reverse of the order `pcidev::note_fault` keeps, so +# both `*_racing_a_post*_completes_exactly_once` models must red with a poll +# completed by neither: +# +# cargo test -p toyos-sched-loom --features fault-posted-before-it-is-set --test loom_watch +# +# `toyos-sched` declares no twin, because this decides nothing in `../src`. +fault-posted-before-it-is-set = [] [dependencies] loom = "0.7" diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index 9b25c203439..3cf04a48328 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -18,14 +18,16 @@ //! cargo test -p toyos-sched-loom --features commit-ignores-notify --test loom_watch //! ``` //! -//! Three more controls: `notify-flag-load-only` lets a post that finds its bits +//! Four more controls: `notify-flag-load-only` lets a post that finds its bits //! already set answer off a load, and //! `a_second_post_is_not_lost_to_a_flag_the_waiter_consumed` must red; //! `gate-fence-off` removes the [`Gate`]'s two fences, and -//! `a_transition_racing_an_opening_gate_is_never_missed` must red; and +//! `a_transition_racing_an_opening_gate_is_never_missed` must red; //! `poll-fire-load-store`, the kernel's own control for the poll's one-shot //! answer, which the ring models below compile, must red every -//! `*_completes_exactly_once` model here. +//! `*_completes_exactly_once` model here; and `fault-posted-before-it-is-set` +//! posts before the readiness is stored, and both `a_poll_registered_racing_*` +//! models must red with a poll completed by neither. //! //! **The ring entry is the kernel's [`Once`], compiled from //! `kernel/src/inbox/once.rs`**, the decision a `PollEntry` makes; what else a @@ -315,14 +317,22 @@ fn poll_racing(post: fn(&World)) { let producer = { let world = world.clone(); loom::thread::spawn(move || { - ready.store(true, Ordering::Relaxed); - post(&world); + // `fault-posted-before-it-is-set` is the control: posted first, the + // readiness can land after both the post and the recheck. + if cfg!(feature = "fault-posted-before-it-is-set") { + post(&world); + ready.store(true, Ordering::Relaxed); + } else { + ready.store(true, Ordering::Relaxed); + post(&world); + } }) }; registrant.join().unwrap(); producer.join().unwrap(); // While `world` lives: its watch's drop answers a live entry as gone. + assert_ne!(poll.posts(), 0, "a poll over a ready object was completed by neither"); assert_eq!(poll.posts(), 1, "a poll over a ready object completes once"); drop(world); } @@ -331,22 +341,34 @@ fn poll_racing(post: fn(&World)) { /// or as gone, and whichever answered it no longer holds it. #[test] fn an_end_racing_a_post_answers_a_poll_once() { - model(|| { - let (world, _rx) = world(); - let poll = Entry::new(); - world.watch.add_ring(poll.clone()); + model(|| end_racing(|w| w.watch.cancel_rings(), World::post)); +} - let ender = { - let world = world.clone(); - loom::thread::spawn(move || world.watch.cancel_rings()) - }; - let poster = loom::thread::spawn(move || world.post()); - ender.join().unwrap(); - poster.join().unwrap(); +/// The same, for the end and the post a handler may make, both in place. +#[test] +fn an_end_in_place_racing_a_post_in_place_answers_a_poll_once() { + model(|| end_racing(|w| w.watch.cancel_rings_in_place(), World::post_in_place)); +} - assert_eq!(poll.posts(), 1); - assert!(!poll.live()); - }); +fn end_racing(end: fn(&World), post: fn(&World)) { + let (world, _rx) = world(); + let poll = Entry::new(); + world.watch.add_ring(poll.clone()); + + let ender = { + let world = world.clone(); + loom::thread::spawn(move || end(&world)) + }; + let poster = { + let world = world.clone(); + loom::thread::spawn(move || post(&world)) + }; + ender.join().unwrap(); + poster.join().unwrap(); + + assert_eq!(poll.posts(), 1); + assert!(!poll.live()); + drop(world); } /// One waiter, two producers, each storing its own condition and then posting. diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index f45e3154805..6575cd7a82d 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -21,9 +21,9 @@ //! can no longer fire out [`FEW`] at a time, grows a full list in a buffer //! allocated with the lock let go, and drops both with it let go. //! [`Watch::post`] takes every ring entry out and fires and frees them with the -//! lock let go; [`Watch::post_in_place`], for a handler, which may not free at -//! all, fires them where they stand, and later registrations sweep the dead -//! out, since an entry is one-shot. +//! lock let go; [`Watch::post_in_place`] and [`Watch::cancel_rings_in_place`], +//! for a handler, which may not free at all, fire them where they stand, and +//! later registrations sweep the dead out, since an entry is one-shot. //! //! **Lock order.** A post in place fires its rings under the list lock, so //! beneath it are each ring's own lock and the watch that ring's submitters @@ -107,7 +107,7 @@ pub struct Poster<'a, M, K, P> { /// How many ring entries one section takes out of the list to drop with the /// lock let go, on the stack and so allocating nothing. -const FEW: usize = 4; +pub const FEW: usize = 4; pub struct Watch>> { list: L, @@ -164,8 +164,10 @@ impl>> Watch { if w.rings.len() > FEW { return core::mem::take(&mut w.rings); } - for (slot, ring) in few.iter_mut().zip(w.rings.drain(..)) { - *slot = Some(ring); + // Indexed, so an entry with no slot panics rather than being + // dropped unfired. + for (at, ring) in w.rings.drain(..).enumerate() { + few[at] = Some(ring); } Vec::new() }); @@ -299,6 +301,23 @@ impl>> Watch { } } + /// [`Self::cancel_rings`] for a context that may not free: every ring + /// entry is fired as [`Fire::Gone`] where it stands, under the list lock, + /// and later registrations sweep it. + pub fn cancel_rings_in_place(&self) { + self.list.with(|w| { + for ring in &w.rings { + ring.fire(Fire::Gone); + } + }); + } + + /// Run `f` holding the list lock, where a registration holds it, touching + /// nothing on the list: an actuator's way to raise an interrupt there. + pub fn holding(&self, f: impl FnOnce() -> U) -> U { + self.list.with(|_| f()) + } + /// Registered threads, for a model or a report. pub fn threads(&self) -> usize { self.list.with(|w| w.threads.len()) @@ -399,24 +418,27 @@ pub mod handler_post { pub const HOLDS: u32 = 4; /// The holds a handler's post ended, per arm: its vector raised inside a - /// watch's list lock, and inside a ring's completions. + /// watch's list lock, inside a ring's completions, and inside the list + /// lock of the watch that ring's own submitters park on. #[derive(Clone, Copy)] pub struct Verdict { pub in_a_list: u32, pub in_a_ring: u32, + pub in_a_rings_watch: u32, } impl Verdict { - pub const GREEN: Self = Self { in_a_list: HOLDS, in_a_ring: HOLDS }; + pub const GREEN: Self = + Self { in_a_list: HOLDS, in_a_ring: HOLDS, in_a_rings_watch: HOLDS }; } impl fmt::Display for Verdict { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, - "{SAID} of {HOLDS} holds per arm, a handler posted into {} inside a list lock \ - and {} inside a ring's completions", - self.in_a_list, self.in_a_ring, + "{SAID} of {HOLDS} holds per arm, a handler posted into {} inside a list lock, \ + {} inside a ring's completions and {} inside a ring's own watch", + self.in_a_list, self.in_a_ring, self.in_a_rings_watch, ) } } @@ -773,6 +795,22 @@ mod tests { assert_eq!(dropped.posts.load(Ordering::Acquire), 1); } + /// A cancel in place answers where the entry stands and lets go of + /// nothing, as a post in place does. + #[test] + fn a_cancel_in_place_answers_every_live_poll_as_gone_and_drops_nothing() { + let w = watch(); + let poll = Arc::new(Poll::default()); + w.add_ring(poll.clone()); + w.cancel_rings_in_place(); + assert_eq!(poll.state.load(Ordering::Acquire), 2); + assert_eq!(Arc::strong_count(&poll), 2, "the cancel let go of the entry it fired"); + let t = task(1); + w.register(&t, 0); + assert_eq!(Arc::strong_count(&poll), 1, "the registration did not sweep it"); + w.unregister(&t); + } + #[test] #[should_panic(expected = "a task waits on at most one watch")] fn a_second_registration_is_loud() { diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs index 8e70b32ca2d..118406fbb3d 100644 --- a/toyos-sched/tests/watch_list.rs +++ b/toyos-sched/tests/watch_list.rs @@ -17,7 +17,7 @@ use toyos_sched::mailbox::{mailbox, PreemptGuard, SchedMsg}; use toyos_sched::park::{prepare, Cancel, Commit, CurrentTask}; use toyos_sched::sync::CellLock; use toyos_sched::task::{TaskKey, TaskShared, TaskState, WaitClass, WakeCause, WakeReason}; -use toyos_sched::watch::{Fire, Poster, Ring, Waiters, Watch}; +use toyos_sched::watch::{Fire, Poster, Ring, Waiters, Watch, FEW}; thread_local! { static HELD: Cell = const { Cell::new(false) }; @@ -187,8 +187,9 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth POSTING.set(true); w.post_in_place(WakeCause::new(WakeReason::Woken), &env); + w.cancel_rings_in_place(); POSTING.set(false); - clean("posting in place"); + clean("posting and cancelling in place"); assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post in place fired every entry"); // Every entry is dead now, and a withdrawn one joins them: these three @@ -228,6 +229,44 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth clean("dropping"); } +/// A live entry that is the last owner of a heap cell, so its drop frees, and +/// that counts its fires on a word the test keeps. +struct Owned { + fired: Arc, + _cell: Box, +} + +impl Ring for Owned { + fn fire(&self, _how: Fire) { + self.fired.fetch_add(1, AcqRel); + } + fn live(&self) -> bool { + self.fired.load(Acquire) == 0 + } +} + +/// One entry past the [`FEW`] a post takes out on its stack: every entry is +/// fired once, and freed with the list lock let go. +#[test] +fn a_post_of_one_entry_past_its_stack_fires_each_once_and_frees_none_under_the_lock() { + let (tx, _rx) = mailbox::(); + let cpus = CpuHandles::new(vec![CpuHandle::new(C0, tx)]); + let env = Poster { cpus: &cpus, kicker: &NoKick, preempt: &NoPreempt }; + let w: Watch>> = + Watch::new(Watched(Mutex::new(Waiters::new()))); + let fired: Vec<_> = (0..=FEW).map(|_| Arc::new(AtomicU32::new(0))).collect(); + for word in &fired { + w.add_ring(Owned { fired: word.clone(), _cell: Box::new(0) }); + } + clean("registering"); + w.post(WakeCause::new(WakeReason::Woken), &env); + clean("posting"); + for (at, word) in fired.iter().enumerate() { + assert_eq!(word.load(Acquire), 1, "entry {at} of {} fired other than once", FEW + 1); + assert_eq!(Arc::strong_count(word), 1, "entry {at} was not let go of"); + } +} + /// A registration that finds the list full sizes a bigger buffer with the lock /// let go. Registrations that fill the list past that buffer before it comes /// back leave it too small to take the list, and moving the list into it then From f4ed0e7b838fb9472ff1e51d021258778c1544cb Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 19:08:55 +0200 Subject: [PATCH 11/14] Answer the fifth review of #634: a released claim's polls are let go of, and the post and cancel bounds are tested at FEW - A claim's release and a device handle's close run in thread context, so after answering their polls in place they sweep them: toyos-sched's new `Watch::sweep` takes the entries that can no longer fire out FEW a section and drops each section with the list lock let go, and `IrqWatch::sweep` is its thread's door. Before this, every entry fired at those two sites, each holding its ring's `Arc`, stayed on the static watch until a registration on it swept it, which for a slot never claimed again is the rest of the boot. The dead-entry take is one function, `take_dead`, shared by a registration and the sweep. - `watch_list` posts exactly FEW live entries and asserts the re-arm after the post is one section, which reds under `>= FEW`; the FEW + 1 test shares its helper. - The cancel-in-place unit test registers FEW + 1 live polls and asserts each is answered as gone and kept, which reds under `.take(FEW)`. - The sweep's tests: a unit test over 2 * FEW + 1 withdrawn entries and a live one, and a `watch_list` test that the sweep after a cancel in place frees nothing under the lock. - Deleted: the `handler-post` slot comment, false since `toyos_pci::slot::reserve` gives a claim its own residue slot first, and the staged ring's room comment, on which nothing depends. - The walk issue's citations follow the lines this moved, and it no longer says a cancel in place leaves its entries for registrations. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...alk-by-the-threads-it-parks-on-one-ring.md | 10 +-- kernel/src/inbox/mod.rs | 2 - kernel/src/object/ops.rs | 5 +- kernel/src/pcidev/mod.rs | 1 + kernel/src/watch.rs | 19 +++-- toyos-sched/src/watch.rs | 81 ++++++++++++++----- toyos-sched/tests/watch_list.rs | 60 +++++++++++--- 7 files changed, 134 insertions(+), 44 deletions(-) diff --git a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md index f23ef3e6b9f..73016ca4e71 100644 --- a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md +++ b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md @@ -15,19 +15,19 @@ Every poll ring's own watch and its completions sit behind an `IrqLock` reaches them through the polls it fires. So any process, not only a device's holder, decides how long a CPU runs with interrupts masked: -- **N threads parked in `submit` on one ring** (`kernel/src/inbox/mod.rs:500`) +- **N threads parked in `submit` on one ring** (`kernel/src/inbox/mod.rs:498`) are N registrations on its watch. Every completion into that ring posts the watch in place (`:326`), which notifies all N under the list lock - (`toyos-sched/src/watch.rs:188`), each a word exchange and, for a parked + (`toyos-sched/src/watch.rs:189`), each a word exchange and, for a parked thread, a mailbox push and perhaps an IPI (`toyos-sched/src/park.rs:117-126`). Each woken thread's unregister is a `position` and a - `remove` over the N (`toyos-sched/src/watch.rs:139-143`), and a registration - that finds the list full copies it (`:221`), all with interrupts masked. + `remove` over the N (`toyos-sched/src/watch.rs:140-144`), and a registration + that finds the list full copies it (`:213`), all with interrupts masked. - **A claim's holder polling its claim from R rings, P polls each** (up to `MAX_PENDING_WATCHES`, 1024, `kernel/src/inbox/mod.rs:199`) makes its device's handler fire R × P entries under the claim's list lock, each taking that ring's completions lock and posting that ring's watch, whose own N threads - it notifies. Entries a post or a cancel in place fired stay in the list until + it notifies. Entries a post in place fired stay in the list until registrations sweep them four at a time. Nothing caps N: a thread costs its process a 128 KiB kernel stack diff --git a/kernel/src/inbox/mod.rs b/kernel/src/inbox/mod.rs index 512d82ee0b7..8e6ed014fd1 100644 --- a/kernel/src/inbox/mod.rs +++ b/kernel/src/inbox/mod.rs @@ -414,8 +414,6 @@ pub(crate) struct Staged(Arc); #[cfg(feature = "boot-actuators")] impl Staged { pub(crate) fn new() -> Self { - // Room for every completion the actuator's holds write: two per hold - // inside the completions, one per hold inside the watch. let depth = 2 * toyos_sched::watch::handler_post::HOLDS; let shm = SharedMemObject::create(crate::mm::PAGE_2M).expect("handler-post: a ring's page"); let page = shm.phys_before_mapping(); diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index 4ad96db3eea..c3dba8f48ad 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -260,7 +260,10 @@ impl WatchRef { match self { Self::Static(watch) => watch.cancel_polls(), Self::Shared(watch) => watch.cancel_polls(), - Self::Irq(watch) => watch.cancel_polls_in_place(), + Self::Irq(watch) => { + watch.cancel_polls_in_place(); + watch.sweep(); + } } } } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index ad133bf7cf8..b8e87c231dd 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1487,6 +1487,7 @@ fn tear_down(slot: usize, mut bound: Bound) { IRQ[slot].clear(); // The function is gone from this slot, so a poll on it is answered rather than left for the next holder's interrupts. WATCHES[slot].cancel_polls_in_place(); + WATCHES[slot].sweep(); log!( "pcidev: PCI {:02x}:{:02x}.{} [{:04x}:{:04x}] released from slot {slot}; reset by {how}", bound.pci.bus, diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index fc350c6daa5..2178a9d99c7 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -11,9 +11,9 @@ //! **A post allocates nothing and may be made under any lock but a poll //! ring's** (`crate::inbox`). A watch an interrupt handler posts is an //! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its post and its -//! cancel are made in place and free nothing. Every other watch's list lock -//! leaves interrupts open, and no handler takes it. Registration allocates, in -//! the syscall that registers. +//! cancel are made in place and free nothing; the thread that ends its source +//! sweeps. Every other watch's list lock leaves interrupts open, and no +//! handler takes it. Registration allocates, in the syscall that registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] //! holds it, so an object cannot be freed under a thread waiting on it, and @@ -44,8 +44,9 @@ pub struct Waitable>(toyos_sched::watch::Watch>; /// A watch an interrupt handler posts, and the watch of a ring such a post -/// completes into. It has no `post` and no `cancel_polls`, which free: only -/// [`IrqWatch::post_in_place`] and [`IrqWatch::cancel_polls_in_place`]. +/// completes into. It has no `post` and no `cancel_polls`, which free: +/// [`IrqWatch::post_in_place`] and [`IrqWatch::cancel_polls_in_place`] free +/// nothing, and [`IrqWatch::sweep`] is a thread's. pub type IrqWatch = Waitable>; /// What an interrupt handler's post takes: an [`IrqWatch`]'s list and a poll @@ -167,6 +168,12 @@ impl IrqWatch { pub fn cancel_polls_in_place(&self) { self.0.cancel_rings_in_place(); } + + /// Let go of every poll a post or a cancel in place answered. A thread's, + /// since it frees: where the source ended, no registration comes to. + pub fn sweep(&self) { + self.0.sweep(); + } } impl> Waitable { @@ -362,8 +369,6 @@ pub mod handler_post { use crate::pcidev::{MAX_FUNCTIONS, VECTORS}; use crate::time::{Budget, Deadline, Duration}; - /// A claim takes the first free slot, so the last is held only when every - /// slot is. const SLOT: usize = MAX_FUNCTIONS - 1; const WINDOW: Budget = Budget::of( diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index 6575cd7a82d..0520ac93b98 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -24,6 +24,7 @@ //! lock let go; [`Watch::post_in_place`] and [`Watch::cancel_rings_in_place`], //! for a handler, which may not free at all, fire them where they stand, and //! later registrations sweep the dead out, since an entry is one-shot. +//! [`Watch::sweep`] lets go of them where no registration will come. //! //! **Lock order.** A post in place fires its rings under the list lock, so //! beneath it are each ring's own lock and the watch that ring's submitters @@ -204,16 +205,7 @@ impl>> Watch { loop { let mut dead: [Option; FEW] = [const { None }; FEW]; let full = self.list.with(|w| { - let (mut at, mut taken) = (0, 0); - // Order is nothing to a ring entry: every post fires them all. - while at < w.rings.len() && taken < FEW { - if w.rings[at].live() { - at += 1; - } else { - dead[taken] = Some(w.rings.swap_remove(at)); - taken += 1; - } - } + take_dead(&mut w.rings, &mut dead); let v = list(w); // Only a buffer that holds the whole list and `item`: another // registration may have outgrown it since it was sized. @@ -312,6 +304,18 @@ impl>> Watch { }); } + /// Let go of every ring entry that can no longer fire, [`FEW`] a section, + /// each section's dropped with the list lock let go: for a thread, after + /// an end in place that no registration may follow. + pub fn sweep(&self) { + loop { + let mut dead: [Option; FEW] = [const { None }; FEW]; + if self.list.with(|w| take_dead(&mut w.rings, &mut dead)) < FEW { + return; + } + } + } + /// Run `f` holding the list lock, where a registration holds it, touching /// nothing on the list: an actuator's way to raise an interrupt there. pub fn holding(&self, f: impl FnOnce() -> U) -> U { @@ -329,6 +333,22 @@ impl>> Watch { } } +/// Take up to [`FEW`] entries that can no longer fire out of `rings` into +/// `dead`, and answer how many. +fn take_dead(rings: &mut Vec, dead: &mut [Option; FEW]) -> usize { + let (mut at, mut taken) = (0, 0); + // Order is nothing to a ring entry: every post fires them all. + while at < rings.len() && taken < FEW { + if rings[at].live() { + at += 1; + } else { + dead[taken] = Some(rings.swap_remove(at)); + taken += 1; + } + } + taken +} + /// A word in front of a watch that its posters read to learn whether a post is /// owed at all, for a waiter whose condition is a sweep of words the posters /// write: the machine's stop, whose posters are every thread's park, band and @@ -795,20 +815,43 @@ mod tests { assert_eq!(dropped.posts.load(Ordering::Acquire), 1); } - /// A cancel in place answers where the entry stands and lets go of + /// A cancel in place answers where each entry stands and lets go of /// nothing, as a post in place does. #[test] fn a_cancel_in_place_answers_every_live_poll_as_gone_and_drops_nothing() { let w = watch(); - let poll = Arc::new(Poll::default()); - w.add_ring(poll.clone()); + let polls: Vec<_> = (0..=FEW).map(|_| Arc::new(Poll::default())).collect(); + for poll in &polls { + w.add_ring(poll.clone()); + } w.cancel_rings_in_place(); - assert_eq!(poll.state.load(Ordering::Acquire), 2); - assert_eq!(Arc::strong_count(&poll), 2, "the cancel let go of the entry it fired"); - let t = task(1); - w.register(&t, 0); - assert_eq!(Arc::strong_count(&poll), 1, "the registration did not sweep it"); - w.unregister(&t); + for (at, poll) in polls.iter().enumerate() { + assert_eq!(poll.state.load(Ordering::Acquire), 2, "poll {at} was not answered as gone"); + assert_eq!(Arc::strong_count(poll), 2, "the cancel let go of poll {at}"); + } + } + + /// Where no registration follows an end in place, the sweep lets go of + /// every entry that can no longer fire, past the [`FEW`] one section + /// takes, and of no live one. + #[test] + fn a_sweep_lets_go_of_every_entry_that_can_no_longer_fire() { + let w = watch(); + let live = Arc::new(Poll::default()); + w.add_ring(live.clone()); + let dead: Vec<_> = (0..=2 * FEW).map(|_| Arc::new(Poll::default())).collect(); + for poll in &dead { + w.add_ring(poll.clone()); + } + for poll in &dead { + poll.withdraw(); + } + w.sweep(); + for (at, poll) in dead.iter().enumerate() { + assert_eq!(Arc::strong_count(poll), 1, "the sweep kept dead entry {at} of {}", dead.len()); + } + assert_eq!(Arc::strong_count(&live), 2, "the sweep let go of a live entry"); + assert_eq!(w.live_rings(), 1); } #[test] diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs index 118406fbb3d..b5637bbd131 100644 --- a/toyos-sched/tests/watch_list.rs +++ b/toyos-sched/tests/watch_list.rs @@ -245,25 +245,65 @@ impl Ring for Owned { } } -/// One entry past the [`FEW`] a post takes out on its stack: every entry is -/// fired once, and freed with the list lock let go. -#[test] -fn a_post_of_one_entry_past_its_stack_fires_each_once_and_frees_none_under_the_lock() { +type OwnedWatch = Watch>>; + +fn owned(fired: &Arc) -> Owned { + Owned { fired: fired.clone(), _cell: Box::new(0) } +} + +/// A thread's post of `n` live entries: every entry is fired once, and freed +/// with the list lock let go. +fn post_live(n: usize) -> OwnedWatch { let (tx, _rx) = mailbox::(); let cpus = CpuHandles::new(vec![CpuHandle::new(C0, tx)]); let env = Poster { cpus: &cpus, kicker: &NoKick, preempt: &NoPreempt }; - let w: Watch>> = - Watch::new(Watched(Mutex::new(Waiters::new()))); - let fired: Vec<_> = (0..=FEW).map(|_| Arc::new(AtomicU32::new(0))).collect(); + let w: OwnedWatch = Watch::new(Watched(Mutex::new(Waiters::new()))); + let fired: Vec<_> = (0..n).map(|_| Arc::new(AtomicU32::new(0))).collect(); for word in &fired { - w.add_ring(Owned { fired: word.clone(), _cell: Box::new(0) }); + w.add_ring(owned(word)); } clean("registering"); w.post(WakeCause::new(WakeReason::Woken), &env); clean("posting"); for (at, word) in fired.iter().enumerate() { - assert_eq!(word.load(Acquire), 1, "entry {at} of {} fired other than once", FEW + 1); - assert_eq!(Arc::strong_count(word), 1, "entry {at} was not let go of"); + assert_eq!(word.load(Acquire), 1, "entry {at} of {n} fired other than once"); + assert_eq!(Arc::strong_count(word), 1, "entry {at} of {n} was not let go of"); + } + w +} + +/// One entry past the [`FEW`] a post takes out on its stack. +#[test] +fn a_post_of_one_entry_past_its_stack_fires_each_once_and_frees_none_under_the_lock() { + post_live(FEW + 1); +} + +/// Exactly the [`FEW`] a post takes out on its stack: the list's buffer is +/// left to the re-arm. +#[test] +fn a_re_arm_after_a_post_of_few_entries_is_one_section() { + let w = post_live(FEW); + let rearmed = owned(&Arc::new(AtomicU32::new(0))); + assert_eq!(sections(|| w.add_ring(rearmed)), 1, "a re-arm after a post regrew the list"); +} + +/// An end in place and the sweep after it: the sweep lets go of every entry, +/// past the [`FEW`] one section takes, and frees each with the lock let go. +#[test] +fn a_sweep_frees_none_under_the_lock() { + let w: OwnedWatch = Watch::new(Watched(Mutex::new(Waiters::new()))); + let fired: Vec<_> = (0..=2 * FEW).map(|_| Arc::new(AtomicU32::new(0))).collect(); + for word in &fired { + w.add_ring(owned(word)); + } + POSTING.set(true); + w.cancel_rings_in_place(); + POSTING.set(false); + clean("cancelling in place"); + w.sweep(); + clean("sweeping"); + for (at, word) in fired.iter().enumerate() { + assert_eq!(Arc::strong_count(word), 1, "entry {at} of {} was not let go of", fired.len()); } } From 485e66fc6a0e43065b1131c4156c17d86d28d688 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 20:42:41 +0200 Subject: [PATCH 12/14] Answer the sixth review of #634: one cancel for every watch, a thread's The claim's release and the close of a claim or an audio device answered their polls with `cancel_polls_in_place` and then `sweep`: a second path to the end state `cancel_rings` already reaches, and a dearer one. It fired every entry under the claim's interrupts-off list lock and then took a masked section per four dead entries, where `cancel_rings` takes the list out in one masked section and fires and frees with the lock let go. Both callers are threads, and with `sweep` on `IrqWatch` a handler could free already, so keeping `cancel_polls` off `IrqWatch` protected nothing. - `cancel_polls` moves to `Waitable`, so every watch has it, and both thread sites make that one call. `pcidev::release`'s line is main's again. - `IrqWatch::cancel_polls_in_place`, `IrqWatch::sweep`, `Watch::cancel_rings_in_place` and `Watch::sweep` are deleted, with the three tests that held them. - `cancel_and_drop_answer_every_live_poll_as_gone` registers one entry past `FEW`, so a cancel or a drop bounded at a section's size reds. - The in-place end model becomes the race the kernel has: a thread's `cancel_rings` against a handler's `post_in_place`. - The walk issue loses every line citation, and states the retention `close_all` leaves on `AUDIO_WATCH` with its bound: `close_all` answering polls would answer other processes' polls on every shared object at every exit, since `close_ends_polls` is true of pipes, which this change does not decide. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...alk-by-the-threads-it-parks-on-one-ring.md | 26 ++++-- kernel/src/object/ops.rs | 5 +- kernel/src/pcidev/mod.rs | 3 +- kernel/src/watch.rs | 35 +++----- toyos-sched/loom/tests/loom_watch.rs | 7 +- toyos-sched/src/watch.rs | 85 ++++--------------- toyos-sched/tests/watch_list.rs | 23 +---- 7 files changed, 52 insertions(+), 132 deletions(-) diff --git a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md index 73016ca4e71..50786fb3d70 100644 --- a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md +++ b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md @@ -11,27 +11,35 @@ Held by the small-kernel track's stage 6 step 2 whose instrument is the only thing that can read it. Every poll ring's own watch and its completions sit behind an `IrqLock` -(`kernel/src/inbox/mod.rs:206`, `:208`), because a device handler's post +(`kernel/src/inbox/mod.rs`), because a device handler's post reaches them through the polls it fires. So any process, not only a device's holder, decides how long a CPU runs with interrupts masked: -- **N threads parked in `submit` on one ring** (`kernel/src/inbox/mod.rs:498`) +- **N threads parked in `submit` on one ring** are N registrations on its watch. Every completion into that ring posts the - watch in place (`:326`), which notifies all N under the list lock - (`toyos-sched/src/watch.rs:189`), each a word exchange and, for a parked - thread, a mailbox push and perhaps an IPI (`toyos-sched/src/park.rs:117-126`). + watch in place, which notifies all N under the list lock + (`toyos-sched/src/watch.rs`), each a word exchange and, for a parked + thread, a mailbox push and perhaps an IPI (`toyos-sched/src/park.rs`). Each woken thread's unregister is a `position` and a - `remove` over the N (`toyos-sched/src/watch.rs:140-144`), and a registration - that finds the list full copies it (`:213`), all with interrupts masked. + `remove` over the N, and a registration + that finds the list full copies it, all with interrupts masked. - **A claim's holder polling its claim from R rings, P polls each** (up to - `MAX_PENDING_WATCHES`, 1024, `kernel/src/inbox/mod.rs:199`) makes its device's + `MAX_PENDING_WATCHES`, 1024) makes its device's handler fire R × P entries under the claim's list lock, each taking that ring's completions lock and posting that ring's watch, whose own N threads it notifies. Entries a post in place fired stay in the list until registrations sweep them four at a time. +- **A process that exits holding an audio device** leaves every entry its + rings registered on `AUDIO_WATCH` there: `close_all` answers no poll, and + the audio handler's post frees none. Each ring's teardown withdraws its + polls and lets go of its page, so an entry keeps only its `Poll` and its ring's + `Inbox`. They stay until registrations on `AUDIO_WATCH` take them out, four + each, and every audio interrupt walks them under the list lock until then. + By reading, a registration's sweep keeps a list no longer than the most + live entries it has held at once, at most 1024 per ring that polled it. Nothing caps N: a thread costs its process a 128 KiB kernel stack -(`kernel/src/process.rs:236`) and no count. Before #634 every one of these +(`kernel/src/process.rs`) and no count. Before #634 every one of these walks ran with interrupts open, under preemption off. By reading, not measured: no instrument in the tree reads an interrupts-off window. diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index c3dba8f48ad..1d4265044b9 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -260,10 +260,7 @@ impl WatchRef { match self { Self::Static(watch) => watch.cancel_polls(), Self::Shared(watch) => watch.cancel_polls(), - Self::Irq(watch) => { - watch.cancel_polls_in_place(); - watch.sweep(); - } + Self::Irq(watch) => watch.cancel_polls(), } } } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index b8e87c231dd..35a4e3afaec 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1486,8 +1486,7 @@ fn tear_down(slot: usize, mut bound: Bound) { } IRQ[slot].clear(); // The function is gone from this slot, so a poll on it is answered rather than left for the next holder's interrupts. - WATCHES[slot].cancel_polls_in_place(); - WATCHES[slot].sweep(); + WATCHES[slot].cancel_polls(); log!( "pcidev: PCI {:02x}:{:02x}.{} [{:04x}:{:04x}] released from slot {slot}; reset by {how}", bound.pci.bus, diff --git a/kernel/src/watch.rs b/kernel/src/watch.rs index 2178a9d99c7..a58e8e8a42f 100644 --- a/kernel/src/watch.rs +++ b/kernel/src/watch.rs @@ -10,10 +10,9 @@ //! //! **A post allocates nothing and may be made under any lock but a poll //! ring's** (`crate::inbox`). A watch an interrupt handler posts is an -//! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its post and its -//! cancel are made in place and free nothing; the thread that ends its source -//! sweeps. Every other watch's list lock leaves interrupts open, and no -//! handler takes it. Registration allocates, in the syscall that registers. +//! [`IrqWatch`]: its list sits behind an [`IrqLock`], and its post is made in +//! place and frees nothing. Every other watch's list lock leaves interrupts +//! open, and no handler takes it. Registration allocates, in the syscall that registers. //! //! A [`Watch`] is a borrowed reference for the whole of a wait: [`Armed`] //! holds it, so an object cannot be freed under a thread waiting on it, and @@ -44,9 +43,8 @@ pub struct Waitable>(toyos_sched::watch::Watch>; /// A watch an interrupt handler posts, and the watch of a ring such a post -/// completes into. It has no `post` and no `cancel_polls`, which free: -/// [`IrqWatch::post_in_place`] and [`IrqWatch::cancel_polls_in_place`] free -/// nothing, and [`IrqWatch::sweep`] is a thread's. +/// completes into. It has no `post`, which frees: [`IrqWatch::post_in_place`] +/// frees nothing. pub type IrqWatch = Waitable>; /// What an interrupt handler's post takes: an [`IrqWatch`]'s list and a poll @@ -138,11 +136,6 @@ impl Watch { ) }) } - - /// Answer every poll registered here as gone: the source it watched ended. - pub fn cancel_polls(&self) { - self.0.cancel_rings(); - } } impl IrqWatch { @@ -162,18 +155,6 @@ impl IrqWatch { self.0.post_in_place(WakeCause::new(WakeReason::Woken), &env); }); } - - /// Answer every poll registered here as gone, where it stands, freeing - /// nothing: the source it watched ended. - pub fn cancel_polls_in_place(&self) { - self.0.cancel_rings_in_place(); - } - - /// Let go of every poll a post or a cancel in place answered. A thread's, - /// since it frees: where the source ended, no registration comes to. - pub fn sweep(&self) { - self.0.sweep(); - } } impl> Waitable { @@ -182,6 +163,12 @@ impl> Waitable { self.0.add_ring(entry); } + /// Answer every poll registered here as gone: the source it watched ended. + /// A thread's, since it frees what it answered. + pub fn cancel_polls(&self) { + self.0.cancel_rings(); + } + /// `handler-post`'s stand where a registration holds the list lock. #[cfg(feature = "boot-actuators")] pub(crate) fn holding(&self, f: impl FnOnce()) { diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index 3cf04a48328..f133bd27764 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -344,10 +344,11 @@ fn an_end_racing_a_post_answers_a_poll_once() { model(|| end_racing(|w| w.watch.cancel_rings(), World::post)); } -/// The same, for the end and the post a handler may make, both in place. +/// The same, for a thread's end racing a handler's post, which is made in +/// place. #[test] -fn an_end_in_place_racing_a_post_in_place_answers_a_poll_once() { - model(|| end_racing(|w| w.watch.cancel_rings_in_place(), World::post_in_place)); +fn an_end_racing_a_post_in_place_answers_a_poll_once() { + model(|| end_racing(|w| w.watch.cancel_rings(), World::post_in_place)); } fn end_racing(end: fn(&World), post: fn(&World)) { diff --git a/toyos-sched/src/watch.rs b/toyos-sched/src/watch.rs index 0520ac93b98..6bd0a6f5b5e 100644 --- a/toyos-sched/src/watch.rs +++ b/toyos-sched/src/watch.rs @@ -21,10 +21,9 @@ //! can no longer fire out [`FEW`] at a time, grows a full list in a buffer //! allocated with the lock let go, and drops both with it let go. //! [`Watch::post`] takes every ring entry out and fires and frees them with the -//! lock let go; [`Watch::post_in_place`] and [`Watch::cancel_rings_in_place`], -//! for a handler, which may not free at all, fire them where they stand, and -//! later registrations sweep the dead out, since an entry is one-shot. -//! [`Watch::sweep`] lets go of them where no registration will come. +//! lock let go; [`Watch::post_in_place`], for a handler, which may not free at +//! all, fires them where they stand, and later registrations sweep the dead +//! out, since an entry is one-shot. //! //! **Lock order.** A post in place fires its rings under the list lock, so //! beneath it are each ring's own lock and the watch that ring's submitters @@ -293,29 +292,6 @@ impl>> Watch { } } - /// [`Self::cancel_rings`] for a context that may not free: every ring - /// entry is fired as [`Fire::Gone`] where it stands, under the list lock, - /// and later registrations sweep it. - pub fn cancel_rings_in_place(&self) { - self.list.with(|w| { - for ring in &w.rings { - ring.fire(Fire::Gone); - } - }); - } - - /// Let go of every ring entry that can no longer fire, [`FEW`] a section, - /// each section's dropped with the list lock let go: for a thread, after - /// an end in place that no registration may follow. - pub fn sweep(&self) { - loop { - let mut dead: [Option; FEW] = [const { None }; FEW]; - if self.list.with(|w| take_dead(&mut w.rings, &mut dead)) < FEW { - return; - } - } - } - /// Run `f` holding the list lock, where a registration holds it, touching /// nothing on the list: an actuator's way to raise an interrupt there. pub fn holding(&self, f: impl FnOnce() -> U) -> U { @@ -800,58 +776,31 @@ mod tests { w.unregister(&t); } + /// One entry past the [`FEW`] a post takes out on its stack, so a cancel + /// or a drop bounded at that many is caught. #[test] fn cancel_and_drop_answer_every_live_poll_as_gone() { + let polls = || -> Vec<_> { (0..=FEW).map(|_| Arc::new(Poll::default())).collect() }; let w = watch(); - let cancelled = Arc::new(Poll::default()); - w.add_ring(cancelled.clone()); - w.cancel_rings(); - assert_eq!(cancelled.state.load(Ordering::Acquire), 2); - - let dropped = Arc::new(Poll::default()); - w.add_ring(dropped.clone()); - drop(w); - assert_eq!(dropped.state.load(Ordering::Acquire), 2); - assert_eq!(dropped.posts.load(Ordering::Acquire), 1); - } - - /// A cancel in place answers where each entry stands and lets go of - /// nothing, as a post in place does. - #[test] - fn a_cancel_in_place_answers_every_live_poll_as_gone_and_drops_nothing() { - let w = watch(); - let polls: Vec<_> = (0..=FEW).map(|_| Arc::new(Poll::default())).collect(); - for poll in &polls { + let cancelled = polls(); + for poll in &cancelled { w.add_ring(poll.clone()); } - w.cancel_rings_in_place(); - for (at, poll) in polls.iter().enumerate() { + w.cancel_rings(); + for (at, poll) in cancelled.iter().enumerate() { assert_eq!(poll.state.load(Ordering::Acquire), 2, "poll {at} was not answered as gone"); - assert_eq!(Arc::strong_count(poll), 2, "the cancel let go of poll {at}"); + assert_eq!(Arc::strong_count(poll), 1, "the cancel kept poll {at}"); } - } - /// Where no registration follows an end in place, the sweep lets go of - /// every entry that can no longer fire, past the [`FEW`] one section - /// takes, and of no live one. - #[test] - fn a_sweep_lets_go_of_every_entry_that_can_no_longer_fire() { - let w = watch(); - let live = Arc::new(Poll::default()); - w.add_ring(live.clone()); - let dead: Vec<_> = (0..=2 * FEW).map(|_| Arc::new(Poll::default())).collect(); - for poll in &dead { + let dropped = polls(); + for poll in &dropped { w.add_ring(poll.clone()); } - for poll in &dead { - poll.withdraw(); - } - w.sweep(); - for (at, poll) in dead.iter().enumerate() { - assert_eq!(Arc::strong_count(poll), 1, "the sweep kept dead entry {at} of {}", dead.len()); + drop(w); + for (at, poll) in dropped.iter().enumerate() { + assert_eq!(poll.state.load(Ordering::Acquire), 2, "dropped poll {at} was not answered as gone"); + assert_eq!(poll.posts.load(Ordering::Acquire), 1); } - assert_eq!(Arc::strong_count(&live), 2, "the sweep let go of a live entry"); - assert_eq!(w.live_rings(), 1); } #[test] diff --git a/toyos-sched/tests/watch_list.rs b/toyos-sched/tests/watch_list.rs index b5637bbd131..63c60e7208e 100644 --- a/toyos-sched/tests/watch_list.rs +++ b/toyos-sched/tests/watch_list.rs @@ -187,9 +187,8 @@ fn nothing_allocates_or_frees_under_the_list_lock_and_a_post_in_place_frees_noth POSTING.set(true); w.post_in_place(WakeCause::new(WakeReason::Woken), &env); - w.cancel_rings_in_place(); POSTING.set(false); - clean("posting and cancelling in place"); + clean("posting in place"); assert!(polls.iter().all(|p| p.0.load(Acquire) == 1), "a post in place fired every entry"); // Every entry is dead now, and a withdrawn one joins them: these three @@ -287,26 +286,6 @@ fn a_re_arm_after_a_post_of_few_entries_is_one_section() { assert_eq!(sections(|| w.add_ring(rearmed)), 1, "a re-arm after a post regrew the list"); } -/// An end in place and the sweep after it: the sweep lets go of every entry, -/// past the [`FEW`] one section takes, and frees each with the lock let go. -#[test] -fn a_sweep_frees_none_under_the_lock() { - let w: OwnedWatch = Watch::new(Watched(Mutex::new(Waiters::new()))); - let fired: Vec<_> = (0..=2 * FEW).map(|_| Arc::new(AtomicU32::new(0))).collect(); - for word in &fired { - w.add_ring(owned(word)); - } - POSTING.set(true); - w.cancel_rings_in_place(); - POSTING.set(false); - clean("cancelling in place"); - w.sweep(); - clean("sweeping"); - for (at, word) in fired.iter().enumerate() { - assert_eq!(Arc::strong_count(word), 1, "entry {at} of {} was not let go of", fired.len()); - } -} - /// A registration that finds the list full sizes a bigger buffer with the lock /// let go. Registrations that fill the list past that buffer before it comes /// back leave it too small to take the list, and moving the list into it then From d098f0d3874d8cf077532cc921c3547bee829a7a Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 20:44:07 +0200 Subject: [PATCH 13/14] issues: an IrqWatch's freeing cancel compiles in a handler With `cancel_polls` on every watch, a handler could free again, and the kernel has no proof of thread context a handler cannot mint to keep it out. `Parkable` was the one candidate: with `IrqWatch`'s cancel taking one, `Parkable::at_entry()` written in `pcidev::note_fault` builds (x86-64 kernel clippy 0), since its only check is a preempt-depth assertion at run time, and the claim's release, reached from `close_all` under the `ProcessData` lock, could not mint one. Held by stage 6, with the review's `note_fault` line as its evidence and a proof no handler can construct as its exit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...hs-freeing-cancel-compiles-in-a-handler.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md diff --git a/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md b/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md new file mode 100644 index 00000000000..739b0b03c73 --- /dev/null +++ b/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md @@ -0,0 +1,29 @@ +--- +status: assigned +kind: defect +opened: 2026-09-30 +--- + +# An IrqWatch's freeing cancel compiles in a handler + +Held by the small-kernel track's stage 6 +(`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md`). + +An interrupt handler may not free: it can interrupt the allocator's holder. +`IrqWatch` has no `post`, so a thread's post written in a handler is refused +at build. But `cancel_polls` is every watch's, and it frees every entry it +takes out. Its callers are the claim's release and the close of a claim or an +audio device, both threads', and no type keeps it out of a handler. + +**Evidence**: `WATCHES[slot].cancel_polls();` written after +`pcidev::note_fault`'s post builds: the x86-64 kernel clippy shape exits 0. +The kernel holds no proof of thread context a handler cannot mint. `Parkable` +proves a context may park, and with `IrqWatch`'s cancel taking one, +`Parkable::at_entry()` written at the same line in `note_fault` builds too +(clippy 0). Its only check is at run time, an assertion on the preempt +depth: by reading, a handler that interrupted Ring 3 passes it, and the +release, reached from `close_all` under the `ProcessData` lock, fails it. + +**Exit**: a proof of thread context no interrupt handler can construct, taken +by `IrqWatch`'s `cancel_polls`, so the `note_fault` line above is refused at +build. From 0a9eb49415c29af8f92d020cbec8bf7976766ca7 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 21:23:17 +0200 Subject: [PATCH 14/14] Answer the final review of #634: an audio claim's release answers its polls The audio claim is exclusive, and `Claim::drop`'s `Class` arm is its release: it runs once, from `drain_zero_handles` with no lock held, as the `PciFunction` arm's `pcidev::release` does. For `HdaAudio` and `VirtioSound` it now calls `AUDIO_WATCH.cancel_polls()` before the claim's flag goes, so a process that exits holding an audio device no longer leaves its rings' entries on the watch for registrations to sweep, and a next holder's polls are never this claim's to answer. The walk issue's bullet on that retention goes with it. The thread sites that answer a device's polls, `pcidev::tear_down`'s, the `Irq` arm of `WatchRef::cancel_polls` and this one, are filed as `issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md`. Each deletion builds: the x86-64 kernel clippy shape exits 0 with the release's `cancel_polls()` deleted, 0 with the `Irq` arm made `{}`, 0 with the audio claim's cancel made `{}`, and 0 unmutated; three checked patches, applied and reverted in one script, tree restored. That issue and the freeing-cancel issue are held by stage 6 step 5, whose exit now cites both. The freeing-cancel issue loses its false clause on `close_all`: `Device` is deferred, and its hook runs with no lock held. The loom doc loses its clause on the answerer no longer holding the entry, which `post_in_place` does not do. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...alk-by-the-threads-it-parks-on-one-ring.md | 8 ----- ...hs-freeing-cancel-compiles-in-a-handler.md | 10 +++--- ...ease-or-close-stops-answering-its-polls.md | 31 +++++++++++++++++++ ...-small-interrupts-post-and-threads-wait.md | 8 +++-- kernel/src/device.rs | 15 ++++++++- toyos-sched/loom/tests/loom_watch.rs | 2 +- 6 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md diff --git a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md index 50786fb3d70..b38d2a9496f 100644 --- a/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md +++ b/issues/kernel/a-process-lengthens-an-interrupts-off-walk-by-the-threads-it-parks-on-one-ring.md @@ -29,14 +29,6 @@ holder, decides how long a CPU runs with interrupts masked: ring's completions lock and posting that ring's watch, whose own N threads it notifies. Entries a post in place fired stay in the list until registrations sweep them four at a time. -- **A process that exits holding an audio device** leaves every entry its - rings registered on `AUDIO_WATCH` there: `close_all` answers no poll, and - the audio handler's post frees none. Each ring's teardown withdraws its - polls and lets go of its page, so an entry keeps only its `Poll` and its ring's - `Inbox`. They stay until registrations on `AUDIO_WATCH` take them out, four - each, and every audio interrupt walks them under the list lock until then. - By reading, a registration's sweep keeps a list no longer than the most - live entries it has held at once, at most 1024 per ring that polled it. Nothing caps N: a thread costs its process a 128 KiB kernel stack (`kernel/src/process.rs`) and no count. Before #634 every one of these diff --git a/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md b/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md index 739b0b03c73..9b29532b441 100644 --- a/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md +++ b/issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md @@ -6,13 +6,14 @@ opened: 2026-09-30 # An IrqWatch's freeing cancel compiles in a handler -Held by the small-kernel track's stage 6 -(`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md`). +Held by the small-kernel track's stage 6 step 5 +(`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md`), +whose exit reads it. An interrupt handler may not free: it can interrupt the allocator's holder. `IrqWatch` has no `post`, so a thread's post written in a handler is refused at build. But `cancel_polls` is every watch's, and it frees every entry it -takes out. Its callers are the claim's release and the close of a claim or an +takes out. Its callers are a claim's release and the close of a claim or an audio device, both threads', and no type keeps it out of a handler. **Evidence**: `WATCHES[slot].cancel_polls();` written after @@ -21,8 +22,7 @@ The kernel holds no proof of thread context a handler cannot mint. `Parkable` proves a context may park, and with `IrqWatch`'s cancel taking one, `Parkable::at_entry()` written at the same line in `note_fault` builds too (clippy 0). Its only check is at run time, an assertion on the preempt -depth: by reading, a handler that interrupted Ring 3 passes it, and the -release, reached from `close_all` under the `ProcessData` lock, fails it. +depth: by reading, a handler that interrupted Ring 3 passes it. **Exit**: a proof of thread context no interrupt handler can construct, taken by `IrqWatch`'s `cancel_polls`, so the `note_fault` line above is refused at diff --git a/issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md b/issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md new file mode 100644 index 00000000000..47cccb8b434 --- /dev/null +++ b/issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md @@ -0,0 +1,31 @@ +--- +status: assigned +kind: defect +opened: 2026-09-30 +--- + +# Nothing fails when a device's release or close stops answering its polls + +Held by the small-kernel track's stage 6 step 5 +(`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md`), +whose exit reads it. + +A device's watch is an `IrqWatch`, and three thread sites answer the polls on +it as gone: `pcidev::tear_down` when a claimed function is released +(`kernel/src/pcidev/mod.rs:1489`), `Claim::drop` when an audio claim goes +(`kernel/src/device.rs:86`), and the close of a claim or an audio device +through `WatchRef::cancel_polls`'s `Irq` arm (`kernel/src/object/ops.rs:263`). +A poll none of them answers waits on interrupts that are the next holder's or +nobody's. No host test compiles `pcidev`, `device` or `ops`, and no guest test +ends a polled device, so each call can go and nothing reds. + +**Evidence**: each deletion builds. The x86-64 kernel clippy shape exits 0 +with the release's `cancel_polls()` deleted, 0 with the `Irq` arm made `{}`, +and 0 with the audio claim's cancel made `{}`. + +**Exit**: `WatchRef`'s cancel is one dispatch over every variant, as main's +`Deref` was, so the `Irq` arm above cannot be written apart from the others; +and a guest test, in which a claim's holder exits with a poll of its claim on a +ring another process holds, reads that poll answered `-NotFound` for a claimed +function and for an audio device, and reds with the release's cancel deleted +and with the audio claim's. diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 55933cb1a97..e7065240b7c 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -185,8 +185,12 @@ times: since what it proves is that passes run. The dump still paints its report on the panel and holds it there, a device the pass reaches; whether that stays is the owner's ruling. **Exit**: `drain_irqs` and the - idle loop's device checks are gone, and both windows are measured - against stage 6's start. + idle loop's device checks are gone, both windows are measured against + stage 6's start, and the exits of + `issues/kernel/an-irq-watchs-freeing-cancel-compiles-in-a-handler.md` + and + `issues/kernel/nothing-fails-when-a-devices-release-or-close-stops-answering-its-polls.md` + are met. ## Standing diff --git a/kernel/src/device.rs b/kernel/src/device.rs index 17c7f60d5a0..66935836c20 100644 --- a/kernel/src/device.rs +++ b/kernel/src/device.rs @@ -79,7 +79,20 @@ impl Claim { impl Drop for Claim { fn drop(&mut self) { match self.what { - Claimed::Class(class) => *taken(class).lock() = false, + Claimed::Class(class) => { + // Before the flag goes: a poll the next holder registers is not this claim's to answer. + match class { + DeviceType::HdaAudio | DeviceType::VirtioSound => { + crate::drivers::AUDIO_WATCH.cancel_polls() + } + DeviceType::Keyboard + | DeviceType::Mouse + | DeviceType::Framebuffer + | DeviceType::PciFunction + | DeviceType::Partition => {} + } + *taken(class).lock() = false; + } // Bus mastering off, then the domain, then the pages: `release` // owns that order, and this is where a dying process reaches it. Claimed::PciFunction(slot) => crate::pcidev::release(slot), diff --git a/toyos-sched/loom/tests/loom_watch.rs b/toyos-sched/loom/tests/loom_watch.rs index f133bd27764..6c57e61b7f0 100644 --- a/toyos-sched/loom/tests/loom_watch.rs +++ b/toyos-sched/loom/tests/loom_watch.rs @@ -338,7 +338,7 @@ fn poll_racing(post: fn(&World)) { } /// The object's end racing its readiness: the poll is answered once, as ready -/// or as gone, and whichever answered it no longer holds it. +/// or as gone. #[test] fn an_end_racing_a_post_answers_a_poll_once() { model(|| end_racing(|w| w.watch.cancel_rings(), World::post));