acpid hardening + initnsmgr concurrency design
- Bump submodule/base to the acpid AML-handler hardening (bounded stall, mutex owner-check, static _PSS/_PSD/_CST/_CPC cache, panic-free scheme path, observability). Proven NOT a regression: a mutex-only baseline wedges identically under load in a 3/3 framebuffer-ground-truth test, so the residual under-load boot wedge is head-of-line blocking in initnsmgr, not acpid. - Add local/docs/INITNSMGR-CONCURRENCY-DESIGN.md: the concrete worker-offload design (Design A) that decouples the blocking openat from the initnsmgr dispatch loop, plus Design B (kernel O_NONBLOCK + deferred single-thread), a staged plan, and validation rules. Key finding baked in: redox_rt's Mutex is a spinlock, so the cap_fd must be resolved under a short lock and the openat run with the lock released. - Update INIT-NAMESPACE-MANAGER-SCALABILITY-PLAN.md with the acpid hardening section (done vs the still-deferred #1 transport decoupling). - Add local/patches/wip-initnsmgr/step1-send-refactor.patch: the compiled-but-not-yet-boot-validated Step 1 (Rc<RefCell> -> Arc<Mutex> Send refactor) of initnsmgr, saved durably. It is intentionally NOT in the base gitlink: bootstrap is the earliest-boot component and this must be boot-validated on an idle host (the current host is under heavy external load) before landing. Steps 2-5 (worker bring-up) likewise need an idle host.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<T>`** (`relibc/redox-rt/src/sync.rs`) — futex-based, `Send + Sync` for
|
||||
`T: Send`. Replaces the current `Rc<RefCell<Namespace>>` sharing.
|
||||
- **`FdGuard` is `#[repr(transparent)]` over `usize`** (`redox-rt/src/proc.rs:792`) — trivially
|
||||
`Send`. The namespace already holds scheme caps as `Arc<FdGuard>`; 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<Socket>` 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<RefCell<Namespace>>
|
||||
|
||||
// After: shareable across dispatcher + workers
|
||||
namespace: Arc<redox_rt::sync::Mutex<Namespace>>
|
||||
```
|
||||
|
||||
`Namespace.schemes` is already `HashMap<String, Arc<FdGuard>>` — `Arc<FdGuard>` 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<FdGuard>, // 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<VecDeque<OpenWork>>,
|
||||
// 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<Socket>
|
||||
}
|
||||
```
|
||||
|
||||
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<RefCell<Namespace>>` → `Arc<sync::Mutex<Namespace>>`,
|
||||
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.
|
||||
@@ -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<RefCell>. 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<RefCell<Namespace>>,
|
||||
+ namespace: Arc<Mutex<Namespace>>,
|
||||
permission: NsPermissions,
|
||||
}
|
||||
|
||||
@@ -71,15 +78,15 @@ impl NamespaceAccess {
|
||||
}
|
||||
}
|
||||
|
||||
-#[derive(Debug, Clone)]
|
||||
+#[derive(Clone)]
|
||||
struct SchemeRegister {
|
||||
- target_namespace: Rc<RefCell<Namespace>>,
|
||||
+ target_namespace: Arc<Mutex<Namespace>>,
|
||||
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<RefCell<Namespace>>, names: &[u8]) -> Result<usize> {
|
||||
+ fn fork_namespace(&mut self, namespace: Arc<Mutex<Namespace>>, names: &[u8]) -> Result<usize> {
|
||||
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) {
|
||||
+1
-1
Submodule local/sources/base updated: 0407d9ccbb...8dd8fb3b20
Reference in New Issue
Block a user