Conversation
MORI-IO always builds its QPs through the ibverbs provider, so every NIC inherits timeout=14 (~537ms of ACK wait) and retry_cnt=7 regardless of vendor. A responder that stalls for longer than that budget, or a fabric that drops ACKs because they come back on a mismarked traffic class, fails the transfer with IBV_WC_RETRY_EXC_ERR, and there is no way to widen the window without rebuilding. Expose timeout, retry_cnt and rnr_retry as MORI_IO_QP_TIMEOUT, MORI_IO_QP_RETRY_CNT and MORI_IO_QP_RNR_RETRY. Defaults are unchanged. Each override is range-checked against the field width so a bad value is warned about and ignored rather than failing ibv_modify_qp during connection setup, and MORI_IO_QP_TIMEOUT=0 warns that it disables the ACK timeout altogether. The values actually applied are logged next to the existing SL/TC line, and the IBV_WC_RETRY_EXC_ERR advice now names the knobs. Beyond mitigation this is a diagnostic: widening the budget separates a responder that is merely slow from ACKs that never arrive at all. Refs: ROCm#655
QP-error CQEs already drive TransferStatus to failed, but a dead CQ or device cannot produce a CQE at all. RdmaAsyncEventMonitor logged IBV_EVENT_CQ_ERR / DEVICE_FATAL and moved on, leaving every in-flight transfer IN_PROGRESS forever and wedging the producer. Forward the async events that make outstanding work uncompletable to a QpErrorHandler, scoped to one QP (QP_FATAL / QP_REQ_ERR / QP_ACCESS_ERR), one CQ (CQ_ERR), or a whole context (DEVICE_FATAL). PORT_ERR is excluded: the link usually recovers and the QP survives, so failing transfers on it would be wrong. RdmaManager::FailInFlightTransfers matches affected endpoints by qpn, CQ handle, or ibv_context, fails their ledgers via a new SubmissionLedger::FailAll, and marks them degraded so later submissions fail fast instead of queueing onto a dead QP. FailAll leaves the ledger empty, so the CQE path cannot clear that flag afterwards. CqCallbackMeta::status becomes atomic and is claimed with exchange(nullptr), since the CQ poller and the monitor thread can now reach the same meta concurrently. Claiming on terminal success also fixes a latent bug where a late flush CQE could flip an already-successful transfer to failed, because TransferStatus::Update is sticky only against errors. Gated by MORI_IO_FAIL_ON_ASYNC_EVENT (default on) under the existing MORI_IO_ENABLE_ASYNC_EVENTS. The monitor is now reset first in ~RdmaManager so its handler cannot run against torn-down endpoints. Refs ROCm#655 Co-Authored-By: Claude <noreply@anthropic.com>
…post-close race FailAll() now marks the ledger closed; Insert()/InsertOrphaned() refuse once closed (returning kInvalidRecordId). This seals the window where a submitter that cleared the degraded check just before a fatal async event still reaches Insert() and posts a signaled WR whose CQE can never arrive. FailInFlightTransfers() now closes admission before failing the ledger, and the CQE recovery path skips a closed ledger (terminal degraded, not the recoverable partial-post kind). Also disambiguate QP-scope async events by ibv context: qp_num is only unique within one device, so on a multi-NIC node a QP_FATAL on one NIC would otherwise fail a healthy endpoint on another NIC sharing the same number. Handle the mid-post rejection: when Insert() returns kInvalidRecordId, release the signaled EP's depth and orphan the siblings' pending WRs, returning a clean failure instead of hanging. Extract OrphanPendingWrsOnOtherEps() to share this between the ibv_post_send-failure and async-retire paths. Adds unit coverage: submission_ledger_closed_after_fail_all and async_event_scope_matching.
…an target) Adds submission_ledger_close_race_stress: N submitter threads calling Insert() race the CQ poller (ReleaseByCqe) and the async monitor (FailAll) on one ledger, 400 iterations. Asserts the partition invariant — every Insert is either accepted or refused with kInvalidRecordId, the ledger ends Closed, and post-race Inserts stay refused. Primarily a ThreadSanitizer target (build with -fsanitize=thread): proves closed_, records_, and sqDepth are synchronized across the three real actors. Mirrors the existing TSan-net pattern in the umbp metadata-store tests and the TransferStatus wait/notify tests in test_transfer_wait.cpp.
…d document the knobs Follow-up polish on the RC retry-budget tunables: - Each MORI_IO_QP_* retry knob now falls back to a transport-wide MORI_RDMA_QP_* when the IO-specific name is unset, so the shmem/CCO QPs that share the ibverbs path can be tuned too; the MORI_IO_QP_* name wins when both are set. - min_rnr_timer is no longer hardcoded to 12: expose MORI_IO_QP_MIN_RNR_TIMER (and its MORI_RDMA_QP_MIN_RNR_TIMER fallback), so a receiver slow to post buffers can widen the RNR backoff instead of exhausting rnr_retry. Sits right next to the already-tunable rnr_retry. - Document the retry-budget knobs and MORI_IO_FAIL_ON_ASYNC_EVENT in docs/MORI-IO-GUIDE.md, which had no mention of them. Also folds in a clang-format wrap fix in common.cpp.
amirakb89
marked this pull request as ready for review
September 18, 2026 22:00
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.
io/rdma: surface QP-error on fatal async events + expose the RC retry budget
Summary
Two related hardening changes for the MoRI-IO RDMA producer path, motivated by
#655 (a single lost KV-write ACK permanently wedging PD-disagg decode).
Make the QP retry budget tunable via
MORI_IO_QP_TIMEOUT,MORI_IO_QP_RETRY_CNT, andMORI_IO_QP_RNR_RETRY. Previously the RC QP wasbuilt with a hardcoded
timeout=14/retry_cnt=7(~0.5 s before teardown),which operators could not widen for GPU-loaded receivers that are alive but
slow to ACK.
Fail in-flight transfers on fatal verbs async events. QP-error CQEs
already drive
TransferStatusto failed, but a dead CQ or device produces noCQE at all — leaving every in-flight transfer
IN_PROGRESSforever andwedging the producer. This forwards the async events that make outstanding
work uncompletable (
QP_FATAL/QP_REQ_ERR/QP_ACCESS_ERR,CQ_ERR,DEVICE_FATAL) to a handler that fails the affected ledgers and marks theendpoints degraded so later submissions fail fast instead of queueing onto a
dead QP.
Relationship to #655
Issue #655 requests five fixes. This PR addresses #1 (producer surfaces the
QP-error instead of polling forever) and #4 (expose the retry budget).
Net effect of this PR: the producer degrades a transient QP failure to a failed
transfer (freeing the sender) and lets operators widen the retry window to reduce
trigger frequency — instead of the producer hanging silently on a dead QP.
Details
Retry-budget env vars
MORI_IO_QP_TIMEOUT0disables it (an unresponsive peer will stall).MORI_IO_QP_RETRY_CNTRETRY_EXC_ERR.MORI_IO_QP_RNR_RETRY7= infinite, the ibverbs default this path uses).Defaults are unchanged, so behavior is identical unless an operator opts in.
Out-of-range values fall back to the built-in default. The applied values are
logged at INFO on the application module
(
ibverbs attr.timeout:X attr.retry_cnt:Y attr.rnr_retry:Z).Async fatal-event handling
RdmaManager::FailInFlightTransfersmatches affected endpoints by qpn, CQhandle, or
ibv_context(per event scope), fails their ledgers via the newSubmissionLedger::FailAll, and marks them degraded.PORT_ERRis deliberately excluded: the link usually recovers and the QPsurvives, so failing transfers on it would be wrong.
CqCallbackMeta::statusbecomes atomic and is claimed withexchange(nullptr), since the CQ poller and the monitor thread can now reachthe same meta concurrently. Claiming on terminal success also fixes a latent
bug where a late flush CQE could flip an already-successful transfer to failed.
MORI_IO_FAIL_ON_ASYNC_EVENT(default on) under the existingMORI_IO_ENABLE_ASYNC_EVENTS. The monitor is reset first in~RdmaManagersoits handler cannot run against torn-down endpoints.
Tests
submission_ledger_fail_all(tests/cpp/io/test_engine.cpp)covers
SubmissionLedger::FailAll: each distinct transfer failed exactly once(shared-meta batches counted once), orphaned records drained, SQ depth
released for every posted WR, failure message propagated, and the
post-
FailAllCQE race (a late CQE must not resurrect a failed transfer).Performance
No regression on the happy path. The only change on the hot completion path is
CqCallbackMeta::statusbecomingstd::atomic(one relaxed load + oneexchangeper completion); everything else runs only on the error/teardown path.A/B measured with
bench_engine, this branch vs. its base (f9697e16), on atwo-node ionic/RoCEv2 setup (MI300-class,
ionic_0, single NIC pair). Write,batch 1, GPU memory, 4 QP/transfer, 500 iters, 50 warmup:
Every point is within ±0.5% (run-to-run RDMA jitter); peak (~41 GB/s), the
latency curve, and the overall shape are identical, and both validate
byte-identical.
Verification
Exercised on the same two-node ionic/RoCEv2 setup:
MORI_IO_QP_TIMEOUT=20 MORI_IO_QP_RETRY_CNT=5 MORI_IO_QP_RNR_RETRY=4, theapplied-attr log changed 1:1 from the default
14/7/7; transfers validatedbyte-identical.
DEVICE_FATALinjected mid-transferdrove
FailInFlightTransfers, marked all endpoints on the context degraded,and the next submission was rejected immediately
(
EP is degraded, rejecting new submissions) rather than hanging.Refs #655