Repository navigation
TransferBench v1.71.00 - #359
AtlantaPepsi wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate correctness, portability, parsing, and build-target issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (8)
Restrict GetXccId CDNA5 instruction to CDNA5 targets · New Include pong subindex when enabling GFX subindices · New Emit advanced transfer fields before the pong token · New Align TDM device guard with supported architectures · New Avoid indexing stale or empty transfer results after failure · New Validate NIC message size for the selected port · New Fix pingpong timing scaling by subiterations · New Reject multi-device pingpong memory halves · New
What changed in this PR
Adds pingpong latency testing, NIC message-size validation, logical CU reporting fixes, and gfx1250-strict support.
Changes:
- Adds pingpong parsing, execution, timing, and latency presets.
- Adds NIC
max_msg_szvalidation and reporting. - Updates GPU architecture and TDM handling.
| File | Review summary |
|---|---|
src/header/TransferBench.hpp |
Critical and moderate issues in XCC handling, pingpong validation, NIC limits, subindex selection, dump parsing, and timing scaling. |
src/header/tdmCopy.h |
Critical architecture guard mismatch enables unsupported TDM targets. |
src/client/Utilities.hpp |
Reviewed result and topology utilities. |
src/client/Topology.hpp |
Reviewed NIC message-size reporting. |
src/client/Presets/Presets.hpp |
Reviewed latency preset registration. |
src/client/Presets/Latency.hpp |
Moderate issues with failed-run result handling and diagonal pair measurement. |
src/client/EnvVars.hpp |
Reviewed pingpong configuration variables. |
src/client/Client.cpp |
Reviewed pingpong transfer display updates. |
docs/install/build_from_source.rst |
Reviewed strict GPU target documentation. |
CMakeLists.txt |
Moderate issue: package build targets omit gfx1250-strict. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| exeInfo.totalSubExecs += t.numSubExecs; | ||
| } else { | ||
| exeInfo.totalPingpong ++; | ||
| } | ||
| exeInfo.useSubIndices |= (t.exeSubIndex != -1 || (t.exeDevice.exeType == EXE_GPU_GFX && !cfg.gfx.prefXccTable.empty())); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness issues remain in ping-pong execution, validation, parsing, and target gating.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (9)
Run unpinned pingpong parameters on only one XCC · New Align TDM device guard with supported architectures Emit advanced transfer fields before the pong token Include pong subindex when enabling GFX subindices Restrict GetXccId CDNA5 instruction to CDNA5 targets Reject multi-device pingpong memory halves Fix pingpong timing scaling by subiterations Validate NIC message size for the selected port Avoid indexing stale or empty transfer results after failure
e37e7de to
07e9f67
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate issues remain in pingpong execution, validation, result handling, and architecture gating.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 7
Open (12)
Reject pingpong subindices below -1 · New Prevent persistent pingpong dependency cycles from hanging · New Run unpinned pingpong parameters on only one XCC Align TDM device guard with supported architectures Emit advanced transfer fields before the pong token Include pong subindex when enabling GFX subindices Restrict GetXccId CDNA5 instruction to CDNA5 targets Allow default numSubExecs for pingpong transfers · New Reject multi-device pingpong memory halves Fix pingpong timing scaling by subiterations Validate NIC message size for the selected port Avoid indexing stale or empty transfer results after failure
| #else | ||
| useSubIndexCount[exe]++; | ||
| int numSubIndices = GetNumExecutorSubIndices(exe); | ||
| if (subIndex >= numSubIndices) { |
| dim3 const gridSize(xccDim, numPingpong, 1); | ||
| dim3 const blockSize(1); |
| if (t.numSubExecs <= 0) | ||
| errors.push_back({ERR_FATAL, "Transfer %d: # of subexecutors must be positive", i}); | ||
| else | ||
| else if (isPingpong) { | ||
| if (t.numSubExecs != 1) | ||
| errors.push_back({ERR_WARN, |
07e9f67 to
91f99bd
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness issues affect ping-pong execution, target guards, NIC validation, and package builds.
Review effort: Lite
Findings: 7
Open (11)
Guard peer access setup to the executor's local MPI rank · New Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Run unpinned pingpong parameters on only one XCC Align TDM device guard with supported architectures Include pong subindex when enabling GFX subindices Restrict GetXccId CDNA5 instruction to CDNA5 targets Propagate RunTransfers failures from latency presets · New Allow default numSubExecs for pingpong transfers Reject multi-device pingpong memory halves Validate NIC message size for the selected port
| } else if (partnerMem.memIndex != exeDevice.exeIndex) { | ||
| if (System::Get().IsVerbose()) { | ||
| System::Get().Log("[INFO] Enabling pingpong peer access: GPU %d -> GPU %d\n", | ||
| exeDevice.exeIndex, partnerMem.memIndex); | ||
| } | ||
| ERR_CHECK(EnablePeerAccess(exeDevice.exeIndex, partnerMem.memIndex)); |
91f99bd to
5ec784d
Compare
* fix (client): pre-resolve master address in LaunchTransferBench The host list was forwarded verbatim as TB_MASTER_ADDR, so an ssh_config alias that the local SSH client understands would fail getaddrinfo() on the workers, leaving rank 0 waiting on connections that never arrive. Expand the entry via ssh -G and prefer a literal IPv4, since workers resolve the master address remotely and only over AF_INET. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @nileshnegi * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues remain in pingpong parsing and memory access, architecture guards, NIC validation, and latency failure propagation.
7 open findings
Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… · New In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… · New Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Allow default numSubExecs for pingpong transfers
6 resolved since last review
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| p.srcMem[0] = nullptr; | ||
| p.srcMem[1] = nullptr; | ||
| p.localFlagMem = nullptr; | ||
| p.flagMem = static_cast<volatile uint8_t*>(static_cast<void*>(rss.dstMem[0])); |
| // Expand pong half | ||
| std::vector<Transfer> pongTransfers; | ||
| for (int r = 0; r < numRanks; r++) { | ||
| if (!RecursiveWildcardTransferExpansion(pongWct, r, numBytes, numSubExecs, pongTransfers)) |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness issues remain in ping-pong synchronization, target guards, validation, and error propagation.
8 open findings
Ensure pingpong slot progression avoids parity-safe reuse · New In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Allow default numSubExecs for pingpong transfers
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| // Advance one stride, plus an extra stride every hp laps so that a slot is never | ||
| // revisited an even number of laps later (which would leave a stale matching value) | ||
| off += sx; if (off >= y) off -= y; | ||
| if (--hopCnt == 0) { | ||
| hopCnt = hp; |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness issues remain in pingpong execution, target handling, validation, and probe tooling.
10 open findings
Zero-subexecutor path performs invalid result processing · New Ensure pingpong slot progression avoids parity-safe reuse In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Allow default numSubExecs for pingpong transfers Preset reference omits latency presets · New
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| for (int i = 0; i < exeInfo.resources.size(); i++) { | ||
| TransferResources& rss = exeInfo.resources[i]; | ||
| if (rss.numLaps != 0) continue; |
| ## Single-node bandwidth presets | ||
|
|
||
| | Preset | Purpose | | ||
| |---|---| | ||
| | `a2a` | All-to-all parallel transfers between every pair of GPUs. | | ||
| | `a2asweep` | GFX-based a2a swept across CU counts and unroll factors (`MEM_TYPE`, `NUM_SUB_EXECS`). | | ||
| | `bmasweep` | Compares DMA vs. Batched-DMA for one-to-many copies (HIP 7.1 / CUDA 12.8+). | | ||
| | `gfxsweep` | Sweeps GFX kernel options for one Transfer. | | ||
| | `hbm` | Local HBM read bandwidth on each GPU. | | ||
| | `healthcheck` | Quick correctness/perf health check (AMD MI300 series only). | | ||
| | `one2all` | All subsets of parallel transfers from one GPU to all others. | | ||
| | `p2p` | Peer-to-peer device-memory matrix between every GPU pair. | | ||
| | `pcopy` | Parallel copies from a single GPU to other GPUs. | | ||
| | `rsweep` | Random sweep through Transfer combinations. | | ||
| | `rwrite` | Parallel remote writes from a single GPU to others. | | ||
| | `scaling` | Scaling test: one GPU → all others, varying SEs, mem types (`CPU_MEM_TYPE`, `GPU_MEM_TYPE`). | | ||
| | `schmoo` | Local/remote read/write/copy scaling between two GPUs. | | ||
| | `smoketest` | Quick DMA/GFX correctness sweep. | | ||
| | `sweep` | Ordered sweep through Transfer combinations. | | ||
| | `wallclock` | Compares wallclock counters across XCCs within one GPU. | |
Copies standardised security scanning config from ROCm/rocm-repo-template. Co-authored-by: haribabug <haribabug@users.noreply.github.com>
- docs/sphinx/requirements.txt: bump gitpython, tornado, pyjwt, urllib3, cryptography (plus cffi and typing-extensions, which it requires), soupsieve and jupyter-core past their HIGH/CRITICAL CVEs; all other pins unchanged - workflows: pin actions to commit SHAs and container images to digests - build-relocatable-packages: grant id-token: write only to the two jobs that upload to S3 instead of the whole workflow Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved security-scan allowlist gaps and functional correctness issues remain.
12 open findings
Global .lock allowlist hides credentials in lockfiles · New Generic secret allowlist suppresses real credential detections · New Zero-subexecutor path performs invalid result processing Ensure pingpong slot progression avoids parity-safe reuse In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Allow default numSubExecs for pingpong transfers Preset reference omits latency presets
Previously missed (3)
In code that hasn't changed since last review

tee pipeline aborts on SIGPIPE when truncated with head

.claude/skills/transferbench-debug/examples/topology-probe.sh:24
With set -o pipefail, truncating the tee pipeline with head can close the pipe while the binary is still writing; tee/the binary then exits on SIGPIPE and the script aborts before completing the probe. Use a reader that consumes the full stream (for example sed -n '1,60p') or capture first and truncate afterward.

Documented presets are not registered in presetFuncMap

.claude/skills/transferbench-run/references/presets.md:25
This reference presents these rows as runnable presets, but neither pcopy nor rwrite is registered in the current presetFuncMap (src/client/Presets/Presets.hpp:68-99), so following either command fails instead of launching a benchmark. Remove them or register the corresponding presets before documenting them here.

Packaging defaults omit gfx1250 GPU targets

CMakeLists.txt:224
Adding the target only to CMake's default list does not add it to the package build path: build_packages_local.sh passes its separate DEFAULT_GPU_TARGETS list to -DGPU_TARGETS, and that list still omits both gfx1250 and gfx1250-strict. Packages built without an explicit GPU_TARGETS override therefore will not contain the target advertised by this change; update the packaging defaults or explicitly document the scope.
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| [[allowlists]] | ||
| # Allowlist by location only where a real first-party secret structurally | ||
| # can't live. Dependency lock files qualify: their contents are generated | ||
| # from a manifest, and the high-entropy strings they carry are artifact | ||
| # digests rather than credentials. Add vendored third-party trees here as | ||
| # callers bring them in, one explicit path per entry. | ||
| description = "Generated dependency lock files" | ||
| paths = [ | ||
| '''.*\.lock$''', | ||
| ] |
| '''(?i)\bINVALID_TOKEN\s*=''', | ||
| '''(?i)\bwrong_secret\s*=''', | ||
| '''(?i)\bsecret\s*=''' |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate correctness, packaging, launcher, documentation, and security-configuration issues remain.
12 open findings
Generic secret allowlist suppresses real credential detections Global .lock allowlist hides credentials in lockfiles Zero-subexecutor path performs invalid result processing Ensure pingpong slot progression avoids parity-safe reuse In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Allow default numSubExecs for pingpong transfers Preset reference omits latency presets
Previously missed (2)
In code that hasn't changed since last review

Packaging defaults omit the new GPU targets

CMakeLists.txt:224
Adding the target only to CMakeLists.txt does not add it to packaged builds: build_packages_local.sh overrides GPU_TARGETS with its own hard-coded default list, which currently omits both gfx1250 and gfx1250-strict. Update that packaging default as well, otherwise the release packages will not contain the target this change advertises.

Referenced presets are not registered

.claude/skills/transferbench-run/references/presets.md:25
These entries are not available presets in the current source: Presets.hpp registers neither pcopy nor rwrite, and no source-side dispatch for either name exists. The run-side reference therefore directs users to commands that will be rejected; remove them or document the actual registered preset names.
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness, packaging, and documentation issues remain, including one critical validation-path issue.
21 open findings
Skip pingpong resources during validation setup · New Generic secret allowlist suppresses real credential detections Global .lock allowlist hides credentials in lockfiles Zero-subexecutor path performs invalid result processing Ensure pingpong slot progression avoids parity-safe reuse In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Sanity check passes expression where byte size is required · New Update packaged default GPU target architectures · New Allow default numSubExecs for pingpong transfers Dryrun command omits required byte-size argument · New Cmdline example omits required byte-size argument · New Cmdline arguments are reversed · New Document cmdline and dryrun size arguments correctly · New Document pingpong grammar, semantics, and GFX restriction · New Document supported dryrun invocation syntax · New
And 1 more that still need to be addressed.
🧠 Review effort: Lite
| Transfer const& t = transfers[resource->transferIdx]; | ||
| // Ping and Pong will start with src value of 0 | ||
| if (t.numLaps != 0) continue; |
|
|
||
| echo | ||
| echo "=== Quick parser sanity check ===" | ||
| "$BINARY" dryrun "1 4 (G0->G0->G1)" 2>&1 | head -10 |
| gfx1250 | ||
| gfx1250-strict) |
| ./TransferBench dryrun "<expression>" # validate parsing, expand wildcards | ||
| TB_DUMP_CFG_FILE=dump.cfg ./TransferBench p2p # dump what a preset actually emits |
| - Quoting issues on the shell side when using `cmdline` (e.g. `G*` getting glob-expanded). | ||
|
|
||
| ### Fix | ||
| 1. **Always quote** `cmdline` arguments: `./TransferBench cmdline "1 4 (G0->G0->G1)"`. |
| TB_DUMP_CFG_FILE=p2p_dump.cfg ./TransferBench p2p | ||
|
|
||
| # "Is the slowness in iter 0 only, or every iter?" | ||
| NUM_WARMUPS=0 NUM_ITERATIONS=20 SHOW_ITERATIONS=1 ./TransferBench cmdline "1 4 (G0->G0->G1)" 256M |
| - `cmdline "<transfer expression>"` — run one ad-hoc transfer | ||
| - `dryrun "<transfer expression>"` — parse and print without executing |
| -2 (G0->G0->G1 4 1M) (G1->G1->G0 8 2M) | ||
| # Copies 1MiB GPU0->GPU1 with 4 CUs, in parallel with 2MiB GPU1->GPU0 with 8 CUs | ||
| ``` | ||
|
|
| ./TransferBench dryrun "1 4 (G0->G0->G1)" | ||
| ./TransferBench dryrun my.cfg |
51c95ca to
6af0ede
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect argument examples, target guards, latency error propagation, packaging defaults, and ping-pong/NIC correctness.
23 open findings
Skip pingpong resources during validation setup Generic secret allowlist suppresses real credential detections Global .lock allowlist hides credentials in lockfiles Zero-subexecutor path performs invalid result processing Ensure pingpong slot progression avoids parity-safe reuse In advanced transfer syntax,numSubExecsis never initialized because the parser only reads it in… Pingpong makes the GPU kernel dereference this destination pointer as a flag, but… Guard peer access setup to the executor's local MPI rank Prevent persistent pingpong dependency cycles from hanging Reject pingpong subindices below -1 Include pong subindex when enabling GFX subindices Diagnostic contains stale version 1.67 limitation · New Update packaged default GPU target architectures Sanity check passes expression where byte size is required Allow default numSubExecs for pingpong transfers Reference lists unregistered pcopy and rwrite presets · New Document supported dryrun invocation syntax Document pingpong grammar, semantics, and GFX restriction Document cmdline and dryrun size arguments correctly Cmdline arguments are reversed
And 3 more that still need to be addressed.
🧠 Review effort: Lite
| "Transfer %d: Cross-rank GPU executor (R%d%c%d) cannot access remote host memory " | ||
| "(%s on rank %d is %s). Fabric-handle sharing only supports GPU memory for 1.67; use a NIC " | ||
| "executor (e.g. R%dN..) for cross-rank transfers involving host memory.", |
| | `pcopy` | Parallel copies from a single GPU to other GPUs. | | ||
| | `rsweep` | Random sweep through Transfer combinations. | | ||
| | `rwrite` | Parallel remote writes from a single GPU to others. | |

Motivation
Technical Details
Test Plan
HSA_DISABLE_GFX12_STRICT=0 ./TransferBenchoutput of gfx1250-strict targetSHOW_ITERATIONSoutput CU ID againstCU_MASKbitsp2p_latencypreset: in/cross-domain, and AMD and Nvidia platformTest Result
p2p_latency example output
Concurrent transfer + pingpong hybrid run example output
Submission Checklist