diff --git a/issues/an-xhci-storm-starves-the-cpu-that-takes-it.md b/issues/an-xhci-storm-starves-the-cpu-that-takes-it.md index 45fcfab28e5..c1a979b2378 100644 --- a/issues/an-xhci-storm-starves-the-cpu-that-takes-it.md +++ b/issues/an-xhci-storm-starves-the-cpu-that-takes-it.md @@ -26,9 +26,9 @@ so another CPU held the VFS lock across a stick write for seconds at a time. `xhci::poll_if_pending`, which `try_lock`s `XHCI` and **returns doing nothing** when the lock is held. Nothing masks the source in between, so every event the controller produces while that lock is held is one more interrupt on the CPU -that cannot service it. The record stays set, the CPU re-enters a pass, declines -the lock again, and the cycle costs it the interrupt budget it needed for its -own timer. +that cannot service it. The declined CPU halts, owed a kick by the lock's +release, but each new event wakes it into another pass that declines the lock +again, and the cycle costs it the interrupt budget it needed for its own timer. That is a CPU making no progress while looking busy, and it is the state `crate::deadline`'s poll relies on *some* CPU escaping. diff --git a/issues/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md b/issues/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md new file mode 100644 index 00000000000..4d891d1538a --- /dev/null +++ b/issues/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md @@ -0,0 +1,28 @@ +--- +status: open +kind: defect +opened: 2026-10-04 +--- + +# T14 cpu7 idles busier under ToyOS than any CPU under Linux + +The `counters` metal row reads each CPU's MPERF over its stamp across the idle +second between its `idle0` and `idle1` reads. Linux's turbostat on the same +machine, idle (`tests/t14-linux/turbostat-idle.txt`), reads 0.11 to 0.36% +machine-wide per 10 s and no CPU above 0.69%. + +cpu7 reads about four times that, on every boot so far: 1.45% at `ce1786ff0` +(pull request #705), 1.42% at `a059144e2`, and 1.86% and 1.85% on the two arms +of pull request #725's run. + +cpu0 read 1.37% and 1.35% on the first two boots. That was the idle loop +spinning on an `irq_ring` record `xhci::poll_if_pending` left when a USB-stick +transfer held `XHCI`; since `XHCI` became an `OwedLock`, cpu0 reads 0.50% with +the change (`a65205a81`) and 1.77% with the whole change reverted +(`b0cae9a19`), cpu1 to cpu6 0.45 to 0.68% on both arms. + +Not yet attributed: the row's own reader runs between the two reads, prints +its `idle0` lines and parks, and which CPUs it and the log's path ran on that +second is not recorded. Owner: the orchestrator, which holds the T14. +**Exit**: a reading that names what ran on cpu7 across that second; then the +cause is fixed, or, if it is the row's own work, folded to the row's doc. diff --git a/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md index db89ec291a6..dda71d398c9 100644 --- a/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -187,8 +187,8 @@ times: `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. + and `port_work_pending` go with the kernel's driver, and `irq_ring` and + `sync::OwedLock`, whose one user is `XHCI`, with them. **Exit**: step 10's. 5. **The pass is the scheduler's.** `drain_irqs` goes: the blocked-task dump and the heartbeat become `pass`'s own, and the TCO feed stays, since what it proves is that passes run. The dump keeps painting its diff --git a/issues/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md b/issues/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md deleted file mode 100644 index d8d97d89a72..00000000000 --- a/issues/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-04 ---- - -# Two T14 CPUs idle busier under ToyOS than any under Linux - -The `counters` metal row, at `ce1786ff0` (pull request #705), read each CPU's -MPERF over its stamp across the idle second between its `idle0` and `idle1` -reads: cpu0 1.37% and cpu7 1.45% busy, cpu3 0.11%, the other five 0.00 or -0.01%. Linux's turbostat on the same machine, idle -(`tests/t14-linux/turbostat-idle.txt`), reads 0.11 to 0.36% machine-wide per -10 s and no CPU above 0.69%. So six CPUs idle deeper than Linux's and two -about four times busier than Linux's machine-wide busiest interval. - -A second boot, at `a059144e2`, read the same two CPUs again: cpu0 1.35% and -cpu7 1.42%, cpu4 0.37%, cpu3 0.11%, the other four 0.00 to 0.03%. The two -busy CPUs reproduce across boots. - -Not yet attributed: the row's own reader runs between the two reads, prints -its `idle0` lines and parks, and which CPUs it and the log's path ran on that -second is not recorded. Owner: the orchestrator, which holds the T14. -**Exit**: a reading that names what ran on cpu0 and -cpu7 across that second; then the cause is fixed, or, if it is the row's own -work, folded to the row's doc. diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index b383b429fd9..ad2cc27b51c 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -47,6 +47,9 @@ post-is-an-answer = [] # # `src/sync.rs`'s two loads of `now` go `Relaxed`, and `ticket_lock` reds. lock-acquire-off = [] +# `src/sync.rs`'s `owed_fence` is dropped from both sides of `OwedLock`, and +# `owed_lock` reds. +owed-fence-off = [] # `src/sched/reap_gate.rs`'s `raise` store goes `Relaxed`, and `reap_gate` # reds. reap-raise-relaxed = [] diff --git a/kernel/loom/Cargo.toml b/kernel/loom/Cargo.toml index 15526da33bd..c49a9debeb6 100644 --- a/kernel/loom/Cargo.toml +++ b/kernel/loom/Cargo.toml @@ -70,6 +70,17 @@ post-is-an-answer = [] # Never on by default and never reachable from a kernel build, which declares # the same name only so `cfg` checking knows it. lock-acquire-off = [] +# The negative control for `OwedLock`'s handoff. It drops `sync.rs`'s +# `owed_fence` from an owing CPU's retry and from a release's look, so each can +# miss the other's write and a CPU is turned away with nobody to answer it, and +# `owed_lock.rs` must red: +# +# cargo test --manifest-path kernel/loom/Cargo.toml --features owed-fence-off \ +# --test owed_lock +# +# Never on by default and never reachable from a kernel build, which declares +# the same name only so `cfg` checking knows it. +owed-fence-off = [] # The negative control for the reap gate's enrolment edge. It makes # `reap_gate.rs`'s `raise` store `Relaxed`, so a claimer is handed a gate # without the work it was raised for, and `reap_gate.rs` must red: diff --git a/kernel/loom/tests/owed_lock.rs b/kernel/loom/tests/owed_lock.rs new file mode 100644 index 00000000000..6056a58220b --- /dev/null +++ b/kernel/loom/tests/owed_lock.rs @@ -0,0 +1,64 @@ +//! Loom: `OwedLock`'s handoff. +//! +//! A CPU that `try_lock_or_owe` turns away halts on the work it left, so it +//! must come back: either its retry took the lock, or the holder's release saw +//! its bit and answered it. That is a store-buffer pair, an owing store then a +//! read of `now` against the release's store to `now` then a read of the owed +//! word. +//! +//! ```text +//! cargo test --manifest-path kernel/loom/Cargo.toml --features owed-fence-off \ +//! --test owed_lock +//! ``` +//! +//! drops the fence from both sides and this file must red. + +#![cfg(feature = "loom")] + +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + +use kernel_loom::sync::OwedLock; +use loom::sync::Arc; + +/// The CPU the model turns away. +const TURNED: u32 = 3; + +/// What the releases answered in this execution; loom runs executions one at a +/// time on one OS thread, so a `std` static reset per execution is the record. +static ANSWERED: AtomicU64 = AtomicU64::new(0); + +fn record(owed: u64) { + ANSWERED.fetch_or(owed, Ordering::Relaxed); +} + +#[test] +fn a_turned_away_cpu_holds_the_lock_or_is_answered() { + static WON: AtomicBool = AtomicBool::new(false); + static TURNED_AWAY: AtomicBool = AtomicBool::new(false); + + loom::model(|| { + ANSWERED.store(0, Ordering::Relaxed); + let lock = Arc::new(OwedLock::new(0u32, record)); + let held = lock.try_lock().expect("an untaken lock is taken"); + + let other = { + let lock = lock.clone(); + loom::thread::spawn(move || lock.try_lock_or_owe(TURNED).map(drop).is_some()) + }; + drop(held); + let won = other.join().unwrap(); + + let answered = ANSWERED.load(Ordering::Relaxed) & (1 << TURNED) != 0; + assert!(won || answered, "cpu {TURNED} was turned away and nobody answered it"); + if won { + WON.store(true, Ordering::Relaxed); + } else { + TURNED_AWAY.store(true, Ordering::Relaxed); + } + }); + + assert!( + WON.load(Ordering::Relaxed) && TURNED_AWAY.load(Ordering::Relaxed), + "the model never both turned the cpu away and let it in, so one side of the handoff went unchecked", + ); +} diff --git a/kernel/src/arch/x86_64/i8042/mod.rs b/kernel/src/arch/x86_64/i8042/mod.rs index 70adb4ba85b..a69c508c16f 100644 --- a/kernel/src/arch/x86_64/i8042/mod.rs +++ b/kernel/src/arch/x86_64/i8042/mod.rs @@ -528,8 +528,6 @@ static PS2: Lock = Lock::new(Decoders { /// Turn whatever the ISR published into events and wakes. Runs at the top of /// every scheduler pass on every CPU, so the idle cost is one atomic load. pub fn service() { - // Unconditional and first: an undrained `irq_ring` record keeps - // `any_pending_self` true, spinning a CPU that never halts. let recorded = crate::irq_ring::take(IrqSource::I8042).is_some(); if QUARANTINE.load(Ordering::Relaxed) { quarantine(); diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 4b65b0e21f0..7d74b6c9985 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -20,7 +20,7 @@ use crate::mm::Mmio; use crate::mm::Dma; use crate::log; use super::pci::PciDevice; -use crate::sync::Lock; +use crate::sync::{OwedGuard, OwedLock}; use toyos_untrusted::Untrusted; use toyos_xhci::job::{Await, Outcome, Outstanding, Stages, CC_SHORT_PACKET, CC_SUCCESS}; use toyos_xhci::port::{self as portmachine, GaveUp, Gone, PortState, Reset, Step}; @@ -1444,7 +1444,22 @@ impl XhciController { /// Every xHCI controller on the machine, in PCI enumeration order. /// /// A `Vec` and not an `Option`: the target laptop has two, and its own ports hang off the second. -static XHCI: Lock> = Lock::new(Vec::new()); +/// +/// Owed, so a CPU whose [`poll_if_pending`] finds it taken halts on the record +/// it leaves instead of spinning a pass on it for as long as a disk transfer +/// holds the lock. +static XHCI: OwedLock> = OwedLock::new(Vec::new(), bring_to_pass); + +const _: () = assert!(crate::scheduler::MAX_CPUS <= u64::BITS as usize, "XHCI's owed word has a bit per cpu"); + +/// [`XHCI`]'s answer: every CPU in `owed` takes a scheduler pass, whose +/// [`poll_if_pending`] finds the record it left. +fn bring_to_pass(owed: u64) { + use kernel::sched::hw::{CpuId, Machine}; + for cpu in (0..u64::BITS).filter(|cpu| owed & (1 << cpu) != 0) { + crate::hw::HW.need_resched(CpuId(cpu)); + } +} /// Empty every USB disk's write cache, on the way to a reset. /// @@ -1568,7 +1583,7 @@ fn lock_settles() -> bool { /// deadlock panic, and a shutdown path that panicked instead of resetting is a /// machine nobody can turn off. Not fair — a competitor taking a ticket wins — /// which is why both callers say what they do without it. -fn take_within(bound: u64) -> Option>> { +fn take_within(bound: u64) -> Option>> { let until = crate::clock::tsc_deadline(bound); loop { if let Some(guard) = XHCI.try_lock() { @@ -1612,7 +1627,7 @@ pub fn poll_if_pending() { } } // Decline rather than queue: `lock()` here would put every CPU with work due on one ticket spinlock. - let Some(mut guard) = XHCI.try_lock() else { return }; + let Some(mut guard) = XHCI.try_lock_or_owe(crate::arch::percpu::cpu_id()) else { return }; // Taken only now the work will be done: taking it before a declined lock would drop a wake with nothing left to re-post it. crate::irq_ring::take(crate::irq_ring::IrqSource::Xhci); let mut wake_at: Option = None; @@ -1693,7 +1708,7 @@ pub(super) fn with_disk_by( } fn on_disk( - mut guard: crate::sync::LockGuard<'static, Vec>, + mut guard: OwedGuard<'static, Vec>, index: usize, f: impl FnOnce(&mut XhciController, usize) -> R, ) -> Option { diff --git a/kernel/src/irq_ring.rs b/kernel/src/irq_ring.rs index a71e5204c1c..366efc9effc 100644 --- a/kernel/src/irq_ring.rs +++ b/kernel/src/irq_ring.rs @@ -65,11 +65,3 @@ pub fn take(source: IrqSource) -> Option { pub fn pending(source: IrqSource) -> bool { 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. -pub fn any_pending_self() -> bool { - SLOTS[percpu::cpu_id() as usize] - .0 - .iter() - .any(|slot| slot.load(Ordering::Relaxed) != 0) -} diff --git a/kernel/src/sched/driver.rs b/kernel/src/sched/driver.rs index eb41187bd06..5aa5e88ced5 100644 --- a/kernel/src/sched/driver.rs +++ b/kernel/src/sched/driver.rs @@ -641,7 +641,6 @@ fn execute(action: Action) { let cpu = CpuId(percpu::cpu_id()); let awake = cpus().get(cpu).doorbell().kick_pending() || crate::preempt::need_resched() - || crate::irq_ring::any_pending_self() || !with_cpu(|c| c.mailbox_is_empty()) // The i8042 verdict needs a pass to notice its deadline; a quiet machine after boot runs none otherwise. || crate::arch::keyboard_controller::verdict_due() diff --git a/kernel/src/sync.rs b/kernel/src/sync.rs index bfa85e8e851..2cfa8c5b9fb 100644 --- a/kernel/src/sync.rs +++ b/kernel/src/sync.rs @@ -1,14 +1,15 @@ +use core::mem::ManuallyDrop; use core::ops::{Deref, DerefMut}; #[cfg(not(feature = "loom"))] use core::cell::UnsafeCell; #[cfg(not(feature = "loom"))] -use core::sync::atomic::{AtomicU32, Ordering}; +use core::sync::atomic::{AtomicU32, AtomicU64, Ordering}; #[cfg(feature = "loom")] use crate::cell::UnsafeCell; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicU32, Ordering}; +use loom::sync::atomic::{AtomicU32, AtomicU64, Ordering}; // Unlock publishes through `now`, not `ticket`; the load that reads `now` // is the one that must carry the acquire. @@ -206,4 +207,94 @@ impl Drop for LockGuard<'_, T> { } } +/// The pair an owing CPU's retry and a release's look each put before their +/// read, so at least one of them sees the other's write. `owed-fence-off` +/// drops it so `kernel-loom`'s `owed_lock` model can prove it is load-bearing. +#[cfg(not(feature = "owed-fence-off"))] +fn owed_fence() { + #[cfg(not(feature = "loom"))] + use core::sync::atomic::fence; + #[cfg(feature = "loom")] + use loom::sync::atomic::fence; + fence(Ordering::SeqCst); +} +#[cfg(feature = "owed-fence-off")] +fn owed_fence() {} + +/// A [`Lock`] a CPU may find taken and leave without waiting: a CPU that +/// [`OwedLock::try_lock_or_owe`] turned away is owed, and the next release hands +/// every owed CPU, one bit each, to the `answer` the lock was built with, which +/// brings it back to try again. What it owes is the turned-away CPU's to know. +pub struct OwedLock { + lock: Lock, + owed: AtomicU64, + answer: fn(u64), +} +impl OwedLock { + #[cfg(not(feature = "loom"))] + pub const fn new(val: T, answer: fn(u64)) -> Self { + Self { lock: Lock::new(val), owed: AtomicU64::new(0), answer } + } + + #[cfg(feature = "loom")] + pub fn new(val: T, answer: fn(u64)) -> Self { + Self { lock: Lock::new(val), owed: AtomicU64::new(0), answer } + } + + #[track_caller] + pub fn lock(&self) -> OwedGuard<'_, T> { + OwedGuard { guard: ManuallyDrop::new(self.lock.lock()), of: self } + } + + pub fn try_lock(&self) -> Option> { + Some(OwedGuard { guard: ManuallyDrop::new(self.lock.try_lock()?), of: self }) + } + + /// [`Self::try_lock`], or `cpu` is owed: `None` only when the holder's + /// release is still to look, and that look answers `cpu`. + pub fn try_lock_or_owe(&self, cpu: u32) -> Option> { + if let Some(won) = self.try_lock() { + return Some(won); + } + let bit = 1u64.checked_shl(cpu).expect("OwedLock: a cpu id past its 64-bit owed word"); + self.owed.fetch_or(bit, Ordering::Relaxed); + owed_fence(); + let won = self.try_lock()?; + self.owed.fetch_and(!bit, Ordering::Relaxed); + Some(won) + } +} + +pub struct OwedGuard<'a, T> { + guard: ManuallyDrop>, + of: &'a OwedLock, +} + +impl Deref for OwedGuard<'_, T> { + type Target = T; + fn deref(&self) -> &T { + &self.guard + } +} + +impl DerefMut for OwedGuard<'_, T> { + fn deref_mut(&mut self) -> &mut T { + &mut self.guard + } +} + +impl Drop for OwedGuard<'_, T> { + fn drop(&mut self) { + // Held across the release and the look, so no switch stands between an owed CPU and its answer. + crate::preempt::disable(); + // SAFETY: `guard` is dropped here, once, and nothing reads it after. + unsafe { ManuallyDrop::drop(&mut self.guard) }; + owed_fence(); + let owed = self.of.owed.swap(0, Ordering::Relaxed); + if owed != 0 { + (self.of.answer)(owed); + } + crate::preempt::enable(); + } +} diff --git a/src/ci.rs b/src/ci.rs index 2e13a0b9b24..857c11475cc 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -250,6 +250,9 @@ pub(crate) const CONTROLS: &[Control] = &[ red(KERNEL_LOOM, "lock-acquire-off", Some("ticket_lock"), &[ Fails("try_lock_observes_the_previous_owners_writes"), ]), + red(KERNEL_LOOM, "owed-fence-off", Some("owed_lock"), &[ + Fails("a_turned_away_cpu_holds_the_lock_or_is_answered"), + ]), red(KERNEL_LOOM, "seqlock-writer-fence-off", Some("panic_console_publish"), &[ Fails("a_snapshot_is_one_publication_whole"), ]),