From aa0996f3c649196dfaf981cbf3e660470c814dbe Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Mon, 20 Apr 2026 23:59:53 +0200 Subject: [PATCH] cleanup: add 4 CPU/GPU memory-safety categories (CPURO, ROMEM, LOCKHOT, BORROW) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expanding the scanner surface per user directive "all should be addressed". Four distinct root causes for CPU-side violations around GPU-owned data — each with its own scan command, fix hierarchy, and learned-patterns entry. - CPURO: CPU reads of GPU-resident data (sizes, reductions) that force implicit sync. Cache at construction, keep stats on device. - ROMEM: `*(ptr as *mut T)` writes where ptr came from a const / read-only source. CUDA mapped-memory flags matter; cuBLAS/cuDNN workspace casts are benign FFI. - LOCKHOT: Mutex/RwLock on per-step path. High-value hits already visible: Mutex, Mutex> in network.rs (every forward pass), Arc> in dqn.rs. Never hold tokio::sync::RwLock across .await. - BORROW: shared-&T promoted to &mut T via unsafe ptr casts or UnsafeCell/RefCell. Real example shipped: gpu_replay_buffer.rs:690 changes CudaSlice → CudaSlice through raw-ptr cast. Scoreboard seeded with 10 findings. Top scores: - ROMEM-001 (25.0) - size_pinned mapped write, verify allocation flag - ROMEM-004 (25.0) - cuBLAS workspace casts, likely false-positive bulk - ROMEM-002 (15.0) - init-time pinned mapped writes - LOCKHOT-001/002/003 (8.3) - dropout + nstep_buffer locks on hot path - BORROW-001 (8.3) - CudaSlice element-type aliasing CPURO is seeded with a scan-task (CPURO-000) to populate per-site findings in iter 9 — too many Vec::len() false positives to list upfront. --- .../superpowers/cleanup/dqn-cleanup-prompt.md | 43 ++++++++++++++++++- .../cleanup/dqn-cleanup-scoreboard.md | 20 +++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/cleanup/dqn-cleanup-prompt.md b/docs/superpowers/cleanup/dqn-cleanup-prompt.md index 4ea5a7933..5fe932f84 100644 --- a/docs/superpowers/cleanup/dqn-cleanup-prompt.md +++ b/docs/superpowers/cleanup/dqn-cleanup-prompt.md @@ -65,13 +65,17 @@ status: active - [ ] [CAT-###] : `:` — S=<1-5> B=<1-5> E=<1-5> score= ``` -Where `CAT` ∈ {PINMEM, GPUSYNC, FFLAG, FALLBACK, ACCOUNT, DIMMIX, DEAD, UNWRAP, MAGIC, PDRIFT, TESTROT, VERIFY}. IDs are stable per category — do not renumber. +Where `CAT` ∈ {PINMEM, CPURO, ROMEM, LOCKHOT, BORROW, GPUSYNC, FFLAG, FALLBACK, ACCOUNT, DIMMIX, DEAD, UNWRAP, MAGIC, PDRIFT, TESTROT, VERIFY}. IDs are stable per category — do not renumber. ## 3. Categories and scan commands | ID prefix | Category | Severity | Scan | |---|---|---|---| | PINMEM | Unpinned `htod` / `dtod` transfers on training hot path — should stage through pinned host memory (`cuMemHostAlloc`) or be eliminated by moving the computation onto GPU. **Top priority.** | 5 | `rg -n '\b(memcpy_htod\|clone_htod\|htod_copy\|memcpy_dtod_sync\|dtod_copy)\b' crates/ml crates/ml-dqn services -g '!**/benches/**' -g '!**/tests/**' -g '!**/smoke_tests/**' -g '!**/examples/**'` | +| CPURO | CPU reads GPU-resident data on hot path (sizes, reductions, metrics that could live on device). A CPU-only read of GPU state forces synchronization even when no data is downloaded. | 5 | `rg -n '\.(len\|numel\|shape\|dims\|is_empty\|first\|last)\(\)' crates/ml/src/trainers crates/ml/src/cuda_pipeline crates/ml-dqn/src/{dqn,branching,gpu_replay_buffer}.rs` — then classify per call site | +| ROMEM | Write to CPU-read-only memory — `*(ptr as *mut T)` where `ptr` came from a `*const` / read-only pinned allocation / FFI handle marked const. Undefined behavior under CUDA's mapped-memory model. | 5 | `rg -n '(as \*mut\|cast_mut\(\))' crates/ml crates/ml-dqn services --type rust -g '!**/tests/**' -g '!**/benches/**'` — classify by origin of source pointer | +| LOCKHOT | `RwLock::(read\|write)` / `Mutex::lock` called on the training hot path (per-step / per-batch). Serializes the CPU side of the loop; also hides data races when the lock is held across an `await`. | 5 | `rg -n '\.(read\|write\|lock\|try_read\|try_write\|try_lock)\(\)' crates/ml/src/trainers crates/ml/src/cuda_pipeline crates/ml-dqn/src/{dqn,branching,network,gpu_replay_buffer}.rs` | +| BORROW | Shared-borrow integrity violations — `&T` promoted to `&mut T` via `as *mut` / `UnsafeCell` / `RefCell` / `transmute`, outside of documented FFI shims. | 5 | `rg -n '(UnsafeCell\|RefCell\|Cell<\|&\*.*as \*mut\|transmute.*\*const.*\*mut)' crates/ml crates/ml-dqn services --type rust -g '!**/tests/**'` | | GPUSYNC | Hidden GPU→CPU sync on hot path | 5 | `rg -n '(dtoh_sync\|sync_reclaim\|\.to_host\(\|htod_sync\|\.synchronize\(\)\|Vec::::from\()' crates/ml crates/ml-dqn services -g '!**/benches/**' -g '!**/tests/**'` | | FFLAG | `enable_*` / `use_*` / `disable_*` booleans | 5 | `rg -n '\b(enable_\|use_\|disable_)\w+\s*:\s*bool' crates/ml crates/ml-dqn services` | | FALLBACK | Silent fallback paths (`unwrap_or_default`, etc.) | 5 | `rg -n 'unwrap_or_default\|unwrap_or_else\|\.or_else\(' crates/ml crates/ml-dqn services` | @@ -238,4 +242,41 @@ Pageable-host → device memcpies bounce through an OS staging buffer (one extra 3. **For `dtod_copy` / `memcpy_dtod_sync`**: switch to `memcpy_dtod_async` on the compute stream. Sync variants serialize the GPU. - **Commit-per-site**: each hot-path htod gets its own `cleanup(pinmem):` commit so regressions can be bisected. +### CPU reads of GPU-resident data on hot path (CPURO) +Even when no data crosses the PCIe bus, a CPU-side `.len()` / `.shape()` / `.numel()` / `.is_empty()` call on a GPU buffer forces the host thread to synchronize on the cudarc handle, which stalls the kernel launch pipeline. Symptoms are subtler than GPUSYNC (no `to_host`) but the stall is real. + +- **Signal**: `gpu_tensor.len()`, `slice.numel()`, `buf.shape()[0]` inside a per-step function. +- **Fix hierarchy**: + 1. **Cache at construction** — store `batch_size` / `n_actions` / `state_dim` as plain `usize` fields on the trainer, populated once in `new()`. Stop re-reading them on each step. + 2. **Keep running statistics on GPU** — reductions (mean/max/sum) via `ReductionKernels::stats()` (already wired for Q-value monitoring) instead of `.iter().sum()`. + 3. **Use event polling** — if the CPU genuinely needs to observe a GPU counter, use `CudaEvent::record()` + `event.query()` (non-blocking) or pinned device-mapped memory (already used for `size_pinned` in gpu_replay_buffer). + +### Writes to CPU-read-only memory (ROMEM) +CUDA's pinned / mapped memory model designates some host buffers as read-only from the CPU side (GPU writes, CPU reads). Casting a `*const` to `*mut` and writing through it is undefined behavior, even if tests happen to pass. FFI shims that wrap `cublas_sys` / `cudnn_sys` APIs legitimately take `*mut` void ptrs for workspaces — those are fine, but flag them so they stay documented. + +- **Signal**: `*(ptr as *mut T) = value` where `ptr` is declared `*const`, comes from `cuMemHostAllocReadOnly`, or was given to `stream.memcpy_dtoh_sync()` as destination. +- **Benign exceptions**: cuBLAS / cuDNN / cutlass workspace pointers (the C APIs take `*mut void` purely for calling convention, not because they write). +- **Fix**: either change the allocation flag to include `cuMemHostAllocMapped` / `cuMemHostAllocWriteCombined`, or split into two buffers (one RO for CPU reads, one WO for GPU writes). Never silently upgrade `*const` to `*mut`. + +### Lock contention on the training hot path (LOCKHOT) +`RwLock::read()` and `Mutex::lock()` on the per-step code path serialize the CPU side of training and can hide deadlocks behind `.await` points when using `tokio::sync::RwLock`. + +- **Signal**: `self.thing.lock()` or `self.thing.read().await` inside the training inner loop, forward pass, or replay-buffer insert path. +- **High-value targets** in this codebase: `std::sync::Mutex` and `Mutex>` in `network.rs` (fired every forward pass), `Arc>` in `dqn.rs` (fired per step), `Arc>` in `constructor.rs` (read frequency unclear — verify). +- **Fix hierarchy**: + 1. **Remove the lock** — if the wrapped state is only touched from one thread, the `Mutex` is cargo-culted. Move to plain `&mut self`. + 2. **Move the lock outside the hot loop** — acquire once per epoch, not per step. + 3. **Use lock-free primitives** — `AtomicU64`, `AtomicF32` (via bit-cast), or single-producer-single-consumer channels. + 4. **Never hold `tokio::sync::RwLock` across `.await`** — take a snapshot and drop the guard before yielding. + +### Shared-borrow integrity violations (BORROW) +Rust's `&T` is a shared read; promoting it to `&mut T` via `as *mut`, `transmute`, or interior mutability (`UnsafeCell`, `RefCell`, `Cell<*mut ...>`) breaks the aliasing contract the optimizer relies on. Miri / ThreadSanitizer will catch it; clippy may not. + +- **Signal (regex)**: `let dst = &mut *(&mut self.field as *mut X as *mut Y);` — transmute-via-ptr-cast to change the element type of a `&mut` reference. +- **Real example (already shipped)**: `gpu_replay_buffer.rs:690` — `&mut *(&mut self.sample_episode_ids as *mut CudaSlice as *mut CudaSlice)`. This changes the element type from i32 to u32 through a `&mut`, which is UB under strict aliasing even though the underlying bytes are identical. +- **Fix hierarchy**: + 1. **Use a type-erased buffer** — store as `CudaSlice` and re-interpret via a typed view function that returns `&[i32]` or `&[u32]` explicitly. + 2. **Transmute the slice reference, not through `&mut *`** — `std::slice::from_raw_parts_mut(ptr as *mut u32, n)` is more explicit (still unsafe but at least the reader knows). + 3. **Change the declared type** — if the buffer is always read as u32, declare it as `CudaSlice`; convert at write time, not read time. + Begin with Step 1. diff --git a/docs/superpowers/cleanup/dqn-cleanup-scoreboard.md b/docs/superpowers/cleanup/dqn-cleanup-scoreboard.md index 8fdae0d86..a36cc256a 100644 --- a/docs/superpowers/cleanup/dqn-cleanup-scoreboard.md +++ b/docs/superpowers/cleanup/dqn-cleanup-scoreboard.md @@ -47,6 +47,26 @@ Target: every per-step `memcpy_htod` / `clone_htod` / `dtod_copy` on the trainin - [ ] [PINMEM-009] `crates/ml/src/cuda_pipeline/gpu_ppo_collector.rs:509,800` — `memcpy_htod(&episode_starts / &rng_seed_staging)`. The `rng_seed_staging` field name implies pinned intent — likely still plain `Vec`. — S=5 B=5 E=3 score=8.3 - [ ] [PINMEM-010] `crates/ml/src/cuda_pipeline/gpu_ppo_collector.rs:361` — `memcpy_htod(&rng_seeds, &mut rng_states)` at init. May be constructor-only (benign). Verify. — S=5 B=1 E=1 score=5.0 +### LOCKHOT (severity 5 — lock contention on training hot path) +- [ ] [LOCKHOT-001] `std::sync::Mutex` — `crates/ml-dqn/src/network.rs:116`. Locked every forward pass. Move to `&mut self` if single-threaded, else lock-free. — S=5 B=5 E=3 score=8.3 +- [ ] [LOCKHOT-002] `Mutex>` — `crates/ml-dqn/src/network.rs:120`. Same forward-pass frequency. — S=5 B=5 E=3 score=8.3 +- [ ] [LOCKHOT-003] `Arc>` — `crates/ml-dqn/src/dqn.rs:904`. Locked every training step on experience insert. — S=5 B=5 E=3 score=8.3 +- [ ] [LOCKHOT-004] `Arc>` — `crates/ml/src/trainers/dqn/trainer/constructor.rs:550`. Verify read frequency; likely hot. — S=5 B=3 E=3 score=5.0 +- [ ] [LOCKHOT-005] `Arc>` — `crates/ml/src/trainers/dqn/trainer/constructor.rs:358`. Read on every trade update. — S=5 B=3 E=3 score=5.0 + +### ROMEM (severity 5 — write to CPU-read-only memory) +- [ ] [ROMEM-001] `*(self.size_pinned as *mut i32)` — `crates/ml-dqn/src/gpu_replay_buffer.rs:349,501`. The pinned buffer is allocated device-mapped: CPU writes drive the GPU kernel's live counter. Verify allocation flag matches (cuMemHostAllocMapped, not ReadOnly). Document invariant. — S=5 B=5 E=1 score=25.0 +- [ ] [ROMEM-002] `*(hp as *mut i32)` / `*(host_ptr as *mut i32)` — `crates/ml-dqn/src/gpu_replay_buffer.rs:287,297` in init. Same pinned-mapped pattern; verify. — S=5 B=3 E=1 score=15.0 +- [ ] [ROMEM-003] `*mut CudaSlice` cast from `*mut CudaSlice` — `crates/ml-dqn/src/gpu_replay_buffer.rs:690`. Cross-lists with BORROW-001 (aliasing via ptr cast changes element type). See BORROW for real fix. — S=5 B=5 E=3 score=8.3 +- [ ] [ROMEM-004] cuBLAS workspace `*mut c_void` casts — `crates/ml/src/cuda_pipeline/shared_cublas_handle.rs:155,210`, `gpu_curiosity_trainer.rs:94,117,238,241,347`. **Likely benign** (FFI calling convention). Classify as false-positive after verifying cublas_sys doesn't write, then bulk-resolve. — S=5 B=5 E=1 score=25.0 (reclassify) + +### BORROW (severity 5 — shared-borrow integrity violations) +- [ ] [BORROW-001] `&mut *(&mut self.sample_episode_ids as *mut CudaSlice as *mut CudaSlice)` — `crates/ml-dqn/src/gpu_replay_buffer.rs:690`. Changes element type of `&mut` reference through raw-ptr casts — UB under strict aliasing. Fix: declare `CudaSlice` directly or expose typed view API. — S=5 B=5 E=3 score=8.3 + +### CPURO (severity 5 — CPU reads of GPU-resident data on hot path) +*Needs per-site classification — many calls are on CPU Vec (benign). Initial bulk scan recommended iter 9.* +- [ ] [CPURO-000] Run initial scan: `rg -n '\.(len|numel|shape|dims|is_empty|first|last)\(\)' crates/ml/src/trainers crates/ml/src/cuda_pipeline crates/ml-dqn/src/{dqn,branching,network,gpu_replay_buffer}.rs` — classify each hit as hot-path-GPU vs CPU-Vec. Populate CPURO-001+ accordingly. — S=5 B=3 E=3 score=5.0 + ### GPUSYNC (severity 5 — `feedback_gpu_cpu_roundtrip.md`) *Many `.to_host()` calls found — need per-site classification (test vs hot path vs diagnostic). Sampling:* - [ ] [GPUSYNC-001] attention forward: `crates/ml-dqn/src/attention.rs:227,228,229,319` — S=5 B=5 E=3 score=8.3