Repository navigation
Fix one-sided remote-read bandwidth inflation via source-slice rotation (SRC_ROTATE_BYTES) - #365
Draft
mustafabar wants to merge 1 commit into
Draft
mustafabar wants to merge 1 commit into
mustafabar wants to merge 1 commit into
Conversation
The source buffer is written once during prep and never modified, and the timing loop reuses that allocation with no invalidation or rotation. Every iteration after the first re-reads identical addresses and can be served from cache instead of the link under test. Remote writes must be published regardless, so the bias is one-sided and always flatters remote read -- measured up to 4.4x on an 8-partition gfx1250 system in DPX. Replicate each GPU source into K identical slices and have iteration i read slice i % K, where K = ceil(SRC_ROTATE_BYTES / numBytes). All slices hold the same reference data, so validation is unaffected. Memory is bounded at ~SRC_ROTATE_BYTES per source (default 256MB), and the change is a no-op for transfers already large enough to defeat the cache. SRC_ROTATE_BYTES=0 restores the previous behaviour. Scoped to GPU-resident sources on a GFX or DMA executor; the NIC executor registers memory regions against a fixed vaddr and must not be rotated, and ping-pong transfers are excluded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Source alignment, cross-rank consistency, byte-count parsing, and subiteration handling need correction, and small-transfer initialization adds substantial overhead.
5 open issues
What changed in this PR
This PR aims to reduce cache-inflated remote-read bandwidth measurements by rotating GPU source buffers between iterations.
Changes:
- Adds source replication and rotation for eligible GFX and DMA transfers, excluding NIC and ping-pong transfers.
- Initializes replicas from the original source and updates GFX parameters before timing.
- Exposes
SRC_ROTATE_BYTESwith a 256 MiB default, help text, and configuration reporting.
| File | Description |
|---|---|
| src/header/TransferBench.hpp | Adds replica allocation, initialization, and per-iteration source selection. |
| src/client/EnvVars.hpp | Adds environment parsing, help text, reporting, and configuration plumbing. |
Review effort: Balanced
| alwaysValidate = GetEnvVar("ALWAYS_VALIDATE", 0); | ||
| blockBytes = GetEnvVar("BLOCK_BYTES" , 256); | ||
| byteOffset = GetEnvVar("BYTE_OFFSET" , 0); | ||
| srcRotateBytes = (size_t)GetEnvVar("SRC_ROTATE_BYTES", 256 * 1024 * 1024); |
| // Scoped to GPU-resident sources driven by a GFX/DMA executor: the NIC executor | ||
| // registers memory regions against a fixed vaddr and must not be rotated. | ||
| rss.numSrcSlices = 1; | ||
| rss.srcSliceFloats = t.numBytes / sizeof(float); |
Comment on lines
+5274
to
+5277
| if (cfg.data.srcRotateBytes > 0 && t.numBytes > 0 && rss.numLaps == 0 && allSrcsOnGpu && | ||
| (exeDevice.exeType == EXE_GPU_GFX || exeDevice.exeType == EXE_GPU_DMA)) { | ||
| size_t const slices = (cfg.data.srcRotateBytes + t.numBytes - 1) / t.numBytes; | ||
| rss.numSrcSlices = (int)std::max((size_t)1, slices); |
Comment on lines
+7808
to
+7812
| for (int slice = 1; slice < resource->numSrcSlices; ++slice) { | ||
| ERR_APPEND(hipMemcpy(resource->srcMem[srcIdx] + initOffset + (size_t)slice * resource->srcSliceFloats, | ||
| resource->srcMem[srcIdx] + initOffset, | ||
| resource->numBytes, hipMemcpyDeviceToDevice), errResults); | ||
| } |
Comment on lines
+7995
to
+7998
| // Advance to this iteration's source slice before timing starts, so repeat iterations | ||
| // read different addresses and cannot be served from cache. | ||
| for (size_t i = 0; i < localExecutors.size(); ++i) | ||
| ERR_APPEND(RotateSourceSlices(iteration, localExecutors[i], *localExeInfos[i]), errResults); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Fix one-sided remote-read bandwidth inflation via source-slice rotation (
SRC_ROTATE_BYTES)Motivation
The source buffer is written once (
hipMemcpyintosrcMem, prep phase) and never modified. Thetiming loop reuses that allocation with no invalidation or rotation, so every iteration after the
first re-reads identical addresses and can be served from cache instead of the link under test.
The bias is one-sided: a remote write must be published regardless, but a remote read can
be satisfied out of a cache level shared with the peer. On an 8-partition gfx1250 system in DPX,
pull is inflated up to 4.4×, peaking at 4763 GB/s against a measured steady state of
~1.27 TB/s. That is the side of the push-vs-pull comparison this benchmark is used to decide.
Technical Details
Replicate each GPU source into K identical slices; iteration
ireads slicei % K. All sliceshold the same reference data, so validation is unaffected by which slice is read. K is auto-sized:
Memory is bounded at ~
SRC_ROTATE_BYTESper source, and this is a no-op for transfers alreadylarge enough to defeat the cache.
DataOptions::srcRotateBytes— default 256MB,0disables.TransferResources— carriesnumSrcSlices,srcSliceFloats.K * numBytes + byteOffset; K decided once per Transfer, since all sources ofa Transfer share one allocation shape.
1..K-1filled device-to-device from slice 0, so prep H2D stays atnumBytes.src[]snapshotted intosubExecSrcBase, keyed offrss.subExecIdxrather than assembly order, so it is correct for everyGFX_BLOCK_ORDER.RotateSourceSlices()— recomputessrc[]from that base and re-uploads the param array(a few KB H2D); iteration folded into
[0, K)to handle negative warmup indices.cpuStartand the barrier, so rotation is untimed.ExecuteDmaTransfer— DMA readssrcMemdirectly and already hasiteration, so itrotates inline.
EnvVars.hpp—SRC_ROTATE_BYTESplumbing, help text, banner line.Scoped to GPU-resident sources on a GFX or DMA executor. The NIC executor registers memory regions
against a fixed vaddr and must not be rotated; ping-pong (
numLaps != 0) is excluded.15 hunks, +127/−4. Applies cleanly to
f8a8d83(v1.71.01) and6e5feaa(v1.71.00).Test Plan
On an 8-partition gfx1250 system in DPX (96 CU / 3 XCC per partition), ROCm 10.2.0:
ALWAYS_VALIDATE=1with rotation on.SRC_ROTATE_BYTES=0as a control.NUM_SUB_EXEC=8, 64M and 256M), on and off.Test Result
f8a8d83ALWAYS_VALIDATE=1, 0 errors with rotation onSweep at 96 CUs, median of 3 reps. Pull = remote read, G0→G1 (on-die). Steady state ~1270 GB/s:
With rotation on, warm (1013) ≈ cold (964) — what a correct steady-state measurement looks like.
SRC_ROTATE_BYTES=0still reproduces the 3.49× inflation on the same build.Remote write is never inflated and never exceeds that steady state, on or off (push w/c:
1.23 → 0.93 at 16M, 1.16 → 1.00 at 64M, 1.00 → 1.01 at 256M).
Scope: on-die peers only. G0→G1 is on-die — two partitions of the same physical GPU, sharing
PCIe bus
0001:04:00.*— so they share a cache level. G0→G2 crosses scale-up (UALoE) to adifferent physical GPU, which is not reachable through that cache level: pull w/c there is
1.01–1.11 across 1M–512M with rotation both on and off, so there is nothing to remove.
Partition mode decides whether the artifact appears. The SPX system does not exhibit it —
stock pull w/c is 1.01–1.30× across 1M–512M. The DPX system shows up to 3.4–4.4×. Same chip
generation, same ROCm; reproduction must match the partition mode.
No cost at a2a scale:
a2a was never contaminated — each GPU reads from 7 peers, so the working set is already 7 × buffer.
The artifact only distorted single-pair measurements in the 4M–64M range.
Known side effect. Push at 4M/8M drops below 1.0 (0.89, 0.83) with rotation on, because
rotation also denies the push path its local source-read cache reuse. Arguably more correct for
a link-bandwidth benchmark, but small-message push does fall slightly versus stock;
SRC_ROTATE_BYTES=0restores previous behavior exactly.Two pre-existing bugs found while validating (NOT caused by this patch)
Both confirmed against a stock build of the same commit. On record only; out of scope here.
(X0->G0->X1)reports ~2,055,873mismatched bytes out of 64MB at the destination, with every memory type (
U,G,F). Themirror pull
(X0->G1->X1)is always clean, so push bandwidth is unmeasurable there.Transfer 0reports a constant ~4.47 msregardless of size (64MB → 14.998 GB/s) while
Executorfor the same run reports a sane747 GB/s. Use the Executor / Aggregate rows on that build.
Submission Checklist