Repository navigation
Fix H2D staging race with host function callback on all GPUs - #13275
Conversation
On unified-memory APUs (e.g. gfx1151 Strix Halo), hsaCopyStagedOrPinned() uses a staging buffer for H2D copies: CPU memcpy from hostSrc into a staging buffer, then SDMA from staging to device memory. The existing code unconditionally skipped the CPU wait for all H2D copies (releaseGpuMemoryFence(true)), which is safe on discrete GPUs where the pinning path is used and GPU barriers ensure ordering. However, on APUs with hostUnifiedMemory_=true, the staging path's CPU memcpy reads from hostSrc before any pending host-function callback (hipLaunchHostFunc) has finished writing to it. This causes a race condition where the memcpy copies stale data (zeros instead of the values the host function was supposed to fill). Fix: Only skip the CPU wait for H2D when the device does NOT have unified memory (i.e. discrete GPUs using the pinning path). On unified-memory APUs, the CPU wait ensures all prior operations — including host function callbacks — complete before the staging memcpy reads the source buffer. Fixes: SWDEV-542012 Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
🟡 Changes recommended
Discrete-GPU staging paths still retain the same race for small transfers or pinning failures.
1 open finding
What changed in this PR
Updates H2D synchronization to prevent host-callback staging races on unified-memory APUs.
Changes:
- Waits for preceding stream work before staged APU H2D copies.
- Preserves skipped waits for discrete GPUs.
| File | Description |
|---|---|
projects/clr/rocclr/device/rocm/rocblit.cpp |
Adjusts H2D fence-wait selection. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The original fix only waited for host callback completion on unified-memory APUs (hostUnifiedMemory_=true). Testing on discrete GPUs (MI300X on Linux, RX 9070 XT on Windows with ROCR backend) showed the same race: hipLaunchHostFunc callback writes to hostSrc while the staging path's CPU memcpy reads stale data (0/100 pass rate on MI300X, 12/100 on RX 9070 XT). Changes: - hsaCopyStagedOrPinned: wait for H2D only in the staging path (pinnedMem_==nullptr), not unconditionally. The pinned/SDMA path is ordered by GPU signal dependencies and does not need a CPU wait. - KernelBlitManager::writeBuffer: add releaseGpuMemoryFence(false) before the shader-blit staging loop — this path was completely unprotected and is hit for small copies (< sdmaCopyThreshold_). Verified 100/100 pass on both paths (4KB kernel-blit, 1MB DMA-blit) on RX 9070 XT (Windows, ROCR) and MI300X (Linux). Fixes: SWDEV-542012 Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
(#13275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Fix race condition where H2D staging copies read from `hostSrc` (via CPU `memcpy`) before a pending `hipLaunchHostFunc` callback has finished writing to it - Affects **all GPUs** using the ROCR backend — both discrete (MI300X, RX 9070 XT) and unified-memory APUs (Strix Halo) - Two staging paths are fixed: - `hsaCopyStagedOrPinned` (DMA blit) — used for large copies (> `sdmaCopyThreshold_`) - `KernelBlitManager::writeBuffer` (shader blit) — used for small copies, was completely unprotected JIRA ID: SWDEV-542012 ## Root Cause The graph captures: `MallocAsync → HostFunc(fill h_array) → H2D → Kernel → D2H → FreeAsync` During `hipGraphLaunch`, the host function runs asynchronously via a signal handler thread. Both H2D staging paths do a CPU `memcpy(stagingBuffer, hostSrc, ...)` without waiting for the host callback to complete, racing with it and copying stale data. The pinned/SDMA path does not have this race because the DMA engine's read from `hostSrc` is ordered by GPU signal dependencies. ## Fix 1. **hsaCopyStagedOrPinned**: Add `releaseGpuMemoryFence(false)` in the H2D staging path (when `pinnedMem_ == nullptr`) before the CPU `memcpy`. The pinned path is not affected — SDMA reads are ordered by the signal chain. 2. **KernelBlitManager::writeBuffer**: Add `releaseGpuMemoryFence(false)` before the shader-blit staging loop. This path was completely missing the wait. ## Test Results | GPU | OS | Backend | Path | Before | After | |-----|----|---------|----- |--------|-------| | MI300X | Linux | ROCR | Kernel blit (4KB) | 0/100 | 100/100 | | MI300X | Linux | ROCR | DMA blit (1MB) | 0/100 | 100/100 | | RX 9070 XT | Windows | ROCR | Kernel blit (4KB) | 12/100 | 100/100 | | RX 9070 XT | Windows | PAL | Kernel blit (4KB) | 100/100 | N/A (PAL unaffected) | ## Test Plan - [x] hipLaunchHostFunc -> hipMemcpyAsync H2D repro passes on MI300X (Linux, ROCR) - [x] hipLaunchHostFunc -> hipMemcpyAsync H2D repro passes on RX 9070 XT (Windows, ROCR) - [x] PAL backend unaffected (Windows, RX 9070 XT) — 100/100 without fix - [ ] No regression on discrete GPUs — skip-wait optimization preserved for pinned path - [ ] Unit_hipLaunchHostFunc_H2D_Kernel_D2H_Capture passes on gfx1151 (Strix Halo APU) 🤖 Generated with [Claude Code](https://claude.com/claude-code) [rocm-systems] ROCm/rocm-systems#13275 (commit 8a45f2c)
## Summary - Fix race condition where H2D staging copies read from `hostSrc` (via CPU `memcpy`) before a pending `hipLaunchHostFunc` callback has finished writing to it - Affects **all GPUs** using the ROCR backend — both discrete (MI300X, RX 9070 XT) and unified-memory APUs (Strix Halo) - Two staging paths are fixed: - `hsaCopyStagedOrPinned` (DMA blit) — used for large copies (> `sdmaCopyThreshold_`) - `KernelBlitManager::writeBuffer` (shader blit) — used for small copies, was completely unprotected JIRA ID: SWDEV-542012 ## Root Cause The graph captures: `MallocAsync → HostFunc(fill h_array) → H2D → Kernel → D2H → FreeAsync` During `hipGraphLaunch`, the host function runs asynchronously via a signal handler thread. Both H2D staging paths do a CPU `memcpy(stagingBuffer, hostSrc, ...)` without waiting for the host callback to complete, racing with it and copying stale data. The pinned/SDMA path does not have this race because the DMA engine's read from `hostSrc` is ordered by GPU signal dependencies. ## Fix 1. **hsaCopyStagedOrPinned**: Add `releaseGpuMemoryFence(false)` in the H2D staging path (when `pinnedMem_ == nullptr`) before the CPU `memcpy`. The pinned path is not affected — SDMA reads are ordered by the signal chain. 2. **KernelBlitManager::writeBuffer**: Add `releaseGpuMemoryFence(false)` before the shader-blit staging loop. This path was completely missing the wait. ## Test Results | GPU | OS | Backend | Path | Before | After | |-----|----|---------|----- |--------|-------| | MI300X | Linux | ROCR | Kernel blit (4KB) | 0/100 | 100/100 | | MI300X | Linux | ROCR | DMA blit (1MB) | 0/100 | 100/100 | | RX 9070 XT | Windows | ROCR | Kernel blit (4KB) | 12/100 | 100/100 | | RX 9070 XT | Windows | PAL | Kernel blit (4KB) | 100/100 | N/A (PAL unaffected) | ## Test Plan - [x] hipLaunchHostFunc -> hipMemcpyAsync H2D repro passes on MI300X (Linux, ROCR) - [x] hipLaunchHostFunc -> hipMemcpyAsync H2D repro passes on RX 9070 XT (Windows, ROCR) - [x] PAL backend unaffected (Windows, RX 9070 XT) — 100/100 without fix - [ ] No regression on discrete GPUs — skip-wait optimization preserved for pinned path - [ ] Unit_hipLaunchHostFunc_H2D_Kernel_D2H_Capture passes on gfx1151 (Strix Halo APU) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
TheRock Submodule Bump ActivityNewest first
|

Summary
hostSrc(via CPUmemcpy) before a pendinghipLaunchHostFunccallback has finished writing to ithsaCopyStagedOrPinned(DMA blit) — used for large copies (>sdmaCopyThreshold_)KernelBlitManager::writeBuffer(shader blit) — used for small copies, was completely unprotectedJIRA ID: SWDEV-542012
Root Cause
The graph captures:
MallocAsync → HostFunc(fill h_array) → H2D → Kernel → D2H → FreeAsyncDuring
hipGraphLaunch, the host function runs asynchronously via a signal handler thread. Both H2D staging paths do a CPUmemcpy(stagingBuffer, hostSrc, ...)without waiting for the host callback to complete, racing with it and copying stale data.The pinned/SDMA path does not have this race because the DMA engine's read from
hostSrcis ordered by GPU signal dependencies.Fix
releaseGpuMemoryFence(false)in the H2D staging path (whenpinnedMem_ == nullptr) before the CPUmemcpy. The pinned path is not affected — SDMA reads are ordered by the signal chain.releaseGpuMemoryFence(false)before the shader-blit staging loop. This path was completely missing the wait.Test Results
Test Plan
🤖 Generated with Claude Code