fix(gpu): drain htod_via on error; narrow the merkle-tail threshold - #892
Merged
Merged
Conversation
Review follow-ups for the round-2 residency work, rebased onto e75bcbe — only the items that commit did not already cover. htod_via error path. Once a chunk's DMA is in flight, `record_event` / `sync_event` returning `Err` drops the staging `MutexGuard` with the device still reading the pinned slab, so the next locker's `ensure_capacity` can `cuMemFreeHost` it mid-copy. `async_dtoh_via` already guards this exact hazard and the file ships a `DrainOnErr` helper for it; `htod_via` was the one site not using it. R2 host-evaluator fallback. If the device decompose and the `H` download both fail under device-only, control reaches the host evaluator, which reads the intentionally-empty trace and panics with a bare out-of-bounds. Assert the device-only contract instead, matching the other fallback arms. Coverage. `batch_inverse_ext3_dev`'s `n == 1` branch is never exercised — `batch_inverse_n1` goes through the host-only short circuit in `batch_inverse_ext3`, as its own comment says. Add a direct device test. Docs. The preprocessed split-tree comment still claimed both trees come back as full host trees (the multiplicity tree is root-only + device resident), and `FriCommitState`'s doc claimed its input is always Arc-shared with a retained `gpu_evals` (only true on the device-only path).
MauroToscano
force-pushed
the
gpu-opt-round2-review-fixes
branch
from
August 3, 2026 18:57
63a816f to
0a11471
Compare
TAIL_MAX_PAIRS = 2048 overshoots. The tail grid-strides a single 128-thread block on one SM, so a level of k pairs is k/128 SEQUENTIAL keccak-f1600s where the per-level launches it replaces spread them over k/128 parallel blocks. At 2048 the first four levels alone are 16+8+4+2 = 30 serial permutations against 4 parallel waves — order +100 us per large tree to save 4 launches worth order 10 us, and it sits on the critical path because the caller's 32-byte root memcpy_dtoh host-blocks on everything queued before it. At the block width the entry level is exactly one permutation per thread, so the tail still collapses the top levels into one launch but adds no serialization at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-ups for #875, rebased onto
e75bcbed— only what that commit did not already cover. Your follow-up landed while I was writing this, so I dropped everything it addressed (gather bounds, the query-0 canary, thetest-faultszero-total guard).Compile-verified (
math-cuda --tests,stark --features cuda), fmt and clippy clean. Not GPU-verified — no CUDA device here, andgpu-testsdoesn't run on PRs.1.
htod_viacan free the pinned slab under an in-flight DMAThe one memory-safety item; everything else here is polish.
Once a chunk's
cuMemcpyHtoDAsync_v2is enqueued,record_event(stream)?/sync_event()?returningErrdrops the stagingMutexGuardwith the device still reading the slab — the next locker'sensure_capacitycan thencuMemFreeHostit mid-copy.async_dtoh_viaguards exactly this (device.rs:761-764: "if the record fails, drain the stream before the guard drops, or the next locker could free the slab mid-copy"), and the file already shipsDrainOnErrfor it —htod_viawas the one site not using it. Armed before checking the memcpy result, since the driver may enqueue before reporting an error.Needs a CUDA failure mid-upload, so low probability.
2.
TAIL_MAX_PAIRS2048 → block widthThe tail grid-strides a single 128-thread block on one SM, so a level of
kpairs costsk/128sequential keccak-f1600s where the per-level launches spread them overk/128parallel blocks. Eachhash_merkle_parentabsorbs 64 B < the 136 B rate, so it's exactly one permutation per pair.At 2048 the first four levels (2048/1024/512/256) are 16+8+4+2 = 30 serial permutations against 4 parallel waves — ~26 extra serial keccak-f latencies to save 4 launches. Rough ledger per large tree: cost ~78–156 µs (at 3–6 µs per serial permutation), saving ~6–10 µs of launch overhead. It stays on the critical path because the caller's 32-byte root
memcpy_dtohhost-blocks on everything queued before it.Scale honestly: ~10–40 ms/epoch, i.e. 0.05–0.5% — this is a threshold that overshoots, not a regression that matters. At the block width the entry level is exactly one permutation per thread: still collapses the top levels into one launch, with zero added serialization. The per-permutation latency is a model, not a 5090 measurement; the direction holds for any per-keccak-f latency above ~0.4 µs, magnitude ±2×.
3. R2 host-evaluator fallback panics with a bare index-OOB
If
evaluate_devsucceeds but bothtry_decompose_extend_d2_devand theHdownload fail under device-only, control reaches the host evaluator, which reads the intentionally-empty trace. Added the device-only contract assert, matching the other fallback arms. Narrow window; improves the message only.4. Coverage: the device
n == 1inverse pathbatch_inverse_ext3_dev'sn == 1branch (a directlaunch_invert_total) is never exercised —batch_inverse_n1goes throughbatch_inverse_ext3's host-only short circuit atinverse.rs:59-64, as its own comment says. Added a direct device test; complements thehtod_viaround-trip test you added.5. Two doc comments
prover.rs:1030still said the split trees "come back as full host trees so the preprocessed opening path and the process-wide precomputed-tree cache work unchanged". Half of it survives — the precomputed tree is still a full host tree and its cache is unchanged — but the multiplicity tree is now root-only + device-resident. This is the comment a reader consults before touching theis_root_onlyassert.fri.rs:43saysFriCommitState's input is Arc-shared with the layer's retainedgpu_evals; sincegpu_evals: (!want_host).then_some(..)that holds only on the device-only path.Flagged, not patched — three failure-mode calls that are yours
I checked these and deliberately left the code alone, because each is a design choice you made with a stated rationale rather than a defect.
The R2 commit assert (
prover.rs:1669). I originally wanted this to be a rejectableProvingErroron the rejectable-beats-panic convention, and that was wrong — I checked the propagation and there is nothing to recover to.ProvingErroris flattened atcontinuation.rs:738/:931via.map_err(|e| Error::Prover(format!("{e:?}")))?and propagates straight up; no caller retries, and there is no CPU-fallback-on-ProvingErrorpath. TheEmptyCommitmentone line below is equally terminal. Panic andErrboth end the prove with no proof, so the assert costs nothing and keeps the better message. Leaving it as-is; no change wanted. (The convention it appeared to violate is really about the verifier, where a malicious proof must be rejected rather than panic the process. This is a prover-side internal invariant with no attacker-controlled input.)is_none_orin the three contract asserts passes an empty outer Vec (num_parts == 0→.first() == None→ assert passes). Unreachable — R2 hitsEmptyCommitmentfirst — and note the right predicate differs per site: at R2 an empty outer Vec should fall through toEmptyCommitment, so a blanket swap tois_some_andwould be wrong there. Hardening only.Your zero-total guard.
#[cfg(any(debug_assertions, feature = "test-faults"))]fixes the "never actually runs" problem fortest-cuda-fallback. Two things it still doesn't reach: the other four GPU suites (plain--release) and production. It also stillassert_ne!s, where the host Fermat it replaced returnedErrthattry_compute_and_invert_inv_denoms_devmaps toNone→ CPU fallback.One more, lower confidence:
prover.rs:2208's assert is now load-bearing in a way its predicate doesn't state. Its loop reads the host trace and the host parts (2255), but it's gated on!host_trace_empty()alone. Sufficient today only becausewant_host == !host_trace_empty(). Decouple those later — e.g. "keep parts device-only whenever a handle exists" — and it silently stops covering 2255.Verified rather than changed
The two risky kernel rewrites are correct, checked numerically rather than by reading:
ntt_dit_8_levels_row_majorvs the per-level reference over Goldilocks, 9(log_n, m, gridDim.y)combinations includinggridDim.ynot dividingn/256andmnot a multiple ofT— bit-identical in all.keccak_merkle_tailvs the multi-launch loop across 13 leaf counts including non-powers-of-two, with the grid-strided thread iterations executed in reverse order to expose any hidden intra-level dependency — every node buffer matched, not just the roots.Also traced and found sound: every consumer of
lde_composition_poly_evaluationsunder empty parts (each is either device-routed or assert-guarded — no path produces a silently wrong or silently empty proof); R3 parts-OOD equivalence argument-by-argument, including inv_denoms atz_powerin both arms and matchingDenomSign; per-part ordering device-vs-host; the Fermat exponent (p³−2, Hamming weight 127, no u128 overflow); the slab layout againstkeccak_comp_poly_leaves_ext3; the preprocessed/mult-tree pairing; and cross-stream ordering for every new resident consumer.Residual risk worth naming:
build_comp_poly_tree_from_slabs_devandgather_ext3_athave no parity test, and they are the only sources of the composition root and the FRI sym evals under device-only — precisely where the host cross-checks are skipped. Rust-level equivalence I did verify (the tree builder launches the identical kernel with identical args over an identically-laid-out buffer), so the residual is kernel-level only.