E5: hazard-audit round-2 fixes (re-register set hygiene, SUMMARY mutex ref)
1. DEFENSIVE (backend_metal.mm, coli_metal_register): re-registering a live base overwrote s.buf and resset_add'ed the new wrapper without removing the OLD one from the residency set -- ARC drops our reference but the set retains the object and keeps its pages resident: a leak and a set/g_slabs divergence. The found branch now stashes the replaced wrapper under g_slab_mtx and, after dropping the lock (round-1 invariant: no Metal call under the slab lock), resset_remove(old)s it before resset_add(b); identical old==b early-outs both set operations. Invariant defended, stated in the comment: residency-set membership mirrors g_slabs exactly. No in-tree caller re-registers a live base today (audited) -- closed defensively. 2. DOC (SUMMARY.md): the moe_submit lifecycle bullet still said resset_flush commits "under g_slab_mtx" -- stale text from before the round-1 mutex split; the code takes g_resset_mtx and never touches the slab lock there. Parenthetical corrected; register bullet updated to describe the re-register hygiene from item 1.
This commit is contained in:
+6
-2
@@ -50,7 +50,10 @@ the `COLI_METAL_RESSET` `getenv` branch in `coli_metal_init`, so `resset_add`/`r
|
|||||||
dirty flag), calls `[rs addAllocation:b]` and sets `g_resset_dirty` — **it does not
|
dirty flag), calls `[rs addAllocation:b]` and sets `g_resset_dirty` — **it does not
|
||||||
commit**. No Metal call ever runs under `g_slab_mtx` (validator round-1 fix; E4's audit
|
commit**. No Metal call ever runs under `g_slab_mtx` (validator round-1 fix; E4's audit
|
||||||
round 2 identified mutex-over-live-Metal-call as the leading suspect for its +12s
|
round 2 identified mutex-over-live-Metal-call as the leading suspect for its +12s
|
||||||
expert-disk regression).
|
expert-disk regression). Re-registering a live base (no in-tree caller does today) drops
|
||||||
|
the replaced wrapper from the set via `resset_remove(old)` before adding the new one
|
||||||
|
(hazard-audit defensive fix — the set would otherwise retain the old buffer, and its
|
||||||
|
pages' residency, forever), keeping set membership an exact mirror of `g_slabs`.
|
||||||
- **`coli_metal_unregister`**: erases the `g_slabs` entry under `g_slab_mtx` (stashing the
|
- **`coli_metal_unregister`**: erases the `g_slabs` entry under `g_slab_mtx` (stashing the
|
||||||
buffer), then calls `resset_remove(b)` **outside `g_slab_mtx`**, before returning.
|
buffer), then calls `resset_remove(b)` **outside `g_slab_mtx`**, before returning.
|
||||||
`resset_remove` (under `g_resset_mtx`) calls `[rs removeAllocation:b]` **and commits
|
`resset_remove` (under `g_resset_mtx`) calls `[rs removeAllocation:b]` **and commits
|
||||||
@@ -58,7 +61,8 @@ the `COLI_METAL_RESSET` `getenv` branch in `coli_metal_init`, so `resset_add`/`r
|
|||||||
function returns. See UNCERTAINTIES for why this asymmetry is deliberate.
|
function returns. See UNCERTAINTIES for why this asymmetry is deliberate.
|
||||||
- **`moe_submit`** (the one function whose `use` list — resolved expert weight/scale slabs —
|
- **`moe_submit`** (the one function whose `use` list — resolved expert weight/scale slabs —
|
||||||
scales with LRU cache size): calls `resset_flush()` at the top (commits any pending adds
|
scales with LRU cache size): calls `resset_flush()` at the top (commits any pending adds
|
||||||
from `resset_add`, under `g_slab_mtx`), then, if `g_resset_enabled`, **skips** the
|
from `resset_add`, under `g_resset_mtx` — it never touches the slab lock), then, if
|
||||||
|
`g_resset_enabled`, **skips** the
|
||||||
`for(auto&b:use) [e useResource:b usage:MTLResourceUsageRead];` loop entirely — residency
|
`for(auto&b:use) [e useResource:b usage:MTLResourceUsageRead];` loop entirely — residency
|
||||||
is already guaranteed by the queue-attached set. Every other `useResource:` call site in the
|
is already guaranteed by the queue-attached set. Every other `useResource:` call site in the
|
||||||
file (`bind_gemv`'s weight/scale buffers, `coli_metal_attn_decode`/`coli_metal_layer_decode`'s
|
file (`bind_gemv`'s weight/scale buffers, `coli_metal_attn_decode`/`coli_metal_layer_decode`'s
|
||||||
|
|||||||
+9
-2
@@ -399,13 +399,20 @@ extern "C" void coli_metal_register(void *base, size_t len) {
|
|||||||
id<MTLBuffer> b = [g_dev newBufferWithBytesNoCopy:base length:len
|
id<MTLBuffer> b = [g_dev newBufferWithBytesNoCopy:base length:len
|
||||||
options:g_res_opts deallocator:nil];
|
options:g_res_opts deallocator:nil];
|
||||||
if (!b) return;
|
if (!b) return;
|
||||||
|
id<MTLBuffer> old = nil; // E5: replaced wrapper on re-register of a live base (defensive)
|
||||||
{
|
{
|
||||||
std::lock_guard<std::mutex> lk(g_slab_mtx); // called from parallel expert_load threads
|
std::lock_guard<std::mutex> lk(g_slab_mtx); // called from parallel expert_load threads
|
||||||
bool found = false;
|
bool found = false;
|
||||||
for (auto &s : g_slabs) if (s.base == base) { s.len = len; s.buf = b; found = true; break; }
|
for (auto &s : g_slabs) if (s.base == base) { old = s.buf; s.len = len; s.buf = b; found = true; break; }
|
||||||
if (!found) g_slabs.push_back({base, len, b});
|
if (!found) g_slabs.push_back({base, len, b});
|
||||||
}
|
}
|
||||||
resset_add(b); // E5: outside g_slab_mtx (no Metal call under the slab lock); still before return
|
// E5, outside g_slab_mtx (no Metal call under the slab lock), before returning. Invariant
|
||||||
|
// defended: set membership mirrors g_slabs exactly -- a re-register of a live base must
|
||||||
|
// drop the replaced wrapper from the set (ARC releases our reference, but the set retains
|
||||||
|
// it and keeps its pages resident forever) before adding the new one. No in-tree caller
|
||||||
|
// re-registers a live base today; defensive.
|
||||||
|
if (old && old != b) resset_remove(old);
|
||||||
|
if (old != b) resset_add(b);
|
||||||
}
|
}
|
||||||
extern "C" void coli_metal_unregister(void *base) {
|
extern "C" void coli_metal_unregister(void *base) {
|
||||||
id<MTLBuffer> b = nil;
|
id<MTLBuffer> b = nil;
|
||||||
|
|||||||
Reference in New Issue
Block a user