Add accumulate method for PortChannel - #784
Open
Changho Hwang (chhwang) wants to merge 2 commits into
Open
Conversation
Changho Hwang (chhwang)
force-pushed
the
chhwang/new-atomic-add
branch
from
August 4, 2026 17:20
4b21685 to
de4997b
Compare
atomicAdd method for PortChannelaccumulate method for PortChannel
Changho Hwang (chhwang)
marked this pull request as ready for review
August 4, 2026 23:36
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Changho Hwang (chhwang)
force-pushed
the
chhwang/new-atomic-add
branch
3 times, most recently
from
August 6, 2026 08:22
ccfecde to
36ce485
Compare
`TriggerData`, `TriggerFlag`, and `TriggerSync` were combinable bits, but the
device API only ever produced five of the seven combinations, and the proxy
tested them with three independent masks. Replace them with one opcode per
operation the API can express:
TriggerNone = 0 TriggerPut = 1 TriggerSignal = 2
TriggerFlush = 3 TriggerPutWithSignal = 4 TriggerPutWithSignalAndFlush = 5
Seven values fit the existing 3-bit field with one spare. `handleTrigger`
becomes a switch that names one operation per case and warns on anything
unknown, so a combination nothing emits is now unrepresentable, and a trigger
whose type field is unset is no longer a valid operation.
The old names are removed rather than redefined. Code that composed them with
`|`, or tested them with `&`, must fail to compile: `TriggerFlush` is 3, which
has the old `TriggerData` bit set, so a mask test would silently misbehave.
No behavior change. mp_unit_tests 29/29 and unit_tests 34/34 on H200.
Add `Connection::accumulate()` and `PortChannel::accumulate()`, which add a 64-bit signed value to remote memory. Unlike `updateAndSync()`, the caller needs to know only its own contribution, not the destination's current value. Carried by the `TriggerAccumulate` opcode, with the operand in the trigger's first word. A zero operand is a no-op and pushes nothing: the proxy treats a trigger whose first word is zero as not yet written, so pushing one would stall it on that slot. The addition has to be a real read-modify-write at the destination, which not every transport can do: - IB: any number of writers, via RDMA fetch-and-add. Rejected in no-atomic mode, where the device has no RDMA atomics. - Ethernet: any number of remote writers. The receiving process performs the update, and its connections are serialized against each other; without that a 7-writer fan-in loses ~74% of updates. - CudaIpc on ROCm: any number of writers. The proxy runs a kernel on the connection's stream, which also orders it against a following signal. A kernel there runs even while the caller's kernel spins. - CudaIpc on CUDA: not supported, throws InvalidUsage. The host cannot read-modify-write device memory, and a proxy-launched kernel either cannot be scheduled while the caller's kernel spins (same context) or costs 2391 us per operation (separate context), against 19 us for a plain remote store. Tested on H200 (CUDA 12.9) and MI300X (ROCm 7.2), including an 8-rank fan-in test with 7 concurrent writers into one address.
Changho Hwang (chhwang)
changed the base branch from
main
to
chhwang/enhanced-fifo
August 7, 2026 19:07
Changho Hwang (chhwang)
force-pushed
the
chhwang/new-atomic-add
branch
from
August 7, 2026 19:07
36ce485 to
776cf75
Compare
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.
Adds
accumulate, a remote 64-bit add, onConnection(host) andPortChannel(device).The caller supplies only its own contribution, not the destination's current value. That is the difference from
updateAndSync(), which assigns an absolute value and therefore requires the sender to own the address. Addition commutes, so arrival order does not matter and several peers can target one counter.Encoding
Adds
TriggerAccumulate = 6to the opcode set provided by thechhwang/enhanced-fifobase branch. The full signed operand occupies the trigger's first word, spanning thesizeandsrcOffsetfields.A zero operand traverses the FIFO and adds nothing. Enhanced FIFO reports readiness with lap parity in the reserved bit of
snd, so trigger payload no longer doubles as a written marker.Transport support
The addition has to be a real read-modify-write at the destination, which not every transport can do.
InvalidUsageInvalidUsageOn Ethernet every peer terminates its socket in the destination process, so one mutex covers all of them. Without it, a 7-writer fan-in loses ~74% of updates on MI300X (1040 of 1400). The destination GPU must not write the address concurrently, because it can be overwritten inside the RMW window.
CudaIpc differs by platform because host access to device memory differs:
A host-side CPU atomic on the peer pointer does not work on ROCm either. GPU memory is host-accessible to the process that allocated it, but an
hipIpcOpenMemHandlemapping is device-only, and the proxy holds exactly that imported pointer. It segfaults.Tests
PortChannelOneToOneTest, per transport:Accumulate— 64 blocks per rank accumulate into the peer each iteration, then signal, flush, wait. After eachwait()the value is checked against a range, which asserts that every add the peer issued before its signal has landed. The lower bound is the ordering property; the upper bound allows the peer to be one iteration ahead, since it resumes on our signal.AccumulateSigned— negative operands, exact total.AccumulateZero— zero and non-zero operands interleaved. Hangs without the no-op guard.AccumulateIbHostNoAtomicRejected,AccumulateCudaIpcRejected— the two unsupported combinations throw.Operands are
2^32 + 1, so a truncated operand shows up as a wrong sum.PortChannelFanInTestis new: ranks 1..N-1 each push 200 accumulates at rank 0's single counter, and rank 0 checks the exact total. It covers IB and Ethernet everywhere, plus CudaIpc on ROCm. It polls until the total stops changing rather than sleeping, becauseEthernetConnection::flush()is a no-op and a sender cannot tell when the receiver applied its update.Validation
--filter=Accumulatemp_unit_testsunit_testsPingPongPerf, CudaIpcThe MI300X node has no IB device, so every IB test fails there with
ib.cc:685 IB transport out of range: 0 >= 0, including tests this PR does not touch. All other suites —allgather_test_cpp,allgather_test_host_offloading,nvls_test,executor_test,python/test/test_mscclpp.py— produce results identical toorigin/mainbuilt the same way.Follow-ups, not in this PR
updateAndSync's running counter into the connection and drop theuint64_t* srcshadow from the public API.MemoryChannel::accumulate<T>()for concurrent intra-node accumulation on CUDA.IbPeerToPeerTest.MemoryConsistencydivides by zero when no IB device is present. Pre-existing.