gitadmin/foxhunt
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

cleanup: add 4 CPU/GPU memory-safety categories (CPURO, ROMEM, LOCKHOT, BORROW)

Browse Source 
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<GpuDropout>, Mutex<Option<DropoutScheduler>> in
  network.rs (every forward pass), Arc<Mutex<NStepBuffer>> 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<i32> → CudaSlice<u32> 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.
This commit is contained in:
jgrusewski
2026-04-20 23:59:53 +02:00
parent f1c9f6025e
commit aa0996f3c6
2 changed files with 62 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
docs/superpowers/cleanup/dqn-cleanup-prompt.md
View File

@@ -65,13 +65,17 @@ status: active
- [ ] [CAT-###] <short description>: `<file>:<line>` — S=<1-5> B=<1-5> E=<1-5> score=<float>
```
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::<f[0-9]+>::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<GpuDropout>` and `Mutex<Option<DropoutScheduler>>` in `network.rs` (fired every forward pass), `Arc<Mutex<NStepBuffer>>` in `dqn.rs` (fired per step), `Arc<RwLock<TrainingMetrics>>` 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<i32> as *mut CudaSlice<u32>)`. 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<u8>` 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<u32>`; convert at write time, not read time.
Begin with Step 1.

20
docs/superpowers/cleanup/dqn-cleanup-scoreboard.md
View File

@@ -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<GpuDropout>` — `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<Option<DropoutScheduler>>` — `crates/ml-dqn/src/network.rs:120`. Same forward-pass frequency. — S=5 B=5 E=3 score=8.3
- [ ] [LOCKHOT-003] `Arc<Mutex<NStepBuffer>>` — `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<RwLock<TrainingMetrics>>` — `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<RwLock<TripleBarrierEngine>>` — `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<u32>` cast from `*mut CudaSlice<i32>` — `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<i32> as *mut CudaSlice<u32>)` — `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<u32>` 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