Repository navigation
feat(clr): add RetiringSignalTracker for device-memory queue signals (part 2) - #13507
Conversation
42e5b1d to
157f83b
Compare
bbb9365 to
ebbfd62
Compare
6db9921 to
e43445a
Compare
ebbfd62 to
1cea10e
Compare
e43445a to
ec68634
Compare
1cea10e to
6a9700d
Compare
ec68634 to
c3699a5
Compare
6a9700d to
70e190c
Compare
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
aff9968 to
f1dd231
Compare
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
d4fce9a to
73c0dc0
Compare
mangupta
left a comment
mangupta
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @mangupta)
[SUGGESTION] Turning DEBUG_CLR_DEV_SIGNALS off does not restore the previous signal behavior
DEBUG_CLR_DEV_SIGNALS defaults to off, but several behavior changes apply to every queue regardless of the flag, including OpenCL queues:
- HwQueueTracker::Create and ManagedBuffer::Create now go through batched Device::CreateSignals (batched=true) instead of per-signal hsa_signal_create.
- ManagedBuffer chunk signals no longer pass the consumer agent list (previously
1, &agent). - hipEventSynchronize completes the submission batch and can run command callbacks on the syncing thread.
- WaitForSignal's pre-check is removed.
- CompleteCpuWait's completion check is changed.
- dispatchBarrierValuePacket reserves its AQL slot before ActiveSignal.
So a regression in these paths cannot be ruled out by setting DEBUG_CLR_DEV_SIGNALS=0, and reverting the device-signal feature alone would not revert them.
Suggested fix: Either gate the batched allocation (and the dropped consumer agent) on QueueDeviceSignals(), or split the flag-independent changes into a separate PR so they can be reviewed and reverted on their own. Also document in the PR description that DEBUG_CLR_DEV_SIGNALS=0 does not restore the previous behavior.
73c0dc0 to
f6162cc
Compare
|
Re: "Turning DEBUG_CLR_DEV_SIGNALS off does not restore the previous signal behavior". Thanks, addressed:
|
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
f6162cc to
8d2162f
Compare
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
850c380 to
7366955
Compare
gandryey
left a comment
gandryey
left a comment
There was a problem hiding this comment.
Read-index retirement with the barrier-bit proof and batched re-arm look like the right direction, and the flag-gated rollout makes sense. Before approving I'd like to settle a few things, mostly about what changes when the flag is off.
- Flag-off path
These changes apply regardless of DEBUG_CLR_DEV_SIGNALS:
- the ActiveSignal / slot changes in dispatchBarrierValuePacket;
- skipping the value check in CacheTimingData;
- removing the WaitForSignal pre-check;
- hipEventSynchronize completing the submission batch.
Please add base vs PR numbers with DEBUG_CLR_DEV_SIGNALS=0 for the hip-tests EventPerformance benchmarks (HIP_TEST_LEVEL=level_1): Performance_hipEventQuery, Performance_hipEventSynchronize (default and blocking), Performance_hipEventElapsedTime and
Performance_hipEventRecord, plus an empty-kernel launch + hipStreamSynchronize round trip. Please also run the full hip-tests suites with the flag off, not only EventTest/StreamTest/GraphsTest1/2. If these shared changes aren't needed for the flag-off path, consider
moving them to a separate PR.
- Event API regressions
This is the table I'm looking at:
┌───────────────────────────────┬──────────┬─────────────────┐
│ Benchmark │ Base │ Current patch │
├───────────────────────────────┼──────────┼─────────────────┤
│ hipEventQuery │ 0.545 µs │ 1.469 µs (2.7×) │
├───────────────────────────────┼──────────┼─────────────────┤
│ hipEventSynchronize, default │ 6.73 µs │ 9.69 µs (1.44×) │
├───────────────────────────────┼──────────┼─────────────────┤
│ hipEventSynchronize, blocking │ 6.72 µs │ 9.69 µs (1.44×) │
├───────────────────────────────┼──────────┼─────────────────┤
│ hipEventElapsedTime │ 2.22 µs │ 8.05 µs (3.6×) │
└───────────────────────────────┴──────────┴─────────────────┘
Was this measured with the flag on or off? If off, these are regressions on the default path. If on, they're expected from CPU reads of device memory (~1 µs per PCIe read), and I'd suggest keeping event signals and timestamps in host memory, as the PR already does for
timing-only uses. Events are what the CPU queries and syncs on; the device-memory win is for GPU-side dependency waits.
- Blocking-sync events
The description reports hipEventSynchronize on a blocking-sync event dropping from 10.9 to 4.7 µs, the same as the default event. A blocking-sync wait sleeps on an interrupt, so it shouldn't match active polling. With the flag on, does a hipEventBlockingSync event
still sleep, or does it now spin? If it spins, that changes what the flag promises applications.
- Multi-GPU and IPC with the flag on
With completion and event signals in VRAM:
- A cross-device hipStreamWaitEvent has the peer GPU read the owner's VRAM. Over XGMI that bypasses the owner's HDP, so a CPU re-arm isn't guaranteed visible to the peer. PCIe P2P goes through the owner's BAR/HDP and is probably fine.
- IPC events are read by other processes.
The gating mentions large BAR and no profiler only. Please either gate the flag to a single GPU / non-IPC, or describe how cross-device and IPC waits are handled.
- Hardware coverage and CPU-wait cost
All data is MI300X. Please note which hardware the flag is validated on (Radeon/PCIe large-BAR parts behave differently with device-memory rings). Since CPU waits poll device memory, please add flag-on numbers for an empty-kernel round trip and for hipStreamSynchronize
on already-completed work. Each poll is a PCIe read, so these are where it costs.
- The parked-signal claim
For reference: on a ROCm 10.1 nightly (2026-08-26) on MI300X, the record → kernel → record → hipEventSynchronize loop does create ~3700 extra signals over 2000 iterations. But it costs only ~0.2 µs per iteration more than the same loop with hipStreamSynchronize (12.6
vs 12.4 µs average), with no visible outliers. Could you share the build and command that show the 75–575 µs outliers, so the improvement can be reproduced?
Summary: approval is fine if the flag-off numbers match base. Points 2–4 should be resolved, or written down as known limitations, before the flag is enabled anywhere.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…(part 2) Part 2 of a series moving CLR completion signals to the ROCr batch API and device memory. This part covers the queue completion signals. DEBUG_CLR_DEV_SIGNALS (default off) places each queue's completion signals in device memory when the device has a large BAR and no profiler tool is attached, the same condition as the graph signals, in the same pool (uncached on gfx94x/gfx95x, coarse-grained on gfx125x). Every host access to those signals then crosses PCIe, so VirtualGPU uses a new RetiringSignalTracker, which proves that a signal's last use finished from host-side state instead of reading the signal, and re-arms signals ahead of use in batches. Otherwise VirtualGPU keeps LegacySignalTracker. Device-memory signals, graph and queue, are HIP-only: OpenCL keeps host signals and LegacySignalTracker. RetiringSignalTracker: - Retirement. Each use records its AQL slot, whether the packet had the barrier bit, and the HW queue generation. A use published with the barrier bit is a fence: its completion proves that every earlier packet on the HW queue completed. The packet processor may advance the read index before a packet completes (HSA System Architecture 2.8.3), so the read index only nominates the newest fence it passed, and one load of that fence's signal confirms it. Reading the newest fence in a batch (ProbeFence) likewise retires the whole batch with one load. A HW queue change (NoteQueueChange) starts a new generation. - Arming. Reclaim walks the ring from the oldest use, re-arms retired slots with direct stores and one HDP flush per 32 slots, and stops at the first busy one. A slot whose signal is still shared (an event, a GPU wait or a pending handler) gets an armed spare instead; the old signal is parked and reused once the tracker holds the last reference. A ring about to reuse a busy slot grows, up to 16 x ROC_SIGNAL_POOL_SIZE, before waiting on the oldest signal. - Timing. A use's timing is collected before its signal is re-armed, so no Timestamp lists an armed signal. A command whose signals wrap the ring, such as a graph launch under the built-in profiler, would otherwise have its async handler wait on a re-armed signal while holding the Timestamp the next use needs, and read the next use's timestamps. - Timing-only uses. A dispatch that takes a signal only because the built-in profiler times it gets an armed host-memory signal from a separate pool (PlaceForUse), so the handler reads its value and timestamps without crossing PCIe. A use that a GPU waiter or a sync may read swaps a device-memory signal back into the slot. A slot keeps its signal when the replacement's allocation fell back to host memory. - GPU waits. Signals returned by WaitingSignal for a GPU waiter are pinned with a reference until the consuming packets have passed and every earlier use has retired, so re-arming cannot hide the old use from the waiter. A drained queue (NoteQueueDrained) releases them. - Store ordering. Host stores to device-memory signals are write-combined. FlushStores fences them, so a peer GPU that waits on a signal over XGMI reads the re-arm, and flushes HDP only when the AQL rings are in system memory: a device-memory ring receives the packets that use the signals through the same BAR after the stores, in order. A signal handed to a ROCr copy is flushed when its use begins, since ROCr's SDMA and blit rings are in system memory. - The async handler holds a reference to the signal until it has processed it, and drops it before the VirtualGPU reference and inside the async-handler scope, so neither the signal nor the HW queue is destroyed on the handler thread outside the deferral path. With DEBUG_CLR_DEV_SIGNALS only. With the flag off, every queue and ManagedBuffer signal is created with hsa_signal_create and a direct-dispatch queue batches DEBUG_CLR_MAX_BATCH_SIZE commands, as before: - HwQueueTracker::Create builds the completion-signal ring (64) and the interrupt pool (5) as two device-memory batches. - The kernarg and staging ManagedBuffer chunk signals are a device-memory batch too. A chunk signal there is re-armed with a direct store, flushed only when the AQL ring is in system memory, since a store through ROCr reads it back across PCIe. - A direct-dispatch queue completes its submission batch after 64 commands instead of DEBUG_CLR_MAX_BATCH_SIZE (1000), unless that flag is set (Device::SubmissionBatchLimit). A hipEventRecord marker doesn't own a batch, so in a record/sync loop every command keeps its signal referenced until the batch completes. The tracker parks those signals, and refills its pools with device allocations inside the timed intervals. - WaitForSignal skips its check before waiting for a device-memory signal, since the wait loads the value first and each load is a PCIe read. Host-memory signals keep the check. Shared changes. These apply with DEBUG_CLR_DEV_SIGNALS off too: - ActiveSignal takes the AQL slot and barrier bit of the packet the signal completes; dispatchBarrierValuePacket reserves its slot before taking the signal. - Timing of a use known to be complete (after a CPU wait, or proven retired) skips the signal value check in CacheTimingData. - A ROCr submission that fails after issuing some of the sub-operations sharing one signal (DMA copyBufferRect line by line, SVM prefetch batch) waits for the issued ones (WaitForIssuedOps) before resetting the slot, with either tracker: their late decrements would otherwise complete the slot's next use early. - A blocking WaitForSignal returns as soon as its 100 us active phase sees completion, instead of also running the blocked wait. - The small-batch sync path (#13532) reads the marker's signal once: CompleteCpuWait checks the GPU-error state directly instead of reloading the signal, and HostQueue::finish skips the HW-event wait for a marker that enqueue already completed. Known limitations with DEBUG_CLR_DEV_SIGNALS=1: - The event APIs read the event's signal and timestamps across PCIe, about 1.1 us per read (EventPerformance table below). The gain is in waits: completion lands in device memory, and GPU waiters poll local memory. - hipEventBlockingSync events spin, as they do with host signals: ROCr's busy-wait signals ignore the wait hint. With the flag on, each poll is a PCIe read. - Validated on MI300X only (PCIe to the host, XGMI between GPUs). IPC events are unaffected: they complete through a separate ROCr IPC signal. Test (MI300X, container_saleel_1, image rocm/ufb-private:vllm-0.26.0-ubuntu24.04-py3.14-nightlies-device-all-cdna-rocm10.1.0a20260826-568afb3a13, ROCm SDK 10.1.0a20260826, ROCr built from this PR's develop base). Host time per call in us, median of 2000: | API | develop | DEBUG_CLR_DEV_SIGNALS=0 | DEBUG_CLR_DEV_SIGNALS=1 | |---|---|---|---| | hipEventSynchronize (empty kernel, record, sync) | 5.3 | 5.3 | 4.5-4.7 | | hipEventSynchronize, blocking-sync event | 5.4 | 5.3 | 4.6-4.7 | | hipEventQuery (completed event) | 0.08 | 0.08 | 1.2 | | hipStreamSynchronize (empty kernel, sync) | 6.5 | 6.5 | 6.3-6.4 | | hipStreamSynchronize (completed work) | 0.14 | 0.14 | 0.14 | | hipDeviceSynchronize (empty kernel, sync) | 6.5 | 6.5 | 6.3-6.5 | hip-tests EventPerformance (HIP_TEST_LEVEL=level_1), average in us. Each timed call includes the harness's hipStreamSynchronize: | Benchmark | develop | DEBUG_CLR_DEV_SIGNALS=0 | DEBUG_CLR_DEV_SIGNALS=1 | |---|---|---|---| | Performance_hipEventRecord | 6.6 | 6.6 | 8.2-8.5 | | Performance_hipEventSynchronize, default | 6.1-6.4 | 6.3 | 8.5-8.9 | | Performance_hipEventSynchronize, blocking | 6.2 | 6.1 | 8.5-8.8 | | Performance_hipEventElapsedTime | 1.4 | 1.4 | 16.6-17.2 | | Performance_hipEventQuery | 0.78 | 0.78 | 5.0-5.2 | - hip_launch_latency -k -g --verify passes with the flag off and on, with and without GPU_CLR_PROFILE_OUTPUT. - hipStreamWaitEvent on GPU1 for a GPU0 event recorded behind a spinning kernel never passed early (0 of 300 rounds), with the flag off and on. - hip-tests EventTest, StreamTest, GraphsTest1, GraphsTest2: failure sets identical to develop with the flag off and on. - rocvirtual.cpp, rocdevice.cpp and rocblit.cpp compile with ROCR_DYN_DLL defined. JIRA ID : AIRUNTIME-2943
7366955 to
9a50284
Compare
|
Thanks German. Updated the PR; numbers are in the description (MI300X, ROCm 10.1 nightly 2026-08-26, ROCr built from this PR's develop base).
|
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
|
Ignoring MSI failure and bypassing to merge. |
TheRock Submodule Bump ActivityNewest first
|
Part 2 of a series moving CLR completion signals to the ROCr batch API
and device memory. This part covers the queue completion signals.
DEBUG_CLR_DEV_SIGNALS (default off) places each queue's completion
signals in device memory when the device has a large BAR and no
profiler tool is attached, the same condition as the graph signals, in
the same pool (uncached on gfx94x/gfx95x, coarse-grained on gfx125x).
Every host access to those signals then crosses PCIe, so VirtualGPU
uses a new RetiringSignalTracker, which proves that a signal's last use
finished from host-side state instead of reading the signal, and
re-arms signals ahead of use in batches. Otherwise VirtualGPU keeps
LegacySignalTracker. Device-memory signals, graph and queue, are
HIP-only: OpenCL keeps host signals and LegacySignalTracker.
RetiringSignalTracker:
barrier bit, and the HW queue generation. A use published with the
barrier bit is a fence: its completion proves that every earlier
packet on the HW queue completed. The packet processor may advance
the read index before a packet completes (HSA System Architecture
2.8.3), so the read index only nominates the newest fence it passed,
and one load of that fence's signal confirms it. Reading the newest
fence in a batch (ProbeFence) likewise retires the whole batch with
one load. A HW queue change (NoteQueueChange) starts a new
generation.
slots with direct stores and one HDP flush per 32 slots, and stops at
the first busy one. A slot whose signal is still shared (an event, a
GPU wait or a pending handler) gets an armed spare instead; the old
signal is parked and reused once the tracker holds the last
reference. A ring about to reuse a busy slot grows, up to
16 x ROC_SIGNAL_POOL_SIZE, before waiting on the oldest signal.
so no Timestamp lists an armed signal. A command whose signals wrap
the ring, such as a graph launch under the built-in profiler, would
otherwise have its async handler wait on a re-armed signal while
holding the Timestamp the next use needs, and read the next use's
timestamps.
built-in profiler times it gets an armed host-memory signal from a
separate pool (PlaceForUse), so the handler reads its value and
timestamps without crossing PCIe. A use that a GPU waiter or a sync
may read swaps a device-memory signal back into the slot. A slot
keeps its signal when the replacement's allocation fell back to host
memory.
pinned with a reference until the consuming packets have passed and
every earlier use has retired, so re-arming cannot hide the old use
from the waiter. A drained queue (NoteQueueDrained) releases them.
write-combined. FlushStores fences them, so a peer GPU that waits on a
signal over XGMI reads the re-arm, and flushes HDP only when the AQL
rings are in system memory: a device-memory ring receives the packets
that use the signals through the same BAR after the stores, in order.
A signal handed to a ROCr copy is flushed when its use begins, since
ROCr's SDMA and blit rings are in system memory.
processed it, and drops it before the VirtualGPU reference and inside
the async-handler scope, so neither the signal nor the HW queue is
destroyed on the handler thread outside the deferral path.
With DEBUG_CLR_DEV_SIGNALS only. With the flag off, every queue and
ManagedBuffer signal is created with hsa_signal_create and a
direct-dispatch queue batches DEBUG_CLR_MAX_BATCH_SIZE commands, as
before:
interrupt pool (5) as two device-memory batches.
device-memory batch too. A chunk signal there is re-armed with a
direct store, flushed only when the AQL ring is in system memory,
since a store through ROCr reads it back across PCIe.
commands instead of DEBUG_CLR_MAX_BATCH_SIZE (1000), unless that
flag is set (Device::SubmissionBatchLimit). A hipEventRecord marker
doesn't own a batch, so in a record/sync loop every command keeps its
signal referenced until the batch completes. The tracker parks those
signals, and refills its pools with device allocations inside the
timed intervals.
signal, since the wait loads the value first and each load is a PCIe
read. Host-memory signals keep the check.
Shared changes. These apply with DEBUG_CLR_DEV_SIGNALS off too:
signal completes; dispatchBarrierValuePacket reserves its slot before
taking the signal.
retired) skips the signal value check in CacheTimingData.
sharing one signal (DMA copyBufferRect line by line, SVM prefetch
batch) waits for the issued ones (WaitForIssuedOps) before resetting
the slot, with either tracker: their late decrements would otherwise
complete the slot's next use early.
sees completion, instead of also running the blocked wait.
CompleteCpuWait checks the GPU-error state directly instead of
reloading the signal, and HostQueue::finish skips the HW-event wait
for a marker that enqueue already completed.
Known limitations with DEBUG_CLR_DEV_SIGNALS=1:
about 1.1 us per read (EventPerformance table below). The gain is in
waits: completion lands in device memory, and GPU waiters poll local
memory.
busy-wait signals ignore the wait hint. With the flag on, each poll is
a PCIe read.
events are unaffected: they complete through a separate ROCr IPC
signal.
Test (MI300X, container_saleel_1, image
rocm/ufb-private:vllm-0.26.0-ubuntu24.04-py3.14-nightlies-device-all-cdna-rocm10.1.0a20260826-568afb3a13,
ROCm SDK 10.1.0a20260826, ROCr built from this PR's develop base).
Host time per call in us, median of 2000:
hip-tests EventPerformance (HIP_TEST_LEVEL=level_1), average in us. Each
timed call includes the harness's hipStreamSynchronize:
with and without GPU_CLR_PROFILE_OUTPUT.
kernel never passed early (0 of 300 rounds), with the flag off and on.
sets identical to develop with the flag off and on.
ROCR_DYN_DLL defined.
JIRA ID : AIRUNTIME-2943