EFA: P4d compatibility, same-node loopback, and completion-error reporting (re-port of #3391)

[dmvevents]

Successor of #3391, re-ported onto the restructured ibverbs backend as announced in #3391 (comment). That branch predates the per-device module split (efa_device/efa_domain/efa_queue_pair), CQ pooling (#4668/#4688), and #4039's test-scaffolding removal, and could no longer merge meaningfully. All review discussion there carries over; the two follow-ups requested in those threads are delivered here as their own commits.

Five commits:

1. Leave an EFA device that cannot do RDMA to another backend — the P4d gate. Main's efa_queue_pair.rs asserts device_caps RDMA read+write at QP creation; this checks the same caps (mirroring libfabric's efa_prov_info.c test) during backend selection instead, so a P4d-class device falls back to TCP gracefully rather than panicking. P5/P5en are unaffected.
2. Serve a same-node EFA transfer with a copy instead of a work request — EFA silently drops same-node loopback at the NIC (libfabric works around it with SHM; raw verbs has no such fallback), so a transfer where both ends land on the same manager becomes a memcpy/cudaMemcpy instead of a WQE that never completes.
3. Report the root cause of a failed op, not the flush behind it — wires is_wr_flush_err() (previously defined but never called) into the error path so a WR_FLUSH_ERR message points at the completion cache's originating error rather than the symptom. Requested in EFA RDMA: P4d compat, loopback fix, GPU Direct, polling optimization #3391 review (@samlurye).
4. Honor the signaled flag on the EFA post path — plumbs signaled through rdmaxcel_efa_post_op/rdmaxcel_efa_post_write so EFA gets the same selective-signaling batching the RC path already had (previously EFA hardcoded IBV_SEND_SIGNALED on every chunk). Also from EFA RDMA: P4d compat, loopback fix, GPU Direct, polling optimization #3391 review.
5. Write down what EFA does differently — the EFA notes doc from EFA RDMA: P4d compat, loopback fix, GPU Direct, polling optimization #3391, updated for the new module layout and with the P4d/P5 max_qp_rd_atom contradiction fixed.

Verification (all real runs, on this branch vs a clean main worktree):

• cargo check -p monarch_rdma and cargo fmt -- --check clean; gcc -fsyntax-only -Wall -Wextra on rdmaxcel.c zero warnings (on stock main the same run shows unused parameter 'signaled' — which is what commit 4 fixes).
• cargo test -p monarch_rdma --lib (linked with -l ibverbs -l efa -l mlx5): 143 passed on this branch vs 127 on stock main — the delta is exactly the 10 new tests plus test-order shuffle; the "failures" in both runs are hardware skips implemented as panics (SKIPPED: no RDMA devices available, CUDA-unavailable) and the failing-name sets diff clean of any regression (every differing test passes in isolation on this branch).
• 10 new tests cover the loopback eligibility matrix (incl. the case that matters most: a GPU remote view must not take the copy path — on main a dmabuf MR maps at iova 0, so an address-based is_device_ptr probe would pass for device memory and memcpy to near-null; the port carries MemoryLocation on the wire instead) and flush-vs-root-cause attribution in both orderings, mutation-tested.

Two review promises resolved differently than announced in the #3391 plan comment, for cause:

• The signaled plumb-through turned out to be real plumbing, not a comment — the parameter already existed end-to-end and only the EFA branch dropped it (hardcoding IBV_SEND_SIGNALED); with sq_sig_all = 0 that flag alone decides CQE generation. All call sites still pass true, so behavior is preserved; flipping any to false is deliberately left as a separate change because the new send-queue credit accounting assumes one CQE per WR.
• The adaptive-polling change from EFA RDMA: P4d compat, loopback fix, GPU Direct, polling optimization #3391 is absorbed by main's PollSleepPolicy + RDMA_CQ_BUSY_POLL_WINDOW and is not re-ported.

One #3391 hunk is dropped rather than ported: the efa_nv_peermem check in validate_execution_context is unreachable on the EFA path (its only consumer sits after the is_efa early-return that existed at merge-base too), and porting it process-wide would have made the Mellanox manager on a mixed host skip its own peer-mapping check.

Open review threads on #3391 that remain live design questions (delivery-complete semantics, max_rdma_size-driven chunking) apply unchanged to this branch.

[samlurye]

Thanks for submitting this. I want to push back on some things here:

• Loopback transfers work fine and should not need special casing: Show that EFA send completions are not local only and that EFA loopback works samlurye/monarch-1#2
• I don't think we should be silently replacing flush errors with the original root cause. The user can find the root cause themselves. This also would break on EFA SRD QPs, where an error on one WR does not transition the QP to an error state or flush subsequent in-flight WRs.
• This PR claims that we have implemented selective signaling for RC QPs, but we have not. Even so, selective signaling would not work for SRD QPs because it is unordered: a completion for a signaled WR tells us nothing about the status of non-signaled WRs.
• EFA_NOTES.md claims that send-CQ completions are local-only, but this cannot be true, because send CQEs can describe WR errors that could only be triggered via interaction with the remote: Show that EFA send completions are not local only and that EFA loopback works samlurye/monarch-1#2

[dmvevents]

@samlurye — you're right on point 3, and the PR description is wrong. Thanks for catching it.

I wrote that the change "plumbs signaled through … so EFA gets the same selective-signaling batching the RC path already had." That asserts an RC capability that does not exist. Checking main:

• rdmaxcel_qp_post_op declares int signaled (rdmaxcel.c:958) and it is the only occurrence of the identifier in the file — grep -c signaled on main's rdmaxcel.c returns 1
• there is no RC branch to have the feature: everything that isn't op_type == 2 (recv) dispatches to rdmaxcel_efa_post_op, and that call (:990) does not forward signaled
• queue_pair.rs:542 hardcodes IBV_SEND_SIGNALED, and the doc comment above it says "Posts a single signaled RDMA op"

So sq_sig_all = 0 gives the capability for selective signaling, and nothing exercises it on any path. "The RC path already had it" is not true, and the unused-parameter warning I cited as evidence is itself the proof that nobody had it.

What the commit actually does, stated correctly: it makes a dead parameter live on the EFA path — wr_flags = signaled ? IBV_SEND_SIGNALED : 0 instead of the hardcoded flag. Every call site still passes true, so behaviour is unchanged; it is groundwork, not a batching win. I'll rewrite that bullet as:

Honor the signaled flag on the EFA post path — rdmaxcel_qp_post_op has always accepted signaled but dropped it before rdmaxcel_efa_post_op, so the EFA branch hardcoded IBV_SEND_SIGNALED and the parameter was dead (hence -Wunused-parameter on stock main). This wires it through. All call sites still pass true, so no behaviour change; selective signaling remains unimplemented on every path.

Your second half of that point stands too, and it is the more important part: selective signaling is not a sound optimisation for SRD at all, because SRD is unordered, so a CQE for a signaled WR says nothing about unsignaled WRs posted before it. That makes the plumbing I added of no use to EFA specifically. If you'd rather the parameter stay dead than carry a flag that can't be safely set on SRD, I'll drop commit 4 and send the -Wunused-parameter fix as (void)signaled; with a comment explaining why — say which you prefer.

On your point 4, I want to separate two claims, because I think we're disagreeing about different sentences.

EFA_NOTES.md says send-CQ completions are "local-only," and you're right that as written it is false — a send CQE can absolutely carry a status derived from remote interaction, which is the case you make. But the property that note was reaching for is narrower and I believe still true: a status == IBV_WC_SUCCESS send CQE on SRD does not confirm remote delivery; it confirms local consumption of the WQE. Those are different statements, and the note collapsed them. We have this measured on our side — ibv_post_send returns 0 and the send CQE completes successfully for a same-node SRD loopback that is silently dropped — which is precisely why "success means delivered" burned us and the note over-corrected into "local-only."

I'll reword to the accurate version:

A successful send CQE on SRD confirms the WQE was consumed locally, not that the payload reached the remote. Error statuses may be remote-derived. Verify delivery at the receiver, never from a successful send completion.

Points 1 and 2 I'm not going to argue before I've reproduced them. On 2 specifically, your reasoning — an error on one SRD WR does not transition the QP to error or flush in-flight WRs — is consistent with everything we've measured about SRD, and if that holds then replacing a flush error with a stored root cause is wrong on EFA for the reason you give, not just stylistically undesirable. I'll read your monarch-1#2 loopback implementation before responding on 1.

Given #4734 is also dirty against current main and overlaps #3391, would you rather I (a) rebase and revise this one in place, or (b) close it and send a smaller PR limited to whatever survives your points 1 and 2? I'd rather not rebase a PR whose premise you're disputing.

[dmvevents]

Following up on my own comment above — I went to the provider source to check your points properly, and two things changed. One of them is that the replacement wording I proposed for the CQ claim is also wrong, so I'd rather correct it here than leave it standing.

Point 3 — you understated it, and commit 4 is worse than "groundwork"

I said the plumbing was harmless because every call site passes true. That's not the real problem. EFA rejects unsignaled WRs at the provider, so the parameter's only non-default value cannot work at all:

/* efa_post_send_validate(), rdma-core efa verbs */
if (unlikely(!(wr_flags & IBV_SEND_SIGNALED) && !qp->sq_sig_all)) {
        verbs_err(..., "SQ[%u] Non signaled WRs not supported\n", ...);
        return EINVAL;
}

That check is reached on exactly the path I patched — efa_send_wr_rdma_write_64/128 → efa_send_wr_alloc → efa_post_send_validate — and our EFA QP is created with sq_sig_all = 0. So signaled = false sets wr_session_err = EINVAL and the SQ rolls back in efa_send_wr_complete; the WR never posts.

So commit 4 is dead plumbing whose only purpose is unreachable on this provider. That's a stronger reason than the ordering argument you gave, and it's decisive. Dropping it.

Point 4 — my replacement wording was also wrong

In my comment above I proposed narrowing "local-only" to "a successful send CQE confirms local WQE consumption, not remote delivery." Having checked, that's still wrong, and for the reason you gave: REMOTE_ERROR_BAD_ADDRESS is documented "RKEY not registered or does not match remote IOVA" — checkable only at the responder — and to_ibv_status maps it to IBV_WC_REM_ACCESS_ERR on the sender's own CQE. A completion generated by that mechanism can't be a local-accept notification.

What actually survives isn't about delivery at all, it's about visibility: fi_efa.7 COMPLETION SEMANTICS notes completions are written to system memory while the payload targets device HBM, so "observing a completion does not by itself guarantee the payload is visible in device memory." That's the real constraint for us, and it's a GPU-flush requirement, not a delivery caveat.

I'll rewrite the EFA_NOTES.md section to that, and drop the local-only sentence entirely. One thing I'll deliberately not claim: FI_EFA_MR_RELAXED_ORDERING says a TX completion guarantees subsequent operations to the same endpoint appear at the target after it — that's an ordering guarantee, and I don't think it should be read as a delivery receipt, so I'm not going to write it as one.

This wording had propagated further than the PR — it was in our internal EFA ground-truth preamble that gets injected into every model query we run on this subject. That's fixed on our side now, so thank you for pushing on it; it was costing us more than this PR.

Point 2 — your premise is right, my doc comment is wrong, but I think the code degrades safely

EFA_IO_COMP_STATUS_FLUSHED is documented "Flushed during QP destroy", so you're right that an SRD WR error doesn't put the QP in an error state or flush WRs behind it. My doc comment asserting the RC cascade unconditionally in an EFA-facing file is wrong and I'll scope it to RC.

On the code, though: observe_error only replaces a stored error when the current one is_flush && !is_flush(new). On SRD is_flush is never true, so it reduces to self.error.is_none() — byte-identical to the existing first_error.is_none(). It's a no-op on EFA rather than a hazard. Happy to drop commit 3 anyway, since it's RC-only and only fires above MAX_RDMA_MSG_SIZE.

Also: our PR body credits this to your #3391 review. Re-reading your comment there, you questioned the value of the selective-signaling optimisation — you didn't ask for a flush-attribution mechanism. That attribution is wrong and I'll fix it.

Point 1 — conceding pending measurement, and our evidence doesn't hold

I can't defend the loopback special-casing. Our claim traces to a single 2026-02-20 run that our own documented constraints disqualify twice: it was on a p4d (which this PR's own notes say can't do EFA RDMA) and on a secondary VPC CIDR (100.64.x — which we separately documented as a total silent drop, from that same run). It also used ibv_ud_pingpong, which is UD, not SRD RDMA. Roughly a hundred of our docs repeat the conclusion and none of them corroborate it.

What I can still stand behind is narrower: libfabric routes intra-node traffic to SHM, so same-node libfabric traffic isn't SRD. That says nothing about raw-verbs loopback. Your self-connected SRD QP test is the experiment we don't have, and your AH explanation — a missing responder-side AH giving REMOTE_ERROR_UNKNOWN_PEER — accounts for our "posts fine, never completes" symptom better than a NIC drop does.

I have P5-class capacity with primary-CIDR nodes. Offer: I'll run your loopback test there and post the counters either way. If it passes, commit 2 comes out.

So, concretely

Of the five commits: commit 1 (the P4d RDMA-capability gate) is the part I'd still like reviewed — it's untouched by your comment, fixes a real panic, and neither main nor your PR addresses it. Commits 2, 3 and 4 I'll drop. Commit 5 (EFA_NOTES.md) I'd keep but rewritten — the dmabuf constraints, the efa_nv_peermem DKMS trap and the GPU-flush section are worth having, the local-only and loopback claims are not.

Would you rather I force-push that reduced version here, or close this and open a single-commit PR for the p4d gate alone? Either is fine; I'd just rather not rebase five commits when three of them are coming out.