feat(cco): BNXT GDA per-packet PSN + per-peer doorbell grouping#421
Merged
Conversation
…ate path Port the BNXT GDA correctness fixes onto the warp-aggregate architecture (#411). All three are BNXT-only; MLX5/IONIC behavior is unchanged. - Per-packet PSN: BNXT PSN must advance by packet count (ceil(bytes/mtu)), not WQE-slot index. Reserve PSNs via wq->msnPack — per lane in reserveWqeSlots, and per warp via warpAggregateBnxtPsn (leader advances once, lanes take a prefix-ordered base). Applied to put/putValue/get/ signal across both the per-lane and warp-aggregate paths. - Reserve-gate drain: BNXT drains to the doorbelled snapshot (dbTouchIdx), not the un-doorbelled reservation, to avoid self-deadlock when WQEs are posted without an immediate doorbell. MLX5/IONIC keep draining to their own reservation. - Error CQE: quietUntil checks the BNXT CQE opcode and traps on a non-OK completion instead of treating the error as progress. Each impl is split into one if constexpr (BNXT) {...} else {...} so each provider path is self-contained; shared SQ-space waits move into waitSqSpace<PrvdType>. Also migrate test_gda_warp_aggregate to the ccoUniqueId ccoCommCreate overload (#411's test predated the bootstrap-API change and used the removed application::BootstrapNetwork overload). Verified on BNXT: 15 cco tests pass single-node; put/get/signal/ warp_aggregate + multiprocess pass cross-node (2-node, 16 ranks); GDA benchmarks saturate line rate (~49 GB/s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ringDoorbellOrdered keeps the dbTouchIdx wait-for-turn (per-QP ordering) but rings per peer-group instead of per lane: - put/get/putValue group active lanes by peer via a __ballot turn loop only in the WarpDefault+CoopThread path; each peer reserves contiguously and the group leader rings one doorbell (new LeaderOnly template path). WarpAggregate / CoopWarp / CoopBlock are a single group and skip the loop. - Unify the BNXT/PSD anti-coalescing lane-walk (shared dbrAddr/UAR) into ringDoorbellWalk<PrvdType>, used by ringDoorbellOrdered's multi-lane path and flushAsyncImpl. This fixes a missing walk on BNXT flush: a multi-peer flush could drop coalesced doorbells and hang. - Drop __syncwarp from the lane-walk. AMD wavefronts are lock-step, so per-lane predication already orders the stores; __syncwarp was also a deadlock (a lane past its turn waited on lanes still spinning in wait-for-turn). benchmark/cco p2p_put_bw / p2p_get_bw drop ccoGdaOptFlagsAggregateRequests: a per-op doorbell is now safe in thread/warp scope, and put's waitSqSpace drains the CQ as the SQ fills, so the deferred-doorbell SQ overflow that hung those scopes is gone. Verified on BNXT: all cco GDA tests pass single-node + cross-node (2-node, 16 ranks); p2p bw saturates ~49 GB/s (line rate) across block/warp/thread scopes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bw kernels split the total size across cooperative units — one WQE / message per unit: block scope = one per block, warp = one per wavefront, thread = one per thread. Previously the table only showed the total size, so it was unclear how big each actual message was or how many pieces the total was cut into. PrintPerfTable now derives units from scope/grid/block/warpSize, prints units=N in the header, and adds a "msg" column (size / units) — the bytes one WQE carries in that scope. E.g. 4MB total: block → 32 units × 128KB, warp → 128 × 32KB, thread → 8192 × 512B, which directly explains the bandwidth differences between scopes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
-s thread_agg = thread scope (one WQE per thread) with WarpMode = ccoGdaWarpAggregate, so the warp's same-peer lanes post as one aggregated batch instead of the default per-peer __ballot grouping. Bandwidth kernels only (ibgda_*_bw templated on WarpMode); latency uses one CoopBlock issuer so it falls back to thread behavior. On BNXT p2p, thread_agg matches thread within noise: after the per-peer doorbell grouping, the WarpDefault path already aggregates same-peer warp lanes into one batch/doorbell, so explicit WarpAggregate adds no measurable bandwidth — it only skips the __ballot group discovery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mpps = WQEs/messages issued per second = GB/s * 1e3 * units / size, computed from existing table data. It exposes the per-WQE issue-rate ceiling: at small messages the rate is flat (~115 Mpps on BNXT) and bandwidth = rate * msg, while large messages are bandwidth-bound (line rate, low Mpps). Bandwidth tables only; latency is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The GDA post sub-mode only takes effect under ccoCoopThread and the work unit is per-thread, so the "Warp" naming was misleading. Rename: ccoGdaWarpMode → ccoGdaThreadMode ccoGdaWarpDefault → ccoGdaThreadIndependent ccoGdaWarpAggregate → ccoGdaThreadAggregate plus the WarpMode template param, the test file/kernels, and bench help text. ccoCoopWarp and physical-warp references (lane id, warp size) are unchanged. Also condense the oversized comment blocks added by the doorbell-grouping work down to their essential rationale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jhchouuu
changed the title
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
1 participant
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.
Summary
Targets cco's GDA (cross-node IBGDA) device path. Fixes BNXT correctness, regroups doorbells per peer (unblocking thread/warp-scope bandwidth), and adds benchmark instrumentation. All changes are BNXT-only on the device side; MLX5/IONIC behavior is preserved.
Verified on BNXT (rocm/mori:ci): all cco GDA tests pass single-node (fork) and cross-node (2-node a07u19↔a07u25, 16 ranks); p2p bandwidth saturates ~49 GB/s (≈98% of 400G line rate) across block/warp/thread scopes.
Commits
fix(cco): BNXT GDA per-packet PSN + reserve-gate drainPorts the BNXT GDA correctness fixes onto #411's warp-aggregate architecture (all BNXT-only):
ceil(bytes/mtu)), not WQE-slot index. Reserved viawq->msnPack— per lane inreserveWqeSlots, and per warp viawarpAggregateBnxtPsn(leader advances once, lanes take a prefix-ordered base). Applied to put / putValue / get / signal.dbTouchIdx), not the un-doorbelled reservation, avoiding self-deadlock when WQEs are posted without an immediate doorbell. MLX5/IONIC keep their original drain target.quietUntiltraps on a non-OK BNXT completion instead of treating the error as progress.perf(cco): group GDA doorbells per peer; unblock thread/warp-scope bwput/get/putValuegroup active lanes by peer via a__ballotturn loop (only in theWarpDefault + CoopThreadpath); each peer reserves contiguously and the group leader rings one doorbell (newLeaderOnlytemplate path).WarpAggregate/CoopWarp/CoopBlockare a single group and skip the loop.ringDoorbellWalk<PrvdType>, used by bothringDoorbellOrderedandflushAsyncImpl— fixing a missing walk on BNXTflush(a multi-peer flush could drop coalesced doorbells and hang).__syncwarpfrom the lane-walk: AMD wavefronts are lock-step, so per-lane predication already orders the stores;__syncwarpwas also a deadlock on divergent entry.ccoGdaOptFlagsAggregateRequests: per-op doorbell is now safe in thread/warp scope andwaitSqSpacedrains the CQ as the SQ fills, so the deferred-doorbell SQ overflow that hung those scopes is gone.feat(cco-bench): perf-table instrumentationmsg+unitscolumns: show how the total size is split (one WQE/message per cooperative unit) and the per-WQE size (size/units).-s thread_agg: thread scope +ccoGdaWarpAggregate, to benchmark the aggregate path. On BNXT p2p it matches plainthreadwithin noise (the per-peer grouping already aggregates same-peer warp lanes).Mppscolumn: WQE/packet issue rate (GB/s * 1e3 * units / size). Exposes the issue-rate ceiling — ~115 Mpps flat for small messages (rate-bound) vs line-rate / low Mpps for large messages (bandwidth-bound).