Skip to content

fix(rdma): cco/shmem indicies wraparound (#626) - #653

Merged
QizhouZhang97 merged 4 commits into
mainfrom
bugfix/put_mem
Sep 11, 2026
Merged

QizhouZhang97 merged 4 commits into
mainfrom
bugfix/put_mem

Conversation

@QizhouZhang97

Copy link
Copy Markdown
Contributor

postIdx / dbTouchIdx / doneIdx wrap at 2^32, so only their order matters. Flow
control mixed uint32 and uint64, underflowing into a forever-spin on an empty SQ;
fetch_max freezes the counter at the wrap; ionic's 24-bit MSN stored raw broke the
in-flight count past 2^24. Fixed all three, plus a boundary test.

wq.postIdx / dbTouchIdx / doneIdx are free-running uint32_t counters, so
only their order is meaningful. Three places got that wrong.

Flow control widened them into uint64_t locals, but
`curPostIdx + numWqesNeeded` still wrapped in 32 bits while dbTouched did
not, so past 2^32 the difference underflowed to ~1.8e19, no free-entry
count could exceed it, and reserveWqeSlots spun forever on an empty SQ.
Every such gate now computes in uint32_t, where all the terms share one
modulus. Same for the raw `>=` comparisons in the collapsed-CQ drains,
which are now signed serial-order compares.

__hip_atomic_fetch_max compares raw words, so at the wrap it keeps the
large pre-wrap value and freezes the counter for good. AtomicMaxSerial
does the same job in serial order. It is only used where several warps
publish concurrently -- postIdx on the bnxt post path, and the lock-free
mlx5 branch of quietUntil; the collapsed-CQ drains hold pollCqLock and
are the sole writer of doneIdx there, so they just compare and store.

ionic reports a 24-bit MSN. Storing it straight into doneIdx left that
counter in a different modulus than dbTouchIdx, so past 2^24 the
in-flight count became garbage and flow control silently stopped working:
WQEs start overwriting slots the NIC has not fetched. doneIdx now
advances by the masked forward delta, capped at dbTouchIdx -- read live,
not from the entry snapshot, since other warps keep doorbelling while we
poll and a CQE can legitimately report an MSN past it.

Adds a test that seeds the counters to the 2^32 boundary and checks the
gate still admits on an empty queue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@jhchouuu jhchouuu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!

Could we also check if this issue exists in mlx5 and bnxt?

@jhchouuu

Copy link
Copy Markdown
Collaborator

Data point from BNXT, in case it helps target this: we hit exactly the bug this PR fixes, and your BNXT hunk resolves it — but the trigger we saw is not a 2^32 wrap, so the rationale and the new test may be worth widening.

What we saw

2 nodes x 8 MI308X (gfx942), bnxt_re, EP internode dispatch_ll over CCO/GDA. Roughly 1 run in 3 hung mid-test. Every time the signature was identical: of 768 waves, 767 exit and one spins forever in quietUntil<BNXT>.

Live state off that wave under rocgdb:

postIdx    = 103
dbTouchIdx = 103
doneIdx    = 0xfffff067      <- 103 - 4096, frozen across re-reads
sqWqeNum   = 4096
targetIdx  = 103

postIdx == dbTouchIdx == targetIdx means everything was posted and doorbelled — nothing was outstanding on the wire. The counter was simply poisoned, exactly as this PR describes: __hip_atomic_fetch_max on uint32_t compiles to global_atomic_umax (confirmed in the ISA), so the wrapped value is the unsigned maximum, wins the max and pins doneIdx, while done()'s (int32_t)(doneIdx - targetIdx) >= 0 reads it as 4096 behind.

The trigger is not the 2^32 wrap

ringDoorbellOrdered rings the doorbell before publishing dbTouchIdx (there is a __threadfence_system() and, in the multi-lane case, a whole ringDoorbellWalk in between), so a collapsed-CQ poller can legitimately observe a completion ahead of dbTouchIdx:

dbTouch reads 102 while the CQE already reports con_indx = 103
completed = (102 & ~4095) | 103 = 103
103 > 102  ->  completed = 103 - 4096 = 0xfffff067

which reproduces the observed value exactly. So this fires within the first sqWqeNum (4096) WQEs of a QP's life, not after 2^32. Two consequences:

  • tests/cpp/cco/test_gda_wraparound.cpp only covers the 2^32 gate, so the case that actually bit us stays uncovered.
  • As written, the rationale invites a reviewer to wave the BNXT hunk off with "we never reach 2^32" — it is the one hunk that fixes a first-4096-WQE hang.

It might also be worth noting that this PR suppresses the bad publication but leaves the computation: if (completed > dbTouch) completed -= wq->sqWqeNum; still discards a valid completion whenever the race fires. That is harmless on BNXT only because PollSingleCqe is level-triggered for cqeNum == 1 — it has no phase check, so it re-reads con_indx and the next iteration recovers. Worth a comment so nobody later "fixes" that poll into a one-shot consume.

Validation

30-round --cmd test, v2_ll, 128 tokens, 16 ranks, JIT cache cleared between variants:

variant hangs
this PR's two header hunks 0 / 12
unpatched 2 / 4 (32% over 19 historical runs)

We had independently written the same signed-ordered fix for the BNXT line before finding this PR, and dropped it in favour of yours. The hunks apply cleanly onto #625 and do not touch flushAsyncImpl, so the two are complementary.


Separately, and not reproduced — we have no ionic/PSD hardware, this is from reading the code only: in the PSD non-CCQE branch the CQE is consumed (cq->cq_consumer = myCqPos + 1) before the new dbTouchIdx cap decides whether to accept it. If the cap rejects, that completion looks unrecoverable there, unlike BNXT where the CQE can be re-read. Might be worth a second look.

QizhouZhang97 and others added 3 commits September 11, 2026 01:30
…ccepted

Follow-up to 939b3ae (#626).

The bnxt drains rebuilt completions against dbTouchIdx, minus sqWqeNum on
overshoot. The doorbell precedes that publish, so a CQE routinely runs ahead and
the rebuild landed a queue depth behind doneIdx -- 0xfffff067 -- where the atomic
max pinned it: a hang inside the first sqWqeNum WQEs, not at 2^32. Both now take a
forward delta, and PollSingleCqe's level-triggered read, which the cap relies on,
is documented. ionic decides before consuming, since a refused MSN cannot be
re-read there; its MSN widening is restored.

test_gda_wraparound gains nine NIC-free cases on a hand-built endpoint, across
both providers and both layers. On gfx942 with mlx5, five fail on main and all
nine pass here, including all three mlx5 sites: quietUntil, the flow-control gate
and the shmem drain. The CQ is synthesized, so no packets cross the NIC; part 2
still covers that on real QPs.

Co-authored-by: Cursor <cursoragent@cursor.com>
…GDA FULL

Part 2 asks for CCO_GDA_CONNECTION_FULL, which cannot form on a runner without
intranode cross-rail RDMA: it returns 1 there, which is why the CCO sweep has
failed since 939b3ae while main stays green. Those runners already announce
themselves with MORI_CCO_SKIP_GDA_FULL, so honour it here and still run part 1,
which needs no NIC -- rather than adding the binary to run_cco_tests.sh's skip
list and losing that coverage too.

Co-authored-by: Cursor <cursoragent@cursor.com>
@QizhouZhang97
QizhouZhang97 merged commit ceac6df into main Sep 11, 2026
24 of 25 checks passed
@QizhouZhang97
QizhouZhang97 deleted the bugfix/put_mem branch September 11, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants