* Fix segfault race between setPeerNicPath and in-flight submitPostSend
EfaEndPoint::submitPostSend was latching peer_fi_addr_ under lock_ and
releasing the lock before calling EfaContext::submitSlicesOnPeer, which
calls fi_write(shared_ep_, ..., peer_fi_addr, ...) outside the lock. A
concurrent setPeerNicPath() / disconnect() (both take the write lock and
call context_.removePeerAddr, i.e. fi_av_remove) could invalidate the
AV slot between the latch and the fi_write, causing libfabric's EFA
provider to dereference a stale entry and segfault.
Repro: SGLang 2P2D GLM-5.1-FP8 prefill, PP=2 TP=8 CP=8 EP=8 topology,
moe_a2a=deepep, 16 EFA NICs, MC_MAX_WR=16384. The P2P handshake port
changes for a peer process after a rebind, setPeerNicPath() takes the
CONNECTED branch and disconnects while sender threads have fi_write
calls in flight for that peer.
Fix: hold the read lock across the entire submitSlicesOnPeer call so
disconnect (write lock) cannot remove the AV entry while a submit is
in flight. Re-check status_ under the lock and fail the batch if a
disconnect raced in between, letting the caller retry with a fresh
setupConnectionsByActive(). Read locks still allow multiple senders to
the same peer to submit in parallel, so no throughput regression.
Refs: https://github.com/kvcache-ai/Mooncake/issues/2022
* fix(efa): skip AV churn on duplicate passive handshake, silence "Re-establish" warnings
Under bilateral sglang P/D traffic every (localNIC, remoteNIC) pair sees two handshakes: the active side inserts the peer into the AV, and the remote side later fires its own active handshake which arrives at our passive handler after we are already CONNECTED. The previous code unconditionally treated that second handshake as a reconnect, logged W "Re-establish EFA connection" and ran fi_av_remove + fi_av_insert. Observed impact on a real run: ~tens of thousands of lines per decode node, and a brief window where in-flight fi_write could see a stale fi_addr_t (the race PR 2023 already hardened against on the submit side).
Cache the EFA address bytes that we successfully inserted. On a passive handshake, if we are CONNECTED and the incoming peer address matches the cache, it is the same peer and the same AV slot — return the local desc and skip the churn entirely. Only reinsert if the peer address genuinely changed (peer process restart → new QPN → different bytes); demote that branch to LOG(INFO) so real reconnects remain visible without being drowned out.
Cache is populated from both active and passive connect paths and cleared in disconnectUnlocked / markDetachedForTeardown so a subsequent passive handshake takes the first-connect path correctly.
Complements PR 2023: PR 2023 holds the read lock across submitPostSend so the AV cannot be removed mid-fi_write; this change removes the AV churn at the source for 99% of the handshakes, so the read-lock hold becomes defense-in-depth rather than the only barrier.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(efa): don't skip fi_av_insert on duplicate passive handshake
Previous commit skipped fi_av_remove + fi_av_insert when the passive peer address matched the cached one. Under bilateral sglang P/D load (128-concurrency, 3K bench) this caused requests to stall: 31 transfers stuck in #transfer-req, prefill #inflight-req pinned at 128 with no progress. The hypothesis that EFA handshakes with an unchanged peer address could be treated as a no-op was wrong — libfabric's EFA provider tracks per-peer transport state (AH activation, RNR backoff, internal sequence bookkeeping) that a fresh fi_av_insert re-seeds, and skipping it leaves the provider in a stale state that silently drops or stalls the fi_write path.
Keep the cached_peer_addr_ field (it's cheap and harmless) but drop the AV-skip fast path. The reinsert now happens on every passive handshake, restoring the functional behavior PR 2023 alone validated. The only surviving optimization is log-level classification: same-address handshake → VLOG(1), different-address → LOG(WARNING). This still eliminates the "Re-establish EFA connection" warning spam on bilateral symmetric traffic without touching semantics.
PR 2023's read-lock widening in submitPostSend is now the primary defense for the race described there, with the AV reinsert once again the common-case path. Verified by repro: sglang-glm5-2p2d bench 3072 1024 500 18 128 no longer stalls.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(efa): don't tear down AV slot when only the peer RPC port rotates
Symptom: after the previous commit (which restored fi_av_insert on every passive handshake) traffic no longer stalls, but decode logs still show a flood of "Peer reconnected with new address, re-establishing" from setPeerNicPath. Each of those lines corresponds to a peer whose EFA endpoint did not move — only the P2PHANDSHAKE RPC port changed.
Root cause: peer_nic_path_ stores the full "host:PORT@nic" string. Every sglang KV transfer bootstrap picks a new RPC port, so setPeerNicPath sees a "different" path and calls disconnectUnlocked() + reinserts, even though the EFA peer address is identical. The RPC port is pure handshake metadata — EFA SRD addressing is keyed on the binary efa_addr (GID/QPN) returned by fi_getname(), which is stable for the lifetime of the peer process. The spurious AV churn pays a fi_av_remove + fi_av_insert plus an AH activation warm-up (first-packet latency hit) on every transfer, exactly the performance impact the user was concerned about.
Fix: in setPeerNicPath, compare the normalized nic path (host+NIC, no port) before tearing anything down. If only the port changed, update the stored string and return; keep peer_fi_addr_ and cached_peer_addr_ intact so the next submitPostSend can reuse the already-active AH. A genuine peer restart is still caught downstream by setupConnectionsByPassive's efa_addr == cached_peer_addr_ check (peer QPN differs after restart → different bytes → real re-establish path).
Also normalize the path equality check in setupConnectionsByPassive's sanity guard so the same port rotation doesn't spuriously reject the handshake.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Revert "fix(efa): don't tear down AV slot when only the peer RPC port rotates"
This reverts commit 4548bf56aaf4ac07f3c98b5434a0ab613874fed5.
* fix(efa): key peer_map_ by full host:port@nic to handle multi-worker peers
Root cause of the "Peer reconnected" spam (and the AH warm-up tax on every KV transfer) in sglang 2P2D with DP>1.
EfaContext::endpoint() was keying peer_map_ by normalizeNicPath(peer) which strips the RPC port. The design comment said this was to "reuse handles across reconnections" since each P2PHANDSHAKE init picks a random port. That premise assumed one Mooncake TransferEngine per peer host — true for DP=1 benchmarks but violated under sglang P/D disaggregation: each DP worker is a separate Python process with its own TransferEngine and its own rpc_port, and they all share the same host IP + EFA NIC on that host.
With DP=N peer workers, all N of them collapse onto a single peer_map_ slot. Every incoming handshake from a different DP worker looks to that slot like "peer reconnected with a new address" (different port), so setPeerNicPath() tears down the AV slot (fi_av_remove + fi_av_insert) and the next fi_write pays an AH warm-up. Under bilateral 128-concurrency traffic this devolves into permanent thrashing; the symptoms reported by the user track this model precisely:
- DP=1: no spam, normal performance (single worker per host, normalization harmless).
- DP=2/4: increasing Re-establish log volume, progressive first-packet latency.
- DP=8: thousands of Re-establish per second, decode #running drops to 1, prefill #inflight stays pinned at 128, timeouts.
Fix: key peer_map_ by the full "host:port@nic" path. Each DP worker is a stable, distinct process with a stable port for its entire lifetime, so the cache still hits on every steady-state lookup — no churn. A genuine peer process restart (new rpc_port) leaves the old EfaEndPoint in the map; that is a few bytes of leak per ex-worker, far cheaper than the per-transfer AV teardown it replaces. peekEndpoint, deleteEndpoint, and warmupSegment's post-failure cleanup all switch to the full path for consistency. setPeerNicPath() stays intact as defensive code but will no longer fire in the common path because each new DP worker now gets its own endpoint.
Undoes the buggy 4548bf5 from a completely different direction: the previous attempt tried to skip AV churn at the setPeerNicPath level, which hid real peer restarts and corrupted rkey routing. The correct fix is upstream of that — separate endpoints per peer process so port changes only happen on actual restart.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* style(efa): clang-format-20 fixup
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: EC2 Default User <ec2-user@ip-172-31-8-212.us-west-1.compute.internal>
c02b669669