E5: carry the fslab-OOM unwind fix (unregister before free)
expert_load's fslab-OOM unwind freed s->slab via compat_aligned_free without coli_metal_unregister -- a pre-existing gap on main/dev (validator-confirmed during E4) that leaves a stale g_slabs entry over freed memory, letting resolve() hand the GPU a dangling pointer. Under COLI_METAL_RESSET=1 the exposure is strictly longer-lived: the wrapped buffer would stay a permanent residency-set member over the freed pages instead of stock's transient last-command-buffer window. Ported from e4/metal-heap validator fix 6753225, adapted to dev's non-heap code shape (no coli_metal_heap_free wrapper -- plain unregister-before-free). The uring_load_add analog (E4 audit round-2 insurance) is deliberately not carried: that arm is #ifdef __linux__-gated while COLI_METAL is macOS-only, so it is dead code on every real build target, and unlike E4 this branch has no allocation-path reason to touch the function. Reachable only through allocation failure mid-load; verified by inspection and clean builds (no OOM-injection harness in tree). SUMMARY.md item 10 updated to "carried on this branch".
This commit is contained in:
+26
-20
@@ -21,9 +21,10 @@ All mechanism code is confined to `c/backend_metal.mm` —
|
||||
`coli_metal_register`/`coli_metal_unregister`'s existing signatures and every call site in
|
||||
`glm.c` (expert_load, uring_load_add, qalloc, kv_alloc, map_of_fd) are untouched; the
|
||||
residency-set bookkeeping lives entirely inside those two functions' existing bodies. The
|
||||
only `glm.c`/`backend_metal.h` touch is one instrumentation hook added in the validator
|
||||
round-1 fixes (`coli_metal_resset_stats` + the gate-on-only `METAL-RESSET:` stats line in
|
||||
`profile_print` — see "Validator round 1 fixes" below). Still a smaller diff shape than E4,
|
||||
`glm.c`/`backend_metal.h` touches are two, both coordinator-sanctioned: the validator
|
||||
round-1 instrumentation hook (`coli_metal_resset_stats` + the gate-on-only `METAL-RESSET:`
|
||||
stats line in `profile_print`) and the ported fslab-OOM unwind fix (see "Validator round 1
|
||||
fixes" item 4). Still a smaller diff shape than E4,
|
||||
which needed a new alloc/free API and four new `glm.c` call-site arms because it changed the
|
||||
allocation function itself.
|
||||
|
||||
@@ -180,17 +181,19 @@ existing disk/wait numbers. `[METAL] residency-set: on` / the two fallback stder
|
||||
3. **REQUIRED, flush cost made harness-visible**: `g_t_resset_flush` +
|
||||
`coli_metal_resset_stats()` + the gate-on-only `METAL-RESSET:` line in `profile_print` —
|
||||
see "Instrumentation parity" above.
|
||||
4. **NOTE — pre-existing fslab OOM-unwind bug becomes a longer-lived artifact under E5**
|
||||
(no code change on this branch, per the fix round's scoping): `expert_load`'s fslab OOM
|
||||
path (`c/glm.c` ~1868–1873 on this base) frees `s->slab` via `compat_aligned_free`
|
||||
**without** `coli_metal_unregister` — on stock this leaves a stale `g_slabs` entry whose
|
||||
GPU exposure ends with the last command buffer that declared it; under E5 the buffer is
|
||||
additionally a **permanent residency-set member** referencing freed host memory until
|
||||
4. **Pre-existing fslab OOM-unwind bug — now CARRIED ON THIS BRANCH** (follow-up commit,
|
||||
coordinator-sanctioned second `glm.c` change): `expert_load`'s fslab OOM path
|
||||
(`c/glm.c` ~1870 on this base) freed `s->slab` via `compat_aligned_free` **without**
|
||||
`coli_metal_unregister` — on stock that leaves a stale `g_slabs` entry whose GPU
|
||||
exposure ends with the last command buffer that declared it; under E5 the buffer would
|
||||
additionally be a **permanent residency-set member** referencing freed host memory until
|
||||
some later realloc of the same slot unregisters by pointer, a strictly longer-lived
|
||||
exposure than stock's transient per-CB one. When E5 graduates to the upstream PR, it must
|
||||
carry the one-line unregister-before-free fix; E4's branch has the reference
|
||||
implementation (`6753225`, "validator round-1 fixes (heap-create latch, unwind
|
||||
unregister)").
|
||||
exposure than stock's transient per-CB one. Fixed by porting E4's reference
|
||||
implementation (`6753225`) to dev's non-heap code shape: `coli_metal_unregister(s->slab)`
|
||||
before the free. The `uring_load_add` analog (E4's audit round-2 "cheap insurance") is
|
||||
deliberately NOT carried: that arm is `#ifdef __linux__`-gated and `COLI_METAL` is
|
||||
macOS-only, so it is dead code on every real build target, and unlike E4 this branch has
|
||||
no allocation-path reason to touch the function at all.
|
||||
|
||||
## Per-seam differences vs E4
|
||||
|
||||
@@ -336,10 +339,13 @@ model run — flagged per the task's hard requirement.**
|
||||
high-confidence. What is **not** independently verified is runtime behavior beyond what
|
||||
the docs state and what the synthetic unit test exercises — no substitute for the
|
||||
orchestrator's real cap-sweep battery.
|
||||
10. **Pre-existing fslab OOM-unwind bug has a strictly longer-lived exposure window under
|
||||
E5** — see "Validator round 1 fixes" item 4. Not fixed on this branch (out of the
|
||||
experiment's scope per the fix round), but the upstream PR built from E5 **must** carry
|
||||
the one-line unregister-before-free fix (reference implementation on E4's branch,
|
||||
commit `6753225`). The bug is only reachable through the fslab-OOM path (allocation
|
||||
failure mid-load), so it cannot affect the orchestrator's controlled A/B runs at sane
|
||||
RAM headroom — flagged for the PR, not for the battery.
|
||||
10. **Pre-existing fslab OOM-unwind bug — carried on this branch** (follow-up commit; see
|
||||
"Validator round 1 fixes" item 4 for the full mechanism). The one-line
|
||||
unregister-before-free fix from E4's `6753225` is ported to dev's non-heap code shape,
|
||||
so the upstream PR built from E5 inherits it automatically. Residual notes: (a) the fix
|
||||
is only reachable through the fslab-OOM path (allocation failure mid-load), so it is
|
||||
untestable without an OOM-injection harness and cannot affect the orchestrator's
|
||||
controlled A/B runs at sane RAM headroom — carried as correctness insurance, verified by
|
||||
inspection + clean builds only; (b) the `__linux__`-gated `uring_load_add` analog is
|
||||
deliberately not carried (dead code on every real build target — rationale in the fixes
|
||||
section).
|
||||
|
||||
@@ -1134,6 +1134,11 @@ static int expert_load_impl(Model *m, int layer, int eid, ESlot *s, int fatal){
|
||||
if(ftot<0 || (uint64_t)ftot > SIZE_MAX/sizeof(float) ||
|
||||
posix_memalign((void**)&s->fslab,16384,fb)){
|
||||
fprintf(stderr,"OOM fslab\n"); if(fatal) exit(1);
|
||||
/* unregister BEFORE freeing -- a stale g_slabs entry would let resolve() hand
|
||||
* the GPU a pointer into freed memory (and under COLI_METAL_RESSET=1, leave the
|
||||
* buffer a permanent residency-set member over it). Ported from e4/metal-heap
|
||||
* validator fix 6753225; pre-existing gap on main/dev. */
|
||||
if(s->slab && g_metal_enabled) coli_metal_unregister(s->slab);
|
||||
compat_aligned_free(s->slab); s->slab=NULL; s->slab_cap=0; /* clean, hidden slot (eid stays -1) */
|
||||
s->fslab=NULL; s->fslab_cap=0; return -1;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user