comms/uniflow/benchmarks: say what --tcp-iface does, and does not, do (#3900) by cppccppccppc · Pull Request #3900 · meta-pytorch/torchcomms

cppccppccppc · 2026-09-01T01:28:27Z

Summary:

Three TCP flags overlap enough to be mistaken for one another, and the one
easiest to misread had no comment at all:

--tcp-iface selects the local address to bind and advertise. It does not
choose the egress NIC -- routing does. On a host where several NICs share a
route, traffic can leave via one device no matter what this names.
--tcp-bind-dev is a bool, not a device name. It promotes whatever
--tcp-iface already names into an SO_BINDTODEVICE pin.
--tcp-bind-devs is the multi-device list, and wins over --tcp-bind-dev
with no warning when both are passed.

The help text called --tcp-iface the "front-end interface for the TCP
transport", which reads like it picks the NIC.

This documents the address-vs-device split on the field and in --help, and
records on tcpBindDevList -- where the precedence is implemented -- that
--tcp-iface keeps selecting the source address when striping, so it wants to
name one of the bound devices. Pointing it at an unbound NIC advertises an
address on a device carrying no lanes: legal, and confusing.

Nothing is added for the parts already covered -- the --tcp-bind-dev field
comment already explains the routing trap, and the --tcp-bind-devs help
already covers i%count, one listener per device, and the peer-agreement
constraints. Restating those would only create places to drift.

Comments and help text only; no behaviour change.

Differential Revision: D117640557

meta-codesync · 2026-09-01T01:29:05Z

@cppccppccppc has exported this pull request. If you are a Meta employee, you can view the originating Diff in D117640557.

…meta-pytorch#3900) Summary: Pull Request resolved: meta-pytorch#3900 Three TCP flags overlap enough to be mistaken for one another, and the one easiest to misread had no comment at all: - `--tcp-iface` selects the local address to bind and advertise. It does not choose the egress NIC -- routing does. On a host where several NICs share a route, traffic can leave via one device no matter what this names. - `--tcp-bind-dev` is a bool, not a device name. It promotes whatever `--tcp-iface` already names into an `SO_BINDTODEVICE` pin. - `--tcp-bind-devs` is the multi-device list, and wins over `--tcp-bind-dev` with no warning when both are passed. The help text called `--tcp-iface` the "front-end interface for the TCP transport", which reads like it picks the NIC. This documents the address-vs-device split on the field and in `--help`, and records on `tcpBindDevList` -- where the precedence is implemented -- that `--tcp-iface` keeps selecting the source address when striping, so it wants to name one of the bound devices. Pointing it at an unbound NIC advertises an address on a device carrying no lanes: legal, and confusing. Nothing is added for the parts already covered -- the `--tcp-bind-dev` field comment already explains the routing trap, and the `--tcp-bind-devs` help already covers `i%count`, one listener per device, and the peer-agreement constraints. Restating those would only create places to drift. Comments and help text only; no behaviour change. Differential Revision: D117640557

Summary: Folded from two adjacent diffs; each half is stated separately below so the two arguments stay reviewable on their own terms. --- accuracy tolerance and ROCm enablement env (was D114431543) -------------- Root-caused the intermittent disagg "accuracy failures" to the accuracy harness + environment, NOT the uniflow connector. The verbose per-mismatch diff added here showed the disagg KV output is correct in every case; the failures were: 1. A degenerate forward pass on the memory-split MI350 instances emits all-'!'/empty garbage -- on the standalone baseline AND (less often) on the disagg output itself. This is environmental (it reproduces on RDMA too, and earlier host-pinning + CU_POINTER_ATTRIBUTE_SYNC_MEMOPS experiments did not change it), not a KV-transfer/connector bug. 2. Strict exact-match flags benign single-token numerical nondeterminism between the disagg and standalone forward passes (e.g. "more" vs "others", which then re-converges). Changes (vllm/fb/standalone_benchmark/disagg_entry.py): - Verbose per-mismatch accuracy diff: full got/expected, first divergence char offset, token_divergence, and the prompt, so failures can be compared token-by-token across transports/connectors. This diagnostic is what isolated the root cause. - Retry the standalone baseline with a fresh LLM up to 3x when any reference is degenerate (all-'!'/empty). - Skip prompts whose BASELINE is degenerate (do not attribute to the connector). - Symmetric guard: skip prompts whose DISAGG OUTPUT is degenerate against a valid baseline (environmental broken forward pass, not a KV/connector failure). - If no prompt is comparable (all degenerate on either side), emit status=baseline_error and fail as INCONCLUSIVE rather than a correctness failure. - Add --accuracy-max-token-divergence N (default 0 = strict exact match) to tolerate <=N differing whitespace tokens as a near-match. Changes (comms/uniflow/benchmarks/run_mast_benchmarks.py): - Forward --accuracy-max-token-divergence to disagg_entry. - MI350/MI300 (ROCm) vLLM enablement env (AMD hosts only): FLASH_ATTENTION_TRITON_AMD_ENABLE=TRUE, VLLM_ROCM_USE_AITER=0, HIPBLASLT_ALLOW_TF32=1 -- stabilizes the forward pass and eliminates the degenerate all-'!' output on MI350. --- TTFT/TPOT and --perf-only (was D114541963) ------------------------------- Adds per-phase latency instrumentation to the disagg accuracy harness plus a --perf-only flag. TTFT is measured as the prefill POST latency and TPOT as the decode POST latency divided by output length (both non-streaming, so robust to server-side streaming quirks). --perf-only skips the correctness gate and runs the perf path directly, for bandwidth/latency sweeps. Plumbed through run_mast_benchmarks.py (Workload.perf_only, arg parser, make_workload, and the forwarded --perf-only app arg). Stacked on the native TCP transport + MAST-enablement work so the same harness can profile RDMA vs TCP. Differential Revision: D114431543

…rrected numbers Summary: --tcp-sockbuf exposes the data connection's SO_SNDBUF/SO_RCVBUF so the value can be swept instead of argued about, and the bench brackets each size's timed loop with TcpTransport::logAndResetPhaseStats() so every bandwidth line is accompanied by where the time went. The script header carried get ~1.0 GB/s, which predated the pinned-staging work and was stale by roughly 9x. It misled a full round of planning, so it is corrected here to measured values along with the link baseline (200G, MTU 1500, RTT 0.046 ms, iperf3 15.4 GB/s single stream and 23.3 GB/s over 8) that makes the numbers interpretable. Recipe 8 records a disproof rather than a hypothesis. The theory was that the 1 MiB buffer pin caps a stream at window/RTT; at 0.046 ms RTT, 1 MiB permits ~22.8 GB/s, so the window was never the constraint. A 6-point sweep with 3 repeats does show the 1 MiB pin is the worst non-degenerate setting (8.63 GB/s at 1 GiB against 9.67 unpinned, disjoint ranges), but the mechanism is autotuning being disabled while the reader does a multi-MiB copy, not the bandwidth-delay product. The 64K arm is a deliberate control: it drops throughput 75%, which is what makes the flat region above 1 MiB trustworthy rather than merely consistent with a dead knob. Also records that a --no-verify arm must never be compared against a verifying one -- that mistake inverted this exact comparison once. Differential Revision: D117132684

Summary: Phase 3d makes slab-backed VRAM ReadReply copies asynchronous on the caller stream. The transport retains the receive slab and destination write reservation until CUDA event completion, polls all pending events from the EventBase so later copies can retire out of order, and drains or quarantines copies safely across query errors and shutdown. Vector-backed fallback remains synchronous because its source storage is reused immediately. The benchmark exposes --no-tcp-async-h2d for controlled comparisons and reports the active mode. # A failed completion probe is not a failed copy Two paths conflated "I could not observe this copy finish" with "this copy did not finish", and reported a transfer error for data that had demonstrably landed. In `stageAsyncH2d`, a failing `eventRecord` arrives after `memcpyAsync` has already succeeded, so the copy is in flight and only the tracking mechanism is gone. The code falls back to `waitForH2dCopy`, and if that wait succeeds the copy has completed and the destination holds the payload -- but the old code then returned the `eventRecord` error, failing the caller's get for a copy that worked. It now completes with `Ok()`. `pollPendingH2d` had the same shape: a failing `eventQuery` left its error in `result`, and a successful `waitForH2dCopy` fell through without clearing it, retiring a completed copy as failed. The recovery now sets `result = Ok()` before retirement. The sibling site at the end of `stageAsyncH2d` deliberately keeps propagating `status`: there the error is the transport stopping or the pending record failing to track, which is a real operation failure even though the synchronize proves the copy finished. Only the two probe-failure paths change. Differential Revision: D117155852

Summary: TcpAsyncAcceptTest derived the address family by comparing the param's clientHost against the "127.0.0.1" literal. That defaults every other spelling -- "localhost", "[::1]", a resolvable hostname -- to AF_INET6 without saying so, and the two socket-buffer tests feed that family to kernelDefaultRcvBuf/rcvBufForRequest. A param added later would probe the wrong family and surface as a confusing skip or a wrong expectation rather than a clear failure. Store the family on AddrFamily and state it at the two INSTANTIATE_TEST_SUITE_P entries, so a new param has to declare which family it is. A helper function would have deduplicated the comparison but kept the default. The five sites this replaces are the two family derivations in the socket-buffer tests, the socket()/sockaddr pair in AsyncAcceptRejectsNonUniflowClient, and the test-name lambda -- which now derives the name from the same field the bodies use, so the two cannot disagree. AddrFamily is defined separately in four test files; this changes only TcpAsyncAcceptTest.cpp. The same pattern remains in TcpConnTest.cpp:48 (inverse polarity) and the other three name lambdas. Differential Revision: D117407765

… sockets Summary: D117132557 threaded TcpSocketConfig into the accept path but configureAcceptedSocket consumed only socketBufSize and hardcoded the other seven fields. That is a worse shape than the old `int acceptRetryCnt` Differential Revision: D117409938

Summary: sendAllVec took (iovec*, int) and tracked its position with a separate idx cursor, so the loop carried `iov + idx`, `iovCnt - idx` and `iov[idx]` arithmetic. std::span carries the count with the pointer, which lets the cursor go away entirely: the retire step becomes iov = iov.subspan(1) and the loop condition becomes !iov.empty(). That is the real win rather than the shorter signature. The arithmetic being deleted lives in the short-write branch, which the comment there notes is not reachable on a blocking socket except via a signal mid-transfer and is not covered by tests -- so it is the least safe place in the function to keep hand-rolled index math. It also drops the static_cast<size_t> on msg_iovlen, since size() is already size_t, and matches the idiom the rest of the interface uses: std::span<const uint8_t> on send, std::span<uint8_t> on recv. <span> was already included and sendAllVec is private with one call site, so there is no ABI consideration. The call site's count becomes size_t and passes std::span{iov}.first(iovCnt), so the cast is removed rather than relocated to the caller. Kept the doc comment. A non-const std::span<iovec> conveys no more about mutation than a non-const iovec* did; what the comment carries is that the mutation is destructive bookkeeping -- the iov must not be reused after the call -- and no signature expresses that. Differential Revision: D117414473

…the transport Summary: Folded from two adjacent diffs; each half is stated separately below so the two arguments stay reviewable on their own terms. --- controller: recv header-wait and payload-drain timings (was D117632427) --- TcpConn::syncRecv() populates the existing RecvPhaseStats with the time spent waiting for the length prefix versus draining the payload, plus frame and byte counts. Splitting the two phases is what makes a slow receive interpretable: a large headerWaitNs means we were waiting on the peer, while a large payloadDrainNs means the socket itself was the limit. All four counters are relaxed fetch_add on atomics already declared in Controller.h, so this adds two steady_clock reads per frame and no synchronisation. --- transport: receive-slab hits, misses and vector receives (was D117632428) --- Adds receiveSlabAttempts_, receiveSlabMisses_, and vectorReceiveCount_ to TcpTransport and reports them on the existing tcp phases log line. A miss means the reader could not get a pinned slab and fell back to a vector-backed receive, which changes the H2D path for that frame, so distinguishing the two is necessary before drawing conclusions from an aggregate drain number. The counters are relaxed atomics incremented in readerLoop(). Differential Revision: D117632428

…rate Summary: A single TCP connection is bounded by what one sender thread can push. On MI350/eth2 that capped a 4 MiB get at ~10.8 GB/s, 46% of the 23.25 GB/s the NIC can carry. Per-thread sampling put the responder's hottest thread at 90.6% of a core, and no configuration change adds a thread: not batch size, not tx-depth (the transport already pipelines a large get's chunks internally), not larger frames, and not DRAM instead of VRAM -- DRAM measured worse. This adds N parallel data sockets per peer connection, one reader and one sender thread per socket, and round-robins frames across them. lanes 1 2 4 8 GB/s 10.78 15.81 23.14 23.10 4 lanes reaches 99.5% of the NIC ceiling. Two things make that credible beyond the median: the 4-lane spread over 4 runs is +/-0.4% where every earlier configuration scattered by +/-20%, which is the signature of a hard ceiling rather than a noisy bottleneck, and 8 lanes buys nothing, which is what saturation looks like. put gains equally, 22.42 GB/s. The default is 4 lanes per connection. Design notes: - Lane identity comes off the wire (TcpLaneHello), not from accept order: the dialer opens the lanes with no ordering guarantee. - The hello is skipped entirely at 1 lane, so that path stays byte-identical and kTcpWireVersion does not move. Bumping it would make every new peer incompatible with every old one, too high a price for a field only multi-lane peers read. A lane-count mismatch is a clean handshake error instead. - Send is pinned to lane 0 and never striped. send()/recv() are a FIFO-matched rendezvous, so striping would pair a SEND with the wrong recv: silent corruption rather than an error. put/get frames each carry their own reqId/segId/offset and so need no ordering help. - enqueueFrames keeps a whole group on one lane, so its all-or-nothing contract costs a single mutex. Spreading a group across lanes would need every target lane's mutex held while waiting for room, which deadlocks against the very senders that would free it. - The per-lane queue cap is kMaxOutQueueBytes / laneCount, so the aggregate buffered bound is unchanged however many lanes are configured. - A failed lane closes every lane's queue: one lane failing already takes the transport down, so leaving the others admitting frames would queue work for a connection that is gone, with no consumer. Rollout: above 1 lane the peers exchange a hello, so a 4-lane peer cannot talk to one that predates lanes or is pinned to 1. The binary needs to be everywhere before the default takes effect on connections that straddle a rollout. Cost is per connection, not per NIC: a process holding 32 peers opens 128 sockets and 256 transport threads on that interface. Untested at high fan-out. Differential Revision: D117545615

…meta-pytorch#3900) Summary: Pull Request resolved: meta-pytorch#3900 Three TCP flags overlap enough to be mistaken for one another, and the one easiest to misread had no comment at all: - `--tcp-iface` selects the local address to bind and advertise. It does not choose the egress NIC -- routing does. On a host where several NICs share a route, traffic can leave via one device no matter what this names. - `--tcp-bind-dev` is a bool, not a device name. It promotes whatever `--tcp-iface` already names into an `SO_BINDTODEVICE` pin. - `--tcp-bind-devs` is the multi-device list, and wins over `--tcp-bind-dev` with no warning when both are passed. The help text called `--tcp-iface` the "front-end interface for the TCP transport", which reads like it picks the NIC. This documents the address-vs-device split on the field and in `--help`, and records on `tcpBindDevList` -- where the precedence is implemented -- that `--tcp-iface` keeps selecting the source address when striping, so it wants to name one of the bound devices. Pointing it at an unbound NIC advertises an address on a device carrying no lanes: legal, and confusing. Nothing is added for the parts already covered -- the `--tcp-bind-dev` field comment already explains the routing trap, and the `--tcp-bind-devs` help already covers `i%count`, one listener per device, and the peer-agreement constraints. Restating those would only create places to drift. Comments and help text only; no behaviour change. Differential Revision: D117640557

… several Summary: Adds opt-in device binding to the TCP transport and uses it to place lanes on more than one NIC, which is what gets a two-host `get` past a single port's ~21 GB/s ceiling. Two pieces, kept in one diff because the second is unusable without the first. **1. `SO_BINDTODEVICE` on TCP sockets.** `TcpSocketConfig::bindToDevice` pins egress to a named interface. It is applied before `bind()`/`connect()` at all three socket-creation sites, since the option has no effect afterwards. `validate()` rejects empty and over-long names. Unset (the default) behaves exactly as before, so no existing caller changes behaviour. **2. Per-lane striping.** `TcpTransportConfig::bindToDevices` places lane `i` on device `i % D`. Both ends need it: for a `get` the bulk data flows listener to dialer and the listener's egress follows its own routing table, so binding only the dialer leaves all `get` payload on one NIC. `bind()` therefore creates one listener per device and advertises all of them; accepted sockets inherit the listener's `sk_bound_dev_if`, which is what actually places the bytes. The wire change is backward compatible, contrary to what the plan predicted. Extra endpoints append after the host bytes as more `{port, hostLen, host}` records and the exact-size check in `deserialize()` becomes "all bytes consumed", which still rejects truncation and trailing junk. A single-device transport serializes byte-identically to a pre-striping build, asserted by `SingleEndpointWireIsUnchanged`. `TcpTopologyInfo` did not have to change, so no coordinated rollout is needed; only the multi-device form requires a new build on both peers, and `connect()` rejects a peer whose device count disagrees. Also fixes two things found while measuring: - `getInterfaceIpv6()` returned the first global address, but eth1/eth3/eth9 on these hosts each carry a deprecated `2803:` address alongside the live `2401:` one. It binds fine and is indistinguishable at the socket layer, but the peer will not reply to it. Replaced by `controller::deviceGlobalIpv6()`, which reads `/proc/net/if_inet6` because `getifaddrs()` cannot express address flags (`ifa_flags` carries the interface's `IFF_*` bits, not the address's `IFA_F_*`). eth2 has no deprecated address, which is why earlier eth2-only runs were unaffected. - The listener pre-arms one `accept()` per device before blocking on any of them. `AsyncAccept` registers its listen fd with the EventBase lazily on the first `accept()`, and an accepted socket's magic exchange runs from that fd's EPOLLIN handler, so accepting device-by-device deadlocks once a device owns two lanes: the listener waits for device 0's second lane, which the dialer only dials after device 1's lane succeeds, which it cannot. The kernel backlog holds the connection but not the application handshake, which times out at 500ms. `logAndResetPhaseStats` now sums every lane instead of reading lane 0 and reports `frames_per_lane`, so the phase line stays meaningful with lanes on different NICs. Benchmark: `--tcp-bind-dev` (single device, follows `--tcp-iface`) and `--tcp-bind-devs eth1,eth2` (explicit list). Device lists are per-host and need not match between peers, because the same physical port is eth3 on one MI350 host and eth0 on the next. Differential Revision: D117632611

…meta-pytorch#3900) Summary: Pull Request resolved: meta-pytorch#3900 Three TCP flags overlap enough to be mistaken for one another, and the one easiest to misread had no comment at all: - `--tcp-iface` selects the local address to bind and advertise. It does not choose the egress NIC -- routing does. On a host where several NICs share a route, traffic can leave via one device no matter what this names. - `--tcp-bind-dev` is a bool, not a device name. It promotes whatever `--tcp-iface` already names into an `SO_BINDTODEVICE` pin. - `--tcp-bind-devs` is the multi-device list, and wins over `--tcp-bind-dev` with no warning when both are passed. The help text called `--tcp-iface` the "front-end interface for the TCP transport", which reads like it picks the NIC. This documents the address-vs-device split on the field and in `--help`, and records on `tcpBindDevList` -- where the precedence is implemented -- that `--tcp-iface` keeps selecting the source address when striping, so it wants to name one of the bound devices. Pointing it at an unbound NIC advertises an address on a device carrying no lanes: legal, and confusing. Nothing is added for the parts already covered -- the `--tcp-bind-dev` field comment already explains the routing trap, and the `--tcp-bind-devs` help already covers `i%count`, one listener per device, and the peer-agreement constraints. Restating those would only create places to drift. Comments and help text only; no behaviour change. Differential Revision: D117640557

meta-codesync · 2026-09-03T18:05:35Z

This pull request has been merged in a12a629.

meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 1, 2026

meta-codesync Bot added the meta-exported label Sep 1, 2026

meta-codesync Bot changed the title ~~comms/uniflow/benchmarks: say what --tcp-iface does, and does not, do~~ comms/uniflow/benchmarks: say what --tcp-iface does, and does not, do (#3900) Sep 1, 2026

cppccppccppc force-pushed the export-D117640557 branch from 68863d7 to ca6462b Compare September 1, 2026 07:05

cppccppccppc force-pushed the export-D117640557 branch 2 times, most recently from 2d58c3a to 47bf541 Compare September 1, 2026 15:26

cppccppccppc force-pushed the export-D117640557 branch from 47bf541 to 08ceba1 Compare September 1, 2026 15:31

cppccppccppc force-pushed the export-D117640557 branch from 08ceba1 to 95e505e Compare September 2, 2026 07:55

cppccppccppc force-pushed the export-D117640557 branch from 95e505e to a2e4ceb Compare September 2, 2026 08:08

cppccppccppc force-pushed the export-D117640557 branch from a2e4ceb to c748096 Compare September 2, 2026 08:25

Peng Chen added 8 commits September 2, 2026 12:08

cppccppccppc force-pushed the export-D117640557 branch from c748096 to ceb23f4 Compare September 2, 2026 23:50

Peng Chen added 2 commits September 2, 2026 16:54

cppccppccppc force-pushed the export-D117640557 branch from ceb23f4 to 3b7a782 Compare September 3, 2026 00:08

meta-codesync Bot closed this in a12a629 Sep 3, 2026

meta-codesync Bot added the Merged label Sep 3, 2026

Navigation Menu

Uh oh!

comms/uniflow/benchmarks: say what --tcp-iface does, and does not, do (#3900) - #3900

comms/uniflow/benchmarks: say what --tcp-iface does, and does not, do (#3900)#3900
cppccppccppc wants to merge 10 commits into
meta-pytorch:mainfrom
cppccppccppc:export-D117640557

cppccppccppc commented Sep 1, 2026 •

edited by meta-codesync Bot

Loading

Uh oh!

meta-codesync Bot commented Sep 1, 2026

Uh oh!

meta-codesync Bot commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

cppccppccppc commented Sep 1, 2026 • edited by meta-codesync Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meta-codesync Bot commented Sep 1, 2026

Uh oh!

meta-codesync Bot commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cppccppccppc commented Sep 1, 2026 •

edited by meta-codesync Bot

Loading