diff --git a/crates/ml/src/cuda_pipeline/gpu_her.rs b/crates/ml/src/cuda_pipeline/gpu_her.rs index 6877992db..c14430505 100644 --- a/crates/ml/src/cuda_pipeline/gpu_her.rs +++ b/crates/ml/src/cuda_pipeline/gpu_her.rs @@ -29,7 +29,7 @@ use std::sync::Arc; -use cudarc::driver::{CudaFunction, CudaSlice, CudaStream, DevicePtr, LaunchConfig, PushKernelArg}; +use cudarc::driver::{CudaFunction, CudaSlice, CudaStream, LaunchConfig, PushKernelArg}; use tracing::info; use crate::MLError; @@ -91,28 +91,6 @@ impl GpuHerConfig { } } -// --------------------------------------------------------------------------- -// GPU-resident relabeled batch -// --------------------------------------------------------------------------- - -/// GPU-resident HER relabeled batch -- staging buffer slices ready for -/// concatenation with the normal PER batch. -#[allow(missing_debug_implementations)] -pub struct HerBatch { - /// Relabeled states `[her_batch_size, state_dim]` -- f32 on device. - pub states: CudaSlice, - /// Relabeled next_states `[her_batch_size, state_dim]` -- f32 on device. - pub next_states: CudaSlice, - /// Actions (copied unchanged) `[her_batch_size]` -- i32 on device. - pub actions: CudaSlice, - /// Recomputed rewards `[her_batch_size]` -- f32 on device. - pub rewards: CudaSlice, - /// Dones (copied unchanged) `[her_batch_size]` -- f32 on device. - pub dones: CudaSlice, - /// Number of relabeled samples in this batch. - pub batch_size: usize, -} - // --------------------------------------------------------------------------- // GpuHer // --------------------------------------------------------------------------- @@ -120,8 +98,9 @@ pub struct HerBatch { /// GPU-accelerated Hindsight Experience Replay. /// /// Owns pre-allocated staging buffers for relabeled experiences and the compiled -/// relabel kernel. Call [`relabel_batch`] to produce a [`HerBatch`] from the -/// GPU replay buffer's tensor storage. +/// relabel kernel. Production callers use [`GpuHer::relabel_batch_with_strategy`] +/// (Future/Final, GPU-native donor selection) and +/// [`GpuHer::generate_random_donors_gpu`] (Random strategy, GPU RNG). #[allow(missing_debug_implementations)] pub struct GpuHer { /// Configuration. @@ -229,136 +208,6 @@ impl GpuHer { }) } - /// Generate a relabeled batch from GPU replay buffer storage. - /// - /// 1. Accepts pre-generated source indices (PER-weighted) and random donor - /// indices (for Random strategy). - /// 2. Uploads index arrays to GPU. - /// 3. Launches the relabel kernel. - /// 4. Returns staging buffer references as a `HerBatch`. - /// - /// # Arguments - /// - /// * `buffer_states` - `[capacity, state_dim]` f32 on device (replay buffer storage) - /// * `buffer_next_states` - `[capacity, state_dim]` f32 on device - /// * `buffer_actions` - `[capacity]` i32 on device - /// * `buffer_rewards` - `[capacity]` f32 on device - /// * `buffer_dones` - `[capacity]` f32 on device - /// * `source_idx_host` - PER-sampled source indices (CPU) - /// * `donor_idx_host` - Donor indices (CPU, strategy-dependent) - /// * `buffer_size` - Current replay buffer occupancy - /// * `her_batch_size` - Number of experiences to relabel - pub fn relabel_batch( - &mut self, - buffer_states: &CudaSlice, - buffer_next_states: &CudaSlice, - buffer_actions: &CudaSlice, - buffer_rewards: &CudaSlice, - buffer_dones: &CudaSlice, - source_idx_host: &[i32], - donor_idx_host: &[i32], - buffer_size: usize, - her_batch_size: usize, - ) -> Result { - if her_batch_size == 0 { - return Err(MLError::InvalidInput( - "HER batch size must be > 0".to_owned(), - )); - } - - let max_her = self.config.her_batch_size(); - if her_batch_size > max_her { - return Err(MLError::InvalidInput(format!( - "HER batch size {her_batch_size} exceeds pre-allocated max {max_her}" - ))); - } - - // Upload source and donor indices to GPU via mapped-pinned staging - // (per-relabel call; pinned staging is allocated+freed each call — - // acceptable on this cold path which runs once per epoch). - super::mapped_pinned::upload_i32_via_pinned( - &self.stream, - &source_idx_host[..her_batch_size], - &mut self.source_indices, - ) - .map_err(|e| MLError::ModelError(format!("HER source_indices upload via pinned: {e}")))?; - super::mapped_pinned::upload_i32_via_pinned( - &self.stream, - &donor_idx_host[..her_batch_size], - &mut self.donor_indices, - ) - .map_err(|e| MLError::ModelError(format!("HER donor_indices upload via pinned: {e}")))?; - - // Launch relabel kernel: one warp (32 threads) per sample - let launch_config = LaunchConfig { - grid_dim: (her_batch_size as u32, 1, 1), - block_dim: (32, 1, 1), - shared_mem_bytes: 0, - }; - let buffer_size_i32 = buffer_size as i32; - - // Safety: all buffer pointers are valid GPU allocations on the same context. - // source_indices and donor_indices contain valid buffer indices in [0, buffer_size). - // Output buffers are pre-allocated with sufficient capacity. - let state_dim_i32 = ml_core::state_layout::STATE_DIM as i32; - unsafe { - self.stream - .launch_builder(&self.relabel_func) - .arg(buffer_states) - .arg(buffer_next_states) - .arg(buffer_actions) - .arg(buffer_rewards) - .arg(buffer_dones) - .arg(&self.source_indices) - .arg(&self.donor_indices) - .arg(&buffer_size_i32) - .arg(&mut self.out_states) - .arg(&mut self.out_next_states) - .arg(&mut self.out_actions) - .arg(&mut self.out_rewards) - .arg(&mut self.out_dones) - .arg(&state_dim_i32) - .launch(launch_config) - .map_err(|e| MLError::ModelError(format!("HER relabel kernel launch: {e}")))?; - } - - // Return references to the staging buffers (data is on GPU, ready for concat) - // Clone the CudaSlice handles — they point to the same device memory. - // The caller will use these for D2D copies into the merged training batch. - let states_slice = slice_clone_f32(&self.stream, &self.out_states, her_batch_size * ml_core::state_layout::STATE_DIM)?; - let next_states_slice = slice_clone_f32(&self.stream, &self.out_next_states, her_batch_size * ml_core::state_layout::STATE_DIM)?; - let actions_slice = slice_clone_i32(&self.stream, &self.out_actions, her_batch_size)?; - let rewards_slice = slice_clone_f32(&self.stream, &self.out_rewards, her_batch_size)?; - let dones_slice = slice_clone_f32(&self.stream, &self.out_dones, her_batch_size)?; - - Ok(HerBatch { - states: states_slice, - next_states: next_states_slice, - actions: actions_slice, - rewards: rewards_slice, - dones: dones_slice, - batch_size: her_batch_size, - }) - } - - /// Generate random donor indices for the Random strategy. - /// - /// Each donor is a uniform random index in `[0, buffer_size)`. - /// This is done on CPU since it is O(her_batch_size) with trivial - /// per-element cost and avoids cuRAND setup. - pub fn generate_random_donors( - buffer_size: usize, - her_batch_size: usize, - ) -> Vec { - use rand::Rng; - use rand::SeedableRng; - use rand::rngs::StdRng; - let mut rng = StdRng::seed_from_u64(crate::cuda_pipeline::mix_seed(0x4E4_5678)); - let mut donors = Vec::with_capacity(her_batch_size); - donors.resize_with(her_batch_size, || rng.gen_range(0..buffer_size as i32)); - donors - } - /// GPU-native donor selection for Future and Final strategies. /// /// Dispatches to the appropriate episode-aware kernel based on the configured @@ -573,63 +422,6 @@ fn alloc_i32( }) } -/// Extract raw CUDA device pointer from a CudaSlice. -/// -/// Same pattern as `raw_device_ptr` in `gpu_dqn_trainer.rs` -- wraps the -/// `SyncOnDrop` guard in `ManuallyDrop` to skip read event recording (safe -/// on same stream, same context). -fn raw_ptr_f32(slice: &CudaSlice, stream: &CudaStream) -> u64 { - let (ptr, guard) = slice.device_ptr(stream); - let _no_drop = std::mem::ManuallyDrop::new(guard); - ptr -} - -/// Extract raw CUDA device pointer from a CudaSlice. -fn raw_ptr_i32(slice: &CudaSlice, stream: &CudaStream) -> u64 { - let (ptr, guard) = slice.device_ptr(stream); - let _no_drop = std::mem::ManuallyDrop::new(guard); - ptr -} - -/// Clone a sub-range of a CudaSlice via async D2D memcpy. -fn slice_clone_f32( - stream: &Arc, - src: &CudaSlice, - n: usize, -) -> Result, MLError> { - let dst = alloc_f32(stream, n, "her_clone_f32")?; - let num_bytes = n * std::mem::size_of::(); - let src_ptr = raw_ptr_f32(src, stream); - let dst_ptr = raw_ptr_f32(&dst, stream); - // Safety: both src and dst are valid device memory on the same context. - // n elements * 4 bytes does not exceed either allocation. - unsafe { - cudarc::driver::result::memcpy_dtod_async( - dst_ptr, src_ptr, num_bytes, stream.cu_stream(), - ).map_err(|e| MLError::ModelError(format!("HER D2D clone f32: {e}")))?; - } - Ok(dst) -} - -/// Clone a sub-range of a CudaSlice via async D2D memcpy. -fn slice_clone_i32( - stream: &Arc, - src: &CudaSlice, - n: usize, -) -> Result, MLError> { - let dst = alloc_i32(stream, n, "her_clone_i32")?; - let num_bytes = n * std::mem::size_of::(); - let src_ptr = raw_ptr_i32(src, stream); - let dst_ptr = raw_ptr_i32(&dst, stream); - // Safety: both src and dst are valid device memory on the same context. - unsafe { - cudarc::driver::result::memcpy_dtod_async( - dst_ptr, src_ptr, num_bytes, stream.cu_stream(), - ).map_err(|e| MLError::ModelError(format!("HER D2D clone i32: {e}")))?; - } - Ok(dst) -} - // --------------------------------------------------------------------------- // Strategy parsing // --------------------------------------------------------------------------- diff --git a/docs/dqn-gpu-hot-path-audit.md b/docs/dqn-gpu-hot-path-audit.md index 2e39e1632..0b4464eb2 100644 --- a/docs/dqn-gpu-hot-path-audit.md +++ b/docs/dqn-gpu-hot-path-audit.md @@ -245,3 +245,26 @@ Atom-stat block-sum reduction unchanged (warp shfl + shared-mem bank, determinis Expected wall-clock saving on this kernel: ~33% of 207ms = ~70ms across the smoke run; on production batch=8192 × 1382 calls per fold, roughly proportional. Lower-bound estimate; actual saving depends on memory bandwidth vs compute mix — at low N (smoke) the kernel may be launch-bound rather than load-bound. nsys re-profile after deployment will quantify. ABI unchanged; no Rust caller changes required. + +### Fix 20 — DtoD-via-pinned guard + orphan HER deletion (2026-05-02) +`scripts/pre-commit-hook.sh` + `crates/ml/src/cuda_pipeline/gpu_her.rs`. Two related changes installing the structural guard against the SP6 Pearl 5 IQN τ failure mode (root cause fixed at `facbf76eb` for that one site) and removing the only remaining orphan callers of the broken pattern. + +**The bug class.** The `mapped_pinned::{upload,clone_to_device}_{f32,i32}_via_pinned` helpers carry the `_via_pinned` suffix to suggest "no HtoD per `feedback_no_htod_htoh_only_mapped_pinned`", but their bodies do `MappedXBuffer::new()` (staging) + `memcpy_dtod_async()` + `stream.synchronize()`. The DtoD copy and synchronize are both forbidden inside CUDA Graph capture (`CUDA_ERROR_STREAM_CAPTURE_INVALIDATED` — exactly the failure pattern in pre-`facbf76eb` Pearl 5 smokes) and add a host stall otherwise. The "_via_pinned" name was a lie. The canonical pattern is `MappedXBuffer` field stored directly + `write_from_slice` for host writes + kernel reads via `.dev_ptr` — used by SP4 portfolio_state, SP4 grad_prev_buf_per_group, and SP6 IQN τ at `facbf76eb`. + +**Guard.** New `check_no_dtod_via_pinned` function in `pre-commit-hook.sh` rejects any staged `.rs` file that calls `upload_(f32|i32)_via_pinned` or `clone_to_device_(f32|i32)_via_pinned`, with a single exception for `mapped_pinned.rs` itself (where the helpers currently live). Per `feedback_no_hiding`: no suppression marker. The error message points at the structural fix (rewrite to `MappedXBuffer` directly). + +Also fixes a pre-existing silent skip: the `gpu-hotpath-guard.sh` invocation used `SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"` which resolved to `.git/hooks/` (the symlink's directory) instead of `scripts/` (the script's actual location), so the guard never ran. Replaced with `readlink -f "$0"` to resolve through the symlink chain. Added an explicit `else` branch that errors if the guard binary is missing — silent skip is worse than no guard. + +**Orphan deletion.** `gpu_her.rs` carried legacy `pub fn relabel_batch(...)`, `pub fn generate_random_donors(...)` (CPU version), `pub struct HerBatch`, `fn slice_clone_f32`, `fn slice_clone_i32` — confirmed zero callers via: +``` +grep -rn '\.relabel_batch(' --include='*.rs' | grep -v 'with_strategy\|_gpu\b' +grep -rn 'HerBatch\|slice_clone_f32\|slice_clone_i32' --include='*.rs' +grep -rn '\.generate_random_donors(' --include='*.rs' | grep -v '_gpu\b' +``` +Production uses `relabel_batch_with_strategy(...)` (line 384, called at `fused_training.rs:1555`) which does pure-DtoD donor selection from a pre-existing GPU buffer — no `*_via_pinned` calls. The orphan held the only `upload_i32_via_pinned` callers in the codebase (lines 279, 285). Per `feedback_no_hiding` (wire-or-delete) the right fix is delete; conversion would have been adding `MappedI32Buffer` machinery to dead code. + +Also removed the now-unused `DevicePtr` import and updated the module docstring header to reference `relabel_batch_with_strategy` + `generate_random_donors_gpu` (the surviving production paths). + +**Scope.** This commit eliminates 2 of the 47 production `*_via_pinned` call sites (the two inside the deleted orphan). The remaining 45 sites across 14 files are cold-path init (constructors, per-fold setup, hyperopt adapters, walk-forward backtest setup) — they don't break operationally because they're outside any captured graph, but they're still graph-capture-fragile and host-stalling. The guard now enforces no NEW calls; the existing 45 will be migrated in subsequent atomic per-buffer commits. After all 45 are converted, the four helpers themselves get deleted from `mapped_pinned.rs`. + +**Validation.** Smoke `smoke-test-82fjk` at the τ-buffer-fix HEAD `facbf76eb` succeeded (`phase=Succeeded progress=1/1`, 5-epoch L40S `multi_fold_convergence`). Magnitude differentiation restored (`q_full=0.462 > q_half=0.409 > q_quarter=0.350` vs baseline frozen Pascal-triangle 0.225/0.280/0.495). Eval distribution unfrozen (`eq=0.596, eh=0.404, ef=0.000` vs baseline single-action collapse). Validates that the SP5+SP6 architecture works once the τ buffer DtoD violation is gone. diff --git a/scripts/pre-commit-hook.sh b/scripts/pre-commit-hook.sh index 1d5e9469b..b2fdb862e 100755 --- a/scripts/pre-commit-hook.sh +++ b/scripts/pre-commit-hook.sh @@ -57,9 +57,15 @@ if [ -n "$STAGED_FILES" ]; then fi fi +# Resolve script directory by following the symlink to the real location. +# When invoked via .git/hooks/pre-commit symlink, $0 points at the symlink +# (.git/hooks/) — `dirname $0` would silently miss the sibling guard scripts +# in scripts/. `readlink -f` resolves through the symlink chain to the real path. +REAL_SCRIPT="$(readlink -f "$0")" +SCRIPT_DIR="$(cd "$(dirname "$REAL_SCRIPT")" && pwd)" + # GPU hot-path leak detection echo "🔎 Checking for GPU→CPU leaks in hot paths..." -SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" if [ -x "$SCRIPT_DIR/gpu-hotpath-guard.sh" ]; then if ! "$SCRIPT_DIR/gpu-hotpath-guard.sh" --staged; then echo "⛔ GPU hot-path leak detected — fix or suppress with // gpu-ok: " @@ -67,8 +73,59 @@ if [ -x "$SCRIPT_DIR/gpu-hotpath-guard.sh" ]; then fi echo " No GPU→CPU leaks in hot paths" echo "" +else + echo "❌ Pre-commit guard missing: $SCRIPT_DIR/gpu-hotpath-guard.sh not found or not executable" + exit 1 fi +# DtoD-via-pinned guard: zero use of upload_*_via_pinned or +# clone_to_device_*_via_pinned anywhere in production code. +# +# These helpers internally do memcpy_dtod_async + stream.synchronize() — the +# "via_pinned" name is a lie. They cause CUDA_ERROR_STREAM_CAPTURE_INVALIDATED +# inside any captured graph and force a host-side stall otherwise. There is no +# legitimate use case: every CPU↔GPU communication path must use +# MappedF32Buffer / MappedI32Buffer directly (cuMemHostAlloc DEVICEMAP — host +# writes via host_ptr, kernel reads dev_ptr, zero copy). +# +# Per feedback_no_hiding: no suppression marker. If you find yourself wanting +# to suppress, rewrite the call site instead. +# +# Reference: gpu_iqn_head.rs Pearl 5 τ buffers (commit facbf76eb) — converted +# from CudaSlice + upload helper to MappedF32Buffer per-branch arrays. +check_no_dtod_via_pinned() { + local staged + staged=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' || true) + if [ -z "$staged" ]; then return 0; fi + + local bad="" + while IFS= read -r f; do + [ -z "$f" ] && continue + # Only skip the helpers' definitions inside mapped_pinned.rs itself + # (where they currently live; the structural fix removes them entirely). + [[ "$f" == *"cuda_pipeline/mapped_pinned.rs" ]] && continue + + local hits + hits=$(grep -nE 'upload_(f32|i32)_via_pinned|clone_to_device_(f32|i32)_via_pinned' "$f" 2>/dev/null \ + | grep -vE '^[0-9]+:[[:space:]]*//' \ + || true) + if [ -n "$hits" ]; then + bad+="${bad:+$'\n'}$f"$'\n'"$hits" + fi + done <<< "$staged" + + if [ -n "$bad" ]; then + echo "❌ DtoD-via-pinned guard violation: upload_*_via_pinned / clone_to_device_*_via_pinned" + echo " These helpers do memcpy_dtod_async + stream.synchronize() —" + echo " graph-capture-incompatible AND host-stalling. There is no" + echo " legitimate use. Rewrite to MappedF32Buffer / MappedI32Buffer" + echo " stored directly (host_ptr writes; kernel reads dev_ptr; no copy)." + echo " No suppression marker — fix the structure." + echo "$bad" | sed 's/^/ /' + return 1 + fi +} + # DQN v2 Invariant 7 enforcement: component-adding commits must update audit docs. check_audit_doc_updates() { local staged=$(git diff --cached --name-only) @@ -122,6 +179,7 @@ check_no_isv_migrations() { check_audit_doc_updates || exit 1 check_no_todo_fixme || exit 1 check_no_isv_migrations || exit 1 +check_no_dtod_via_pinned || exit 1 echo "✅ All pre-commit checks passed!" echo ""