diff --git a/local/docs/INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md b/local/docs/INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md index f3a98e42d3..0d134aa3dd 100644 --- a/local/docs/INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md +++ b/local/docs/INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md @@ -160,6 +160,44 @@ protocol). This is the cleanest enabler for Option A.1 and immediately activates retry, but it changes scheme-open semantics system-wide — the widest blast radius, so it needs the most care and the strongest validation. +## acpid hardening (2026-07-21): the daemon-local half of the same problem + +`acpid` is the same shape as `initnsmgr` in miniature: single-threaded, serving a scheme (`acpi`) +on the very thread that evaluates AML. A slow or stuck AML method stops it answering scheme +requests, which is what let the mutex bug wedge the whole system. Six hardening items were +identified; five are **done** (safe, verifiable, reduce or bound the ways acpid can stall the +transport), one is **designed-but-deferred** (the same deferred-response rewrite as Option A above, +in a boot-critical daemon). + +**Done (`local/sources/base/drivers/acpid`):** + +- **AML mutex recursion + ownership + timeout units** (`aml_physmem.rs`, the original fix): a nested + acquire by the owning thread bumps a depth instead of self-deadlocking; timeout read as + milliseconds; any wait bounded to 5s. Mirrors ACPICA `exmutex.c:140`. +- **`stall()` bounded per ACPICA** (`aml_physmem.rs`): refuse `>255us` and warn `>100us` instead of + an unbounded CPU busy-spin on the serving thread (ACPICA `exsystem.c:129-147`). +- **`release` owner-check** (`aml_physmem.rs`): a release of a not-currently-held mutex, or by a + non-owning thread, is logged (ACPICA `AE_NOT_ACQUIRED` / owner-mismatch, `exmutex.c:287,376`) + rather than silently corrupting depth. +- **Static processor-method cache** (`scheme.rs`): `_PSS`/`_PSD`/`_CST`/`_CPC` are fixed after boot + but cpufreqd polls them; cache the first evaluation so later reads never re-run the AML + interpreter under the global lock. This removes acpid's dominant *recurring* head-of-line source. +- **Panic-free scheme path** (`acpi.rs`): every `release_global_lock().expect(...)` on the + AML-evaluation path became log-and-continue, and a `result.ok()?.unwrap()` that panicked on + `Ok(None)` (absent method) became `?`. A scheme-daemon panic kills the `acpi` scheme and wedges + every consumer, so the serving path must not panic on odd firmware. +- **Observability** (`aml_physmem.rs`, `acpi.rs`): log AML evaluations ≥50ms and mutex-acquire + timeouts — the early-warning signal before a stall becomes a wedge. + +**Deferred (#1 — decouple AML execution from the transport):** make acpid keep answering the `acpi` +scheme while a slow AML method runs, by rewriting its loop from `process_requests_nonblocking` onto +the low-level `next_request` → park the `CallRequest` `Tag` → run AML on a single serialized worker +→ `write_response(tag)` when done. AML must stay serialized (ACPI requires it); only the transport +is parallelized. This is exactly Option A's deferred-response shape, in a boot-critical daemon, and +cannot be cleanly validated under the current external load — so it is designed here, not done +blind. With the cache and bounds above, acpid's remaining exposure is a *single* slow/first AML +evaluation (bounded to seconds, not infinite), which is the tail this rewrite would close. + ## Validation expectations - Framebuffer screendump (QMP) is ground truth; the serial mirror is racy and often silent on a diff --git a/local/docs/INITNSMGR-CONCURRENCY-DESIGN.md b/local/docs/INITNSMGR-CONCURRENCY-DESIGN.md new file mode 100644 index 0000000000..983abf02de --- /dev/null +++ b/local/docs/INITNSMGR-CONCURRENCY-DESIGN.md @@ -0,0 +1,190 @@ +# initnsmgr Concurrency Design — worker-offload of the blocking open + +**Status:** Design (not implemented). Companion to +`INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md`, which states *why* this is needed; this document +states *how*. Implementation must be validated on an idle host or real hardware — see "Validation". + +## Problem restated (precisely) + +`local/sources/base/bootstrap/src/initnsmgr.rs` runs the init namespace manager as a single thread: + +``` +run(): loop { + req = socket.next_request() // one request + resp = req.handle_sync(&mut scheme) // synchronous — runs the handler inline + socket.write_response(resp) +} +``` + +Every path resolution in a restricted namespace (every login shell, much of init's own spawn path) +is proxied here. The only handler that can block is `openat` → `open_scheme_resource`, which does a +**blocking `syscall::openat(cap_fd, …)`** to the *provider* daemon. If that provider is briefly not +servicing its socket (descheduled under load, mid-`tick`, in a one-shot startup window), the +`openat` blocks, the loop stops, and **every** other request queues behind it. The wedge surfaces at +whatever boot stage was in flight (e.g. "ahcid" in the logs) — that stage is the *symptom location*, +not the cause. The cause is head-of-line blocking on a single serving thread. + +All the other handlers are fast, in-memory operations that must **not** be parallelized (they mutate +shared namespace state): `dup` (ForkNs / ShrinkPermissions / IssueRegister), `unlinkat`, `on_close`, +`on_sendfd`, `getdents`, `fstat`, and the `namespace:`/`""` (list) branches of `openat`. Only the +`open_scheme_resource` branch — a blocking call to another daemon — needs to move off the loop. + +## Design A — worker-offload (self-contained in bootstrap; recommended first) + +Keep resolution serialized; move only the blocking `openat` to a small worker pool. The dispatcher +resolves the target `cap_fd` under a lock (fast), then hands the *blocking* call to a worker and +keeps accepting requests. The worker replies out-of-band when the open completes. + +### Building blocks (all verified available in the no_std bootstrap) + +- **`redox_rt::sync::Mutex`** (`relibc/redox-rt/src/sync.rs`) — futex-based, `Send + Sync` for + `T: Send`. Replaces the current `Rc>` sharing. +- **`FdGuard` is `#[repr(transparent)]` over `usize`** (`redox-rt/src/proc.rs:792`) — trivially + `Send`. The namespace already holds scheme caps as `Arc`; a worker clones the `Arc` + (not the fd), so `FdGuard`'s `Drop` (which closes the fd) fires only when the last reference is + gone — no double-close. +- **`Socket`** (redox-scheme) is an fd wrapper; `write_response(&self, …)` takes `&self`, and + `CallRequest → Tag` is `Send`. Share the socket as `Arc` so a worker can reply. +- **`redox_rt::thread::rlct_clone_impl`** (`redox-rt/src/thread.rs`) — the raw thread primitive. + **This is the risky part**: bootstrap is `#![no_std]` and does not link relibc's `pthread`, so a + worker thread must be brought up by hand (stack `mmap`, TCB alloc/init, TLS, then `rlct_clone_impl`) + — the minimal subset of what `relibc/src/pthread/mod.rs:203` + `platform/redox/mod.rs:1089` do. + Budget this as the main implementation cost. + +### State changes + +```rust +// Before: single-threaded interior mutability +namespace: Rc> + +// After: shareable across dispatcher + workers +namespace: Arc> +``` + +`Namespace.schemes` is already `HashMap>` — `Arc` is `Send`, so the +map is `Send` once the outer cell is a `Mutex`. The `NamespaceScheme.handles`/`next_id` bookkeeping +stays on the dispatcher thread (never touched by workers). + +### Work item + queue + +```rust +struct OpenWork { + tag: Tag, // reply target (Send) + cap_fd: Arc, // provider scheme cap (Send; refcount keeps it alive) + reference: String, // path within the provider scheme + flags: usize, + fcntl_flags: u32, +} + +// Shared, bounded queue + futex wakeup (no std::mpsc in no_std): +struct WorkQueue { + inner: redox_rt::sync::Mutex>, + // futex word bumped on push; workers futex-wait on it when the queue is empty. +} +``` + +Bounded (e.g. 64). On overflow the dispatcher falls back to handling the open inline (degrades to +today's behavior for that one request rather than dropping it) — never unbounded growth. + +### Dispatcher openat path + +```rust +// openat, scheme != "namespace" and != "" (list): +let cap_fd = { + let ns = ns_access.namespace.lock(); // fast: hashmap lookup + ns.get_scheme_fd(scheme).cloned() // Arc clone, released with the lock +}; +let Some(cap_fd) = cap_fd else { return Err(ENODEV) }; + +queue.push(OpenWork { tag: req.tag(), cap_fd, reference, flags, fcntl_flags }); +// DO NOT write a response here — the worker will. Return a "deferred" marker so +// run() skips write_response for this request. +``` + +This needs the low-level request API (`next_request` → keep the `CallRequest`/`Tag`, respond later) +rather than `handle_sync`, which always writes a response. redox-scheme already exposes `Tag` / +`Response::return_external_fd(fd, tag)` for exactly this. + +### Worker loop + +```rust +loop { + let work = queue.pop_blocking(); // futex-wait when empty + let res = syscall::openat(work.cap_fd.as_raw_fd(), + &work.reference, work.flags, work.fcntl_flags as usize); + let resp = match res { + Ok(fd) => Response::return_external_fd(fd, work.tag), + Err(e) => Response::err(e.errno, work.tag), + }; + let _ = socket.write_response(resp, SignalBehavior::Restart); // Arc +} +``` + +Results arrive out of order — fine, the kernel matches by `tag`. If the client died meanwhile, its +`on_close` already ran on the dispatcher; the late `write_response` targets a dead tag and the +kernel discards it (must be verified to be a no-op, not an error). + +### Concurrency invariants + +- **Namespace mutation stays serialized** under the `Mutex`; workers never touch namespace state, + only a cloned `cap_fd`. So there is no ordering hazard between a `fork`/`register` and an in-flight + open — the open captured its `cap_fd` before being queued. +- **Provider serialization is unchanged**: each provider still serializes its own requests; we only + stop *initnsmgr* from serializing *unrelated* providers behind one slow one. +- **Worker count**: 2–4. This is fault-isolation, not throughput — enough that a couple of slow + providers cannot stall the rest. + +## Design B — kernel `O_NONBLOCK` on open + single-thread deferred (no bootstrap threads) + +Avoids the no_std thread bring-up entirely, at the cost of a kernel change with a wider blast radius. + +1. **Kernel**: make `UserInner::call_inner` (`kernel/src/scheme/user.rs`) honor `O_NONBLOCK` on the + open opcode — return `EAGAIN` instead of `.block()`ing when the provider has not taken/answered + the request, using the existing cancellation path. +2. **initnsmgr** stays single-threaded and event-driven (the `RawEventQueue` pattern acpid already + uses): try `openat` with `O_NONBLOCK`; on `EAGAIN`, park `(tag, cap_fd, reference, flags)` in a + pending list, subscribe to the provider fd's readiness, and continue `next_request`. On readiness, + retry and `write_response(tag)`. +3. **Bonus**: this immediately activates fbcond's existing handoff-retry (it already opens with + `O_NONBLOCK` and retries; today the retry never fires because the first open blocks). + +B is architecturally cleaner (no threads in the earliest-boot component) but changes scheme-open +semantics for *every* scheme in the system, so it needs the strongest system-wide validation. + +## Recommendation + +Start with **A**. It is contained in one component and does not change kernel or redox-scheme +semantics for the rest of the system, so a regression is bounded to initnsmgr and reversible by +reverting one file. Its cost is the manual thread bring-up in no_std; budget that explicitly. +Consider **B** later as the cleaner long-term shape, or if upstream Redox moves scheme-open toward +non-blocking semantics anyway. + +## Staged implementation plan (A) + +1. **Refactor sharing, no behavior change**: `Rc>` → `Arc>`, + still fully synchronous (lock/handle/unlock inline). Build + boot: must be identical to today. + This isolates the mechanical Send refactor from the concurrency change. +2. **Thread bring-up spike**: bring up ONE worker thread in bootstrap that does nothing but log a + heartbeat via the debug fd, and prove it survives boot. This de-risks the hardest part alone. +3. **Introduce the queue + single worker**, route only `open_scheme_resource` through it, deferred + response. Keep the inline fallback on queue-full. Boot + measure. +4. **Scale to N=2–4 workers**, add the bounded-queue overflow fallback and dead-tag handling. +5. **Load test**: N≥10 boots on an idle host, plus an induced-slow-provider test (a provider that + sleeps before servicing its socket) to confirm one slow provider no longer wedges the rest. + +Each step is independently bootable and revertable. + +## Validation + +- Framebuffer screendump (QMP) is ground truth; the serial mirror is racy. +- **One boot proves nothing** — the failure is a race; compare rates across N≥10 boots **on an idle + host**. External load (e.g. a background `opencode` at 200–290% CPU) contaminates every measurement + and must be absent for a verdict. +- Success: N≥10 consecutive clean boots to a working brush login, and no head-of-line wedge under an + induced slow-provider. + +## Risk / rollback + +- Blast radius of A is initnsmgr only; revert = restore one file + the `submodule/base` gitlink. +- The thread bring-up is the sole high-risk element; step 2 isolates it before any concurrency logic. +- Do NOT ship any step that was only validated under external load. diff --git a/local/patches/wip-initnsmgr/step1-send-refactor.patch b/local/patches/wip-initnsmgr/step1-send-refactor.patch new file mode 100644 index 0000000000..8f552e9f91 --- /dev/null +++ b/local/patches/wip-initnsmgr/step1-send-refactor.patch @@ -0,0 +1,123 @@ +diff --git a/bootstrap/src/initnsmgr.rs b/bootstrap/src/initnsmgr.rs +index 12d678c4..cf1d8cf8 100644 +--- a/bootstrap/src/initnsmgr.rs ++++ b/bootstrap/src/initnsmgr.rs +@@ -1,8 +1,6 @@ +-use alloc::rc::Rc; + use alloc::string::{String, ToString}; + use alloc::sync::Arc; + use alloc::vec::Vec; +-use core::cell::RefCell; + use core::fmt::Debug; + use core::mem; + use hashbrown::HashMap; +@@ -11,6 +9,13 @@ use log::{error, warn}; + use redox_path::RedoxPath; + use redox_path::RedoxScheme; + use redox_rt::proc::FdGuard; ++// Namespace state is shared between the dispatcher and (from the worker-offload ++// step) the open workers, so it uses redox_rt's Send/Sync Mutex instead of the ++// single-threaded Rc. NOTE: this Mutex is a spinlock -- never hold it ++// across a blocking syscall; resolve the cap_fd under the lock, clone the Arc, ++// and do the blocking openat with the lock released. See ++// local/docs/INITNSMGR-CONCURRENCY-DESIGN.md. ++use redox_rt::sync::Mutex; + use redox_scheme::{ + CallerCtx, OpenResult, RequestKind, Response, SendFdRequest, SignalBehavior, Socket, + scheme::{SchemeState, SchemeSync}, +@@ -59,9 +64,11 @@ impl Namespace { + } + } + +-#[derive(Debug, Clone)] ++// No `Debug` derive: redox_rt's Mutex wraps an UnsafeCell and is not Debug. ++// `Clone` clones the Arc (a refcount bump), not the namespace. ++#[derive(Clone)] + struct NamespaceAccess { +- namespace: Rc>, ++ namespace: Arc>, + permission: NsPermissions, + } + +@@ -71,15 +78,15 @@ impl NamespaceAccess { + } + } + +-#[derive(Debug, Clone)] ++#[derive(Clone)] + struct SchemeRegister { +- target_namespace: Rc>, ++ target_namespace: Arc>, + scheme_name: String, + } + + impl SchemeRegister { + fn register(&self, fd: FdGuard) -> Result<()> { +- let mut ns = self.target_namespace.borrow_mut(); ++ let mut ns = self.target_namespace.lock(); + if ns.schemes.contains_key(&self.scheme_name) { + return Err(Error::new(EEXIST)); + } +@@ -88,7 +95,7 @@ impl SchemeRegister { + } + } + +-#[derive(Debug, Clone)] ++#[derive(Clone)] + enum Handle { + Access(NamespaceAccess), + Register(SchemeRegister), +@@ -122,7 +129,7 @@ impl<'sock> NamespaceScheme<'sock> { + + fn add_namespace(&mut self, id: usize, schemes: Namespace, permission: NsPermissions) { + let handle = Handle::Access(NamespaceAccess { +- namespace: Rc::new(RefCell::new(schemes)), ++ namespace: Arc::new(Mutex::new(schemes)), + permission, + }); + self.handles.insert(id, handle); +@@ -183,9 +190,9 @@ impl<'sock> NamespaceScheme<'sock> { + Ok(scheme_fd) + } + +- fn fork_namespace(&mut self, namespace: Rc>, names: &[u8]) -> Result { ++ fn fork_namespace(&mut self, namespace: Arc>, names: &[u8]) -> Result { + let new_id = self.next_id; +- let new_namespace = namespace.borrow().fork(names).map_err(|e| { ++ let new_namespace = namespace.lock().fork(names).map_err(|e| { + error!("Failed to fork namespace {}: {}", new_id, e); + e + })?; +@@ -258,8 +265,13 @@ impl<'sock> SchemeSync for NamespaceScheme<'sock> { + flags: NewFdFlags::empty(), + }); + } ++ // STEP 1 (mechanical): the lock is held across the blocking openat ++ // inside open_scheme_resource. That is safe only because this is ++ // still single-threaded. STEP 3 must resolve+clone the cap_fd under ++ // a short lock here and run the openat with the lock released, or ++ // the spinlock would burn a worker while a provider is slow. + _ => self.open_scheme_resource( +- &ns_access.namespace.borrow(), ++ &ns_access.namespace.lock(), + scheme.as_ref(), + reference.as_ref(), + flags, +@@ -329,7 +341,7 @@ impl<'sock> SchemeSync for NamespaceScheme<'sock> { + error!("Namespace with ID {} not found", fd); + Error::new(ENOENT) + })?; +- let mut ns = ns_access.namespace.borrow_mut(); ++ let mut ns = ns_access.namespace.lock(); + + let redox_path = RedoxPath::from_absolute(path).ok_or(Error::new(EINVAL))?; + let (scheme, reference) = redox_path.as_parts().ok_or(Error::new(EINVAL))?; +@@ -411,7 +423,7 @@ impl<'sock> SchemeSync for NamespaceScheme<'sock> { + return Err(Error::new(EACCES)); + } + +- let ns = ns_access.namespace.borrow(); ++ let ns = ns_access.namespace.lock(); + + let opaque_offset = opaque_offset as usize; + for (i, (name, _)) in ns.schemes.iter().enumerate().skip(opaque_offset) { diff --git a/local/sources/base b/local/sources/base index 0407d9ccbb..8dd8fb3b20 160000 --- a/local/sources/base +++ b/local/sources/base @@ -1 +1 @@ -Subproject commit 0407d9ccbb7c9f08346e927a4b3c4c0d3d7d5e29 +Subproject commit 8dd8fb3b2059825c5733e0687c14f0f449ac3d3c