fix(shmem): public RDMA quiet returned before the caller's WQEs completed#404
Merged
Conversation
#395 introduced the DrainToLive split (false = snapshot recycle gate, true = wait for live postIdx) and converted the per-PE ShmemQuietThread(pe) overload to true, but left the public quiet-all ShmemQuietThread() and per-QP ShmemQuietThread(pe, qpId) overloads on the default snapshot drain (false). Under multi-warp contention on a shared QP, warps that lose the drain lock return without waiting for their own posted WQEs to complete. A GET reads its local destination right after quiet, so an unwaited completion yields stale data (0xDEAD). Symptom: concurrent_get_thread thread-scope GET fails intermittently on bnxt; block / large-chunk GET and PUT happened to mask it. Make both public overloads real completion waits (DrainToLive=true), matching the per-PE overload. The recycle gate is unaffected: it calls ShmemQuietThreadKernelImpl directly with the default snapshot drain, which must stay snapshot to avoid self-deadlock on its own un-doorbelled reservation. Keeps #395's outstandingWqe removal and per-lane PSN fix intact. Verified on bnxt: concurrent_get_thread passes across repeated runs (was ~4/7 subtests failing); put / atomic / put_signal / put_imm thread tests and the EP internode correctness test (which exercises quiet-all) stay green.
The thread-scope shmem examples only printf'd PASSED/FAILED and main() always returned 0, so failing cases never set a non-zero exit; combined with the CI shmem steps lacking set -e (step status = last command only), broken cases passed CI silently (this is how the bnxt GET thread-scope regression went unnoticed). - concurrent_get_thread / concurrent_put_thread / concurrent_put_imm_thread / concurrent_put_signal_thread: count failed verifications and return non-zero from main(). Fix concurrent_put_imm_thread's verification, which was dead code (PE 1 verified but the PASS/FAIL prints were gated on myPe==0 inside the myPe==1 block, and PE 0 always printed PASSED without verifying). For put_thread only the RDMA put-correctness tests feed the counter; the P2P-only direct-access test is excluded since it is expected to differ under MORI_DISABLE_P2P. For put_signal, also compare the signal counter, which was printed but never checked. - Add set -e to the MORI-CPP shmem (P2P) and (IBGDA) steps so any failing example fails the step. The atomic examples (atomic_fetch/atomic_nonfetch) verify on the receiver via a device-side spin, so a failure manifests as a hang -> timeout -> non-zero, now caught by set -e. Verified on bnxt in both P2P and IBGDA: all examples exit 0 when correct; built against the pre-fix shmem, concurrent_get_thread exits non-zero (4/7 failing).
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.
What
The public
ShmemQuietThread()andShmemQuietThread(pe, qpId)overloads used the snapshot drain (DrainToLive=false), so under multi-warp contention on a shared QP a warp could return from quiet before its own posted WQEs completed. A GET reads its local destination right after quiet, so this yields stale data (0xDEAD);concurrent_get_threadthread-scope GET failed intermittently on bnxt.Fix
Make both public quiet overloads real completion waits (
DrainToLive=true), matching the per-PE overload. The recycle gate is unaffected — it callsShmemQuietThreadKernelImpldirectly and must stay snapshot.Test hardening
The failure was invisible to CI because the shmem examples always returned
0and the CI steps had noset -e. Made the CPP shmem examples return non-zero on verification failure and addedset -eto the shmem CI steps.Verification
On bnxt (P2P and IBGDA): all shmem examples pass; built against the old code,
concurrent_get_threadnow exits non-zero.