From a65205a8160fc58fe2a064681cbaaf302c538bbe Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 4 Oct 2026 13:14:39 +0200 Subject: [PATCH 1/5] kernel: a CPU that finds XHCI taken halts, and the release kicks it On the T14 an idle cpu0 ran its idle pass in a tight loop for as long as a USB-stick transfer held XHCI on another CPU: a diagnostic boot of #717's trace_read (commit 53f75eacb) read 4,613 idle declines in about 5.3 ms whose only reason was bit 2, an undrained irq_ring record, beside 4,618 TimerArm records, while every other CPU wrote 87 records or fewer. The xHCI ISR posts its record on cpu0, where every device interrupt lands; poll_if_pending declines a taken XHCI and leaves the record, rightly, since taking it would drop the wake; and the idle look's irq_ring::any_pending_self saw it and refused to halt, so the pass ran again, declined again, and so on until the transfer's CPU let go. QEMU's stick answers fast enough that the window is short; the T14's is milliseconds. The obligation the record carries now moves to the holder instead of keeping cpu0 awake. sync::OwedLock is a Lock whose try_lock_or_owe, when turned away, sets the caller's bit in an owed word, fences, and tries once more; every release fences and swaps the word, handing each owed CPU to the lock's answer. The two fences are a store-buffer pair: either the retry sees the release or the release sees the bit. XHCI is an OwedLock whose answer kicks each owed CPU (need_resched for itself), whose next pass's poll_if_pending finds the record again. With that, the only record a pass could leave is owed to a release, and every ISR that posts one also sets need_resched, so the idle look's any_pending_self is redundant and goes, with the function. kernel-loom's owed_lock model drives a release against a turned-away CPU and holds that it either took the lock or was answered; owed-fence-off drops both fences and the model reds, and src/ci.rs runs that control. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WcU2Dsw6mDYtwYfzVHPzM8 --- ...busier-under-toyos-than-any-under-linux.md | 12 +++ kernel/Cargo.toml | 3 + kernel/loom/Cargo.toml | 11 +++ kernel/loom/tests/owed_lock.rs | 65 +++++++++++++ kernel/src/arch/x86_64/i8042/mod.rs | 2 - kernel/src/drivers/xhci/mod.rs | 29 +++++- kernel/src/irq_ring.rs | 8 -- kernel/src/sched/driver.rs | 1 - kernel/src/sync.rs | 91 ++++++++++++++++++- src/ci.rs | 3 + 10 files changed, 207 insertions(+), 18 deletions(-) create mode 100644 kernel/loom/tests/owed_lock.rs diff --git a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md index d8d97d89a72..b8885f749af 100644 --- a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md +++ b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md @@ -18,6 +18,18 @@ 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. +**cpu0, named in another window.** A diagnostic boot of pull request #717's +`trace_read` (commit `53f75eacb`, its readback quoted on #717) recorded why +each idle pass declined to halt: across about 5.3 ms cpu0 wrote 4,613 +declines whose only reason was an undrained `irq_ring` record and 4,618 +`TimerArm`s, and every other CPU 87 records or fewer. An xHCI record is the +only one a pass leaves: `xhci::poll_if_pending` declines a taken `XHCI` and +leaves it, and a USB-stick transfer holds `XHCI` on another CPU for as long as +the device takes, so cpu0, where every device interrupt lands, ran pass after +pass until the transfer's CPU let go. `XHCI` is an `OwedLock` since: a +declined CPU halts and the release kicks it. That window is not the row's idle +second, and nothing has yet read cpu7's. + 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. 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..5258441b823 --- /dev/null +++ b/kernel/loom/tests/owed_lock.rs @@ -0,0 +1,65 @@ +//! 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, and x86's TSO lets either read miss the other side's store, so no +//! guest test can stand in for it. +//! +//! ```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..792065563f7 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,26 @@ 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) { + let me = crate::arch::percpu::cpu_id(); + for cpu in (0..u64::BITS).filter(|cpu| owed & (1 << cpu) != 0) { + if cpu == me { + crate::preempt::set_need_resched(); + } else { + crate::arch::irqchip::kick_cpu(cpu); + } + } +} /// Empty every USB disk's write cache, on the way to a reset. /// @@ -1568,7 +1587,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 +1631,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 +1712,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..7d34b2cf163 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::{fence, AtomicU32, AtomicU64, Ordering}; #[cfg(feature = "loom")] use crate::cell::UnsafeCell; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicU32, Ordering}; +use loom::sync::atomic::{fence, 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,90 @@ 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() { + 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 f69ee1a8440..fbdaf685feb 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"), ]), From b0cae9a19402fc79a9a479b8ec97aac65ed771a3 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 4 Oct 2026 13:18:24 +0200 Subject: [PATCH 2/5] NEGATIVE CONTROL, reverted by the next commit: the tree back at origin/main Reverts a65205a81 whole, so this commit's tree is 1c1c70f77's, from which the counters row's base arm is staged for the T14 beside the fix's. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WcU2Dsw6mDYtwYfzVHPzM8 --- ...busier-under-toyos-than-any-under-linux.md | 12 --- kernel/Cargo.toml | 3 - kernel/loom/Cargo.toml | 11 --- kernel/loom/tests/owed_lock.rs | 65 ------------- kernel/src/arch/x86_64/i8042/mod.rs | 2 + kernel/src/drivers/xhci/mod.rs | 29 +----- kernel/src/irq_ring.rs | 8 ++ kernel/src/sched/driver.rs | 1 + kernel/src/sync.rs | 91 +------------------ src/ci.rs | 3 - 10 files changed, 18 insertions(+), 207 deletions(-) delete mode 100644 kernel/loom/tests/owed_lock.rs diff --git a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md index b8885f749af..d8d97d89a72 100644 --- a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md +++ b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md @@ -18,18 +18,6 @@ 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. -**cpu0, named in another window.** A diagnostic boot of pull request #717's -`trace_read` (commit `53f75eacb`, its readback quoted on #717) recorded why -each idle pass declined to halt: across about 5.3 ms cpu0 wrote 4,613 -declines whose only reason was an undrained `irq_ring` record and 4,618 -`TimerArm`s, and every other CPU 87 records or fewer. An xHCI record is the -only one a pass leaves: `xhci::poll_if_pending` declines a taken `XHCI` and -leaves it, and a USB-stick transfer holds `XHCI` on another CPU for as long as -the device takes, so cpu0, where every device interrupt lands, ran pass after -pass until the transfer's CPU let go. `XHCI` is an `OwedLock` since: a -declined CPU halts and the release kicks it. That window is not the row's idle -second, and nothing has yet read cpu7's. - 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. diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index ad2cc27b51c..b383b429fd9 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -47,9 +47,6 @@ 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 c49a9debeb6..15526da33bd 100644 --- a/kernel/loom/Cargo.toml +++ b/kernel/loom/Cargo.toml @@ -70,17 +70,6 @@ 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 deleted file mode 100644 index 5258441b823..00000000000 --- a/kernel/loom/tests/owed_lock.rs +++ /dev/null @@ -1,65 +0,0 @@ -//! 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, and x86's TSO lets either read miss the other side's store, so no -//! guest test can stand in for it. -//! -//! ```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 a69c508c16f..70adb4ba85b 100644 --- a/kernel/src/arch/x86_64/i8042/mod.rs +++ b/kernel/src/arch/x86_64/i8042/mod.rs @@ -528,6 +528,8 @@ 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 792065563f7..4b65b0e21f0 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::{OwedGuard, OwedLock}; +use crate::sync::Lock; 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,26 +1444,7 @@ 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. -/// -/// 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) { - let me = crate::arch::percpu::cpu_id(); - for cpu in (0..u64::BITS).filter(|cpu| owed & (1 << cpu) != 0) { - if cpu == me { - crate::preempt::set_need_resched(); - } else { - crate::arch::irqchip::kick_cpu(cpu); - } - } -} +static XHCI: Lock> = Lock::new(Vec::new()); /// Empty every USB disk's write cache, on the way to a reset. /// @@ -1587,7 +1568,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() { @@ -1631,7 +1612,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_or_owe(crate::arch::percpu::cpu_id()) else { return }; + let Some(mut guard) = XHCI.try_lock() 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; @@ -1712,7 +1693,7 @@ pub(super) fn with_disk_by( } fn on_disk( - mut guard: OwedGuard<'static, Vec>, + mut guard: crate::sync::LockGuard<'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 366efc9effc..a71e5204c1c 100644 --- a/kernel/src/irq_ring.rs +++ b/kernel/src/irq_ring.rs @@ -65,3 +65,11 @@ 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 5aa5e88ced5..eb41187bd06 100644 --- a/kernel/src/sched/driver.rs +++ b/kernel/src/sched/driver.rs @@ -641,6 +641,7 @@ 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 7d34b2cf163..bfa85e8e851 100644 --- a/kernel/src/sync.rs +++ b/kernel/src/sync.rs @@ -1,15 +1,14 @@ -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::{fence, AtomicU32, AtomicU64, Ordering}; +use core::sync::atomic::{AtomicU32, Ordering}; #[cfg(feature = "loom")] use crate::cell::UnsafeCell; #[cfg(feature = "loom")] -use loom::sync::atomic::{fence, AtomicU32, AtomicU64, Ordering}; +use loom::sync::atomic::{AtomicU32, Ordering}; // Unlock publishes through `now`, not `ticket`; the load that reads `now` // is the one that must carry the acquire. @@ -207,90 +206,4 @@ 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() { - 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 fbdaf685feb..f69ee1a8440 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -250,9 +250,6 @@ 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"), ]), From 5bb60387538f89930c3d5f43d2c4d5604a4e18dc Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 4 Oct 2026 13:19:15 +0200 Subject: [PATCH 3/5] Restore a65205a81 after staging the negative control's base arm Reverts b0cae9a19, so the tree is a65205a81's again. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WcU2Dsw6mDYtwYfzVHPzM8 --- ...busier-under-toyos-than-any-under-linux.md | 12 +++ kernel/Cargo.toml | 3 + kernel/loom/Cargo.toml | 11 +++ kernel/loom/tests/owed_lock.rs | 65 +++++++++++++ kernel/src/arch/x86_64/i8042/mod.rs | 2 - kernel/src/drivers/xhci/mod.rs | 29 +++++- kernel/src/irq_ring.rs | 8 -- kernel/src/sched/driver.rs | 1 - kernel/src/sync.rs | 91 ++++++++++++++++++- src/ci.rs | 3 + 10 files changed, 207 insertions(+), 18 deletions(-) create mode 100644 kernel/loom/tests/owed_lock.rs diff --git a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md index d8d97d89a72..b8885f749af 100644 --- a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md +++ b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md @@ -18,6 +18,18 @@ 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. +**cpu0, named in another window.** A diagnostic boot of pull request #717's +`trace_read` (commit `53f75eacb`, its readback quoted on #717) recorded why +each idle pass declined to halt: across about 5.3 ms cpu0 wrote 4,613 +declines whose only reason was an undrained `irq_ring` record and 4,618 +`TimerArm`s, and every other CPU 87 records or fewer. An xHCI record is the +only one a pass leaves: `xhci::poll_if_pending` declines a taken `XHCI` and +leaves it, and a USB-stick transfer holds `XHCI` on another CPU for as long as +the device takes, so cpu0, where every device interrupt lands, ran pass after +pass until the transfer's CPU let go. `XHCI` is an `OwedLock` since: a +declined CPU halts and the release kicks it. That window is not the row's idle +second, and nothing has yet read cpu7's. + 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. 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..5258441b823 --- /dev/null +++ b/kernel/loom/tests/owed_lock.rs @@ -0,0 +1,65 @@ +//! 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, and x86's TSO lets either read miss the other side's store, so no +//! guest test can stand in for it. +//! +//! ```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..792065563f7 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,26 @@ 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) { + let me = crate::arch::percpu::cpu_id(); + for cpu in (0..u64::BITS).filter(|cpu| owed & (1 << cpu) != 0) { + if cpu == me { + crate::preempt::set_need_resched(); + } else { + crate::arch::irqchip::kick_cpu(cpu); + } + } +} /// Empty every USB disk's write cache, on the way to a reset. /// @@ -1568,7 +1587,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 +1631,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 +1712,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..7d34b2cf163 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::{fence, AtomicU32, AtomicU64, Ordering}; #[cfg(feature = "loom")] use crate::cell::UnsafeCell; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicU32, Ordering}; +use loom::sync::atomic::{fence, 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,90 @@ 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() { + 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 f69ee1a8440..fbdaf685feb 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"), ]), From b976920bc8b4ac8a4ea1b90b7082b94127c2a7d1 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 4 Oct 2026 13:33:37 +0200 Subject: [PATCH 4/5] sync: import owed_fence's fence inside it, so owed-fence-off builds clean `--ci host`'s clippy shape with every control on denied the unused import of `fence` that owed-fence-off leaves at the top of sync.rs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WcU2Dsw6mDYtwYfzVHPzM8 --- kernel/src/sync.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/kernel/src/sync.rs b/kernel/src/sync.rs index 7d34b2cf163..2cfa8c5b9fb 100644 --- a/kernel/src/sync.rs +++ b/kernel/src/sync.rs @@ -4,12 +4,12 @@ use core::ops::{Deref, DerefMut}; #[cfg(not(feature = "loom"))] use core::cell::UnsafeCell; #[cfg(not(feature = "loom"))] -use core::sync::atomic::{fence, AtomicU32, AtomicU64, Ordering}; +use core::sync::atomic::{AtomicU32, AtomicU64, Ordering}; #[cfg(feature = "loom")] use crate::cell::UnsafeCell; #[cfg(feature = "loom")] -use loom::sync::atomic::{fence, AtomicU32, AtomicU64, 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. @@ -212,6 +212,10 @@ impl Drop for LockGuard<'_, T> { /// 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")] From c1aadf63ea9d5196d9dd7f2b3169662f29002500 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 4 Oct 2026 14:02:29 +0200 Subject: [PATCH 5/5] Review round 1: XHCI's answer is the scheduler's need_resched; issues kept true - xhci: bring_to_pass calls KernelHw::need_resched per owed bit instead of re-deriving its own-CPU-or-kick branch. - loom/owed_lock.rs: drop the false TSO clause; on x86 both sides are locked read-modify-writes and cannot show the reordering. - The kernel-is-small track's step 4 names sync::OwedLock, whose one user is XHCI, so it goes with the kernel's driver. - The xHCI-storm issue no longer says a left record re-enters a pass: the declined CPU halts, and only each new interrupt or the release's kick wakes it. - The idle-busy issue is cpu7's alone after the T14 run on #725 (cpu0 0.50% fixed, 1.77% reverted; cpu7 1.86% and 1.85%), so it is renamed to t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux and records both arms. No citation of the old slug exists in the tree. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WcU2Dsw6mDYtwYfzVHPzM8 --- ...hci-storm-starves-the-cpu-that-takes-it.md | 6 +-- ...er-under-toyos-than-any-cpu-under-linux.md | 28 ++++++++++++++ ...-small-interrupts-post-and-threads-wait.md | 4 +- ...busier-under-toyos-than-any-under-linux.md | 38 ------------------- kernel/loom/tests/owed_lock.rs | 3 +- kernel/src/drivers/xhci/mod.rs | 8 +--- 6 files changed, 36 insertions(+), 51 deletions(-) create mode 100644 issues/kernel/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md delete mode 100644 issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md diff --git a/issues/kernel/an-xhci-storm-starves-the-cpu-that-takes-it.md b/issues/kernel/an-xhci-storm-starves-the-cpu-that-takes-it.md index bb45e184aa2..16958ddd3eb 100644 --- a/issues/kernel/an-xhci-storm-starves-the-cpu-that-takes-it.md +++ b/issues/kernel/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/kernel/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md b/issues/kernel/t14-cpu7-idles-busier-under-toyos-than-any-cpu-under-linux.md new file mode 100644 index 00000000000..4d891d1538a --- /dev/null +++ b/issues/kernel/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/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 b72c844e22d..8a12a82f195 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 @@ -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/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md b/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md deleted file mode 100644 index b8885f749af..00000000000 --- a/issues/kernel/two-t14-cpus-idle-busier-under-toyos-than-any-under-linux.md +++ /dev/null @@ -1,38 +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. - -**cpu0, named in another window.** A diagnostic boot of pull request #717's -`trace_read` (commit `53f75eacb`, its readback quoted on #717) recorded why -each idle pass declined to halt: across about 5.3 ms cpu0 wrote 4,613 -declines whose only reason was an undrained `irq_ring` record and 4,618 -`TimerArm`s, and every other CPU 87 records or fewer. An xHCI record is the -only one a pass leaves: `xhci::poll_if_pending` declines a taken `XHCI` and -leaves it, and a USB-stick transfer holds `XHCI` on another CPU for as long as -the device takes, so cpu0, where every device interrupt lands, ran pass after -pass until the transfer's CPU let go. `XHCI` is an `OwedLock` since: a -declined CPU halts and the release kicks it. That window is not the row's idle -second, and nothing has yet read cpu7's. - -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/loom/tests/owed_lock.rs b/kernel/loom/tests/owed_lock.rs index 5258441b823..6056a58220b 100644 --- a/kernel/loom/tests/owed_lock.rs +++ b/kernel/loom/tests/owed_lock.rs @@ -4,8 +4,7 @@ //! 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, and x86's TSO lets either read miss the other side's store, so no -//! guest test can stand in for it. +//! word. //! //! ```text //! cargo test --manifest-path kernel/loom/Cargo.toml --features owed-fence-off \ diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 792065563f7..7d74b6c9985 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1455,13 +1455,9 @@ const _: () = assert!(crate::scheduler::MAX_CPUS <= u64::BITS as usize, "XHCI's /// [`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) { - let me = crate::arch::percpu::cpu_id(); + use kernel::sched::hw::{CpuId, Machine}; for cpu in (0..u64::BITS).filter(|cpu| owed & (1 << cpu) != 0) { - if cpu == me { - crate::preempt::set_need_resched(); - } else { - crate::arch::irqchip::kick_cpu(cpu); - } + crate::hw::HW.need_resched(CpuId(cpu)); } }