Skip to content

Commit 7df9208

Browse files
颜明宇claude
andcommitted
net/barex: fix CTS reordering corruption on multi-chunk transfers
CTS control messages are delivered by ACCL's multi-threaded callback pool, so they can be enqueued out of the receiver's post order. The sender paired its k-th isend with whichever CTS happened to be at the queue head, writing chunk k's payload into the buffer posted for a different chunk. The proxy pipelines up to 16 chunks per message, so transfers of ~32MB and above (8+ in-flight chunks) corrupted data while small transfers appeared fine; perf runs passed because they never verify bytes. Stamp each CTS with the receiver's post-order sequence number (stored in the CTS message's former tail padding; wire size stays 64 bytes) and make the sender consume CTS strictly in sequence order, retrying via the proxy when the next-expected CTS has not arrived yet. Verified on 810e (PPU dev5,6 + vsolar_1): byte-verify at 1MB, 16MB+8K, 32MB, 64MB now ALL_MATCH twice in a row (previously MISMATCH at 32/64MB), and -R 0 staging-path verify at 64/128/256MB ALL_MATCH (256MB = 64 chunks, 4x ring-buffer wrap). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 6e85731 commit 7df9208

1 file changed

Lines changed: 32 additions & 9 deletions

File tree

flagcx/adaptor/net/barex_adaptor.cc

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@
8282
#include <atomic>
8383
#include <cstdlib>
8484
#include <cstring>
85-
#include <deque>
8685
#include <map>
8786
#include <mutex>
8887
#include <random>
@@ -153,6 +152,10 @@ struct BarexCtsMsg {
153152
uint64_t size;
154153
uint32_t nKeys;
155154
uint32_t rkeys[kMaxNics];
155+
uint32_t seq; /* receiver's post-order index; sender matches in this order
156+
because CTS are delivered via a multi-threaded callback
157+
pool and may otherwise reorder — see barexIsend/OnRecvCall.
158+
Occupies the struct's former tail padding (size stays 64). */
156159
};
157160
static_assert(sizeof(BarexCtsMsg) == 64, "CTS wire layout must be stable");
158161

@@ -228,8 +231,15 @@ struct BarexComm {
228231
bool isSend = false;
229232
std::atomic<bool> dead{false};
230233

231-
std::mutex mu; /* guards ctsQueue + slot alloc */
232-
std::deque<BarexCtsMsg> ctsQueue; /* sender side: CTS from receiver */
234+
std::mutex mu; /* guards ctsPending + slot alloc + seq counters */
235+
/* Sender side: CTS from the receiver, keyed by the receiver's post-order
236+
seq. The sender consumes strictly in seq order (sendExpectedSeq) so a
237+
chunk's data always lands in the buffer the receiver posted for that
238+
same chunk, regardless of the order the callback pool delivered the
239+
CTS. Receiver side: recvSeq stamps each outgoing CTS in post order. */
240+
std::map<uint64_t, BarexCtsMsg> ctsPending;
241+
uint64_t recvSeq = 0; /* receiver: next CTS seq to stamp */
242+
uint64_t sendExpectedSeq = 0; /* sender: next CTS seq to consume */
233243
BarexRequest requests[kMaxRequests];
234244

235245
BarexRequest *allocRequest() {
@@ -341,7 +351,7 @@ class BarexNetCallback : public XChannelCallback {
341351
return;
342352
}
343353
std::lock_guard<std::mutex> lk(comm->mu);
344-
comm->ctsQueue.push_back(cts);
354+
comm->ctsPending[cts.seq] = cts;
345355
return;
346356
}
347357

@@ -921,13 +931,22 @@ static flagcxResult_t barexIsend(void *sendComm, void *data, size_t size,
921931
BarexRequest *req = nullptr;
922932
{
923933
std::lock_guard<std::mutex> lk(comm->mu);
924-
if (comm->ctsQueue.empty())
925-
return flagcxSuccess; /* receiver not ready — proxy retries */
934+
/* Consume CTS strictly in the receiver's post order. The proxy calls
935+
isend once per chunk in increasing chunk index, so chunk k must pair
936+
with the CTS the receiver posted for its k-th irecv (seq == k). If
937+
that CTS has not arrived yet (the callback pool may deliver a later
938+
seq first), leave sendExpectedSeq untouched and let the proxy retry —
939+
do NOT pair this chunk with a different CTS or the data lands in the
940+
wrong buffer. */
941+
auto it = comm->ctsPending.find(comm->sendExpectedSeq);
942+
if (it == comm->ctsPending.end())
943+
return flagcxSuccess; /* CTS for this chunk not here yet — retry */
926944
req = comm->allocRequest();
927945
if (req == nullptr)
928-
return flagcxSuccess; /* request pool exhausted — retry */
929-
cts = comm->ctsQueue.front();
930-
comm->ctsQueue.pop_front();
946+
return flagcxSuccess; /* request pool exhausted — retry (keep CTS) */
947+
cts = it->second;
948+
comm->ctsPending.erase(it);
949+
comm->sendExpectedSeq++;
931950
}
932951

933952
/* Receiver posted cts.size; both sides run the same chunk schedule so
@@ -984,9 +1003,12 @@ static flagcxResult_t barexIrecv(void *recvComm, int n, void **data,
9841003
BarexEngine *e = comm->engine;
9851004

9861005
BarexRequest *req = nullptr;
1006+
uint64_t seq = 0;
9871007
{
9881008
std::lock_guard<std::mutex> lk(comm->mu);
9891009
req = comm->allocRequest();
1010+
if (req != nullptr)
1011+
seq = comm->recvSeq++; /* stamp in post order (same lock as alloc) */
9901012
}
9911013
if (req == nullptr)
9921014
return flagcxSuccess; /* pool exhausted — proxy re-posts */
@@ -999,6 +1021,7 @@ static flagcxResult_t barexIrecv(void *recvComm, int n, void **data,
9991021
cts.addr = (uint64_t)(uintptr_t)data[0];
10001022
cts.size = sizes[0];
10011023
cts.nKeys = mr->nKeys;
1024+
cts.seq = (uint32_t)seq;
10021025
memcpy(cts.rkeys, mr->rkeys, sizeof(cts.rkeys));
10031026

10041027
memp_t msg;

0 commit comments

Comments
 (0)