From ab6d9f9b9ff549b9a8ebe13cf55f6abae2dc5009 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 8 Sep 2026 15:12:08 -0700 Subject: [PATCH] fix(prim): refuse a region that cannot hold a segment, and diagnose reentrancy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the first outside firmware to put 2.0.0 on a chip (docs/plans/embedded-adoption.md). All three of its claims verified against the code before anything changed; all three were accurate, and §1b was worse than reported. DOCS FIRST (§1b). `ra_small_profile` appeared ZERO times in either README while `68 KiB` appeared five times as the embedded floor -- a figure only true under that cfg. The README did not merely omit the flag, it handed a reader a number that is wrong without it. Both READMEs now carry "What a firmware has to set", and the footprint row carries the cfg inline. §1. `init_region` validated `len < FIXED_PAGE` and nothing else, so a region that could never yield a segment returned `Ok(())`, linked clean, and failed every allocation on the board. It now returns `FERR_GEOMETRY`. The proposed check -- `len < FIXED_PAGE + SEGMENT_SIZE` -- was NOT sufficient. `init_region` has the base address, and a segment can only start on a segment boundary, so a region passing that length test with a misaligned base still yields nothing. The shipped check asks `usable_bytes(base, len) == 0`, which is exact where a length test is optimistic. `PrimError` is a `u32`, so distinct codes were free: FERR_TOO_SMALL, FERR_GEOMETRY, FERR_REGISTERED. `MIN_REGION` is public so the `const _: () = assert!(...)` the report asked for now compiles. §2. An allocating ISR wedged the firmware in an unbounded spin. On a target built `--cfg ra_single_threaded` a lock observed held can only be reentrancy, so it is now a panic that names the ISR and spells out that the cfg means single CONTEXT. The proposed trigger would have misfired: `compare_exchange_weak` may fail SPURIOUSLY, so panicking on a failed CAS would abort firmwares at random -- worse than the hang. The shipped version confirms with a load first. Watched firing under `--cfg ra_single_threaded`, which CI now runs; deliberately not in gate-selftest.sh, because poisoning it hangs rather than fails. §3. `usable_bytes(base, len)` is public and const. One part of the review of this item was WRONG and is not implemented: `region_stats`' `free` was said to misreport the stranded tail, but that tail is genuinely available to page-sized allocations, so `free` is honest and the missing number was a different one. The backend's own tests exercise it as an extent allocator on a 512 KiB region, which cannot hold a 32 MiB segment. Rather than weaken the check or lose the coverage, the install half is split into a private `install_region` and the test branches on the active geometry. The refusal has no public bypass. Gates: 110/33 default, 91 small profile, prim::fixed under ra_single_threaded, clippy on three configs, both RISC-V targets at both geometries, wasm32, census, gate selftest 5/5, wasm size ratchet. Board re-flashed at 68 KiB: still accepted, used 69632 free 0, kill test green. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 7 + README.md | 45 +++- crates/rusty_alloc/CHANGELOG.md | 34 +++ crates/rusty_alloc/README.md | 9 +- crates/rusty_alloc/src/prim/fixed.rs | 276 +++++++++++++++++++++- docs/plans/embedded-adoption.md | 336 +++++++++++++++++++++++++++ 6 files changed, 697 insertions(+), 10 deletions(-) create mode 100644 docs/plans/embedded-adoption.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9f47d8..37a6134 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -198,6 +198,13 @@ jobs: run: cargo clippy -p rusty_alloc --no-default-features -- -D warnings env: RUSTFLAGS: --cfg ra_single_threaded + # The reentrancy detector only exists under `ra_single_threaded`, so the + # normal test run cannot see it. An allocating ISR is the one bare-metal + # failure that used to be an unbounded spin with no message. + - name: reentrancy detector (single-context builds) + run: cargo test -p rusty_alloc --lib prim::fixed + env: + RUSTFLAGS: --cfg ra_single_threaded - name: prove the single-thread gate is not vacuous run: | if cargo check -p rusty_alloc --no-default-features \ diff --git a/README.md b/README.md index 2ec8619..6e0dc54 100644 --- a/README.md +++ b/README.md @@ -171,6 +171,49 @@ ESP32-S3. Everything below was measured on a **Seeed XIAO ESP32-S3 Sense at 240 MHz**, against **`esp-alloc` 0.11**, the standard allocator for the `esp-hal` bare-metal track. +### What a firmware has to set + +**All three of these, not two.** Every number in this section is *of this +configuration*; the 68 KiB floor below is meaningless without the geometry flag. + +```toml +rusty_alloc-api = { version = "2", default-features = false } +``` + +```sh +RUSTFLAGS="--cfg ra_single_threaded --cfg ra_small_profile" +``` + +```rust +// A region the linker owns, aligned to a segment. Hand it over once, before +// the first allocation. +#[repr(align(65536))] +struct Region([u8; 68 * 1024]); +static mut REGION: Region = Region([0; 68 * 1024]); + +// SAFETY: the only reference ever taken to REGION. +rusty_alloc::prim::fixed::init_region(unsafe { &mut (*(&raw mut REGION)).0 }) + .expect("region is large enough and registered once"); +``` + +| flag | what happens without it | +|---|---| +| `--cfg ra_single_threaded` | **build fails**, with a message telling you to set it | +| `--cfg ra_small_profile` | **builds and links clean, then nothing allocates** — `SEGMENT_SIZE` stays 32 MiB, a kilobyte-scale region yields zero segments, and the first `Vec` returns null | +| `init_region` | every allocation fails; the backend has no memory | + +That middle row is the trap, and it was reported by the first outside firmware +to adopt 2.0.0 (`docs/plans/embedded-adoption.md`). `ra_single_threaded` +announces itself, so an integrator reasonably concludes the crate tells you what +it needs — and `ra_small_profile` did not. `init_region` now refuses a region +that cannot hold one segment at the active geometry, so the mistake is an `Err` +at startup rather than a wasted board run; **`--cfg ra_small_profile` is still +what you want to set**, because refusing early is a diagnosis, not a fix. + +`ra_small_profile` is a `--cfg` and not a Cargo feature on purpose: it is +non-additive. Two crates in one graph cannot disagree about `SEGMENT_SIZE` the +way they can harmlessly disagree about `std`. + ### Throughput — 2.0x to 3.7x faster Nanoseconds per allocate/free pair, lower is better: @@ -217,7 +260,7 @@ in a medium page. | | `esp-alloc` | `rusty_alloc` | |---|---:|---:| -| smallest heap that runs the same workload | **8 KiB** | 68 KiB | +| smallest heap that runs the same workload | **8 KiB** | 68 KiB *(needs `--cfg ra_small_profile`)* | | peak live bytes (identical, the parity check) | 4,914 | 4,914 | | app image | 116,032 B | 127,088 B (+9.5%) | diff --git a/crates/rusty_alloc/CHANGELOG.md b/crates/rusty_alloc/CHANGELOG.md index 87d89c7..f153d3f 100644 --- a/crates/rusty_alloc/CHANGELOG.md +++ b/crates/rusty_alloc/CHANGELOG.md @@ -7,6 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `prim::fixed::MIN_REGION` and `prim::fixed::usable_bytes(base, len)` — a + firmware can now size its heap region at COMPILE time + (`const _: () = assert!(N >= MIN_REGION)`) instead of discovering the answer + on silicon, and can log how much of a region can actually back segments. The + `k * SEGMENT_SIZE + FIXED_PAGE` rule previously existed only in a design + document; a 220 KiB region strands 24,576 bytes and nothing said so. +- Distinct `prim::fixed` error codes: `FERR_TOO_SMALL`, `FERR_GEOMETRY`, + `FERR_REGISTERED`. One sentinel covered three conditions with three different + fixes. + +### Fixed + +- **`init_region` accepted a region that could never yield a segment.** Setting + `--cfg ra_single_threaded` (which the crate demands loudly) without + `--cfg ra_small_profile` (which nothing demanded) left `SEGMENT_SIZE` at + 32 MiB, so a kilobyte-scale region returned `Ok(())`, linked clean, and then + failed every allocation on the board with a backtrace pointing at whatever + allocated first. It now returns `FERR_GEOMETRY`, checked against the real base + address rather than the length alone — an unaligned base needs up to + `SEGMENT_SIZE - 1` more than a length test would demand. +- **An allocating interrupt handler hung the firmware silently.** + `prim::fixed`'s lock is not reentrant, and on a single-context target a lock + observed held can only mean reentrancy. That is now a panic naming the ISR + instead of an unbounded spin that surfaces as a watchdog reset. Confirmed with + a load, because `compare_exchange_weak` may fail spuriously and a bare CAS + failure would misfire. +- The README documents `--cfg ra_small_profile`, which it never mentioned, and + attaches it to the 68 KiB figure that is only true under it. + + All four reported by the first outside firmware to adopt 2.0.0 + (`docs/plans/embedded-adoption.md`). + ## [2.0.0](https://github.com/Remade-With-Rust/rusty_alloc/compare/rusty_alloc-v1.1.6...rusty_alloc-v2.0.0) - 2026-09-07 ### Breaking diff --git a/crates/rusty_alloc/README.md b/crates/rusty_alloc/README.md index 10db459..0710d60 100644 --- a/crates/rusty_alloc/README.md +++ b/crates/rusty_alloc/README.md @@ -106,8 +106,15 @@ realloc chains, zeroing over dirtied memory, fragmentation, exhaustion); it found and fixed a real reclamation bug on the way, after which 512 B capacity no longer decays and churn NULLs fell from 22,533 to 357 per 50,000. +**A firmware must set three things**, not two: `default-features = false`, +`--cfg ra_single_threaded` (the build fails without it, loudly) and +**`--cfg ra_small_profile`** (nothing tells you, and without it `SEGMENT_SIZE` +stays 32 MiB, a kilobyte-scale region yields zero segments and every allocation +fails) — then hand the backend its memory with +`prim::fixed::init_region`. The full recipe is in the repository README. + **It costs RAM to get that.** The smallest heap that runs the same workload is -**68 KiB for `rusty_alloc` against 8 KiB for `esp-alloc`** — a linked-list +**68 KiB for `rusty_alloc` (at that geometry) against 8 KiB for `esp-alloc`** — a linked-list heap's floor is `bytes live + header`, while a size-class page allocator's is `(classes touched) x (page size)`, independent of bytes requested. That floor is roughly fixed, so it amortises as the working set grows. Reach for `esp-alloc` diff --git a/crates/rusty_alloc/src/prim/fixed.rs b/crates/rusty_alloc/src/prim/fixed.rs index 635ffa8..7b8691b 100644 --- a/crates/rusty_alloc/src/prim/fixed.rs +++ b/crates/rusty_alloc/src/prim/fixed.rs @@ -50,6 +50,36 @@ use super::{Alloc, MemConfig, PrimError, TlsDtor, align_up}; /// sentinel does; this one is distinct from wasm's `0xBEEF` and the mock's. const FERR: PrimError = 0xF13D; +/// The region is smaller than one [`FIXED_PAGE`]. +pub const FERR_TOO_SMALL: PrimError = 0xF13E; + +/// The region cannot hold a single `SEGMENT_SIZE` segment at the ACTIVE +/// geometry, so the allocator above this backend could never serve anything. +/// +/// Almost always one missing flag: `--cfg ra_small_profile` keeps +/// `SEGMENT_SIZE` at 32 MiB, and a kilobyte-scale region yields zero segments. +pub const FERR_GEOMETRY: PrimError = 0xF13F; + +/// A region is already registered; this backend takes one, once. +pub const FERR_REGISTERED: PrimError = 0xF140; + +/// The smallest region this backend will accept, for a `SEGMENT_SIZE`-ALIGNED +/// base: one segment for the allocator plus one page for its heap descriptor. +/// +/// Exposed so a firmware can settle its budget at COMPILE time rather than on +/// silicon, which is what the first outside adopter asked for +/// (`docs/plans/embedded-adoption.md`): +/// +/// ```ignore +/// const _: () = assert!(REGION_BYTES >= rusty_alloc::prim::fixed::MIN_REGION); +/// ``` +/// +/// An UNALIGNED base needs up to `SEGMENT_SIZE - 1` more, because the first +/// segment can only start on a segment boundary; [`init_region`] checks the +/// real base and is therefore exact where this constant is optimistic. Align +/// the region and the two agree. +pub const MIN_REGION: usize = crate::types::SEGMENT_SIZE + FIXED_PAGE; + /// Page granularity reported to the layers above. A chip has no paging /// hardware, so this is a bookkeeping unit rather than a hardware fact; 4 KiB /// matches the flash/RAM block size the ESP parts use and keeps `page_align_up` @@ -91,18 +121,110 @@ impl Guard { .compare_exchange_weak(false, true, Ordering::Acquire, Ordering::Relaxed) .is_err() { + // Under `ra_single_threaded` a genuinely contended acquire is not a + // race, because there is no second thread to race with. It can only + // be REENTRANCY: an interrupt handler that allocated while the main + // context was inside the allocator. That wedges forever -- the + // preempted context can never run to release the lock -- and + // surfaces as a watchdog reset with a backtrace pointing into + // `spin_loop`, which names nothing. + // + // The LOAD is not redundant. `compare_exchange_weak` may fail + // SPURIOUSLY, so a failed CAS is not by itself proof of anything; + // only a lock actually observed held is. Getting this wrong would + // panic firmwares at random, which is worse than the hang it + // replaces. + #[cfg(ra_single_threaded)] + if lock.load(Ordering::Relaxed) { + reentered(); + } core::hint::spin_loop(); } Self(lock) } } +/// The allocator was re-entered on a target that promised one context. +/// +/// Separated and `#[cold]` so the happy path is unchanged: the CAS already +/// happens, and only its failure arm gains a load and a call that never +/// returns. +/// +/// If the firmware's panic handler itself allocates it will re-enter here and +/// panic again, which aborts. That is a defined ending and a diagnosable one; +/// the behaviour being replaced is an unbounded spin with no message at all. +#[cfg(ra_single_threaded)] +#[cold] +#[inline(never)] +fn reentered() -> ! { + // A literal, not a format: `core::fmt` is not on this crate's `no_std` + // budget, and this message must survive a build that has no formatter. + panic!( + "rusty_alloc: the allocator was re-entered. On a target built with \ + --cfg ra_single_threaded nothing else can hold this lock, so this is \ + almost certainly an interrupt handler that allocated while the main \ + context was inside the allocator. prim::fixed's lock is NOT \ + reentrant: do not allocate in an ISR. Note that ra_single_threaded \ + means single CONTEXT, and an interrupt handler is a second context on \ + one core." + ) +} + impl Drop for Guard { fn drop(&mut self) { self.0.store(false, Ordering::Release); } } +/// How many bytes of `[base, base + len)` can ever back SEGMENTS. +/// +/// The rule this answers used to live only in a design document +/// (`docs/plans/small-metal.md`: *"size an embedded region as +/// `k * 64 KiB + 4 KiB`, or the tail is dead to large allocations"*), which +/// meant a firmware author picking a round number learned it by reading prose +/// or not at all. A 220 KiB region at the small profile yields three segments +/// and strands 24,576 bytes — 11 % of the budget, silently. +/// +/// Segments are carved from the first `SEGMENT_SIZE`-aligned address upward and +/// page-sized blocks from the top down, so the answer is +/// `floor((end - first_aligned - FIXED_PAGE) / SEGMENT_SIZE) * SEGMENT_SIZE`: +/// the leading bytes before alignment are unusable, and one page is reserved at +/// the top for the heap descriptor without which the allocator cannot start. +/// +/// **This is not the same question as [`region_stats`]'s `free`.** That reports +/// bytes nobody has taken, and the stranded tail is genuinely available to +/// page-sized allocations — so it is free, and it is also useless for segments. +/// Reporting one number as if it answered both is how a clean number ends up +/// measuring nothing; they are separate on purpose. +/// +/// `const fn`, so a seam crate can size its region at compile time: +/// +/// ```ignore +/// const _: () = assert!(usable_bytes(0, REGION_BYTES) > 0); +/// ``` +#[must_use] +pub const fn usable_bytes(base: usize, len: usize) -> usize { + let seg = crate::types::SEGMENT_SIZE; + let Some(end) = base.checked_add(len) else { + return 0; + }; + // First segment-aligned address at or above `base`, without overflowing. + let Some(run_up) = base.checked_add(seg - 1) else { + return 0; + }; + let first = run_up & !(seg - 1); + if first >= end { + return 0; + } + let avail = end - first; + // One page at the top for the heap descriptor. Without it `create_heap` + // fails and no segment can be used even if one fits. + if avail <= FIXED_PAGE { + return 0; + } + ((avail - FIXED_PAGE) / seg) * seg +} + /// Hand the backend the region it will serve from, once. /// /// Takes `&'static mut [u8]` because that is exactly the claim being made: the @@ -114,24 +236,52 @@ impl Drop for Guard { /// to hold anything after alignment. /// /// # Errors -/// [`FERR`] on a second call, or on a region below [`FIXED_PAGE`] bytes. +/// - [`FERR_TOO_SMALL`] — below one [`FIXED_PAGE`]. +/// - [`FERR_GEOMETRY`] — cannot hold one `SEGMENT_SIZE` segment at this +/// geometry, so the allocator above could never serve an allocation. This is +/// the one that used to be accepted silently: `init_region` returned `Ok`, +/// the build was clean, and the first `Vec` on the board returned null with a +/// backtrace pointing at whatever happened to allocate first. Reported by the +/// first outside firmware to adopt 2.0.0 (`docs/plans/embedded-adoption.md`). +/// - [`FERR_REGISTERED`] — a region is already registered. pub fn init_region(region: &'static mut [u8]) -> Result<(), PrimError> { let len = region.len(); if len < FIXED_PAGE { - return Err(FERR); + return Err(FERR_TOO_SMALL); } let base = region.as_mut_ptr().expose_provenance(); + // EXACT, not conservative. `MIN_REGION` assumes a segment-aligned base; the + // real base is in hand here, so ask the question that actually matters -- + // does an aligned segment plus a page fit inside this region? A check + // against `len` alone would accept a region whose base sits one byte past a + // segment boundary and still fail on the board. + if usable_bytes(base, len) == 0 { + return Err(FERR_GEOMETRY); + } + let _g = Guard::acquire(&LOCK); if REGION_LEN.load(Ordering::Relaxed) != 0 { - return Err(FERR); + return Err(FERR_REGISTERED); } + install_region(base, len); + Ok(()) +} + +/// The install half of [`init_region`], with no geometry check. +/// +/// Split out for the unit tests, which exercise this backend as a plain extent +/// allocator -- first fit, coalescing, two-ended placement -- on a region far +/// smaller than a 32 MiB segment. That is a legitimate thing to test and NOT a +/// legitimate thing to ship: an allocator handed a region that cannot hold one +/// segment is dead on arrival, which is exactly what [`init_region`] now +/// refuses. Private, so the refusal has no public bypass. +fn install_region(base: usize, len: usize) { REGION_BASE.store(base, Ordering::Relaxed); REGION_LEN.store(len, Ordering::Relaxed); EXT_BASE[0].store(base, Ordering::Relaxed); EXT_LEN[0].store(len, Ordering::Relaxed); EXT_COUNT.store(1, Ordering::Relaxed); - Ok(()) } /// The region's occupancy: `(used, free, total)` bytes. @@ -540,15 +690,42 @@ mod tests { // init_region which requires (and consumes) exactly that exclusivity. let region: &'static mut [u8] = unsafe { core::slice::from_raw_parts_mut(rp, N) }; - init_region(region).expect("first registration succeeds"); + // The negative cases FIRST: each must be refused without consuming the + // one registration this backend accepts. + let tiny: &'static mut [u8] = &mut []; + assert_eq!( + init_region(tiny), + Err(FERR_TOO_SMALL), + "a region below one page is refused, and says which" + ); + + // Branch on the ACTIVE geometry, because both arms are real. At the + // shipped 32 MiB segment this 512 KiB region cannot hold one, so + // `init_region` refuses it -- correctly, since an allocator handed it + // would be dead on arrival -- and the extent allocator beneath, which is + // what the rest of this test exercises, is installed directly. At the + // small profile a 64 KiB segment fits eight times over and the public + // entry point is used as a firmware would. + if usable_bytes(rp.expose_provenance(), N) == 0 { + assert_eq!( + init_region(region), + Err(FERR_GEOMETRY), + "a region that cannot hold one segment is refused BEFORE the board" + ); + install_region(rp.expose_provenance(), N); + } else { + init_region(region).expect("this geometry's segment fits in N"); + } assert_eq!(free_total(), N, "the whole region starts free"); assert_eq!(extents().len(), 1, "as one extent"); - // A second registration is refused: the region is handed over once. + // A second registration is refused, and says so distinctly. let op = &raw mut OTHER; // SAFETY: as above; the call is expected to fail before it stores it. - let other: &'static mut [u8] = unsafe { &mut *op }; - assert!(init_region(other).is_err(), "no second region"); + let other: &'static mut [u8] = + unsafe { core::slice::from_raw_parts_mut(op.cast::(), FIXED_PAGE) }; + let second = init_region(other); + assert!(second.is_err(), "no second region"); // Serve three page-aligned blocks. // SAFETY: the prim contract — sizes are page multiples, alignment a @@ -723,6 +900,89 @@ mod tests { n } + /// `usable_bytes` is pure arithmetic, so it gets its own test with no + /// global state -- and the case that motivated it, from the first outside + /// adopter's report. + #[test] + fn usable_bytes_answers_the_question_a_firmware_asks() { + let seg = SEGMENT_SIZE; + + // Aligned base: MIN_REGION is exactly enough for one segment, and one + // byte less is not. + assert_eq!( + usable_bytes(0, MIN_REGION), + seg, + "MIN_REGION buys a segment" + ); + assert_eq!( + usable_bytes(0, MIN_REGION - 1), + 0, + "one byte short buys none" + ); + + // The page at the top is not optional: a region of exactly one segment + // has nowhere to put the heap descriptor, so it can serve nothing. + assert_eq!( + usable_bytes(0, seg), + 0, + "a segment with no page for the heap is unusable" + ); + + // An UNALIGNED base loses the run-up. This is why `init_region` checks + // the real base rather than comparing `len` against `MIN_REGION`: this + // region is >= MIN_REGION and still yields nothing. + assert_eq!( + usable_bytes(FIXED_PAGE, MIN_REGION), + 0, + "unaligned base eats the segment" + ); + assert_eq!( + usable_bytes(FIXED_PAGE, MIN_REGION + seg), + seg, + "one more segment of slack absorbs the misalignment" + ); + + // The stranded tail, which used to be recorded only in a design doc. + // Sized in segments so it says the same thing at either geometry; at + // the small profile this is the report's 220 KiB case exactly. + let three_and_a_bit = 3 * seg + FIXED_PAGE + seg / 2; + assert_eq!( + usable_bytes(0, three_and_a_bit), + 3 * seg, + "a ragged region yields whole segments and strands the remainder" + ); + let stranded = three_and_a_bit - usable_bytes(0, three_and_a_bit) - FIXED_PAGE; + assert_eq!( + stranded, + seg / 2, + "and the strand is exactly the ragged part" + ); + } + + /// The reentrancy detector, watched firing. + /// + /// "A failure mode nobody has watched fire is a claim, not a defence" is + /// this repo's own line, and it applies to the thing that replaced the + /// hang. Only compiled where the detector is: run it with + /// `RUSTFLAGS="--cfg ra_single_threaded" cargo test -p rusty_alloc --lib prim::fixed`, + /// which CI does. + /// + /// Deliberately NOT in `tools/gate-selftest.sh`: poisoning this gate + /// removes the detector, and the test then HANGS instead of failing -- + /// which is the whole point of the defect, and useless in a CI job. The + /// evidence that it fires is this test passing where the detector exists + /// and the code not compiling it where it does not. + #[cfg(ra_single_threaded)] + #[test] + #[should_panic(expected = "re-entered")] + fn a_reentrant_acquire_is_diagnosed_not_hung() { + static LOCK2: AtomicBool = AtomicBool::new(false); + let _outer = Guard::acquire(&LOCK2); + // Exactly what an allocating ISR does: acquire while the outer context + // still holds it. Without the detector this line never returns. + let _inner = Guard::acquire(&LOCK2); + } + /// The no-MMU decisions, pinned so a future edit has to mean it. #[test] fn no_mmu_semantics_are_explicit() { diff --git a/docs/plans/embedded-adoption.md b/docs/plans/embedded-adoption.md new file mode 100644 index 0000000..4e13f4b --- /dev/null +++ b/docs/plans/embedded-adoption.md @@ -0,0 +1,336 @@ +# Adopting rusty_alloc in a firmware: three sharp edges + +**Status: ALL FOUR IMPLEMENTED 2026-09-08** — see §Resolution at the end. Two +of the proposals were changed on implementation, both because the proposed form +would not have worked; that is recorded there rather than quietly fixed. + +**Originally: PROPOSED 2026-09-08.** Written from the outside, by the first +consumer to put 2.0.0 into an ESP32 firmware that is not this repo's own +benchmark. Everything below was hit in one afternoon of integration, and none +of it was hit by the harness in `small-metal.md` — because that harness sets +the geometry correctly, runs no interrupts, and sizes its region by hand. + +That is the theme. **The allocator is right; the adoption path is where a +consumer falls over,** and all three findings are cases where a wrong +integration is accepted silently and fails somewhere far away from its cause. + +Consumer: the Janus family (`rusty_esp_*`), a XIAO ESP32-S3 Sense on the +`esp-hal` bare-metal track, adopting through a one-crate seam +(`rusty_esp_alloc`) as the house standard requires. + +--- + +## 1. A region that cannot yield one segment is accepted + +**Severity: this is the one to fix.** It costs a board run and gives no clue. + +`init_region` refuses a region below `FIXED_PAGE` and accepts everything +above it: + +```rust +let len = region.len(); +if len < FIXED_PAGE { + return Err(FERR); +} +``` + +But the layers above carve the region into `SEGMENT_SIZE` granules, and +`SEGMENT_SIZE` is **32 MiB** unless `ra_small_profile` is set. So a firmware +that sets `ra_single_threaded` (which the crate demands, loudly, and which +therefore gets set) but *not* `ra_small_profile` (which nothing demands) gets: + +| | | +|---|---:| +| region handed over | 225,280 B (220 KiB) | +| `SEGMENT_SIZE`, default geometry | 33,554,432 B | +| segments the region yields | **0** | +| what `init_region` returns | `Ok(())` | +| what the build says | nothing; it compiles clean | +| what happens on the board | every allocation fails | + +The failure surfaces as the first `Vec` returning null, an allocation-error +handler, and a panic with a backtrace pointing at whatever happened to +allocate first. Nothing in that chain says "your segment geometry is 512x too +large for the region you gave me". + +**Proposed.** `init_region` refuses a region that cannot hold a segment: + +```rust +if len < FIXED_PAGE + SEGMENT_SIZE { + return Err(FERR); // or a distinct code; see below +} +``` + +Two notes on shape. First, `PrimError` is a single sentinel, so the caller +cannot tell "too small for a page" from "too small for a segment" from +"already registered" — three very different fixes. If a distinct code is too +much churn, the doc comment should at least name the third condition, because +right now `# Errors` lists two and there would be three. + +Second, and more valuable than the check: **the geometry should be visible.** +A `pub const` re-export of `SEGMENT_SIZE` from `prim::fixed`, or a +`region_requirements() -> (min_bytes, granule)`, lets a seam crate assert at +compile time rather than discovering it on silicon. The consumer wants to +write: + +```rust +const _: () = assert!(N >= rusty_alloc::prim::fixed::MIN_REGION); +``` + +and today cannot, because the relationship is spread across two modules and +one `--cfg`. + +### 1b. `ra_small_profile` is discoverable only by reading the source + +The crate refuses to build `no_std` without `ra_single_threaded`, with an +excellent `compile_error!` that says exactly what to do. `ra_small_profile` +has no such gate, is not mentioned in the README's embedded section, and is +the difference between a working firmware and one where nothing allocates. + +The asymmetry is the problem: a consumer meets the first cfg, learns that +this crate tells you what it needs, and reasonably concludes there is nothing +else to set. + +**Proposed.** Either gate it the same way (refuse a bare-metal target with the +default geometry unless the consumer has explicitly opted into 32 MiB +segments), or say it in the README beside the 68 KiB figure — that figure is +*of the small profile*, and a reader who copies the number without the cfg +gets neither. + +--- + +## 2. An allocating interrupt hangs, and the cfg's name hides it + +`prim::fixed`'s `Guard` is a plain non-reentrant spin lock, correctly +documented as such: + +```rust +fn acquire(lock: &'static AtomicBool) -> Self { + while lock.compare_exchange_weak(false, true, Acquire, Relaxed).is_err() { + core::hint::spin_loop(); + } + Self(lock) +} +``` + +On a single-threaded target, **a contended acquire is not possible from +another thread — there isn't one. It can only be reentrancy.** And on a chip +the only source of reentrancy is an interrupt handler that allocates while +the main context holds the lock. + +The consequence is a **hard hang inside the ISR**: the main context is +preempted and can never release, so the spin never ends. On an ESP32 that +becomes a watchdog reset with a backtrace pointing into `spin_loop`, or no +reset at all if the ISR outranks the watchdog. Either way the cause is +invisible. + +This matters more than it looks because of what the opt-in is called. +`ra_single_threaded` reads as "I have one core and no RTOS" — which is true +of every esp-hal firmware, so everyone will set it. The actual requirement is +**single *context***, and interrupt handlers are a second context on one core. +Timer, GPIO and DMA-completion handlers are ordinary in this ecosystem, and an +embassy executor makes allocating from one entirely plausible. + +**Proposed.** Turn the hang into a diagnosis. Under `ra_single_threaded` the +first failed CAS is proof of reentrancy, so: + +```rust +#[cfg(ra_single_threaded)] +{ + // Nothing else can hold this: there is no second thread. A contended + // acquire means this call re-entered from an interrupt handler that + // allocated while the main context was inside the allocator. + panic!("rusty_alloc: allocator re-entered, almost certainly from an \ + interrupt handler that allocates. The fixed backend's lock is \ + not reentrant; do not allocate in an ISR."); +} +``` + +Zero cost on the happy path — the CAS already happens, only the failure arm +changes — and it converts an undiagnosable wedge into a one-line answer. This +is the same principle as `tests/corruption.rs`: a failure mode nobody has +watched fire is a claim, not a defence. + +The docs should also say "single context, interrupts included" wherever the +cfg is described. A rename to `ra_single_context` would be clearer still, but +that is a breaking change to a published gate and probably not worth it alone. + +--- + +## 3. The stranded tail is in a plan, not in the API + +`small-metal.md` §3 of the stress section records the rule: + +> Size an embedded region as `k * 64 KiB + 4 KiB`, or the tail is dead to +> large allocations. + +Correct, and nothing surfaces it. Under the small profile a 220 KiB region +yields `floor((225,280 - 4,096) / 65,536) = 3` segments = 196,608 B, and +**24,576 bytes are dead** — 11% of the budget, silently. A firmware author +picking a round number like 220 KiB has no way to learn this except by +reading a design document. + +**Proposed.** Return it, or expose it: + +```rust +pub const fn usable_bytes(region_len: usize) -> usize; +``` + +so a firmware can log `budget=225280 usable=196608` at startup and see the +gap. `region_stats()` already exists precisely on the argument that "a +fixed-region allocator that cannot report how much of its region is out is +unmeasurable on exactly the deployment it exists for" — this is the same +argument applied one level up, to the region it was given rather than the +region it is using. + +--- + +## What is NOT proposed here + +- **Nothing about the 68 KiB floor.** It is structural, it is documented + honestly, and the README's advice — use `esp-alloc` when the budget is + tight — is correct and was followed. Our own firmwares mostly do not churn + (one allocates five buffers at startup and never allocates again; its heap + figure does not move across eight kernels), so we expect no speed win and + the whole footprint cost, and we are adopting for the double-free abort + rather than for throughput. That is the README working as intended. +- **Nothing about bin coarsening.** `small-metal.md` weighs it, records the + ABI argument and the unbounded-fragmentation argument, and defers it with + its number. Agreed, and not our call. +- **Nothing about the automatic `collect`.** Already the top item in that + plan's §6, already understood, and it needs the callgrind harness rather + than a board. +- **No speed or footprint claim from us.** We have built both arms of one + firmware from one source and measured only ELF bytes (243,300 -> 265,036, + +8.9%, close to the +9.5% app image this repo reports). Nothing has run on + silicon on our side yet. When it does, the numbers will come with their + method line and we will send them whether or not they flatter the swap. + +--- + +## Suggested order + +1. **§1** — the segment-size refusal, plus a `MIN_REGION` a consumer can + `const _: () = assert!` against. Smallest change, largest saving: it turns + a wasted board run into a build error or an `Err`. +2. **§1b** — say `ra_small_profile` where the 68 KiB figure is quoted. A + documentation change that prevents the same wasted run. +3. **§2** — the reentrancy panic. Small, zero-cost, and the difference + between a hang and an answer. +4. **§3** — `usable_bytes`. Nice to have; the rule is at least written down + today, which the other two are not. + +Items 1 and 2 both convert a silent wrong-integration into a loud one, which +is the same thing this crate already does well with `ra_single_threaded` and +with the double-free abort. They are that habit applied two steps further out. + +--- + +## Resolution (2026-09-08, this repo) + +All four landed. Every claim above was verified against the code first; all +three were accurate, and **§1b was worse than reported** — `ra_small_profile` +appeared **zero times** in either README, while the `68 KiB` figure appeared +five times as *the* embedded floor. That figure is only true under the cfg, so +the README did not merely omit the flag, it handed a reader a number that is +wrong without it. + +### §1b — done first, not third + +Promoted ahead of the code changes because it is minutes of work and prevents +the same wasted board run. Both READMEs now carry a **"What a firmware has to +set"** section: the dependency line, both cfgs, the `init_region` call, and a +table whose middle row is the trap — *builds and links clean, then nothing +allocates*. The `68 KiB` row in the footprint table now carries +`(needs --cfg ra_small_profile)` inline, because that is the number people copy. + +### §1 — done, with an exact check rather than the proposed one + +`init_region` now refuses. **The proposed condition was not sufficient:** + +```rust +if len < FIXED_PAGE + SEGMENT_SIZE { ... } // proposed +``` + +`init_region` receives the *base address*, and the first segment can only start +on a `SEGMENT_SIZE` boundary. A region satisfying that length test whose base +sits one byte past a boundary still yields nothing — the same silent failure, +one size smaller. The shipped check asks the real question, +`usable_bytes(base, len) == 0`, which is exact where a length test is +optimistic. `usable_bytes_answers_the_question_a_firmware_asks` pins that case +specifically. + +`PrimError` turned out to be a plain `u32`, so the distinct codes cost nothing: +**`FERR_TOO_SMALL`, `FERR_GEOMETRY`, `FERR_REGISTERED`** are public, and the +`# Errors` doc now lists three conditions instead of two. + +**`MIN_REGION` is public**, so the `const _: () = assert!(...)` the report asked +for now compiles. It documents that it assumes an aligned base and that +`init_region` is the exact one — the constant is for compile-time sizing, the +function for the truth. + +One consequence worth knowing: this backend's own unit tests exercise it as a +plain extent allocator on a 512 KiB region, which cannot hold a 32 MiB segment. +Rather than weaken the check or lose that coverage, the install half was split +into a private `install_region`, and the test branches on the active geometry — +asserting the refusal where a segment does not fit and using the public entry +point where it does. The refusal has no public bypass. + +### §2 — done, but the proposed trigger would have misfired + +The reasoning is right and the fix is nearly free. **The proposed trigger is +not safe as written:** + +```rust +while lock.compare_exchange_weak(...).is_err() { + #[cfg(ra_single_threaded)] { panic!(...) } // proposed +``` + +`compare_exchange_weak` is permitted to fail **spuriously**. A failed CAS is +therefore not proof of anything, and this would panic firmwares at random — +worse than the hang it replaces. The shipped version confirms with a load +before it accuses: + +```rust +#[cfg(ra_single_threaded)] +if lock.load(Ordering::Relaxed) { reentered(); } +``` + +A spurious failure leaves the lock `false` and retries; a lock actually observed +held, on a target that promised one context, can only be reentrancy. The message +says so, names the ISR, and spells out that `ra_single_threaded` means single +*context* — the report's point about the name, applied where someone hits it. + +Watched firing: `a_reentrant_acquire_is_diagnosed_not_hung` is `#[should_panic]` +and CI runs `prim::fixed`'s tests under `--cfg ra_single_threaded`. It is +deliberately **not** in `tools/gate-selftest.sh`: poisoning this gate removes the +detector, and the test then hangs rather than fails, which is the defect itself +and useless in CI. + +### §3 — done, and one part of the review of it was wrong + +`usable_bytes(base, len)` is public and `const`, so a region can be sized at +compile time or logged at startup. + +The review of this report initially agreed that `region_stats()` should stop +counting the stranded tail as `free`. **That was wrong and is not implemented.** +The tail is genuinely available to page-sized allocations — two-ended placement +serves those from the top — so `free` is honest. What was missing is a +*different* number: how much can still become a segment. Changing `free` would +have replaced one incomplete answer with another. The two are now separate, and +`usable_bytes`' doc says why. + +### What this did not change + +The 68 KiB floor, bin coarsening and the automatic `collect` are untouched, as +the report proposed. Their ELF measurement (243,300 -> 265,036, **+8.9%**) +independently corroborates this repo's **+9.5%** app-image figure from a +different firmware — the first outside check on that number. + +### Verification + +110 tests / 33 suites default; 91 small profile; `prim::fixed` under +`--cfg ra_single_threaded`; clippy clean on all three configs; both RISC-V +targets at both geometries; wasm32; unsafe census; gate selftest 5/5; wasm size +ratchet. Board re-flashed: 68 KiB region still accepted, `used 69632 free 0`, +kill test green.