restore: re-apply UDMA P2P perf demo (#40)#45
Conversation
This reverts commit a02b856.
Introduce a new memory_consume transport that copies payload through the IPC peer window and synchronizes via SyncCollectives outer flags, so both ranks stay active. Wire the host launch path, perf config helpers (P2PTransportUsesIpc / P2PTransportBothRanksActive), sweep defaults, unit tests, and a source-guard test.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a64d97054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uint32_t chunkBytes = remaining > TILEXR_UDMA_MAX_WQE_BYTES ? TILEXR_UDMA_MAX_WQE_BYTES : remaining; | ||
| auto remoteAddr = UDMARegisteredRemoteAddr(registry, targetRank, byteOffset + chunkOffset); | ||
| UDMAWrite(args, remoteAddr, localBytes + chunkOffset, targetRank, qpIdx, chunkBytes); | ||
| (void)UDMAQuietStatusQp(args, targetRank, qpIdx); |
There was a problem hiding this comment.
For existing kernels that post with UDMAPutNbi/UDMAGetNbi and call UDMAQuiet later, this added per-chunk quiet makes the NBI API synchronous even for a single chunk, so callers can no longer batch WQEs and the all-gather/P2P paths silently lose the intended overlap. UDMAPostSend already polls when the SQ is near overflow, so completion should stay with the explicit quiet call instead of every NBI post.
Useful? React with 👍 / 👎.
| for (int i = 0; i < options.iters; ++i) { | ||
| LaunchP2PKernel(stream, commArgsDev, reinterpret_cast<GM_ADDR>(srcDev), reinterpret_cast<GM_ADDR>(dstDev), | ||
| reinterpret_cast<GM_ADDR>(debug), options, dstOffset, transferBytes, pattern, measuredMagic, i + 1); |
There was a problem hiding this comment.
When transport=data_as_flag runs with iters > 1 (the sweep script defaults to 20), the IPC window is cleared only once before this loop, but DataAsFlagCheckAndRecv treats the ready flags left by the first measured iteration as completion for later iterations. Those later launches can read stale payload without waiting for the sender, so the CSV can report valid checks and unrealistically low timings; clear/use an epoch per measured iteration or avoid multi-iter legacy data_as_flag runs.
Useful? React with 👍 / 👎.
| __gm__ uint8_t* localBytes = reinterpret_cast<__gm__ uint8_t*>(const_cast<__gm__ T*>(localSrc)); | ||
| uint32_t remaining = byteCount; | ||
| uint64_t chunkOffset = 0; | ||
| while (remaining > 0) { |
There was a problem hiding this comment.
When UDMAPutSignalNbi is used as a pure notification (byteCount == 0), the range checks above still allow it, but this loop is skipped and no WRITE_WITH_NOTIFY is posted, so the remote signal is never delivered. The previous implementation always posted the notify, so callers using a signal-only completion token will hang waiting for a signal that cannot arrive.
Useful? React with 👍 / 👎.
| if [ "${transport}" = "direct_urma" ] || [ "${transport}" = "udma" ]; then | ||
| export TILEXR_UDMA_QP_NUM="${block_dim}" | ||
| else | ||
| export TILEXR_UDMA_QP_NUM="${TILEXR_UDMA_QP_NUM:-1}" |
There was a problem hiding this comment.
For transport=direct_urma_post_only with block_dim > 1, this wrapper leaves TILEXR_UDMA_QP_NUM at its previous/default value instead of matching block_dim, even though the post-only kernel also limits active blocks by udmaInfo->qpNum. As a result, post-only concurrency sweeps label rows with the requested block_dim while only one QP/block may actually run, corrupting the reported scaling data.
Useful? React with 👍 / 👎.
| if (isSender) { | ||
| TileXRUdmaDemoCopyBytesGmToGm( | ||
| peerBase + dstByteOffset + offset, srcGM + offset, tBuf, sliceBytes); | ||
| sync.SetOuterFlag(magic, step); |
There was a problem hiding this comment.
With transport=memory_consume and iters > 1, the sender sets this one-way flag and can immediately enqueue the next iteration against the same IPC window; WaitOuterFlag accepts a later step with the same magic as satisfying an earlier wait, so a slower receiver can skip the matching handoff and copy data after it has already been overwritten by a future iteration. Add a receiver-to-sender ack or rotate buffers/epochs before the sender reuses the window.
Useful? React with 👍 / 👎.
| uint32_t alignedStart = static_cast<uint32_t>( | ||
| (rawStart / TileXR::BLOCK_UNIT_BYTE) * TileXR::BLOCK_UNIT_BYTE); | ||
| uint32_t alignedEnd = wqeIdx + 1 == wqeCount ? total : static_cast<uint32_t>( | ||
| ((rawEnd + TileXR::BLOCK_UNIT_BYTE - 1) / TileXR::BLOCK_UNIT_BYTE) * TileXR::BLOCK_UNIT_BYTE); |
There was a problem hiding this comment.
When a transfer is split across multiple QPs and a weighted boundary is not already 32-byte aligned, this rounds the next slice's start down while the previous slice's end is rounded up. Adjacent QPs then write overlapping ranges, so the benchmark moves more data than the bytes column reports and skews the bandwidth numbers; use a single consistently rounded boundary between neighboring slices.
Useful? React with 👍 / 👎.
|
|
||
| if (rank == 0) { | ||
| TileXR::Demo::P2PRankStatus srcStatus = localStatus; | ||
| TileXR::Demo::P2PRankStatus dstStatus = localStatus; |
There was a problem hiding this comment.
When test_type=4 is run directly with the default empty TILEXR_P2P_LOG_DIR, rank 0 seeds dstStatus from its own localStatus and never reads rank 1's result. For the default 0→1 unidir run, payload mismatches detected on the receiver are dropped, so the demo can append a clean row and exit success; require a log directory or exchange rank statuses through the barrier path.
Useful? React with 👍 / 👎.
2 participants
Summary
This PR restores the UDMA P2P perf demo that was merged in #40 and subsequently reverted in #44.
Re-applies the demo with the following changes:
Reverts the revert commit a02b856 (PR #44) so the demo is back on main.
Test
tests/udma/run_tests.shtests/udma/demo/run_tilexr_udma_p2p_perf.sh