diff --git a/.agents/porting-inventory.md b/.agents/porting-inventory.md index 37abbaa7d..56d2fe208 100644 --- a/.agents/porting-inventory.md +++ b/.agents/porting-inventory.md @@ -1482,12 +1482,17 @@ Examples: `examples/cli` ✅ (C-API client), `examples/server` ✅ (OpenAI serve CPU-backend device forward AFTER a bf16 CUDA one; at f32 the two arms land in different size classes and never trade blocks. The pool already carries exactly this invariant for STREAMS — `AuxPool()` exists because "two streams - sharing one pool BREAKS" its reuse ordering — and the fix used here is that - same sanctioned seam: the CPU arm runs under an `ActivePoolScope` with its - own pool. **The DEVICE half of the invariant is still unstated at the pool - itself, and a size-keyed device-blind free list in a multi-device process is - a trap for the next caller. Repairing it is a shared-hot-path change and is - owed as its own row, not this one.** + sharing one pool BREAKS" its reuse ordering — and the first fix used that + same sanctioned seam: the CPU arm ran under an `ActivePoolScope` with its + own pool. **That workaround is GONE, and so is the fault it worked around.** + `POOL-DEVICE-KEY` ([#516](https://github.com/mudler/vllm.cpp/issues/516), + [`specs/pool-device-key.md`](specs/pool-device-key.md)) states the DEVICE + half at the pool itself: a `DevicePool` is bound to one backend, `Pool(b)` + is the only spelling and there is no device-less one, every operation + throws on a foreign backend, and the per-caller scope in + `test_ltx2_device.cpp` was DELETED in the same change — because a list of + remembered callers is what this fault was, and leaving one behind would + have disarmed the only test that exposes the silent-NaN direction. * **OWED, and precisely:** (a) the prompt-K/V cache on the device path, which is REFUSED by name rather than ignored; (b) an FP4-RESIDENT arm — the `LinearDev` seam is one parameter away from the shared Marlin W4A16 diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index e7f826828..f527e40f7 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -117,6 +117,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#501](https://github.com/mudler/vllm.cpp/issues/501) | `PERF-27B-LMHEAD-FP4` | `AlphaVecBf16TakesTwoLaunch` bounded a COUNT of ulp mismatches instead of their MAGNITUDE, and was RED on its first CUDA run at ~26% — the double-rounding population the bf16-D lever produces by construction. Replaced by a max-ulp bound (`<= 1`, and `<= 0` at a pow2 alpha), measured 0/1-ulp only over 2.17M words on GB10, spec [`perf-fp8-alpha-fold.md`](specs/perf-fp8-alpha-fold.md) §The bf16-vs-f32 divergence is DOUBLE ROUNDING | bug | | [#521](https://github.com/mudler/vllm.cpp/issues/521) | `PERF-27B-LMHEAD-FP4` | [`perf-fp8-alpha-fold.md`](specs/perf-fp8-alpha-fold.md) `:19`/`:211` claim the bf16-D lever "also applies to 35B-A3B" — it is INERT there: `GdnOutDType(dense_model=false)` is F32 on a MoE, contradicting the code's own comment at `qwen3_5.cpp:3617-3619` | bug | | [#391](https://github.com/mudler/vllm.cpp/issues/391) | `PERF-CPU-DECODE-BARRIER` | CPU backend: batch-1 decode is barrier-bound (47% sync), and paged attention branches per element | perf | +| [#516](https://github.com/mudler/vllm.cpp/issues/516) | `POOL-DEVICE-KEY` | `vllm::Pool()`'s free list is keyed by size class with NO DEVICE in the key, so a `cudaMalloc` block reaches a CPU `DBuf` (SIGSEGV) and a host block reaches a CUDA forward (uniform `0x7fff0000` NaN); spec [`pool-device-key.md`](specs/pool-device-key.md), lands through `row/MODEL-DIFFUSION-LTX25` | bug | | [#299](https://github.com/mudler/vllm.cpp/issues/299) | `ROAD-V1-C1` | `FUSION-DENSE-MIGRATE`: 5 dense SwiGLU models bypass the MUST-route merged-GEMM seam with no stated blocker (spec [`fusion-dense-migrate.md`](specs/fusion-dense-migrate.md)) | bug | | [#314](https://github.com/mudler/vllm.cpp/issues/314) | `ROAD-V1-C1` | `FUSION-DENSE-MIGRATE` glue half: `glm4`/`phi3` still hand-call add+RMSNorm instead of `vt::FusedChain` (split out of #299, which closed the merged-GEMM half only) | bug | | [#337](https://github.com/mudler/vllm.cpp/issues/337) | `ROAD-V1-C1` | `FUSION-DENSE-MIGRATE`: the five dgx SACRED paged-engine gates are OWED after the merged-GEMM fold (`test_{commandr,glm4,minicpm,minicpm3,phi3}_paged_engine` SKIP on a CPU box) | bug | diff --git a/.agents/specs/pool-device-key.md b/.agents/specs/pool-device-key.md new file mode 100644 index 000000000..5df3f324f --- /dev/null +++ b/.agents/specs/pool-device-key.md @@ -0,0 +1,704 @@ +# `POOL-DEVICE-KEY` — put the DEVICE in the `vllm::Pool()` free-list key + +**Issue:** [#516](https://github.com/mudler/vllm.cpp/issues/516) (open). +**Row:** `POOL-DEVICE-KEY`. **Base:** `row/MODEL-DIFFUSION-LTX25` @ `310fa1688`, +REBASED there from the stale `aac24761`, which was not an ancestor of +`origin/main` (§11 F1). The dgx pair in §11 was measured one campaign move +earlier, at `aa6aa0ecd`; §11 says why that is still the right pair. +**Owning file:** this spec. **Status at write time:** spec committed before any +implementation, per AGENTS.md "Spec before code". + +## Now + +`DONE` on `row/POOL-DEVICE-KEY`, landing THROUGH the campaign branch +`row/MODEL-DIFFUSION-LTX25` rather than onto `main` directly. That is a +sequencing fact, not a preference: `tests/vllm/models/test_ltx2_device.cpp` does +not exist on `main` at all, and it is the ONLY test that exposes the silent-NaN +direction, so removing its per-caller workaround — which §4 D6 requires, because +a list of remembered callers is what this fault was — needs LTX-2.5 to land +first. + +Records this row owns: this spec, the `#516` line in the issue table of +[`../roadmap_v1.md`](../roadmap_v1.md), the `POOL-DEVICE-KEY` paragraph under +"Backend detail" in [`docs/STATUS.md`](../../docs/STATUS.md), and the +`porting-inventory.md` §L8 note, which said the shared `DevicePool` "is +DEVICE-BLIND … repairing it is owed as its own row" and would have contradicted +the tree the moment this merged. `docs/BENCHMARKS.md` is deliberately NOT +written: this row claims no measurement on any speed, latency or memory axis +(§6), and a row with nothing to record there records nothing there. + +§10's nine UNATTRIBUTED dgx failures are no longer open against this row: the +BEFORE/AFTER pair in §11 reproduces all nine on the base WITHOUT this change, so +they are measured pre-existing and belong to whoever owns those suites. The one +test that moves is `test_minimax_h3`, SEGFAULT → Passed, which is +[#486](https://github.com/mudler/vllm.cpp/issues/486). + +Still not established, and not claimed: why a host `aligned_alloc` block yields +a uniform quiet NaN on GB10 rather than running correct-but-slow through ATS +(§7). `MoeAuxStreamFor`'s index-only aux-stream key is a recorded latent trap of +the same family, deliberately not widened into here. + +## 1. Scope + +`include/vllm/model_executor/models/device_pool.h` — the shared, process-wide +caching device allocator every dense/MoE/diffusion forward draws its scratch +from — keys its free list by **byte size class only**. The device is not in the +key. A block allocated through backend A is handed to a `DBuf` running on +backend B whenever the two ask for the same size class in the same process. + +In scope: + +- `device_pool.h`: the key, the pool lifetime, the `Pool()`/`AuxPool()`/ + `ActivePool()` accessors, `Drain`, and the backend-less `Put` overload. +- `dense_device_glue.h` and `qwen3_5.cpp` `DBuf`: how a released pool block is + handed to a `shared_ptr` (the ~28 copy-pasted deleters that name neither the + pool nor the device). +- The two existing **per-caller workarounds** for this same fault, which the fix + makes unnecessary and which must be REMOVED so the detector stays armed: + `tests/vllm/models/test_ltx2_device.cpp` (`cpu_pool` scope, the SILENT-NaN + direction) and `tests/vllm/models/test_deepseek_v2_forward.cpp` + (`cuda_pool`/`cpu_pool`, the SIGSEGV direction). +- `DevicePoolPolicy` memoization in `dense_device_glue.h`/`qwen3_5.cpp`, which + caches the FIRST device's residency policy in a function-local static and + applies it to every later device — the same ambient-device assumption, one + layer up. + +Out of scope, explicitly: + +- The size-class rounding itself. `VT_POOL_EXACT=1` (exact keying, reuse kept) + was MEASURED still red, so over-allocation is not the fault and the class + arithmetic is preserved byte-for-byte. +- `VT_POOL_BYPASS`, which stays a debugging lane and keeps its semantics. +- Any per-model change. If the fix makes a shipped model's gate red, STOP (§8). +- `ltx2_loader.*`, `ltx2_text_encoder.*`, the render path / engine wiring / + server flag, and `ltx2_device.cpp` — concurrently owned by three other rows. + This row needs no edit in any of them: `ltx2_device.cpp` reaches the pool only + through `DBuf`. + +## 2. The defect, as measured + +One fault, two symptoms, selected by direction: + +| direction | consumer | symptom | +|---|---|---| +| `cudaMalloc` block → CPU-backend forward | any CPU-backend case | **SIGSEGV** in `__memcpy_sve ← UploadStream ← PrepareStreamDev`, `compute-sanitizer` CLEAN (the fault is host-side) | +| CPU `aligned_alloc` block → CUDA forward | the shipped 21B DiT | **silent all-NaN output** | + +Census of the silent direction: `video n=1024 nan=1024 inf=0 zero=0`, +`audio n=512 nan=512`, every element the identical `0x7fff0000` — the bf16 +canonical quiet NaN widened by `WidenBf16`. A UNIFORM quiet NaN means a NaN was +COMPUTED and propagated; it is not plausible garbage read from a wrong pointer. + +Three arms already discriminate the cause: + +- `VT_POOL_BYPASS=1` (free list removed) → `test_ltx2_device` SUCCESS 13/13, + 6176 assertions. +- a per-case `DevicePool` via `ActivePoolScope` → SUCCESS 13/13, 6176. +- `VT_POOL_EXACT=1` (reuse kept, rounding removed) → still FAILURE. + +So it is **cross-device reuse**, not over-allocation, and not the pool's +existence. + +Why it stayed latent: it needs a bf16 host-backend device forward to run AFTER a +bf16 CUDA one in the same process. At f32 the two arms land in different size +classes and never trade blocks. The bug is old; only the ordering is new. + +**Not established, and not required for the fix:** why a host `aligned_alloc` +block yields NaN on GB10 rather than merely running slowly through ATS. Unified +memory makes host pointers device-addressable, so the naive expectation is +correct-but-slow — yet the red forward is FASTER (0.230–0.252 s) than the green +solo one (0.594 s). Untested candidates: `aligned_alloc`'s 64-byte alignment vs +`cudaMalloc`'s 256 breaking a vectorised/TMA load, or kernels that require true +device memory. Recorded in §7 as an open question; the fix makes the ordering +unreachable either way. + +## 3. Upstream anchors + +vLLM does not have this bug because it never had this design: every allocation +record carries its own device, and every cache operation is device-scoped. Read +at the pinned oracle `555967922` (`$VLLM_SOURCE`) and at the local +`torch 2.11.0+cu130` headers: + +- `vllm/device_allocator/__init__.py:12-14` — `HandleType` is documented + `# py_device, py_size_or_aligned_size, py_ptr, py_handle`. The **device is + field 0 of the handle**; the size is field 1. Ours keyed on field 1 alone. +- `vllm/device_allocator/cumem.py:200-219` — the free callback recovers the + allocation from the pointer and reads the device back OUT of the handle + (`device, size, d_mem, _ = data.handle`, `torch.cuda.synchronize( + data.handle[0])`). A released block is re-associated with the device it came + from, never with whoever asks next. +- `c10/cuda/CUDACachingAllocator.h:118-172` — the whole `CUDAAllocator` + interface is parameterized by `c10::DeviceIndex device` + (`getMemoryFraction`, `cacheInfo`, `getDeviceStats`, `releasePool`, …); the + cache is per-device by construction. + +Our `AuxPool()` comment already cites the STREAM half of the same invariant — +"two streams sharing one pool BREAKS" its reuse ordering, and torch answers that +with `record_stream`. The DEVICE half was never stated. + +This row therefore mirrors upstream's **partitioning** (one cache per device), +not its stream tracking, which stays out of scope: our reuse ordering is +single-queue per pool and `AuxPool` remains the seam for the second stream. + +`vllm/platforms/interface.py` `Platform` (our `platforms/interface.h` +`residency_policy()`) is likewise per-platform, which is why memoizing ONE +policy for the whole process (§1) is the same class of mistake. + +## 4. Design + +The device becomes structural, not a field someone must remember to pass. + +**D1 — one `DevicePool` per device.** `DevicePool` gains a bound backend +(`explicit DevicePool(vt::Backend&)`). `Pool(vt::Backend& b)` and +`AuxPool(vt::Backend& b)` resolve a per-backend instance from a process-wide +table; the no-argument `Pool()` and `AuxPool()` are **removed**, so an +unqualified "the pool" can no longer be spelled. `vt::Backend*` is the device +identity: the registry hands out exactly one `Backend*` per `Device{type,index}` +(`vt::RegisterBackend(Device, Backend*)`, `kMaxDevicesPerType`), and +`GetBackend(type)` and `GetBackend(Device{type,0})` return the identical +pointer. Keying on the backend needs NO new virtual on `vt::Backend` and so +ripples into no backend implementation. + +Lookup is a `std::mutex` + small vector, fronted by a thread-local +last-(backend,pool) memo, so the steady-state hot path is one pointer compare — +the pool exists to remove `cudaMalloc`, and must not pay a hash for it. + +**D2 — the pool VERIFIES its device on every use.** `Get`, both `Put`s and +`Drain` keep their existing `vt::Backend&` parameter and now throw +`std::logic_error` when it is not the pool's own backend. This is a hard runtime +check, not `assert`: the SACRED builds are Release/NDEBUG, where an `assert` +would compile out and hand back the pre-fix behavior. It is the standing +detector for this defect class, and it is what makes an `ActivePoolScope` +pointed at another device's pool a loud refusal instead of a silent corruption. + +**D3 — `ActivePool` resolves per device.** The thread-local becomes an +*override* defaulting to null; `ActivePool(vt::Backend& b)` returns the override +when set and `Pool(b)` otherwise. `ActivePoolScope` is unchanged in shape and +keeps serving the aux-stream case (`AuxPool(b)`), which is a stream distinction, +not a device one. + +**D4 — `DBuf::ReleaseShared()` replaces ~28 hand-rolled deleters.** Every site +today is literally `alloc_bytes()`, then `Release()`, then a `std::shared_ptr` +whose deleter closes over the byte count alone and calls `Pool().Put(alloc, q)`. +That idiom names neither the device nor the pool, so +it would also return AUX-pool blocks to the MAIN pool — a second bug in the same +three lines, but a LATENT one: enumerated at the base commit, none of the nine +`Release()` sites (`gemma4_moe.cpp:1197,1541`, `qwen3_5.cpp:6324,6520,6820, +7100,7134,8034,8065`) is inside or transitively under any of the four +`ActivePoolScope` regions (`laguna.cpp:2574`, `qwen3_5.cpp:5468,8644,8964`), +which are leaf-ward of all of them, so the old deleter and the DBuf's own +`pool_` agreed on every path that actually ran. The one place it WAS live at +base is `test_deepseek_v2_forward`'s own workaround, which this change deletes. +`ReleaseShared()` captures the buffer's OWN pool and +backend, so both are right by construction, and the backend-less +`Put(size_t, void*)` overload is removed with its last caller. Uncapped +retention is preserved for these cross-step buffers via a new +`Put(vt::Backend&, size_t, void*)`. + +**D5 — the residency policy is memoized per device type**, not once per process. + +**D6 — the two per-caller workarounds are removed.** They are the list of places +someone remembered; the fix is the property. Removing them is what proves it. + +Not chosen, and why: + +- *A composite `(Backend*, class)` key inside ONE pool.* Needs a + `void*`→`Backend*` side map to serve the backend-less `Put`, i.e. a second + hash operation on the hottest allocation path in the tree, to keep an overload + that should not exist. +- *A `virtual Device device() const` on `vt::Backend`.* Reaches every backend + implementation for information the caller already holds. Explicit stop + condition for this row. +- *Per-caller `ActivePoolScope`.* Already rejected in #516 and re-rejected here: + it is a list of remembered places, and the current red is exactly the siblings + nobody scoped. + +## 5. Tests + +**T1 (RED-first, the row's own gate) — `tests/vllm/models/test_device_pool.cpp`, +new target `test_device_pool`.** Hardware-free: two distinguishable fake +`vt::Backend`s, the technique `test_backend_multidevice` / +`test_reference_tier` already use, on `Device{kCPU,0}` and `Device{kCPU,1}` — +two INDICES of one registered type, not two types. `kXPU` was the first idea +and is wrong for the pool cases: it has no registered platform, so +`ResolveDevicePoolPolicy` throws before the pool is ever reached and every case +would measure the platform registry instead. `kXPU` earns exactly one case, the +one that is ABOUT that throw (T1.7). Cases: + +1. **The defect.** Allocate on A, free, then allocate the same size class on B. + The block B receives MUST NOT be the block A freed, and must come from B's + own `Alloc`. RED before the fix. +2. **Reuse survives.** Get/Put/Get on ONE backend returns the identical pointer. + Without this, "fixed" is indistinguishable from `VT_POOL_BYPASS` — the pool's + whole reason to exist is reuse. +3. **Size-class rounding survives.** Two byte sizes in one class still trade one + block, and `VT_POOL_EXACT` still separates them. +4. **`Drain(b)` is device-scoped.** Draining A frees A's blocks only; B's free + list is untouched and no block is freed through the wrong backend. +5. **A cross-device `ActivePoolScope` is REFUSED** (throws, names both devices) + rather than served. +6. **`ReleaseShared()` returns the block to its own pool and backend**, and an + AUX-scoped buffer returns to the AUX pool, not the main one. +7. **A backend whose PLATFORM is unregistered is refused.** D5's per-device-type + memoization calls `platforms::GetPlatform` once per device TYPE instead of + once per process, so an unregistered type now throws where it used to inherit + the first device's cap. That is the correct answer and it is a NEW failure + mode, so it gets a case rather than a comment. + +**The two debug lanes stay GREEN.** §10 hands `VT_POOL_BYPASS=1` to the next +reader as the cheap discriminator, so this suite must not red under it. Cases +2, 4, 6 and the AUX half of 6 state the ACTIVE lane's behavior (a pooled hit by +default, a fresh driver block under bypass); case 3 states sharing by default +and SEPARATION under `VT_POOL_EXACT`, which is the assertion its second clause +above always promised and never had. Both vars are read once into a +function-local static in `DevicePool`, so a process is in one lane for its whole +life and no case can toggle them. + +**T2 (end-to-end corroboration) — `test_ltx2_device --order-by=rand +--rand-seed=7`.** No checkpoint, no NAS, ~2 s. RED before the fix (exit 139, +`:533 FATAL ERROR: test case CRASHED: SIGSEGV`), and note the trap it carries: +**44 assertions, 0 failed, beside a SIGSEGV** — grep `Status:` and the CASE +count, never `assertions:` alone. Deterministic: default and `--order-by=name` +and seeds 1 and 7 are all red; the only green ordering is green by declaration +order, not by safety. + +**T3 (blast radius).** 20+ test binaries mix a CUDA and a CPU backend in one +process. Enumerate them from the tree (not from memory) and run the full suite +before and after. + +**T4 (#486 hypothesis).** `test_minimax_h3` SIGSEGVs on GB10 when two CUDA cases +share a process, `compute-sanitizer` clean. Run it under `VT_POOL_BYPASS=1` and +again after the fix. Green either way is a result and gets recorded; it does NOT +gate this row and the connection is not forced if it does not hold. + +Mutation targets a reviewer should exercise: delete the device from the key +(T1.1 must fail); delete the D2 device check (T1.5 must fail); make +`ReleaseShared` use the main pool (T1.6 must fail); make `Drain` free every +bucket (T1.4 must fail). + +## 6. Gates + +Correctness only; this row claims no performance result. + +- `test_device_pool` — new, must be RED before and GREEN after, with both + outputs captured. +- `test_ltx2_device` — GREEN at `--rand-seed=7`, at `--order-by=name`, and at + default order, **with no per-case pool scoping anywhere in the file**. +- Baselines that must not move (case/assertion counts): + `test_ltx2` 29/1615 · `test_ltx2_vae` 16/1816 · `test_ltx2_text_encoder` + 17/3350 · `test_ltx2_pipeline` 35/2358 · `test_ltx2_loader` 20/2363 · + `test_ltx2_video` 17/170 · `test_ops_attention_cross` 9/32 · + `test_minimax_h3` 79/57395 (CPU) · `test_minimax_h3_video_fold` 6/137 · + `test_video_engine` 11/254 · `test_capi` FULL 55/505. +- Full `ctest` on the CPU host before and after, and on dgx (`-j 1`, GB10 + unified memory OOM-reboots the box under a parallel CUDA suite). +- Every doctest result reported as its `Status:` line AND its case count; the + assertion count DROPS when cases throw and reads clean beside a crash. + +Build discipline: never redirect build output to `/dev/null`; chain on the build +exit; clean-rebuild after a header change, because an incremental build masks +`-Werror` and `device_pool.h` is a header. + +## 7. Risks + +| # | risk | mitigation | +|---|---|---| +| R1 | `Backend*` is not device identity if one backend object is registered for two `Device` indices | The registry stores one pointer per `Device{type,index}`; asserted by `test_backend_multidevice`. Recorded as the assumption it is. | +| R2 | The per-backend lookup lands on the hottest allocation path | Thread-local last-(backend,pool) memo: one pointer compare in steady state. No hash, no lock, on the hit path. | +| R3 | D2's runtime check costs a branch per `Get` | One perfectly-predicted compare against a member; the alternative (`assert`) is compiled out of exactly the Release builds the gates run. | +| R4 | ~28 deleter sites across 25 model files is a wide diff | Every site is byte-identical today, and each becomes ONE line via `ReleaseShared()` — the diff SHRINKS the call sites and removes an idiom that can be got wrong. No `ltx2_*` file is touched. | +| R5 | Removing the two per-caller workarounds could red a suite for an unrelated reason | They are removed in the SAME change that makes them unnecessary; if either stays red, that is a finding to report, not a scope to re-apply (§8). | +| R6 | A pool is now created per backend, so a mixed-backend process holds two free lists | That is the point. Retention is bounded by each device's own peak scratch, and `Drain` is now device-correct where before it freed one device's blocks through another's backend. | +| R7 | Static-destruction order for a table of pools that print `VT_POOL_STATS` at exit | Pools are owned by a function-local static table and outlive every model object; the stats path is unchanged. | + +## 8. Stop conditions + +- Keying by device turns out to need an API change that ripples into every + backend → STOP, report the design, write nothing. +- The fix makes a SHIPPED model's gate red → STOP and report; do not adjust the + model. +- `NEEDS_CONTEXT` for missing binding context; `NEEDS_DECISION` for a material + disagreement — never a silent scope change. +- Never weaken a bound, delete an assertion, or scope a caller away to reach + green. In particular, the LTX-2.5 shipped case is the ONLY test exposing the + SILENT direction: it does not get an `ActivePoolScope`. +- GPU work waits on `$HOME/gpu.lock` on dgx with a BOUNDED `flock -w`; on + timeout, report and stop. Never kill a holder. + +## 9. Evidence to record + +The committed SHA of this spec (before implementation); the RED `test_device_pool` +and RED `test_ltx2_device --rand-seed=7`; the GREEN of both plus the LTX-2.5 +device suite with no per-case scoping; the enumerated mixed-backend binaries and +the full suite before/after; the #486 result either way; every baseline as a +`Status:` line and case count; the exact `flock` lines, the wait, and `docker ps` +at both ends; `git log --oneline` and the final SHA. + +## 10. Outcome + +Spec `17532ea0b` (this file, committed before any implementation), RED test +`f4be8a4e2`, fix `1a2eb35ec` — the four original commits, later REBASED onto +`row/MODEL-DIFFUSION-LTX25` @ `2d437d5a9` (see §11), so those SHAs name the +history rather than the branch. + +**Environment.** dgx.casa, GB10 sm_121a, CUDA 13.0.88, configured with all three +MANDATORY confirmations printed: `CUTLASS found at ~/cutlass-4.5.0; enabling +sm120a NVFP4 cutlass GEMM`, `FlashAttention-2 prefill/decode: ENABLED for +arch(es) [121a]`, `Triton AOT: vendored tree …/sm_121a matches triton_kernels/ +(MANIFEST hashes OK)`. Both arms are CLEAN builds (`rm -rf build`), RED +1508/1508 exit 0 and GREEN 1508/1508 exit 0; the CPU host likewise clean, +1219/1219 exit 0, zero warnings under `-Werror`. `local-ai-worker` was stopped +before any GPU work and left DOWN. `mnt-nas_share.mount` had lost its boot race +after a reboot and was restarted with `sudo -n systemctl restart`; the shipped +DiT was then proven READABLE (21,025,119,068 bytes, first 16 bytes dumped) +before the opt-in case was allowed to run. + +**RED, at `f4be8a4e2`.** Both directions, on the same binary: + +| run | result | +|---|---| +| `test_device_pool` (CPU host) | 4 cases / 2 passed / 2 failed · 15 assertions / 5 failed · FAILURE · exit 1 | +| `test_ltx2_device --order-by=rand --rand-seed=7` | **exit 139**, `:533 FATAL ERROR: test case CRASHED: SIGSEGV` · 4 cases / 3 passed / 1 failed / 9 skipped · **44 assertions / 0 failed** | +| `--rand-seed=1` | exit 139, SIGSEGV at `:518` · 7 / 6 / 1 / 6 · 481 assertions / 0 failed | +| `--order-by=name` | exit 139, SIGSEGV at `:452` · 7 / 6 / 1 / 6 · 455 assertions / 0 failed | +| default order, `LTX2_SHIPPED_DIT` set (21B FP8) | **exit 1**, `:925 FATAL ERROR: REQUIRE( std::isfinite(v) )` · 13 / 12 / 1 · 4639 assertions / 1 failed — the SILENT direction | +| default order, fixture UNSET | SUCCESS 13/552 — the skip that impersonates a repair (§7.0(d)) | +| `test_minimax_h3` | exit 139, SIGSEGV at `:3974` · 38 / 36 / 2 / 41 · 42,724 assertions | +| `test_minimax_h3` + `VT_POOL_BYPASS=1` | SUCCESS 79 / 79 · 451,993 assertions · exit 0 | + +**GREEN, at `1a2eb35ec`** (`b4618b8c7` before an amend that added the +doc-gate argument to the message; the tree is identical), with NO per-case pool scoping anywhere (both +workarounds deleted): + +| run | result | +|---|---| +| `test_device_pool` | 8 / 8 · 26 assertions · SUCCESS (also under `--order-by=rand` seeds 1 and 7) | +| `test_ltx2_device` seed 7 / seed 1 / by-name / default | 13 / 13 · 552 assertions · SUCCESS · exit 0, all four | +| `test_ltx2_device` + shipped 21B DiT, default AND seed 7 | **13 / 13 · 6176 assertions · SUCCESS · exit 0** | +| `test_minimax_h3` (pool ON) | **79 / 79 · 451,993 assertions · SUCCESS · exit 0** | +| `test_deepseek_v2_forward` | 11 / 11 · 1558 assertions · SUCCESS | + +**#486 is this bug.** Asked as a question, not assumed: at the RED commit +`test_minimax_h3` SIGSEGVs with the pool on and is 79/79 with `VT_POOL_BYPASS=1` +— the bypass lane changes nothing but the free list. At the fixed commit it is +79/79 with the pool ON. The hypothesis is confirmed by measurement in both +directions. + +**ENOSPC re-verification.** The local box hit 100% disk during this session +(operator note). Every build log here was grepped for `No space left` — zero hits +in the local RED build, the local clean AFTER build, the dgx RED build, the dgx +GREEN build and the dgx AFTER `ctest` — and the local gates were then re-run +CHAINED to their build in one command, with `ninja` reporting "no work to do" +first, so no result here comes from a stale binary left behind by a died build. + +**Baselines, CPU host, full `ctest`: 402/402 passed, exit 0.** Every named +baseline byte-for-byte where it was: `test_ltx2` 29/1615 · `test_ltx2_vae` +16/1816 · `test_ltx2_text_encoder` 17/3350 · `test_ltx2_pipeline` 35/2358 · +`test_ltx2_loader` 20/2363 · `test_ltx2_video` 17/170 · +`test_ops_attention_cross` 9/32 · `test_minimax_h3` 79/57395 · +`test_minimax_h3_video_fold` 6/137 · `test_video_engine` 11/254 · `test_capi` +55/505. + +**Full CUDA `ctest -j 1` on dgx, AFTER: 98% passed, 9 failed out of 437.** The +nine, with their exact signatures: + +| test | signature | +|---|---| +| `test_serve_low_tools` | the Python bench-tooling suite (no C++, no GPU) | +| `test_linear_method` | `:246 CHECK( after == before + 1 )` → `0 == 1` — the `fused_gate_up` counter did not move, i.e. the fused Marlin gate-up path FELL BACK; its numeric arm passed at `bitexact=12288/12288 max_abs=0`, which is exactly what a fallback to the split path looks like | +| `test_ops_gdn` | `:728 CHECK( bad == 0 )` → `2609 == 0`, a GDN kernel numeric check | +| `test_capi` | **SEGFAULT** at `:482` "capi: custom logits processor forces the generated token (ABI v8)" — 4 cases / 3 passed / 1 failed / 51 skipped, 47 assertions / 0 failed | +| `test_glm4_moe_lite_paged_engine`, `test_qwen3_apc_e2e`, `test_minicpm3_paged_engine`, `test_internlm2_paged_engine`, `test_llama_paged_engine` | checkpoint-gated paged-engine suites | + +**These nine are UNATTRIBUTED, and that is stated rather than glossed.** The dgx +full-suite BEFORE arm was NOT run: the box was at 99–100% disk with a single 31 GB +build tree, so the RED and GREEN trees could not coexist, and the GPU lock was +shared with three other agents (one bounded `flock -w 2700` wait expired without +acquiring). Without that arm, "pre-existing" would be an inference, and this row +does not report inferences as measurements. + +What IS measured and bears on them: the same binaries are GREEN on the CPU host +in a 402/402 full run — including `test_capi` at its recorded 55/505 and +`test_linear_method` — and every pool-adjacent gate on dgx is green +(`test_ltx2_device` 13/13·6176 in four orderings, `test_minimax_h3` 79/79, +`test_deepseek_v2_forward` 11/11, `test_device_pool` 8/8). None of the nine +signatures is a cross-device scratch symptom: two are a dispatch counter and a +kernel numeric check that this diff does not reach, five are checkpoint-gated, +one is Python. + +**`test_capi` IS A TIMING FLAKE, and that one is measured, not inferred.** It +reproduced on the CPU host, where this row's change is CPU-only and every pool +gate is green: a first full `ctest -j 1` had it at its recorded 55/505 SUCCESS, +a second full run on the SAME binary (ninja: "no work to do") failed it, and +standalone it is **55/55 · 505 assertions · SUCCESS**, then `--repeat +until-fail:8` passed **8 of 8** with per-run wall times of 0.78, 1.02, 1.65, +2.00, 4.21, 12.56, 228.94 and 339.88 s. A test whose duration spans three orders +of magnitude on a contended box is timing-sensitive, which is what +`.agents/environment.md` already records for `test_capi`. Non-deterministic on a +host where the pool change cannot produce it ⇒ not this row's. + +**The exact next step, for whoever picks up the remaining eight.** Re-run them +standalone on this GREEN tree and again on a pre-fix tree; the cheap +discriminator that needs no second build is `VT_POOL_BYPASS=1`, which removes the +free list entirely — a failure that survives bypass cannot be a pooling failure. +The rerun script is `~/work/pool-device-key/dgx_rerun.sh`; it was written and +shipped but never ran, because two consecutive bounded `flock -w 2700` waits +expired without acquiring the shared GPU lock (three other agents were rendering +on it). Reported and stopped, per the lock protocol, rather than camping. + +**Blast radius, MEASURED not guessed.** `VT_POOL_STATS=1` makes every pool print +one line naming its backend at exit, so the full suite counts pools per binary +rather than grepping for device names. On the CPU host **42 of 402 test binaries +instantiate a `DevicePool` at all**, and exactly one — `test_device_pool` itself, +with its 11 fakes — instantiates more than one, which is the expected answer for +a host with a single backend. The grep-level upper bound is 51 test sources that +name both `kCUDA` and `kCPU`. + +**What was rejected.** A composite `(Backend*, class)` key inside one pool: it +needs a `void*`→`Backend*` side map to serve the backend-less `Put`, i.e. a +second hash operation on the hottest allocation path, to keep an overload that +should not exist. A `virtual Device device() const` on `vt::Backend`: reaches +every backend implementation for information the caller already holds, and was +an explicit stop condition. Per-caller `ActivePoolScope`: a list of remembered +places, which is what the current red already disproved. + +**Why the defaults are what they are.** The device check is a runtime `throw` +rather than an `assert` because the gate builds are Release/NDEBUG, where an +assert compiles out and hands back exactly the pre-fix behaviour. The pool +lookup is a thread-local last-(backend,pool) memo rather than a hash because a +`DBuf` resolves its pool on every construction and the pool exists to avoid a +synchronizing `cudaMalloc`. Size-class rounding is untouched: `VT_POOL_EXACT=1` +was measured still red, so it was never the fault. + +**Not established, and left open.** Why a host `aligned_alloc` block yields a +uniform quiet NaN on GB10 rather than running correct-but-slow through ATS. The +fix makes the ordering unreachable, so the question is now academic for this row, +but it is not answered and is not claimed to be. + +**Related, NOT fixed here.** `MoeAuxStreamFor` (`qwen3_5.cpp`) caches its aux +stream on `d.q.device.index` alone, so a CPU device 0 and a CUDA device 0 collide +in the key. It is unreachable today because the only call site is gated on +`Backend::SupportsAuxStream()`, which no host backend answers true to — so it is +a latent trap of this same family rather than a live defect, and widening this +diff to it was not worth the review surface. Recorded here so the next reader +finds it. + +**Owed at the time of §10, and PAID in §11:** the `#516` line in the issue table +of `.agents/roadmap_v1.md`, and the `porting-inventory.md` §L8 note which read +"the shared `DevicePool` is DEVICE-BLIND … repairing it is owed as its own row". + +## 11. Review round: the landing state, and the dgx pair §10 could not get + +A fresh review PASSED the fix and FAILED the landing state. Six findings; all +six closed. The endorsed code — the device key, `ReleaseShared`, both workaround +removals, `test_device_pool` — is unchanged in behaviour. + +### F1 — the base was stale, and its resolution would have reverted main + +The row was based on `aac24761`, which is **not an ancestor of `origin/main`**, +and `git merge-tree origin/main 7336def93` CONFLICTED in `device_pool.h`. Main +had changed that same file in `49539559d` / `8fa2ecdbb` (Windows contracts, +[#117](https://github.com/mudler/vllm.cpp/issues/117)) three ways the reviewed +header lacked: `__builtin_clzll` → `std::bit_width`, an `std::overflow_error` +guard in `ClassOf`, and `static size_t SizeClassForTest(size_t)`. Taking the +row's side of that hunk — the heavily rewritten side, which is what a resolver +reaches for — would have silently reverted the portability fix and the overflow +guard, and `tests/vt/test_cpu_isa_x86.cpp` calls `SizeClassForTest` **nine** +times including `CHECK_THROWS_AS(…, std::overflow_error)`, so the tree would not +have compiled. + +Rebased onto `row/MODEL-DIFFUSION-LTX25`, which contains `origin/main`, and +`device_pool.h` resolved BY HAND: all three of main's changes +sit **on top of** the device-keyed rewrite. `git diff ..HEAD -- +device_pool.h` is exactly three code hunks (the two includes, `SizeClassForTest`, +and the `ClassOf` body) plus the F4 comment. `test_cpu_isa_x86` is **6 cases / +8242 assertions / SUCCESS**. + +**Every number in §10 was measured on a tree missing main's last 201 commits and +none of it is carried forward.** The re-run caught drift §10 could not have seen: +`test_ltx2_vae` is **36 cases / 3039 assertions**, not the 16/1816 §6 records, +and `test_ltx2_loader` is **24 / 4817**, not 20/2363. Those are campaign-branch +commits (`6c9374ebc`, the VAE encoders, and the L9A NVFP4 loader work); this row +touches neither file. §6's baseline list is therefore stale by construction on a +moving campaign branch — what this row can honestly assert is that it moves +nothing, which the full 415/415 below and the dgx pair above both say. + +The rebase also caught a **new instance of this row's own defect**, added to the +campaign branch after the row was written: `ltx2_video.cpp:1154` called +`ActivePool()->Drain(backend)` — "the pool" resolved with no device, then drained +through a backend that may not have allocated its blocks. It is now +`ActivePool(backend).Drain(backend)`, and the old form is no longer spellable. +That is the argument for D1 restated by events: the removal of the no-argument +accessor is the fix; the free-list key alone would not have caught this. + +### F3 — the dgx BEFORE/AFTER pair, which §10 reported as not run + +Both arms on dgx.casa (GB10 sm_121a, CUDA 13.0.88), same session, back to back, +`ctest -j 1`, CLEAN builds (`rm -rf build`) from the SAME base, all three +MANDATORY confirmations printed for each (`CUTLASS found at ~/cutlass-4.5.0`, +`FlashAttention-2 … ENABLED for arch(es) [121a]`, six `MANIFEST hashes OK` +vendored Triton trees). `BUILD_EXIT=0` and zero `No space left`/`BFD assertion` +in both build logs and both ctest logs. + +| | BEFORE (`aa6aa0ecd`, no fix) | AFTER (this branch) | +|---|---|---| +| build | `BUILD_EXIT=0`, 34 GB tree | `BUILD_EXIT=0`, 34 GB tree | +| lock | acquired 08:26:13Z, released 09:22:00Z | waited 13m45s, acquired 09:40:20Z, released 10:34:02Z | +| disk at start / end | 62 GB free (99%) / 62 GB | 96 GB free (98%) / 60 GB | +| result | **10 failed of 449** | **9 failed of 450** | + +**The denominators differ by one because this row ADDS a test.** 449 → 450 is +`test_device_pool` existing in the AFTER arm and not in the BEFORE one; it is not +drift, and comparing 10-of-449 against 9-of-450 without that reads one test off. +The like-for-like statement is on the 449 tests both arms share: **10 failures +before, 9 after, and the one that leaves is `test_minimax_h3`.** + +| test | BEFORE | AFTER | +|---|---|---| +| `test_minimax_h3` | **`***Exception: SegFault` 11.73 s** (`ctest-before.log:51`) | **Passed 19.06 s** | +| `test_device_pool` | (does not exist) | **Passed 0.25 s** (new test #50) | + +Every other failure is the same set in both arms, and **every one of them is +already tracked by an existing issue** — checked, not assumed: + +| test | both arms | tracked by | +|---|---|---| +| `test_serve_low_tools` | Failed | [#233](https://github.com/mudler/vllm.cpp/issues/233) | +| `test_linear_method` | Failed | [#233](https://github.com/mudler/vllm.cpp/issues/233) (`VT_MARLIN_DENSE` defaults ON, so `fused_gate_up` never increments) | +| `test_glm4_moe_lite_paged_engine` | Failed | [#233](https://github.com/mudler/vllm.cpp/issues/233) (SACRED gate, token divergence) | +| `test_capi` | **SEGFAULT** | [#248](https://github.com/mudler/vllm.cpp/issues/248) | +| `test_ops_gdn` | Failed | [#305](https://github.com/mudler/vllm.cpp/issues/305), probably — see below | +| `test_qwen3_apc_e2e` | Failed | [#248](https://github.com/mudler/vllm.cpp/issues/248) | +| `test_minicpm3_paged_engine` | Failed | [#248](https://github.com/mudler/vllm.cpp/issues/248) | +| `test_internlm2_paged_engine` | Failed | [#614](https://github.com/mudler/vllm.cpp/issues/614), filed by this row | +| `test_llama_paged_engine` | Failed | [#248](https://github.com/mudler/vllm.cpp/issues/248) | + +**So all nine of §10's UNATTRIBUTED failures are now MEASURED pre-existing rather +than argued.** Nothing in this diff moves any of them, the row does not claim to +fix them, and they are named rather than adopted. This supersedes §10's "eight +remain open" as a statement about this row. + +**`test_capi` is NOT the timing flake §10 called it, and that correction +matters.** §10's flake evidence — `--repeat until-fail:8` passing 8/8 with wall +times spanning 0.78 s to 339.88 s — was gathered on the **CPU host**, where the +test fails without crashing. On dgx it **SEGFAULTs**, in **both** arms, in +**1.95 s** (BEFORE) and **1.97 s** (AFTER), under `ctest -j 1`. A sub-2-second +segfault is not a three-orders-of-magnitude timing spread, and #248 says so +directly: "`test_capi` is not the documented flake … this run was `ctest -j 1`, +so that explanation is unavailable and the SIGSEGV needs a real diagnosis." It is +pre-existing here either way, but it goes back to #248 as a crash, not into a +flake story it does not fit. Whether the two arms' crashes are the SAME crash is +**not established**: neither `LastTest.log` survives (both 34 GB build trees were +deleted to keep a 98%-full box under control) and these runs were plain +`ctest -j 1` without `--output-on-failure`, so the only recorded evidence is the +signal, the test index and the duration. + +**`test_internlm2_paged_engine` had no issue at all**, so this row filed +[#614](https://github.com/mudler/vllm.cpp/issues/614) rather than folding it +into a neighbour's: #248 lists four +paged-engine-family failures and this is not among them, and a search over open +and closed issues returns nothing. `test_ops_gdn` is attributed to #305 only +tentatively — #305 is a `conv_state` cross-block race and §10 recorded this +failure as `:728 CHECK( bad == 0 )` → `2609 == 0`, a numeric check that may or +may not be that race — so it is recorded as probable, not confirmed. + +It also **re-confirms #486 in both directions on one box**: SEGFAULT with the +pool device-blind, Passed with it device-keyed, same binary recipe, same lock, +14 minutes apart. §10 asserted this from two separate sessions; it is now one +paired measurement. + +The build trees were deleted after each arm (the box was at 98–99% throughout). + +**The pair was measured at base `aa6aa0ecd`; the branch is now rebased onto +`310fa1688`, fifteen commits later.** Chasing a moving campaign branch with a +five-hour paired CUDA gate does not terminate, so the honest thing is to name the +base the pair was taken at and say what would invalidate it. What the pair +measures is THIS ROW'S delta against a common base, and the fifteen intervening +commits are LTX-2.5 loader/VAE/NVFP4 work plus their goldens and specs: none +touches `device_pool.h`, `dense_device_glue.h`, the `DBuf` deleters, or any pool +accessor, and a tree-wide sweep for a device-less `Pool()` / `AuxPool()` / +`ActivePool()` spelling comes back empty on the new base. The CPU gate below WAS +re-run in full on `310fa1688`. A campaign commit that touched the pool would +invalidate the pair and is the one thing to re-check before merging. + +### F4 — a genuine hazard, but LATENT, and the assertions said "live" + +`device_pool.h`, `dense_device_glue.h` and §4 D4 claimed the old deleter idiom +also returned AUX-pool blocks to the MAIN pool as a *live* second bug. Enumerated +at the base: none of the nine `Release()` sites (`gemma4_moe.cpp:1197,1541`, +`qwen3_5.cpp:6324,6520,6820,7100,7134,8034,8065`) is inside or transitively under +any of the four `ActivePoolScope` regions (`laguna.cpp:2574`, +`qwen3_5.cpp:5468,8644,8964`), which are leaf-ward of all of them. All three now +say "would have" / latent. `ReleaseShared` is unchanged: a hazard one call site +away from real is worth removing structurally, and `ltx2_video.cpp` above is that +call site arriving. + +### F5 — the suite reds under the lane §10 recommends + +`VT_POOL_EXACT=1` was 7/8 and `VT_POOL_BYPASS=1` was 3/8 at the reviewed head, +and §10 hands `VT_POOL_BYPASS=1` to the next reader as "the cheap discriminator +that needs no second build". A suite that reds under the lane it recommends +costs that reader an hour deciding whose red it is. Every affected case now +states the ACTIVE lane's behaviour, and the size-class case states SEPARATION +under `VT_POOL_EXACT` — §5 T1.3's second clause, promised since the spec was +written and asserted nowhere. All three lanes are green: + +| lane | result | +|---|---| +| default | 9 cases / 30 assertions / SUCCESS (also `--order-by=rand` seeds 1 and 7) | +| `VT_POOL_BYPASS=1` | 9 / 24 / SUCCESS | +| `VT_POOL_EXACT=1` | 9 / 31 / SUCCESS | + +### F6 — a new throw with no test, and one un-bounded index + +D5's per-device-type memoization turned `platforms::GetPlatform` into a +per-type call, so a backend whose platform was never registered now throws where +it used to inherit the first device's cap. That is correct — a cap read off +another platform is a wrong number, not a default — and it had no test. T1.7 +covers it on `kXPU`, the one `DeviceType` with no `RegisterPlatform` call in the +tree, with a `REQUIRE_FALSE(HasPlatform(kXPU))` precondition so the case goes RED +and asks to be re-pointed if an XPU platform ever lands. The +`cached[static_cast(type)]` index gains the same `VT_CHECK` bound +`platforms::Index()` (`platform.cpp:40-44`) applies to that identical value, in +both mirrored copies. + +### Mutation evidence for the NEW assertions + +Product mutated in place, focused suite rebuilt and run, tree restored +byte-for-byte (md5 verified) after each: + +| mutation | result | +|---|---| +| `ResolveDevicePoolPolicy` returns cap 0 for an unregistered platform | 8 passed / **1 failed** — T1.7 only, `CHECK_THROWS_AS … did NOT throw at all!` | +| `ClassOf` ignores `VT_POOL_EXACT` | under `VT_POOL_EXACT=1`: 8 passed / **1 failed**, 4 assertions — the size-class case only | +| `Bypass()` forced `false` | under `VT_POOL_BYPASS=1`: 4 passed / **5 failed** | + +### Local gate, on the RESOLVED tree + +CPU host, at base `310fa1688`, CLEAN rebuild (`rm -rf build` — `device_pool.h` +is a header and an incremental build masks `-Werror`): `CONFIGURE_EXIT=0`, +`BUILD_EXIT=0`, **zero** `warning`, **zero** `No space left`/`BFD assertion`, +`ctest -N` **415**, full `ctest` **415/415 passed, exit 0** (one skip, +`test_voxtral_e2e`, checkpoint-gated). `test_cpu_isa_x86` — the suite F1's +resolution had to keep compiling — **6 / 8242 / SUCCESS**. `test_ltx2_device` +13/498 SUCCESS at default, `--order-by=name` and `--rand-seed=7`. +`test_deepseek_v2_forward` 11/1052. `test_minimax_h3` 79/57395. `test_capi` +55/505. + +`scripts/agent-preflight.sh` reports `doc-checkpoint range` FAIL on **three +commits, none of them this row's** — `b0aa475a3`, `d67f8125e` and `aa6aa0ecd`, +all pre-existing on the campaign branch and reproduced exactly by +`check-doc-checkpoint.py --base origin/main --head origin/row/MODEL-DIFFUSION-LTX25`. +Over this row's own range (`row/MODEL-DIFFUSION-LTX25..HEAD`) the same checker is +OK. `test_cpu_x86_llamacpp_floor` failed once at local load average 28 and is +10/10 OK standalone; it is a contention-sensitive harness test this diff does not +reach. + +### What is still not established + +Why a host `aligned_alloc` block yields a uniform quiet NaN on GB10 rather than +running correct-but-slow through ATS (§7, unchanged — the fix makes the ordering +unreachable, so the question is academic for this row and is not claimed to be +answered). `MoeAuxStreamFor`'s `d.q.device.index`-only key stays a recorded +latent trap of the same family, not fixed here. The nine dgx failures above +belong to other rows and are named, not adopted. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 8818d03e1..44def9c45 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -58,6 +58,7 @@ are our reading of their documented behavior, not measurements. | KV events (block create / evict publish) | ◐ no transport | ✅ | ☐ | ☐ | | Prefix-cache matching unit | ◐ resolver only | ✅ | ☐ | ☐ | | Compute directly on quantized blocks | ✅ | ☐ | ☐ | ✅ | +| Scratch allocator keyed by device (two backends, one process) | ✅ since [#516](https://github.com/mudler/vllm.cpp/issues/516); a pool is bound to one backend and refuses any other, and a backend with no registered platform is refused rather than given another's residency cap | ✅ device is field 0 of the allocation handle | ✅ | ✅ | | Automatic memory sizing (no hand-tuned budget) | ☐ hand-typed block count | ☐ percent, hand-tuned | ☐ | ◐ | | Memory cap with a pre-flight error instead of an OOM | ☐ | ◐ KV pool only | ◐ | ☐ | diff --git a/docs/STATUS.md b/docs/STATUS.md index 14712576c..c3b91dde4 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -1414,6 +1414,17 @@ platform missing from `CurrentPlatform()`'s hardcoded walk registers and answers correctly but is NEVER selected, with no compiler diagnostic. `test_platform` now gates that every `DeviceType` is in the walk and CPU is last. +**The device-scratch pool is now ONE POOL PER DEVICE (`POOL-DEVICE-KEY`, #516).** +It was a process-wide free list keyed by byte size class with no device in the +key, so in a mixed-backend process a block allocated through one backend was +handed to the next caller of that class on another: a `cudaMalloc` block reaching +a CPU forward SIGSEGVs host-side with `compute-sanitizer` clean, and a host block +reaching a CUDA forward returned a uniform `0x7fff0000` quiet NaN. A pool is now +bound to a backend, `Pool(b)` is the only spelling, every operation refuses a +foreign backend, and the two per-caller workarounds are deleted. `test_device_pool` +gates it without a GPU; `VT_POOL_BYPASS`/`VT_POOL_EXACT` keep their meanings and +the suite is green under both. + **CUDA architectures.** The runtime-gated production arch is GB10 `sm_121a` (every gate model, every benchmark). A build-supported cross-family fan-out (`sm_80/86/87/89`, `sm_90a`, `sm_100a/103a`, `sm_110`) compiles single-arch, diff --git a/docs/USAGE.md b/docs/USAGE.md index a33892a42..ad1a0130f 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -128,6 +128,32 @@ context is never torn down, so the pointers stayed mapped — it simply produced corrupted or zeroed output tokens, intermittently ([#237](https://github.com/mudler/vllm.cpp/issues/237)). +More than one **backend** in one process is likewise supported — a CPU forward +running beside a CUDA one, which is what a diffusion pipeline with a host-side +stage does. Until +[#516](https://github.com/mudler/vllm.cpp/issues/516) it was not: the shared +device-scratch pool was a single process-wide free list keyed by byte size class +with no device in the key, so a block allocated through one backend was handed +to the next caller of that size class on another. It has two symptoms and the +direction picks which: a `cudaMalloc` block reaching a CPU forward segfaults in +the host `memcpy`, and a host block reaching a CUDA forward produces output that +is uniformly NaN rather than wrong. Neither can happen now — a scratch pool is +bound to one backend and refuses any other with a `std::logic_error` naming both +— and no user-facing flag or env var selects the behaviour: it is unconditional. + +One consequence is worth knowing before you add a backend. The scratch pool's +residency cap now comes from *that device's* platform rather than from whichever +device resolved first, so constructing a buffer on a backend whose platform was +never registered raises instead of silently inheriting another platform's cap. A +cap read off the wrong platform is a wrong number, not a default, and every +backend the tree ships registers one. + +`VT_POOL_BYPASS=1` and `VT_POOL_EXACT=1` keep exactly the meanings +[ENVIRONMENT.md](ENVIRONMENT.md) records for them. They are debugging lanes, not +timing configurations, and the pool's own test suite is green under both, so +either one stays usable as a discriminator when something else is under +suspicion. + ## Starting an agent-assisted contribution Run `scripts/agent-start.py` first. It reports an inherited worktree role or, diff --git a/include/vllm/model_executor/models/dense_device_glue.h b/include/vllm/model_executor/models/dense_device_glue.h index 35afd0cfb..aa8c06af6 100644 --- a/include/vllm/model_executor/models/dense_device_glue.h +++ b/include/vllm/model_executor/models/dense_device_glue.h @@ -17,6 +17,8 @@ // DBuf — move-only pooled device allocation + tensor view. #pragma once +#include +#include #include #include #include @@ -25,6 +27,8 @@ #include "vllm/model_executor/models/qwen3_5_weights.h" // OwnedTensor #include "vllm/platforms/interface.h" #include "vt/backend.h" +#include "vt/device.h" // kNumDeviceTypes +#include "vt/dtype.h" // VT_CHECK #include "vt/ops.h" namespace vllm { @@ -62,19 +66,40 @@ inline Tensor Reshape(const Tensor& src, const std::vector& shape) { // The device-scratch residency policy (BACKEND-PLATFORM item 2), resolved from // the running device's platform. The DevicePool soft cap is platform data (0 == -// uncapped, GB10 today ⇒ pool behavior byte-for-byte unchanged). Memoized in a -// function-local static: DBuf is a per-op hot path and the process runs on ONE -// device, so the virtual dispatch is paid exactly once. Mirrors qwen3_5.cpp. +// uncapped, GB10 today ⇒ pool behavior byte-for-byte unchanged). Mirrors +// qwen3_5.cpp. +// +// Memoized PER DEVICE TYPE, not once per process. The previous single +// function-local static cached whichever device asked FIRST and then applied its +// cap to every later device — the same ambient-device assumption #516 fixed one +// layer down, and a mixed-backend process would have run a CUDA DBuf under the +// CPU platform's policy. DBuf is a per-op hot path, so the virtual dispatch is +// still paid at most once per device type. +// +// A backend whose platform was never REGISTERED now throws out of +// `platforms::GetPlatform` instead of inheriting whichever device asked first. +// That is the point: a residency cap read off another platform is a wrong +// number wearing a default's clothes. Gated by +// tests/vllm/models/test_device_pool.cpp. struct DevicePoolPolicy { size_t cap_bytes = 0; // residency_policy().device_pool_cap_bytes (0 == uncapped) }; inline DevicePoolPolicy ResolveDevicePoolPolicy(const Dev& d) { - static const DevicePoolPolicy p = [&] { - const auto rp = - vllm::platforms::GetPlatform(d.q.device.type).residency_policy(); - return DevicePoolPolicy{rp.device_pool_cap_bytes}; - }(); - return p; + // Stored as cap+1 so that 0 means "not resolved yet" and a genuine cap of 0 + // (every platform today) still caches. Racing threads resolve the same device + // type to the same value, so the benign double-resolve needs no lock. + static std::array, vt::kNumDeviceTypes> cached{}; + // Same bound, same place, as platforms::Index() (src/vllm/platforms/ + // platform.cpp) applies to this identical value before indexing ITS registry. + // An out-of-range DeviceType is only reachable by a cast, and the two lookups + // must not disagree about whether that is a throw or a stray write. + const size_t idx = static_cast(d.q.device.type); + VT_CHECK(idx < vt::kNumDeviceTypes, "invalid device type"); + const size_t seen = cached[idx].load(std::memory_order_relaxed); + if (seen != 0) return DevicePoolPolicy{seen - 1}; + const auto rp = vllm::platforms::GetPlatform(d.q.device.type).residency_policy(); + cached[idx].store(rp.device_pool_cap_bytes + 1, std::memory_order_relaxed); + return DevicePoolPolicy{rp.device_pool_cap_bytes}; } // Owned device allocation + tensor view, routed through the SHARED DevicePool so @@ -91,7 +116,11 @@ class DBuf { bytes_ = static_cast(numel) * vt::SizeOf(dt); alloc_bytes_ = bytes_ == 0 ? 1 : bytes_; cap_ = ResolveDevicePoolPolicy(d).cap_bytes; - pool_ = ActivePool(); + // THIS DEVICE's pool, unless an ActivePoolScope overrides it (the aux + // stream). Remembered so the block returns to the pool it came from even if + // this DBuf outlives the scope. See device_pool.h: there is no + // device-less pool to fall back on. + pool_ = &ActivePool(*b_); p_ = pool_->Get(*b_, alloc_bytes_); t_ = MakeTensor(p_, dt, d.q.device, shape); if (host != nullptr && bytes_ > 0) b_->Copy(d.q, p_, host, bytes_); @@ -130,16 +159,44 @@ class DBuf { b_->Synchronize(d.q); } // Relinquish the pool block WITHOUT returning it (dtor becomes a no-op); the - // caller takes over the Pool().Put obligation for alloc_bytes(). + // caller takes over the Put obligation for alloc_bytes(). Prefer + // ReleaseShared() below, which discharges that obligation correctly by + // construction. void* Release() { void* p = p_; p_ = nullptr; return p; } + // Move the block into a shared_ptr that returns it to THIS buffer's own pool + // and backend when the last owner drops it — the carrier every cross-step + // hand-off (device logits, MTP hidden states, MoE scratch) wants. + // + // It replaces ~28 copies of a hand-written deleter that closed over the byte + // count ALONE and called `Pool().Put(alloc, q)`. That idiom named neither the + // device nor the pool, so it returned every such block to the one global pool + // — a block from another device (#516), which was LIVE, and a block drawn + // from the aux-stream pool, which was not: none of the nine `Release()` sites + // sat inside or under any of the four `ActivePoolScope` regions, so the old + // deleter and the buffer's own `pool_` always agreed in practice. It was a + // hazard one new call site away from being real, and it is gone either way, + // because the carrier now captures the pool it came from rather than + // re-deriving it. + std::shared_ptr ReleaseShared() { + DevicePool* const pool = pool_; + Backend* const b = b_; + const size_t alloc = alloc_bytes_; + void* const p = Release(); + // A moved-from or already-released buffer owns nothing; a shared_ptr built + // over a null pointer with a custom deleter would still RUN that deleter and + // push null into the free list. + if (p == nullptr) return {}; + return std::shared_ptr(p, [pool, b, alloc](void* q) { pool->Put(*b, alloc, q); }); + } + private: Backend* b_; - DevicePool* pool_ = &Pool(); + DevicePool* pool_ = nullptr; void* p_ = nullptr; size_t bytes_ = 0; size_t alloc_bytes_ = 0; diff --git a/include/vllm/model_executor/models/device_pool.h b/include/vllm/model_executor/models/device_pool.h index 60dd6df72..b1d8f450b 100644 --- a/include/vllm/model_executor/models/device_pool.h +++ b/include/vllm/model_executor/models/device_pool.h @@ -1,10 +1,9 @@ // Shared process-wide caching device allocator (DevicePool) — extracted VERBATIM // from the Qwen3.6 forward (qwen3_5.cpp) so the dense Qwen3 forward (qwen3.cpp) // reuses the SAME pooled-scratch machinery instead of raw per-op Backend -// Alloc/Free. This is a pure relocation: the class body, the Pool()/AuxPool() -// singletons, and the thread-local ActivePool()/ActivePoolScope are byte-for-byte -// the qwen3_5.cpp definitions, so the 27B/35B gate-model behavior is unchanged -// (the header is included by qwen3_5.cpp in place of its old inline copies). +// Alloc/Free. The relocation was byte-for-byte the qwen3_5.cpp definitions; what +// has changed since is that a pool is now bound to ONE DEVICE (see below), and +// the accessors take the backend that names it. // // Rationale: both cudaMalloc AND cudaFree SYNCHRONIZE the whole device, so the // per-op DBuf alloc/free churn in a forward (thousands of tiny scratch buffers per @@ -15,8 +14,28 @@ // on the same queue, and CUDA stream ordering guarantees the op that last touched // the block has completed before any reused op runs — no host sync needed. Blocks // are never returned to the driver (leak at process exit, like the cublasLt -// workspace); the pool is bounded by the forward's peak concurrent scratch. The -// pool is backend-agnostic (CPU malloc/free too — a harmless bounded cache there). +// workspace); the pool is bounded by the forward's peak concurrent scratch. +// +// ONE POOL PER DEVICE (#516, .agents/specs/pool-device-key.md). Until this was +// fixed there was ONE pool for the whole process and its free list was keyed by +// byte size class alone, so a block allocated through one backend was handed to +// the next caller of that size class whatever device it was running on. One +// fault, two symptoms, chosen by direction: a cudaMalloc block reaching a +// CPU-backend forward SIGSEGVs host-side (and compute-sanitizer is CLEAN, +// because the fault is not on the device), while a host block reaching a CUDA +// forward returned a UNIFORM 0x7fff0000 quiet NaN — computed and propagated, not +// garbage read. vLLM never had the bug because it never had the design: its +// allocation handle carries the device as field 0 +// (vllm/device_allocator/__init__.py:12-14 @ pin 555967922) and torch's cache is +// per-device by construction (c10/cuda/CUDACachingAllocator.h:118-172). +// +// So a `DevicePool` is BOUND to one backend, `Pool(b)` resolves the pool for a +// device, and there is deliberately NO way to spell "the pool" without naming a +// device. The `vt::Backend*` IS the device identity: the registry hands out +// exactly one Backend* per Device{type,index} (vt/backend.h, kMaxDevicesPerType), +// and GetBackend(type) and GetBackend(Device{type,0}) return the identical +// pointer — so the device enters the key with no new virtual on vt::Backend and +// therefore no edit to any backend implementation. #pragma once #include @@ -26,9 +45,11 @@ #include #include #include +#include #include #include #include +#include #include #include "vt/backend.h" @@ -49,9 +70,21 @@ namespace vllm { // its own class bucket. VT_POOL_EXACT=1 restores exact keying (A/B measurement). class DevicePool { public: + // A pool serves exactly ONE device, named at construction. Resolve one with + // Pool(b) / AuxPool(b) rather than building your own; a directly-constructed + // pool is for tests that want an isolated free list. + explicit DevicePool(vt::Backend& b) : backend_(&b) {} + DevicePool(const DevicePool&) = delete; + DevicePool& operator=(const DevicePool&) = delete; + + // Size-class rounding is `private static`, and `tests/vt/test_cpu_isa_x86.cpp` + // exercises it directly (including the overflow throw) without a backend to + // build a pool on — this is that seam. It is deliberately `static`, so binding + // a pool to a device did not change it. static size_t SizeClassForTest(size_t bytes) { return ClassOf(bytes); } void* Get(vt::Backend& b, size_t bytes) { + RequireOwnDevice(b, "Get"); // BYPASS lane (VT_POOL_BYPASS=1) — the pool is a DETECTOR BLIND SPOT and // this is how you see through it. Two ways it hides a real defect from // compute-sanitizer: @@ -65,12 +98,7 @@ class DevicePool { // a real Free, which restores both boundaries for the detector. It is a // debugging lane only: it reinstates the per-op cudaMalloc/cudaFree sync // storm this pool exists to remove, so it is never a timing configuration. - // The backend is remembered here so the no-backend Put overload (the - // cross-step shared_ptr deleter) can free through it. - if (Bypass()) { - backend_ = &b; - return b.Alloc(bytes); - } + if (Bypass()) return b.Alloc(bytes); const size_t key = ClassOf(bytes); { std::lock_guard lk(mu_); @@ -89,13 +117,24 @@ class DevicePool { // Uncapped retention (deliberately-retained cross-step buffers: the device // logits / MTP hidden handed off via a shared_ptr deleter). Bytes are always // returned to the free list — the cross-step buffers are not cap-evicted. - void Put(size_t bytes, void* p) { - // Bypass: free for real so a later use-after-free traps. `backend_` is set by - // the Get that produced `p`, so it is non-null whenever a Put can be reached; - // the null guard keeps the lane from leaking a block if that ever stops - // holding rather than dereferencing a null backend. + // + // This used to take no backend at all, which is how ~28 copy-pasted + // `shared_ptr` deleters came to name neither the device nor the pool: they + // closed over a byte count and called `Pool().Put(alloc, q)`, so a block from + // ANY device was returned to the one global pool. The aux-stream half of that + // was LATENT rather than live: the deleter would have returned an AuxPool + // block to the main pool, but no `Release()` site sat under an + // `ActivePoolScope` — the four scope regions are leaf-ward of all nine of + // them — so the wrong-pool return was reachable only by adding a site, which + // is precisely the mistake that then costs a debugging campaign. + // `DBuf::ReleaseShared()` is now the only way to build that carrier and it + // captures the buffer's own pool and backend, so neither half can come back + // (#516). + void Put(vt::Backend& b, size_t bytes, void* p) { + RequireOwnDevice(b, "Put"); + // Bypass: free for real so a later use-after-free traps. if (Bypass()) { - if (backend_ != nullptr) backend_->Free(p); + b.Free(p); return; } const size_t key = ClassOf(bytes); @@ -110,6 +149,7 @@ class DevicePool { // When a discrete GPU sets a bound, scratch over the cap is freed to the driver // rather than pooled, so the reuse pool self-limits without a model edit. void Put(vt::Backend& b, size_t bytes, void* p, size_t cap) { + RequireOwnDevice(b, "Put"); if (Bypass()) { b.Free(p); return; @@ -137,8 +177,11 @@ class DevicePool { // // SAFETY: `free_` only ever holds blocks a DBuf already returned, so nothing // live is touched. Under VT_POOL_BYPASS the free list is always empty (Put - // frees straight through) and this is a no-op. + // frees straight through) and this is a no-op. And because a pool now holds + // ONE device's blocks, `b.Free` is guaranteed to be the allocator that made + // them — before #516 a drain could hand one device's block to another's Free. size_t Drain(vt::Backend& b) { + RequireOwnDevice(b, "Drain"); std::lock_guard lk(mu_); size_t freed = 0; for (auto& entry : free_) { @@ -156,14 +199,38 @@ class DevicePool { if (std::getenv("VT_POOL_STATS") != nullptr) { const uint64_t h = hits_.load(), m = misses_.load(); const double rate = (h + m) ? 100.0 * static_cast(h) / static_cast(h + m) : 0.0; + // The backend pointer identifies WHICH device's pool this line is about: + // a mixed-backend process now prints one line per device, and two lines + // with no way to tell them apart would be worse than one wrong line. std::fprintf(stderr, - "[DevicePool] hits=%llu misses(cudaMalloc)=%llu hit-rate=%.2f%% distinct-classes=%zu\n", + "[DevicePool backend=%p] hits=%llu misses(cudaMalloc)=%llu hit-rate=%.2f%% " + "distinct-classes=%zu\n", + static_cast(backend_), static_cast(h), static_cast(m), rate, free_.size()); } } private: + // The device check, on EVERY pool operation. A hard runtime throw and NOT an + // `assert`: the SACRED gate builds are Release/NDEBUG, where an assert is + // compiled out and the pre-fix behavior — a block silently crossing devices — + // would come straight back. It is one predictable compare against a member, + // against a `cudaMalloc` this pool exists to avoid. + // + // The only way to reach it is an `ActivePoolScope` pointing at another + // device's pool, which is precisely the mistake this row exists to make + // impossible to make quietly. + void RequireOwnDevice(vt::Backend& b, const char* op) const { + if (&b == backend_) return; + char msg[192]; + std::snprintf(msg, sizeof(msg), + "DevicePool::%s called with backend %p on a pool bound to backend %p: a scratch " + "block must never cross devices (see .agents/specs/pool-device-key.md, #516)", + op, static_cast(&b), static_cast(backend_)); + throw std::logic_error(msg); + } + // VT_POOL_BYPASS=1 turns every Get/Put into a raw driver Alloc/Free (see Get). // Read once: it must not change between an allocation and its matching free, // or a pooled block would be handed to Backend::Free (or a driver block leaked @@ -197,21 +264,66 @@ class DevicePool { } std::mutex mu_; - // Backend the last Get allocated through, so the no-backend Put overload can - // free under bypass. One device per process (see ResolveDevicePoolPolicy), so - // this is stable; unused when bypass is off. - vt::Backend* backend_ = nullptr; + // THE DEVICE, and the reason this class exists in this shape. Every block in + // `free_` was allocated by this backend and will be freed by it; nothing else + // may draw from or return to this pool. + vt::Backend* backend_; std::unordered_map> free_; size_t retained_ = 0; // bytes (class-rounded) held in free_, for the soft cap std::atomic hits_{0}; std::atomic misses_{0}; }; -inline DevicePool& Pool() { - static DevicePool p; +namespace detail { + +// Process-wide table of per-device pools. Tiny by construction: one entry per +// `vt::Backend*` the process ever allocates through, i.e. one per +// Device{type,index}. Entries are never erased, which is what lets Pool()'s +// memo below hold a raw pointer. +class PoolTable { + public: + DevicePool& For(vt::Backend& b) { + std::lock_guard lk(mu_); + for (const auto& e : pools_) + if (e.first == &b) return *e.second; + pools_.emplace_back(&b, std::unique_ptr(new DevicePool(b))); + return *pools_.back().second; + } + + private: + std::mutex mu_; + std::vector>> pools_; +}; + +inline PoolTable& MainPoolTable() { + static PoolTable t; + return t; +} + +// The (backend -> pool) resolution, memoized per thread. A DBuf resolves its +// pool on EVERY construction — thousands per forward step — and this pool's +// whole purpose is to avoid a synchronizing cudaMalloc, so paying a lock and a +// scan for it would be self-defeating. A process drives one device per host +// thread at a time, so the steady state here is a single pointer compare. +inline DevicePool& MemoizedPool(PoolTable& table, vt::Backend& b, + vt::Backend*& last_backend, DevicePool*& last_pool) { + if (last_backend == &b) return *last_pool; + DevicePool& p = table.For(b); + last_backend = &b; + last_pool = &p; return p; } +} // namespace detail + +// THE scratch pool for a device. There is deliberately no no-argument spelling: +// "the pool" without a device is the defect (#516), not an ergonomic shortcut. +inline DevicePool& Pool(vt::Backend& b) { + thread_local vt::Backend* last_backend = nullptr; + thread_local DevicePool* last_pool = nullptr; + return detail::MemoizedPool(detail::MainPoolTable(), b, last_backend, last_pool); +} + // --- Aux-stream scratch pool (ENG-MOE-SHARED-AUX) ---------------------------- // The MoE shared-expert overlap (MoeBlockFusedMarlinCuda) issues the shared MLP // on a SECOND CUDA stream concurrent with the routed experts on the main stream. @@ -228,26 +340,47 @@ inline DevicePool& Pool() { // share a live block. Blocks are handed back to the pool they came from (DBuf // stores its owning pool), so a buffer allocated in the aux region and destroyed // after the join still returns to the aux pool. +// The aux pool is per-device too: the stream distinction and the device +// distinction are independent, and a process with two devices running the MoE +// overlap needs one aux pool per device, not one shared between them. #ifdef VT_MARLIN_NVFP4 // only the Marlin MoE overlap path draws from AuxPool -inline DevicePool& AuxPool() { - static DevicePool p; - return p; +namespace detail { +inline PoolTable& AuxPoolTable() { + static PoolTable t; + return t; +} +} // namespace detail +inline DevicePool& AuxPool(vt::Backend& b) { + thread_local vt::Backend* last_backend = nullptr; + thread_local DevicePool* last_pool = nullptr; + return detail::MemoizedPool(detail::AuxPoolTable(), b, last_backend, last_pool); } #endif -// Thread-local "active" scratch pool a DBuf constructs from. Defaults to the main -// Pool(); the aux-stream overlap region swaps it to AuxPool() for the duration of -// the shared-expert issue via ActivePoolScope. Single host thread drives the +// Thread-local OVERRIDE of the scratch pool a DBuf constructs from. Null means +// "this device's own Pool(b)" — the default, and the only correct default, since +// a thread-local cannot know which device the next DBuf will be built on. The +// aux-stream overlap region swaps it to AuxPool(b) for the duration of the +// shared-expert issue via ActivePoolScope. Single host thread drives the // forward, and the aux ops are issued in one contiguous block, so the swap is a // simple RAII stack. -inline DevicePool*& ActivePool() { - thread_local DevicePool* p = &Pool(); +// +// An override pointing at ANOTHER device's pool is no longer a silent +// corruption: the pool checks its backend on every operation and throws. +inline DevicePool*& ActivePoolOverride() { + thread_local DevicePool* p = nullptr; return p; } +inline DevicePool& ActivePool(vt::Backend& b) { + DevicePool* const override_pool = ActivePoolOverride(); + return override_pool != nullptr ? *override_pool : Pool(b); +} struct ActivePoolScope { DevicePool* prev; - explicit ActivePoolScope(DevicePool* p) : prev(ActivePool()) { ActivePool() = p; } - ~ActivePoolScope() { ActivePool() = prev; } + explicit ActivePoolScope(DevicePool* p) : prev(ActivePoolOverride()) { + ActivePoolOverride() = p; + } + ~ActivePoolScope() { ActivePoolOverride() = prev; } ActivePoolScope(const ActivePoolScope&) = delete; ActivePoolScope& operator=(const ActivePoolScope&) = delete; }; diff --git a/src/vllm/model_executor/models/commandr.cpp b/src/vllm/model_executor/models/commandr.cpp index ec5373edb..72827bd68 100644 --- a/src/vllm/model_executor/models/commandr.cpp +++ b/src/vllm/model_executor/models/commandr.cpp @@ -238,10 +238,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/deepseek_v2.cpp b/src/vllm/model_executor/models/deepseek_v2.cpp index cd0a71155..05940fbc2 100644 --- a/src/vllm/model_executor/models/deepseek_v2.cpp +++ b/src/vllm/model_executor/models/deepseek_v2.cpp @@ -635,10 +635,7 @@ ForwardLogits WrapDeviceLogits(DBuf&& dlogits, int64_t rows, int64_t vocab) { fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/gemma.cpp b/src/vllm/model_executor/models/gemma.cpp index 8e19810a6..8f1b3c256 100644 --- a/src/vllm/model_executor/models/gemma.cpp +++ b/src/vllm/model_executor/models/gemma.cpp @@ -254,10 +254,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/gemma2.cpp b/src/vllm/model_executor/models/gemma2.cpp index 0f9e291fc..5189445d1 100644 --- a/src/vllm/model_executor/models/gemma2.cpp +++ b/src/vllm/model_executor/models/gemma2.cpp @@ -368,10 +368,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/gemma3.cpp b/src/vllm/model_executor/models/gemma3.cpp index 9db030dcc..ed59a4df7 100644 --- a/src/vllm/model_executor/models/gemma3.cpp +++ b/src/vllm/model_executor/models/gemma3.cpp @@ -367,10 +367,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/gemma4.cpp b/src/vllm/model_executor/models/gemma4.cpp index 02ec38894..3147f7077 100644 --- a/src/vllm/model_executor/models/gemma4.cpp +++ b/src/vllm/model_executor/models/gemma4.cpp @@ -732,10 +732,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/gemma4_moe.cpp b/src/vllm/model_executor/models/gemma4_moe.cpp index 1966e727e..238d09669 100644 --- a/src/vllm/model_executor/models/gemma4_moe.cpp +++ b/src/vllm/model_executor/models/gemma4_moe.cpp @@ -1193,9 +1193,7 @@ Gemma4MoeScratch RunGemma4Moe(vt::Queue& q, const Gemma4MoeLayerWeights& moe, } Gemma4MoeScratch r; r.tensor = acc.t(); - const size_t alloc = acc.alloc_bytes(); - void* p = acc.Release(); - r.storage = std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + r.storage = acc.ReleaseShared(); if (profile) { const auto t_all1 = clock::now(); static std::atomic ncalls{0}; @@ -1537,9 +1535,7 @@ Gemma4MoeScratch RunGemma4Moe(vt::Queue& q, const Gemma4MoeLayerWeights& moe, // TLS-owned: non-owning view for DualRmsNorm; next call overwrites same buffer. r.storage = std::shared_ptr(acc.ptr(), [](void*) {}); } else { - const size_t alloc = acc.alloc_bytes(); - void* p = acc.Release(); - r.storage = std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + r.storage = acc.ReleaseShared(); } if (profile) { diff --git a/src/vllm/model_executor/models/glm4.cpp b/src/vllm/model_executor/models/glm4.cpp index 6210a8f55..0a9489064 100644 --- a/src/vllm/model_executor/models/glm4.cpp +++ b/src/vllm/model_executor/models/glm4.cpp @@ -278,10 +278,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/granite.cpp b/src/vllm/model_executor/models/granite.cpp index efea4b5e6..d2ad868d2 100644 --- a/src/vllm/model_executor/models/granite.cpp +++ b/src/vllm/model_executor/models/granite.cpp @@ -275,10 +275,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/kimi_linear.cpp b/src/vllm/model_executor/models/kimi_linear.cpp index 9f64e4300..19663eb43 100644 --- a/src/vllm/model_executor/models/kimi_linear.cpp +++ b/src/vllm/model_executor/models/kimi_linear.cpp @@ -92,10 +92,7 @@ ForwardLogits WrapKimiLinearDeviceLogits(dense_attn::DBuf&& dlogits, int64_t row fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/kimi_linear_device.cpp b/src/vllm/model_executor/models/kimi_linear_device.cpp index 0ec6047ad..8faf25b70 100644 --- a/src/vllm/model_executor/models/kimi_linear_device.cpp +++ b/src/vllm/model_executor/models/kimi_linear_device.cpp @@ -879,10 +879,7 @@ ForwardLogits WrapDeviceLogits(DBuf&& dlogits, int64_t rows, int64_t vocab) { fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* pp = dlogits.Release(); - fl.device_storage = - std::shared_ptr(pp, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/laguna.cpp b/src/vllm/model_executor/models/laguna.cpp index 7c3c42278..dbf1f9de2 100644 --- a/src/vllm/model_executor/models/laguna.cpp +++ b/src/vllm/model_executor/models/laguna.cpp @@ -2571,7 +2571,7 @@ struct LagunaGraph { vt::Backend& b = vt::GetBackend(dev); b.RecordEvent(aux_fork, q); // event0.record() on the main stream (hn ready) b.QueueWaitEvent(aux_q, aux_fork); // aux waits event0 before reading hn - ActivePoolScope guard(&AuxPool()); // shared scratch from AuxPool (see device_pool.h) + ActivePoolScope guard(&AuxPool(b)); // shared scratch from AuxPool (see device_pool.h) LagunaSharedExpertMarlinInto(aux_q, lw.moe, hn.data(), H, so.data()); // fp4 shared on aux b.RecordEvent(aux_done, aux_q); // event1.record() on the aux stream (join target) } diff --git a/src/vllm/model_executor/models/minicpm.cpp b/src/vllm/model_executor/models/minicpm.cpp index c956bc199..5949a6541 100644 --- a/src/vllm/model_executor/models/minicpm.cpp +++ b/src/vllm/model_executor/models/minicpm.cpp @@ -279,10 +279,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/minicpm3.cpp b/src/vllm/model_executor/models/minicpm3.cpp index 749ead39c..98117965e 100644 --- a/src/vllm/model_executor/models/minicpm3.cpp +++ b/src/vllm/model_executor/models/minicpm3.cpp @@ -305,10 +305,7 @@ ForwardLogits WrapDeviceLogits(DBuf&& dlogits, int64_t rows, int64_t vocab) { fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/minimax_h3_pipeline.cpp b/src/vllm/model_executor/models/minimax_h3_pipeline.cpp index 4f952ba78..5f9541693 100644 --- a/src/vllm/model_executor/models/minimax_h3_pipeline.cpp +++ b/src/vllm/model_executor/models/minimax_h3_pipeline.cpp @@ -556,7 +556,7 @@ MiniMaxH3T2vaResult MiniMaxH3GenerateT2va(vt::Device device, const MiniMaxH3T2va // BOX DOWN: measured 85 GiB resident at this point against a ~18 GiB decode // in a 122 GiB unified pool, and the driver OOM (NV_ERR_NO_MEMORY) rebooted // the machine. Draining costs one cudaFree per retained block, once. - const size_t drained = ActivePool()->Drain(vae_backend); + const size_t drained = ActivePool(vae_backend).Drain(vae_backend); if (std::getenv("VT_POOL_STATS") != nullptr) { std::fprintf(stderr, "[h3] drained %.2f GiB of denoise scratch before VAE decode\n", static_cast(drained) / (1024.0 * 1024.0 * 1024.0)); diff --git a/src/vllm/model_executor/models/muse_glimmer.cpp b/src/vllm/model_executor/models/muse_glimmer.cpp index 817ab9cee..9008b90e5 100644 --- a/src/vllm/model_executor/models/muse_glimmer.cpp +++ b/src/vllm/model_executor/models/muse_glimmer.cpp @@ -456,10 +456,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/olmo2.cpp b/src/vllm/model_executor/models/olmo2.cpp index bd5006c5f..da7cd0df1 100644 --- a/src/vllm/model_executor/models/olmo2.cpp +++ b/src/vllm/model_executor/models/olmo2.cpp @@ -351,10 +351,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/opt.cpp b/src/vllm/model_executor/models/opt.cpp index c66b0462d..f0b93a759 100644 --- a/src/vllm/model_executor/models/opt.cpp +++ b/src/vllm/model_executor/models/opt.cpp @@ -319,9 +319,7 @@ ForwardLogits WrapDeviceLogits(DBuf&& dlogits, int64_t rows, int64_t vocab) { fl.device_tensor = dlogits.t(); // The pool block's lifetime moves into a shared_ptr whose deleter returns it // to the DevicePool (mirrors qwen3.cpp/qwen3_5.cpp WrapDeviceLogits). - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/phi.cpp b/src/vllm/model_executor/models/phi.cpp index 3f29798b2..84c16c683 100644 --- a/src/vllm/model_executor/models/phi.cpp +++ b/src/vllm/model_executor/models/phi.cpp @@ -239,10 +239,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/phi3.cpp b/src/vllm/model_executor/models/phi3.cpp index 6452d8381..1009ded6e 100644 --- a/src/vllm/model_executor/models/phi3.cpp +++ b/src/vllm/model_executor/models/phi3.cpp @@ -225,10 +225,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/qwen3.cpp b/src/vllm/model_executor/models/qwen3.cpp index 35ec52bc5..6630ce235 100644 --- a/src/vllm/model_executor/models/qwen3.cpp +++ b/src/vllm/model_executor/models/qwen3.cpp @@ -324,10 +324,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca // The pool block's lifetime moves into a shared_ptr whose deleter returns it to // the DevicePool — no per-step cudaMalloc/cudaFree, and the buffer safely // outlives sampling (mirrors qwen3_5.cpp WrapDeviceLogits). - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/qwen3_5.cpp b/src/vllm/model_executor/models/qwen3_5.cpp index 8604c3474..0d62853ff 100644 --- a/src/vllm/model_executor/models/qwen3_5.cpp +++ b/src/vllm/model_executor/models/qwen3_5.cpp @@ -611,20 +611,30 @@ Tensor Reshape(const Tensor& src, const std::vector& shape) { // the running device's platform (per-object: keyed on the DBuf's own // device.type, NOT the process-global CurrentPlatform). The DevicePool soft cap // is now platform data, not an inline constant — a discrete GPU sets a bound and -// this file is unchanged. Memoized in a function-local static because DBuf is a -// per-op hot path and the process runs on ONE device (all DBufs share it), so the -// virtual dispatch is paid exactly once; platforms are fixed at static -// registration, so the value never changes afterward. +// this file is unchanged. Memoized PER DEVICE TYPE because DBuf is a per-op hot +// path; platforms are fixed at static registration, so the value never changes +// afterward. It used to be ONE function-local static, which cached whichever +// device asked first and applied that cap to every later one — the same +// ambient-device assumption #516 fixed one layer down (dense_device_glue.h +// carries the identical repair). A backend whose platform was never REGISTERED +// therefore throws out of GetPlatform rather than inheriting the first device's +// cap — a cap read off another platform is a wrong number, not a default. struct DevicePoolPolicy { size_t cap_bytes = 0; // residency_policy().device_pool_cap_bytes (0 == uncapped) }; DevicePoolPolicy ResolveDevicePoolPolicy(const Dev& d) { - static const DevicePoolPolicy p = [&] { - const auto rp = - vllm::platforms::GetPlatform(d.q.device.type).residency_policy(); - return DevicePoolPolicy{rp.device_pool_cap_bytes}; - }(); - return p; + // cap+1, so 0 means "not resolved yet" and a genuine cap of 0 (every platform + // today) still caches. Racing threads resolve the same type to the same value. + static std::array, vt::kNumDeviceTypes> cached{}; + // Same bound platforms::Index() applies to this identical value before + // indexing ITS registry (src/vllm/platforms/platform.cpp). + const size_t idx = static_cast(d.q.device.type); + VT_CHECK(idx < vt::kNumDeviceTypes, "invalid device type"); + const size_t seen = cached[idx].load(std::memory_order_relaxed); + if (seen != 0) return DevicePoolPolicy{seen - 1}; + const auto rp = vllm::platforms::GetPlatform(d.q.device.type).residency_policy(); + cached[idx].store(rp.device_pool_cap_bytes + 1, std::memory_order_relaxed); + return DevicePoolPolicy{rp.device_pool_cap_bytes}; } // --- Fused-MoE per-layer resident constants (M2.5 Phase 2, CUDA-graph unblock) - @@ -839,11 +849,12 @@ class DBuf { // (BACKEND-PLATFORM item 2), not an inline constant. 0 == uncapped (GB10 // today) ⇒ pool behavior is byte-for-byte unchanged. cap_ = ResolveDevicePoolPolicy(d).cap_bytes; - // Draw from the thread-local active pool (main Pool() by default, AuxPool() - // inside the shared-expert overlap region), and REMEMBER it so the block - // returns to the same pool even when this DBuf outlives the ActivePoolScope - // (the aux region returns sd/gl, destroyed after the join). See AuxPool(). - pool_ = ActivePool(); + // Draw from THIS DEVICE's pool (Pool(b)) unless an ActivePoolScope overrides + // it for the shared-expert overlap region (AuxPool(b)), and REMEMBER the + // pool so the block returns to the one it came from even when this DBuf + // outlives the scope (the aux region returns sd/gl, destroyed after the + // join). See AuxPool(). + pool_ = &ActivePool(*b_); p_ = pool_->Get(*b_, alloc_bytes_); t_ = MakeTensor(p_, dt, d.q.device, shape); if (host != nullptr) b_->Copy(d.q, p_, host, bytes_); @@ -879,13 +890,28 @@ class DBuf { size_t bytes() const { return bytes_; } size_t alloc_bytes() const { return alloc_bytes_; } // Relinquish ownership of the pool block WITHOUT returning it (the dtor becomes - // a no-op). The caller takes over the Pool().Put obligation for `alloc_bytes()`. - // The Tensor view (t()) still holds the raw data pointer after this. + // a no-op). The caller takes over the Put obligation for `alloc_bytes()`. + // The Tensor view (t()) still holds the raw data pointer after this. Prefer + // ReleaseShared(), which discharges that obligation correctly by construction. void* Release() { void* p = p_; p_ = nullptr; return p; } + + // Move the block into a shared_ptr that returns it to THIS buffer's own pool + // and backend. Replaces the hand-written deleter that closed over the byte + // count alone and called `Pool().Put(alloc, q)`, naming neither the device nor + // the pool — so it returned another device's block, and an aux-stream block, + // to the main device's free list (#516; see dense_device_glue.h). + std::shared_ptr ReleaseShared() { + DevicePool* const pool = pool_; + Backend* const b = b_; + const size_t alloc = alloc_bytes_; + void* const p = Release(); + if (p == nullptr) return {}; + return std::shared_ptr(p, [pool, b, alloc](void* q) { pool->Put(*b, alloc, q); }); + } void Zero(Dev d) { b_->Memset(d.q, p_, 0, bytes_); } // Copies the buffer back to host and blocks until the queue is idle. void Download(Dev d, void* host) { @@ -895,7 +921,7 @@ class DBuf { private: Backend* b_; - DevicePool* pool_ = &Pool(); // owning scratch pool (main Pool() or AuxPool()) + DevicePool* pool_ = nullptr; // owning scratch pool (this device's Pool() or AuxPool()) void* p_ = nullptr; size_t bytes_ = 0; size_t alloc_bytes_ = 0; @@ -5733,7 +5759,7 @@ DBuf MoeBlockFusedMarlinCuda(Dev d, const MoeBlockWeights& w, const HfConfig& cf Dev auxd{d.b, ax->q}; // Draw the shared path's scratch from AuxPool so the concurrent main-stream // routed allocations never share a live block with it (see AuxPool()). - ActivePoolScope guard(&AuxPool()); + ActivePoolScope guard(&AuxPool(d.b)); sp_aux.emplace(SharedExpertUngated(auxd, w, cfg, dh, T, true)); d.b.RecordEvent(ax->done, ax->q); // event1.record() on the aux stream } @@ -6618,10 +6644,7 @@ Qwen3_5MTPHiddenStates MtpFinalize(Dev device, const Qwen3_5MTPWeights& weights, vt::RmsNormArgs{eps, true}, &residual.t()); Qwen3_5MTPHiddenStates out; out.tensor = normalized.t(); - const size_t allocation = normalized.alloc_bytes(); - void* storage = normalized.Release(); - out.storage = std::shared_ptr( - storage, [allocation](void* ptr) { Pool().Put(allocation, ptr); }); + out.storage = normalized.ReleaseShared(); return out; } @@ -6814,10 +6837,7 @@ MoeBlockOutput RunMoeBlock(vt::Queue& queue, const MoeBlockWeights& weights, DBuf out = MoeBlock(d, weights, config, dh, T); MoeBlockOutput r; r.tensor = out.t(); - const size_t alloc = out.alloc_bytes(); - void* p = out.Release(); // dtor now a no-op; we own the Pool().Put obligation - r.storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + r.storage = out.ReleaseShared(); return r; } @@ -7114,10 +7134,7 @@ static ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t vocab) { fl.rows = dlogits.t().shape[0]; fl.vocab = vocab; fl.device_tensor = dlogits.t(); // view (raw data ptr survives Release) - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); // dtor now a no-op; we own the Pool().Put - fl.device_storage = std::shared_ptr( - p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } @@ -7394,10 +7411,7 @@ ForwardLogits Qwen3_5Model::ForwardDeviceTap( gdn_state, weights, config, logits_indices, &tap_view); if (hidden_out != nullptr) { hidden_out->tensor = tap.t(); - const size_t allocation = tap.alloc_bytes(); - void* storage = tap.Release(); - hidden_out->storage = std::shared_ptr( - storage, [allocation](void* ptr) { Pool().Put(allocation, ptr); }); + hidden_out->storage = tap.ReleaseShared(); } return WrapDeviceLogits(d, std::move(dlogits), config.vocab_size); } @@ -7428,10 +7442,7 @@ ForwardLogits Qwen3_5Model::ForwardDeviceMultiTap( gdn_state, weights, config, logits_indices, /*hidden_tap=*/nullptr, &aux_out->layer_ids, &aux_view); aux_out->tensor = aux.t(); - const size_t allocation = aux.alloc_bytes(); - void* storage = aux.Release(); - aux_out->storage = std::shared_ptr( - storage, [allocation](void* ptr) { Pool().Put(allocation, ptr); }); + aux_out->storage = aux.ReleaseShared(); return WrapDeviceLogits(d, std::move(dlogits), config.vocab_size); } @@ -8328,10 +8339,7 @@ ForwardLogits Qwen3_5DenseModel::ForwardDeviceTap( logits_indices, &tap_view); if (hidden_out != nullptr) { hidden_out->tensor = tap.t(); - const size_t allocation = tap.alloc_bytes(); - void* storage = tap.Release(); - hidden_out->storage = std::shared_ptr( - storage, [allocation](void* ptr) { Pool().Put(allocation, ptr); }); + hidden_out->storage = tap.ReleaseShared(); } return WrapDeviceLogits(d, std::move(dlogits), config.vocab_size); } @@ -8359,10 +8367,7 @@ ForwardLogits Qwen3_5DenseModel::ForwardDeviceMultiTap( logits_indices, /*hidden_tap=*/nullptr, &aux_out->layer_ids, &aux_view); aux_out->tensor = aux.t(); - const size_t allocation = aux.alloc_bytes(); - void* storage = aux.Release(); - aux_out->storage = std::shared_ptr( - storage, [allocation](void* ptr) { Pool().Put(allocation, ptr); }); + aux_out->storage = aux.ReleaseShared(); return WrapDeviceLogits(d, std::move(dlogits), config.vocab_size); } @@ -8629,11 +8634,14 @@ struct PinnedStepInputs { // cudaMalloc mid-capture (aborts capture). Isolating them in their own pool leaves // the main Pool() exactly as the eager pre-warm step left it, so every allocation // the captured region makes is a pool HIT. DBuf remembers its owning pool, so these -// buffers return here on slot reset. Single device per process (ResolveDevicePool -// Policy), so a static instance is safe. -static DevicePool& PersistentDecodeInputPool() { - static DevicePool p; - return p; +// buffers return here on slot reset. +// +// PER DEVICE, like every other pool (#516): the isolation this wants is from the +// main scratch pool of the SAME device, and a process-wide instance would hand +// one device's retained decode inputs to another's captured forward. +static DevicePool& PersistentDecodeInputPool(vt::Backend& b) { + static detail::PoolTable table; + return table.For(b); } // Option A per-step input staging: copy the slot's refreshed host inputs into its @@ -9072,7 +9080,7 @@ ForwardLogits Qwen3_5DecodeGraph::Step( return false; }(); { - ActivePoolScope persistent_scope(&PersistentDecodeInputPool()); + ActivePoolScope persistent_scope(&PersistentDecodeInputPool(d.b)); s.dev = std::make_unique(BuildStepDevInputs( d, s.positions, s.attn_meta, s.gdn_meta, gdn_state_slots)); MaybeBuildAttnCosSin(d, *s.dev, impl_->config, S, fp4_attn); @@ -9493,7 +9501,7 @@ ForwardLogits Qwen3_5DenseDecodeGraph::Step( return false; }(); { - ActivePoolScope persistent_scope(&PersistentDecodeInputPool()); + ActivePoolScope persistent_scope(&PersistentDecodeInputPool(d.b)); s.dev = std::make_unique(BuildStepDevInputs( d, s.positions, s.attn_meta, s.gdn_meta, gdn_state_slots)); MaybeBuildAttnCosSin(d, *s.dev, impl_->config, S, fp4_attn); diff --git a/src/vllm/model_executor/models/qwen3_moe.cpp b/src/vllm/model_executor/models/qwen3_moe.cpp index 0285ce512..3f0919bb8 100644 --- a/src/vllm/model_executor/models/qwen3_moe.cpp +++ b/src/vllm/model_executor/models/qwen3_moe.cpp @@ -220,10 +220,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.device_tensor = dlogits.t(); // The pool block's lifetime moves into a shared_ptr whose deleter returns it to // the DevicePool — no per-step cudaMalloc/cudaFree (mirrors qwen3.cpp). - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/qwen3_vl.cpp b/src/vllm/model_executor/models/qwen3_vl.cpp index 344ad4057..f5cc33508 100644 --- a/src/vllm/model_executor/models/qwen3_vl.cpp +++ b/src/vllm/model_executor/models/qwen3_vl.cpp @@ -257,10 +257,7 @@ ForwardLogits WrapDeviceLogits(DBuf&& dlogits, int64_t rows, int64_t vocab) { fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); return fl; } diff --git a/src/vllm/model_executor/models/stablelm.cpp b/src/vllm/model_executor/models/stablelm.cpp index ebee67ef2..028028abe 100644 --- a/src/vllm/model_executor/models/stablelm.cpp +++ b/src/vllm/model_executor/models/stablelm.cpp @@ -237,10 +237,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/model_executor/models/voxtral.cpp b/src/vllm/model_executor/models/voxtral.cpp index cf42b249c..cdaa8604b 100644 --- a/src/vllm/model_executor/models/voxtral.cpp +++ b/src/vllm/model_executor/models/voxtral.cpp @@ -274,10 +274,7 @@ ForwardLogits WrapDeviceLogits(Dev d, DBuf&& dlogits, int64_t rows, int64_t voca fl.rows = rows; fl.vocab = vocab; fl.device_tensor = dlogits.t(); - const size_t alloc = dlogits.alloc_bytes(); - void* p = dlogits.Release(); - fl.device_storage = - std::shared_ptr(p, [alloc](void* q) { Pool().Put(alloc, q); }); + fl.device_storage = dlogits.ReleaseShared(); (void)d; return fl; } diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index 1d3757787..cae2c6116 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -23,7 +23,7 @@ #include #include "vllm/model_executor/model_loader/safetensors_reader.h" -#include "vllm/model_executor/models/device_pool.h" // ActivePool()/DevicePool::Drain +#include "vllm/model_executor/models/device_pool.h" // ActivePool(b)/DevicePool::Drain #include "vllm/model_executor/models/ltx2.h" #include "vllm/model_executor/models/ltx2_audio_vae.h" #include "vllm/model_executor/models/ltx2_connector.h" @@ -1160,7 +1160,13 @@ VideoResult Ltx2VideoEngine::Generate(const VideoGenParams& gen) { const char* off = std::getenv("VLLM_LTX2_POOL_DRAIN"); if (off == nullptr || off[0] != '0') { vt::Backend& backend = vt::GetBackend(im.device.type); - const size_t drained = ActivePool()->Drain(backend); + // `ActivePool(backend)`, not the device-less `ActivePool()` this line was + // written against (#516): "the pool" without a device is what handed one + // device's block to another, and draining it through `backend` was the + // same assumption twice — resolve a pool with no device, then free its + // blocks through an allocator that may not have made them. There is no + // device-less spelling any more, and `Drain` refuses a foreign backend. + const size_t drained = ActivePool(backend).Drain(backend); if (std::getenv("VT_POOL_STATS") != nullptr) { std::fprintf(stderr, "[ltx2] phase '%s' drained %.2f GiB of denoise scratch\n", phase.name.c_str(), diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 71cf69993..51f78217c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -165,6 +165,13 @@ foreach(_gateup_lever VT_DENSE_MARLIN_GATEUP VT_NVFP4_MARLIN VT_MOE_FUSED_W13) PROPERTIES ENVIRONMENT "${_gateup_lever}=0") endforeach() vllm_cpp_add_test(test_qwen35_plain_weights vllm/models/test_qwen35_plain_weights.cpp) +# POOL-DEVICE-KEY (#516, .agents/specs/pool-device-key.md): the shared scratch +# pool's free list must be keyed by DEVICE as well as size class, or a block +# allocated through one backend is handed to a DBuf running on another — +# host-side SIGSEGV in one direction, silent all-NaN output in the other. +# Hardware-free: two fake backends stand in for two devices, so this runs +# everywhere and in ~milliseconds rather than only where a GPU is present. +vllm_cpp_add_test(test_device_pool vllm/models/test_device_pool.cpp) vllm_cpp_add_test(test_moe_resident_lifetime vllm/models/test_moe_resident_lifetime.cpp) vllm_cpp_add_test(test_qwen3_load vllm/models/test_qwen3_load.cpp) vllm_cpp_add_test(test_qwen3_forward vllm/models/test_qwen3_forward.cpp) diff --git a/tests/vllm/models/test_deepseek_v2_forward.cpp b/tests/vllm/models/test_deepseek_v2_forward.cpp index e94897ca1..fb6cda4a4 100644 --- a/tests/vllm/models/test_deepseek_v2_forward.cpp +++ b/tests/vllm/models/test_deepseek_v2_forward.cpp @@ -45,7 +45,6 @@ #include "vllm/model_executor/model_loader/safetensors_reader.h" #include "vllm/model_executor/models/deepseek_v2.h" -#include "vllm/model_executor/models/device_pool.h" #include "vllm/transformers_utils/hf_config.h" #include "vt/backend.h" #include "vt/dtype.h" @@ -576,20 +575,16 @@ TEST_CASE("deepseek-v2 forward: CUDA agrees with CPU and is bit-exact run to run REQUIRE(p.mla.qk_head_dim() == 192); // the ONLY head_dim the CUDA MLA prefill has REQUIRE(p.mla.head_size() == 576); - // PER-BACKEND SCRATCH POOLS, deliberately. The shared `DevicePool` - // (device_pool.h) is a process-wide singleton keyed ONLY on a byte size class - // and is documented "backend-agnostic"; that is safe for the engine, which - // drives exactly one device per process, but NOT for a test binary that runs a - // CPU forward and a CUDA forward in the same process — the second backend - // would be handed the first backend's recycled pointers (observed: SIGSEGV in - // the CPU arm on a CUDA block). Giving each arm its own pool is a TEST-LOCAL - // fix; the hazard itself is pre-existing and unrelated to MLA, recorded in the - // W7 ledger row rather than papered over here. - static vllm::DevicePool cuda_pool; - static vllm::DevicePool cpu_pool; + // A CPU forward and a CUDA forward in ONE process, with NO per-arm pool + // scoping. This used to need `static vllm::DevicePool cuda_pool/cpu_pool` and + // an `ActivePoolScope` around each arm, because the shared `DevicePool` was a + // process-wide singleton keyed ONLY on a byte size class: the second backend + // was handed the first backend's recycled pointers, observed as a SIGSEGV in + // the CPU arm on a CUDA block. The pool is now one-per-device (device_pool.h, + // .agents/specs/pool-device-key.md, #516), so the workaround is removed and + // this case is once more a detector for the hazard. Do not re-add the scopes. std::vector cuda, again, cpu; { - const vllm::ActivePoolScope scope(&cuda_pool); cuda = RunTinyCuda(w); // Bit-exact run to run on device (nothing in the chain is non-deterministic). again = RunTinyCuda(w); @@ -602,10 +597,7 @@ TEST_CASE("deepseek-v2 forward: CUDA agrees with CPU and is bit-exact run to run // bf16-GEMM-accumulation-order wide (the CPU arm runs the per-expert reference // loop and cuBLASLt/grouped kernels reduce in a different order), so this is a // NUMERIC agreement check, not a bit check. - { - const vllm::ActivePoolScope scope(&cpu_pool); - cpu = RunTiny(w); - } + cpu = RunTiny(w); REQUIRE(cpu.size() == cuda.size()); double scale = 1e-6, worst = 0.0; for (float x : cpu) scale = std::max(scale, std::abs(static_cast(x))); diff --git a/tests/vllm/models/test_device_pool.cpp b/tests/vllm/models/test_device_pool.cpp new file mode 100644 index 000000000..466243c74 --- /dev/null +++ b/tests/vllm/models/test_device_pool.cpp @@ -0,0 +1,462 @@ +// vllm.cpp original (the shared scratch pool is a vt-runtime deviation, porting +// inventory §9.1); vLLM has no mirror because it never had this design — its +// allocation handle carries the device as field 0 +// (`vllm/device_allocator/__init__.py:12-14` @ pin 555967922) and torch's cache +// is per-device by construction (`c10/cuda/CUDACachingAllocator.h:118-172`). +// +// THE GATE FOR #516 (.agents/specs/pool-device-key.md): `vllm::Pool()` was a +// process-wide free list keyed by BYTE SIZE CLASS ONLY. The device was not in +// the key, so a block allocated through one backend was handed to a `DBuf` +// running on another. One fault, two symptoms, chosen by direction: +// +// cudaMalloc block -> CPU-backend forward : SIGSEGV, compute-sanitizer CLEAN +// (the fault is host-side) +// host block -> CUDA forward : SILENT all-NaN output +// +// This file is the DIRECT gate. It needs no GPU, no checkpoint and no NAS: two +// distinguishable fake backends stand in for two devices (the technique +// tests/vt/test_backend_multidevice.cpp and tests/vt/test_reference_tier.cpp +// already use), and every case goes through `dense_attn::DBuf` — the seam every +// production forward allocates from — so it holds the same path the LTX-2.5 +// device suite crashes on rather than a paraphrase of it. +// +// Two test-side properties are load-bearing and neither is a style choice. +// (1) A fake backend NEVER returns a block to the C allocator: these cases +// compare pointer identity ACROSS a free, and a freed pointer is not a value you +// may reason about. (2) A fake backend is never destroyed before exit, so no +// stack or heap address is ever reused — the pool is keyed on the backend's +// identity, and an address reused by a later case would make one case's pool +// answer another case's question. +// +// THE TWO DEBUG LANES ARE GREEN HERE, ON PURPOSE. `VT_POOL_BYPASS=1` removes the +// free list and `VT_POOL_EXACT=1` removes size-class rounding; the spec's own +// §10 hands `VT_POOL_BYPASS=1` to whoever picks up the unattributed dgx failures +// as "the cheap discriminator that needs no second build". A suite that reds +// under the very lane it tells you to use is a suite that costs its next reader +// an hour deciding whether the red is theirs. So: a case whose subject is REUSE +// states what the ACTIVE lane does — a pooled hit by default, a fresh driver +// block under bypass — and the case whose subject is the SIZE CLASS states +// sharing by default and SEPARATION under `VT_POOL_EXACT` (spec §5 T1.3's +// second clause, which had no assertion until now). Both env vars are read ONCE +// into a function-local static inside `DevicePool`, so a process is in exactly +// one lane for its whole life and no case can toggle them. +#include + +#include +#include +#include +#include +#include + +#include "vllm/model_executor/models/dense_device_glue.h" +#include "vllm/platforms/interface.h" +#include "vt/backend.h" +#include "vt/device.h" + +namespace { + +using vllm::dense_attn::DBuf; +using vllm::dense_attn::Dev; +using vt::Backend; +using vt::Device; +using vt::DeviceType; +using vt::DType; +using vt::Queue; + +// A host-memory backend that remembers WHICH allocations are its own, so a case +// can ask the question that matters — "did this block come from THIS device?" — +// instead of inferring it from a pointer that merely happens to differ. +class TagBackend final : public Backend { + public: + ~TagBackend() override { + for (void* p : owned_) std::free(p); + } + void* Alloc(size_t bytes) override { + void* p = std::malloc(bytes == 0 ? 1 : bytes); + owned_.push_back(p); + ++allocs_; + return p; + } + // Deliberately does NOT std::free: see the file header. The block stays valid + // and stays owned; only the fact of the Free is recorded. + void Free(void* p) override { + freed_.push_back(p); + ++frees_; + } + void Memset(Queue&, void* p, int v, size_t bytes) override { std::memset(p, v, bytes); } + void Copy(Queue&, void* dst, const void* src, size_t bytes) override { + std::memcpy(dst, src, bytes); + } + Queue CreateQueue() override { return Queue{}; } + bool UnifiedMemory() const override { return true; } + + bool Owns(const void* p) const { + for (const void* q : owned_) + if (q == p) return true; + return false; + } + bool WasFreed(const void* p) const { + for (const void* q : freed_) + if (q == p) return true; + return false; + } + int allocs() const { return allocs_; } + int frees() const { return frees_; } + + private: + std::vector owned_; + std::vector freed_; + int allocs_ = 0; + int frees_ = 0; +}; + +// Process-lifetime fakes: see the file header, property (2). +TagBackend& NewBackend() { + static std::vector> keep; + keep.push_back(std::make_unique()); + return *keep.back(); +} + +// Two devices of the same TYPE. `Device{type,index}` is exactly how the backend +// registry addresses discrete devices (vt/backend.h, kMaxDevicesPerType), and +// keeping the type equal keeps the platform lookup the DBuf constructor performs +// (`ResolveDevicePoolPolicy`) on a registered platform, so these cases test the +// pool and nothing else. +Queue QueueOn(int32_t index) { + Queue q; + q.device = Device{DeviceType::kCPU, index}; + q.handle = nullptr; + return q; +} + +// The two debug lanes, spelled EXACTLY as `DevicePool::Bypass()` and +// `DevicePool::ClassOf()` spell them ("=1", first character only), so this file +// cannot disagree with the header about which lane a process is in. +bool PoolBypass() { + const char* e = std::getenv("VT_POOL_BYPASS"); + return e != nullptr && e[0] == '1'; +} +bool PoolExact() { + const char* e = std::getenv("VT_POOL_EXACT"); + return e != nullptr && e[0] == '1'; +} + +} // namespace + +// ═══════════════════════════════════════════════════════════════════════════ +// THE DEFECT. A block freed on device 0 must never be handed to device 1. +// +// Every case below uses its OWN size class, so no case can be decided by what an +// earlier one left in a free list — including under `--order-by=rand`. +// ═══════════════════════════════════════════════════════════════════════════ +TEST_CASE("device pool: a block freed on one device is NEVER handed to another") { + TagBackend& a = NewBackend(); + TagBackend& b = NewBackend(); + Queue qa = QueueOn(0); + Queue qb = QueueOn(1); + + // Identical shape and dtype on both devices, so both land in the SAME size + // class. That is the whole precondition: at differing size classes the two + // arms never trade blocks, which is why the f32 LTX-2.5 arms could not reach + // this and the bf16 ones could. + const std::vector shape{1024}; // 4096 bytes + void* on_a = nullptr; + { + DBuf x(Dev{a, qa}, DType::kF32, shape); + on_a = x.ptr(); + } // returned to device 0's free list here + + void* on_b = nullptr; + { + DBuf y(Dev{b, qb}, DType::kF32, shape); + on_b = y.ptr(); + } + + REQUIRE(on_a != nullptr); + REQUIRE(on_b != nullptr); + // The pointer identity IS the defect: before the device entered the key these + // were the same block, and device 1 then wrote through device 0's allocation. + CHECK(on_b != on_a); + // ...and the stronger statement that identity check stands in for: each block + // came out of its OWN device's allocator. + CHECK(a.Owns(on_a)); + CHECK(b.Owns(on_b)); + CHECK_FALSE(b.Owns(on_a)); + CHECK_FALSE(a.Owns(on_b)); +} + +// The same ordering the other way round. The direction decides the SYMPTOM (a +// host block reaching a device forward is the silent-NaN direction; a device +// block reaching a host forward is the SIGSEGV one), so a fix that separates +// them in only one direction is not a fix. +TEST_CASE("device pool: the reverse direction is separated too") { + TagBackend& a = NewBackend(); + TagBackend& b = NewBackend(); + Queue qa = QueueOn(0); + Queue qb = QueueOn(1); + const std::vector shape{4096}; // 8192 bytes @ bf16 + + void* on_b = nullptr; + { + DBuf y(Dev{b, qb}, DType::kBF16, shape); + on_b = y.ptr(); + } + void* on_a = nullptr; + { + DBuf x(Dev{a, qa}, DType::kBF16, shape); + on_a = x.ptr(); + } + CHECK(on_a != on_b); + CHECK(b.Owns(on_b)); + CHECK(a.Owns(on_a)); + CHECK_FALSE(a.Owns(on_b)); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// ...AND THE POOL MUST STILL BE A POOL. Without the two cases below, "fixed" +// would be indistinguishable from `VT_POOL_BYPASS=1`, which also passes every +// case above and reinstates the per-op cudaMalloc/cudaFree sync storm the pool +// exists to remove. The fix has to separate the devices WITHOUT ending reuse. +// ═══════════════════════════════════════════════════════════════════════════ +TEST_CASE("device pool: reuse on ONE device still returns the identical block") { + TagBackend& a = NewBackend(); + Queue qa = QueueOn(0); + const std::vector shape{4096}; // 16384 bytes @ f32 + + void* first = nullptr; + { + DBuf x(Dev{a, qa}, DType::kF32, shape); + first = x.ptr(); + } + const int allocs_after_first = a.allocs(); + void* second = nullptr; + { + DBuf y(Dev{a, qa}, DType::kF32, shape); + second = y.ptr(); + } + if (PoolBypass()) { + // The bypass lane HAS no free list — every Get is an exact-size driver + // Alloc and every Put a real Free, which is the whole reason it exists (it + // restores the allocation boundaries compute-sanitizer needs). Reuse is + // therefore the wrong expectation here, not a regression, and asserting the + // lane's own behavior is what keeps the lane usable as a discriminator. + CHECK(second != first); + CHECK(a.allocs() == allocs_after_first + 1); + CHECK(a.WasFreed(first)); + } else { + CHECK(second == first); // a pool HIT... + CHECK(a.allocs() == allocs_after_first); // ...proven by the allocator counter + } +} + +TEST_CASE("device pool: size-class rounding still lets nearby sizes share a block") { + // 32,400 and 32,768 bytes round to the same class (kClassBits=4 keeps the top + // four significant bits), which is what makes a prefill of a different token + // count a pool hit instead of a synchronous cudaMalloc. Preserved + // deliberately: `VT_POOL_EXACT=1` (reuse kept, rounding removed) was MEASURED + // still red, so the rounding is not the fault and is not what this row + // changes. + TagBackend& a = NewBackend(); + Queue qa = QueueOn(0); + + void* first = nullptr; + { + DBuf x(Dev{a, qa}, DType::kF32, {8100}); // 32,400 bytes + first = x.ptr(); + } + const int allocs_after_first = a.allocs(); + void* second = nullptr; + { + DBuf y(Dev{a, qa}, DType::kF32, {8192}); // 32,768 bytes, same class + second = y.ptr(); + } + const size_t lo_class = vllm::DevicePool::SizeClassForTest(32400); + const size_t hi_class = vllm::DevicePool::SizeClassForTest(32768); + if (PoolBypass()) { + CHECK(second != first); // no free list at all, so nothing to share + } else if (PoolExact()) { + // Spec §5 T1.3's second clause, "`VT_POOL_EXACT` still separates them", + // which had no assertion anywhere until this one. Exact keying is the A/B + // arm that MEASURED the rounding innocent (still red), so it has to keep + // being a real second behavior and not merely a variable nobody reads. + CHECK(lo_class == 32400); + CHECK(hi_class == 32768); + CHECK(lo_class != hi_class); + CHECK(second != first); + CHECK(a.allocs() == allocs_after_first + 1); + } else { + CHECK(lo_class == hi_class); // ...one class... + CHECK(lo_class == 32768); // ...and it is the larger of the two + CHECK(second == first); + CHECK(a.allocs() == allocs_after_first); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// The three properties that follow from "a pool is bound to one device", each +// of which was silently wrong before and none of which the DBuf cases above +// would catch on their own. +// ═══════════════════════════════════════════════════════════════════════════ + +TEST_CASE("device pool: Drain frees ONE device's blocks, through ITS OWN backend") { + // Before the device entered the key there was one free list, so the MiniMax-H3 + // phase-change drain (minimax_h3_pipeline.cpp) handed every retained block to + // whichever backend it was called with — including another device's blocks, to + // an allocator that never made them. + TagBackend& a = NewBackend(); + TagBackend& b = NewBackend(); + Queue qa = QueueOn(0); + Queue qb = QueueOn(1); + const std::vector shape{16384}; // 65,536 bytes @ f32 + + void* held_by_b = nullptr; + { + DBuf x(Dev{a, qa}, DType::kF32, shape); + } + { + DBuf y(Dev{b, qb}, DType::kF32, shape); + held_by_b = y.ptr(); + } + if (PoolBypass()) { + // Bypass frees straight through, so both destructors already returned their + // block to their OWN backend and there is nothing retained to drain. That is + // the same device-scoped property this case is about, enforced by the + // absence of a free list rather than by the key — worth stating, because it + // is the arm the spec sends a reader to when a drain is under suspicion. + CHECK(a.frees() == 1); + CHECK(b.frees() == 1); + CHECK(vllm::Pool(a).Drain(a) == 0); + return; + } + REQUIRE(a.frees() == 0); + REQUIRE(b.frees() == 0); + + const size_t drained = vllm::Pool(a).Drain(a); + CHECK(drained > 0); + CHECK(a.frees() == 1); + CHECK(b.frees() == 0); // device 1's retained block was NOT freed by device 0 + + // ...and device 1's free list is intact: its next request of that class is + // still the same block, i.e. the drain did not quietly empty it. + { + DBuf y(Dev{b, qb}, DType::kF32, shape); + CHECK(y.ptr() == held_by_b); + } +} + +TEST_CASE("device pool: a pool bound to another device is REFUSED, not served") { + // `ActivePoolScope` is the one remaining way to hand a DBuf a pool that is not + // its device's. It is a legitimate seam — the aux CUDA stream uses it — so it + // stays, and the pool checks the backend instead. A hard throw, not an + // `assert`: the gate builds are Release/NDEBUG, where an assert is compiled + // out and the silent cross-device hand-off would come straight back. + TagBackend& a = NewBackend(); + TagBackend& b = NewBackend(); + Queue qa = QueueOn(0); + vllm::DevicePool bs_pool(b); // a pool that belongs to device 1 + + const vllm::ActivePoolScope wrong(&bs_pool); + CHECK_THROWS_AS(DBuf(Dev{a, qa}, DType::kF32, {64}), std::logic_error); +} + +TEST_CASE("device pool: ReleaseShared returns the block to the pool it CAME FROM") { + // The cross-step carrier (device logits, MTP hidden states, MoE scratch) used + // to be built by hand at ~28 sites, from a deleter that closed over the byte + // count ALONE and called `Pool().Put(alloc, q)`. So it returned the block to + // the one global pool whatever device it came from — and, separately, whatever + // POOL it came from, which silently drained the aux-stream pool into the main + // one. `ReleaseShared()` captures both. + TagBackend& a = NewBackend(); + Queue qa = QueueOn(0); + const std::vector shape{12288}; // 49,152 bytes @ f32 + + void* raw = nullptr; + { + std::shared_ptr carrier; + { + DBuf x(Dev{a, qa}, DType::kF32, shape); + raw = x.ptr(); + carrier = x.ReleaseShared(); + } + // The DBuf is gone but the carrier holds the block: this device's free list + // must NOT have it yet, so a same-class request allocates afresh. + DBuf other(Dev{a, qa}, DType::kF32, shape); + CHECK(other.ptr() != raw); + } + if (PoolBypass()) { + // No free list, so the block does not come back. What the lane still proves + // — and it is the half that matters — is that the carrier's deleter ran + // against the buffer's OWN backend rather than an ambient one. + CHECK(a.WasFreed(raw)); + return; + } + // Carrier dropped -> the block is back in THIS device's pool. + { + DBuf again(Dev{a, qa}, DType::kF32, shape); + CHECK(again.ptr() == raw); + } +} + +TEST_CASE("device pool: a scoped pool's block returns to the SCOPED pool, not the device's") { + // The aux-stream shape, which the hand-written deleters got wrong on every + // path that used them: a block drawn under an ActivePoolScope and handed to a + // shared_ptr must come back to the SCOPED pool. Returning it to the device's + // main pool is how a second stream's block ends up in the first stream's free + // list — the race AuxPool() exists to prevent. + TagBackend& a = NewBackend(); + Queue qa = QueueOn(0); + vllm::DevicePool scoped(a); // same DEVICE, different pool + const std::vector shape{24576}; // 98,304 bytes @ f32 + + void* raw = nullptr; + { + std::shared_ptr carrier; + { + const vllm::ActivePoolScope scope(&scoped); + DBuf x(Dev{a, qa}, DType::kF32, shape); + raw = x.ptr(); + carrier = x.ReleaseShared(); + } // scope ends BEFORE the carrier is dropped, deliberately + } + + if (PoolBypass()) { + // Neither pool retains anything under bypass; the deleter freed the block + // to the backend it was allocated from, which is all this lane can show. + CHECK(a.WasFreed(raw)); + return; + } + // The device's main pool must not have acquired it... + { + DBuf from_main(Dev{a, qa}, DType::kF32, shape); + CHECK(from_main.ptr() != raw); + } + // ...the scoped pool must have. + { + const vllm::ActivePoolScope scope(&scoped); + DBuf from_scoped(Dev{a, qa}, DType::kF32, shape); + CHECK(from_scoped.ptr() == raw); + } +} + +TEST_CASE("device pool: a backend whose PLATFORM is unregistered is REFUSED, not defaulted") { + // The residency policy is memoized PER DEVICE TYPE now (spec §4 D5), where it + // used to be one function-local static for the process. That closed the + // ambient-device hole one layer above the pool, and it opened a new failure + // mode with it: `platforms::GetPlatform` VT_CHECK-throws for an unregistered + // type, so a DBuf on such a backend now throws where it previously inherited + // whichever device happened to resolve FIRST. That is the correct answer — a + // `device_pool_cap_bytes` read off another platform is a wrong number, not a + // default — but a new throw with no test is a new throw nobody has run. + // + // kXPU is the type with no `RegisterPlatform` call anywhere in the tree (grep + // src/vllm/platforms/*.cpp: cpu, cuda, rocm, vulkan, metal, tenstorrent). The + // REQUIRE_FALSE states that as the precondition it is, so the day an XPU + // platform lands this case goes RED and asks to be re-pointed rather than + // quietly asserting nothing. + REQUIRE_FALSE(vllm::platforms::HasPlatform(DeviceType::kXPU)); + TagBackend& a = NewBackend(); + Queue q; + q.device = Device{DeviceType::kXPU, 0}; + q.handle = nullptr; + CHECK_THROWS_AS(DBuf(Dev{a, q}, DType::kF32, {64}), std::runtime_error); +} diff --git a/tests/vllm/models/test_ltx2_device.cpp b/tests/vllm/models/test_ltx2_device.cpp index 6bf3e8329..46bea0b78 100644 --- a/tests/vllm/models/test_ltx2_device.cpp +++ b/tests/vllm/models/test_ltx2_device.cpp @@ -43,9 +43,6 @@ #include "support/max_abs_diff.h" #include "vllm/model_executor/model_loader/safetensors_reader.h" -// For the CPU-backend arm's own scratch pool; see the CUDA-vs-host case for why -// sharing the process-wide `Pool()` across two DEVICES is what crashed. -#include "vllm/model_executor/models/device_pool.h" #include "vllm/model_executor/models/ltx2.h" #include "vllm/model_executor/models/ltx2_loader.h" #include "vt/backend.h" @@ -595,41 +592,27 @@ TEST_CASE("ltx2 device: CUDA tracks the HOST forward, not just the golden") { const vllm::Ltx2DitOutputs cuda_bf16 = Ltx2DitForwardDevice(q, p, staged_bf16.weights, &m.video, &m.audio, vt::DType::kBF16); - // THE CPU-BACKEND ARM NEEDS ITS OWN SCRATCH POOL, and this is not a style - // preference. `vllm::Pool()` is a process-wide singleton whose free list is - // keyed by SIZE CLASS ONLY (device_pool.h) — the device is not part of the - // key. So a block `cudaMalloc`ed for the CUDA arm three lines up is handed - // straight back to a CPU-backend `DBuf` of the same size class, and the CPU - // backend's `Copy` is a host `memcpy` on what is a device pointer. - // - // MEASURED, not theorised: without this scope the case SIGSEGVs on GB10 in + // THE CPU-BACKEND ARM RUNS RIGHT AFTER THE CUDA ONE, ON PURPOSE, AND WITH NO + // POOL SCOPING. That ordering is the only thing in the tree that reaches the + // #516 hazard from the SIGSEGV side, and it used to need a per-case + // `DevicePool` to survive: `vllm::Pool()` was a process-wide singleton keyed + // by SIZE CLASS ONLY, so a block `cudaMalloc`ed for the CUDA arm three lines + // up was handed straight back to a CPU-backend `DBuf` of the same size class, + // and the CPU backend's `Copy` is a host `memcpy` on what is a device pointer. + // MEASURED, not theorised: the case SIGSEGV'd on GB10 in // `__memcpy_sve <- UploadStream <- PrepareStreamDev`, and compute-sanitizer - // reports ZERO device errors because the fault is host-side. It had never been - // reachable before, because no test had run a bf16 CPU-backend device forward - // AFTER a bf16 CUDA one — at f32 the two arms land in different size classes - // and never trade blocks. - // - // The pool already carries exactly this invariant for STREAMS: `AuxPool()` - // exists because "two streams sharing one pool BREAKS" its reuse ordering, and - // the remedy there is the same as here — a distinct execution context gets a - // distinct pool, via the `ActivePoolScope` seam the pool provides for it. What - // is NOT stated at the pool is the DEVICE half of the same invariant, and a - // size-keyed, device-blind free list shared by a multi-device process is a trap - // for the next caller rather than a property of this test. Recorded as owed; - // it is a shared hot path and repairing it is its own row, not this one. + // reported ZERO device errors, because the fault is host-side. // - // `cpu_pool` is declared BEFORE the buffers that draw from it: a `DBuf` returns - // its block to the pool it was built from, so that pool has to outlive it. - vllm::DevicePool cpu_pool; + // The pool is now one-per-device (device_pool.h, .agents/specs/ + // pool-device-key.md), so the scope is gone and this case is again a DETECTOR + // for the hazard rather than a caller that was scoped away from it. Do not + // re-introduce an `ActivePoolScope` here: it would pass whether or not the + // pool is correct. vt::Queue cpuq{Cpu(), nullptr}; - vllm::Ltx2DitOutputs cpu_bf16; - { - vllm::ActivePoolScope cpu_scope(&cpu_pool); - const Ltx2DitDeviceWeights host_bf16 = - Ltx2StageDitWeightsToDevice(cpuq, p, set.views, vt::DType::kBF16); - cpu_bf16 = Ltx2DitForwardDevice(cpuq, p, host_bf16.weights, &m.video, &m.audio, - vt::DType::kBF16); - } + const Ltx2DitDeviceWeights host_bf16 = + Ltx2StageDitWeightsToDevice(cpuq, p, set.views, vt::DType::kBF16); + const vllm::Ltx2DitOutputs cpu_bf16 = + Ltx2DitForwardDevice(cpuq, p, host_bf16.weights, &m.video, &m.audio, vt::DType::kBF16); REQUIRE(cuda_bf16.video.size() == cpu_bf16.video.size()); const double bv = MaxAbsDiff(cuda_bf16.video, cpu_bf16.video.data(), cpu_bf16.video.size()); const double ba = MaxAbsDiff(cuda_bf16.audio, cpu_bf16.audio.data(), cpu_bf16.audio.size());