comms/uniflow/tcp: count data sockets per NIC, and widen the lane hello to match (#3922) by cppccppccppc · Pull Request #3922 · meta-pytorch/torchcomms

cppccppccppc · 2026-09-01T01:32:30Z

Summary:

Folded from two adjacent diffs; each half is stated separately below so the two
arguments stay reviewable on their own terms.

--- count lanes per NIC (was D117721988) -------------------------------------

numSockets was the total lane count for a connection, so striping divided it:
the default 4 gave 4 lanes on one NIC and still only 4 across two, two lanes per
device. That is the configuration measured at 27.6 GB/s with both NICs half-fed,
against ~35 GB/s for 8 lanes. Reaching the striped number required every caller
to know to double the value, and nothing enforced it -- the only guard was that
lanes must not fall below the device count.

The lane count is now per device: laneCount = numSocketsPerDevice * devices.
The default 4 gives 4 lanes on one NIC, unchanged, and 8 across two. The field
and CLI flag are renamed (numSocketsPerDevice, --tcp-sockets-per-nic) because
the value's meaning changed; leaving the old names would let a stale
--tcp-num-sockets 8 silently become 16 lanes.

Two consequences fall out of the arithmetic:

The kMaxLanes 255 cap now applies to the product, not the configured value,
since the lane hello addresses lanes with a uint8_t.
The "lanes below device count" guard is unreachable: the product is always at
least the device count. Removed, along with the test comment describing it.

Because 4 lanes per device is what the per-NIC ceiling measurement actually
supports -- one 200G NIC saturates near 4 lanes at ~21 GB/s -- per-device is also
the unit the default was always implicitly expressed in.

--- widen the hello fields that count forced (was D117726316) ----------------

kMaxLanes was 255 because TcpLaneHello::laneIndex and laneCount were each a
uint8_t. That ceiling now binds sooner than it used to: lanes are counted per
device, so the total is numSocketsPerDevice * devices and the device count
contributes to it, rather than the total being something a caller states
directly.

Both fields become uint16_t and the spare rsvd byte goes away, taking the
struct from 16 to 17 bytes and its static_assert with it. kMaxLanes is raised
to 1024.

Raising the constant alone would have been silently wrong rather than merely
insufficient: the send path casts with static_cast<uint8_t>(i), so lane 256
would have gone out as index 0, two lanes would have claimed the same slot, and
the listener would have mis-paired sockets with no error anywhere.

kTcpWireVersion is deliberately not bumped. The hello is only exchanged when
more than one lane is configured, so a single-lane transport stays byte-identical
to a peer built before lanes existed, and only multi-lane peers read these bytes
-- the same reasoning already recorded on the struct for why adding it did not
need a version bump. Multi-lane peers across this change do not interoperate,
which is acceptable while that code is unlanded.

Differential Revision: D117721988

meta-codesync · 2026-09-01T01:33:13Z

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

…lo to match (meta-pytorch#3922) Summary: Pull Request resolved: meta-pytorch#3922 Folded from two adjacent diffs; each half is stated separately below so the two arguments stay reviewable on their own terms. --- count lanes per NIC (was D117721988) ------------------------------------- `numSockets` was the total lane count for a connection, so striping divided it: the default 4 gave 4 lanes on one NIC and still only 4 across two, two lanes per device. That is the configuration measured at 27.6 GB/s with both NICs half-fed, against ~35 GB/s for 8 lanes. Reaching the striped number required every caller to know to double the value, and nothing enforced it -- the only guard was that lanes must not fall *below* the device count. The lane count is now per device: `laneCount = numSocketsPerDevice * devices`. The default 4 gives 4 lanes on one NIC, unchanged, and 8 across two. The field and CLI flag are renamed (`numSocketsPerDevice`, `--tcp-sockets-per-nic`) because the value's meaning changed; leaving the old names would let a stale `--tcp-num-sockets 8` silently become 16 lanes. Two consequences fall out of the arithmetic: - The `kMaxLanes` 255 cap now applies to the product, not the configured value, since the lane hello addresses lanes with a uint8_t. - The "lanes below device count" guard is unreachable: the product is always at least the device count. Removed, along with the test comment describing it. Because 4 lanes per device is what the per-NIC ceiling measurement actually supports -- one 200G NIC saturates near 4 lanes at ~21 GB/s -- per-device is also the unit the default was always implicitly expressed in. --- widen the hello fields that count forced (was D117726316) ---------------- `kMaxLanes` was 255 because `TcpLaneHello::laneIndex` and `laneCount` were each a `uint8_t`. That ceiling now binds sooner than it used to: lanes are counted per device, so the total is `numSocketsPerDevice * devices` and the device count contributes to it, rather than the total being something a caller states directly. Both fields become `uint16_t` and the spare `rsvd` byte goes away, taking the struct from 16 to 17 bytes and its static_assert with it. `kMaxLanes` is raised to 1024. Raising the constant alone would have been silently wrong rather than merely insufficient: the send path casts with `static_cast<uint8_t>(i)`, so lane 256 would have gone out as index 0, two lanes would have claimed the same slot, and the listener would have mis-paired sockets with no error anywhere. kTcpWireVersion is deliberately not bumped. The hello is only exchanged when more than one lane is configured, so a single-lane transport stays byte-identical to a peer built before lanes existed, and only multi-lane peers read these bytes -- the same reasoning already recorded on the struct for why adding it did not need a version bump. Multi-lane peers across this change do not interoperate, which is acceptable while that code is unlanded. Differential Revision: D117721988

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

…lo to match (meta-pytorch#3922) Summary: Pull Request resolved: meta-pytorch#3922 Folded from two adjacent diffs; each half is stated separately below so the two arguments stay reviewable on their own terms. --- count lanes per NIC (was D117721988) ------------------------------------- `numSockets` was the total lane count for a connection, so striping divided it: the default 4 gave 4 lanes on one NIC and still only 4 across two, two lanes per device. That is the configuration measured at 27.6 GB/s with both NICs half-fed, against ~35 GB/s for 8 lanes. Reaching the striped number required every caller to know to double the value, and nothing enforced it -- the only guard was that lanes must not fall *below* the device count. The lane count is now per device: `laneCount = numSocketsPerDevice * devices`. The default 4 gives 4 lanes on one NIC, unchanged, and 8 across two. The field and CLI flag are renamed (`numSocketsPerDevice`, `--tcp-sockets-per-nic`) because the value's meaning changed; leaving the old names would let a stale `--tcp-num-sockets 8` silently become 16 lanes. Two consequences fall out of the arithmetic: - The `kMaxLanes` 255 cap now applies to the product, not the configured value, since the lane hello addresses lanes with a uint8_t. - The "lanes below device count" guard is unreachable: the product is always at least the device count. Removed, along with the test comment describing it. Because 4 lanes per device is what the per-NIC ceiling measurement actually supports -- one 200G NIC saturates near 4 lanes at ~21 GB/s -- per-device is also the unit the default was always implicitly expressed in. --- widen the hello fields that count forced (was D117726316) ---------------- `kMaxLanes` was 255 because `TcpLaneHello::laneIndex` and `laneCount` were each a `uint8_t`. That ceiling now binds sooner than it used to: lanes are counted per device, so the total is `numSocketsPerDevice * devices` and the device count contributes to it, rather than the total being something a caller states directly. Both fields become `uint16_t` and the spare `rsvd` byte goes away, taking the struct from 16 to 17 bytes and its static_assert with it. `kMaxLanes` is raised to 1024. Raising the constant alone would have been silently wrong rather than merely insufficient: the send path casts with `static_cast<uint8_t>(i)`, so lane 256 would have gone out as index 0, two lanes would have claimed the same slot, and the listener would have mis-paired sockets with no error anywhere. kTcpWireVersion is deliberately not bumped. The hello is only exchanged when more than one lane is configured, so a single-lane transport stays byte-identical to a peer built before lanes existed, and only multi-lane peers read these bytes -- the same reasoning already recorded on the struct for why adding it did not need a version bump. Multi-lane peers across this change do not interoperate, which is acceptable while that code is unlanded. Differential Revision: D117721988

… 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

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

Summary: `MultiTransportFactory` hardcoded `controller::TcpSocketConfig{}` when it registered the TCP factory, so the whole `TcpTransportConfig` surface was unreachable from the public API. Every caller that goes through `UniflowAgent` -- SGLang, vLLM, TorchStore -- was locked to the compiled-in defaults for `socketBufSize`, `numSockets`, `bindToDevices`, `tcpNoDelay`, keepalive, and `userTimeout`. Lane striping in particular was unreachable: the transport gained the capability but nothing outside the C++ benchmarks could ask for it. This adds `tcpTransportConfig` to `MultiTransportFactoryOptions` and forwards it from `UniflowAgentConfig`. Null keeps the transport's own defaults, so no behaviour changes for anyone who does not set it. # Why the whole struct, by pointer The field is a `shared_ptr<const TcpTransportConfig>` to a forward-declared type rather than an `optional<TcpTransportConfig>` by value. TCP is AMD-only -- see the `ovr_config//gpu:amd` deps in BUCK, and `tcp-transport` is deliberately not in `exported_deps` -- so a by-value member would drag `TcpTransport.h` into every consumer of `MultiTransport.h` and fail to compile where the transport is not built. `optional` needs a complete type; `shared_ptr` does not, and its deleter is type-erased at construction, so declaration and destruction are both fine against an incomplete type. The struct layout stays platform-independent, so callers need no `#ifdef` to leave it unset. Whole struct rather than mirrored individual fields: the alternative duplicates `TcpTransportConfig`'s fields and their defaults in two more headers, which then drift. The cost is that callers can reach the socket timeouts that `configureAcceptedSocket` has ordering constraints around, which is why the field documents itself as an override seam rather than something to populate wholesale. # Why this is a config path and not a better default I went looking for a wrong default and did not find one -- I found that no single value is right. `--tcp-sockbuf` against `SO_SNDBUF`/`SO_RCVBUF`, get, 64 MiB, tx-depth 2, medians of 5 on two MI350 hosts, as autotune/1MiB: 1 NIC, 1 lane 1.256x 11.62 against 9.25 autotune wins 1 NIC, 4 lanes 0.976x 20.28 against 20.77 pinning wins 1 NIC, 8 lanes 1.007x 21.98 against 21.83 wash 2 NIC, 4 lanes 1.195x 28.01 against 23.43 autotune wins, ranges disjoint 2 NIC, 8 lanes 1.101x 38.49 against 34.95 autotune wins Pinning `SO_RCVBUF` disables Linux receive-window autotuning and caps a stream at window/RTT, so it hurts most when per-lane bandwidth is high and helps slightly when it is not. That flips with NIC count, lane count, and fanout -- and lane count is per-connection, so a rank holds `numSockets * peers` sockets and its aggregate window depends on a peer count the transport cannot see. The 1 MiB default is left alone: it is defensible on one NIC, and picking any other constant would just be wrong somewhere else. Differential Revision: D117643757

…lo to match (meta-pytorch#3922) Summary: Pull Request resolved: meta-pytorch#3922 Folded from two adjacent diffs; each half is stated separately below so the two arguments stay reviewable on their own terms. --- count lanes per NIC (was D117721988) ------------------------------------- `numSockets` was the total lane count for a connection, so striping divided it: the default 4 gave 4 lanes on one NIC and still only 4 across two, two lanes per device. That is the configuration measured at 27.6 GB/s with both NICs half-fed, against ~35 GB/s for 8 lanes. Reaching the striped number required every caller to know to double the value, and nothing enforced it -- the only guard was that lanes must not fall *below* the device count. The lane count is now per device: `laneCount = numSocketsPerDevice * devices`. The default 4 gives 4 lanes on one NIC, unchanged, and 8 across two. The field and CLI flag are renamed (`numSocketsPerDevice`, `--tcp-sockets-per-nic`) because the value's meaning changed; leaving the old names would let a stale `--tcp-num-sockets 8` silently become 16 lanes. Two consequences fall out of the arithmetic: - The `kMaxLanes` 255 cap now applies to the product, not the configured value, since the lane hello addresses lanes with a uint8_t. - The "lanes below device count" guard is unreachable: the product is always at least the device count. Removed, along with the test comment describing it. Because 4 lanes per device is what the per-NIC ceiling measurement actually supports -- one 200G NIC saturates near 4 lanes at ~21 GB/s -- per-device is also the unit the default was always implicitly expressed in. --- widen the hello fields that count forced (was D117726316) ---------------- `kMaxLanes` was 255 because `TcpLaneHello::laneIndex` and `laneCount` were each a `uint8_t`. That ceiling now binds sooner than it used to: lanes are counted per device, so the total is `numSocketsPerDevice * devices` and the device count contributes to it, rather than the total being something a caller states directly. Both fields become `uint16_t` and the spare `rsvd` byte goes away, taking the struct from 16 to 17 bytes and its static_assert with it. `kMaxLanes` is raised to 1024. Raising the constant alone would have been silently wrong rather than merely insufficient: the send path casts with `static_cast<uint8_t>(i)`, so lane 256 would have gone out as index 0, two lanes would have claimed the same slot, and the listener would have mis-paired sockets with no error anywhere. kTcpWireVersion is deliberately not bumped. The hello is only exchanged when more than one lane is configured, so a single-lane transport stays byte-identical to a peer built before lanes existed, and only multi-lane peers read these bytes -- the same reasoning already recorded on the struct for why adding it did not need a version bump. Multi-lane peers across this change do not interoperate, which is acceptable while that code is unlanded. Differential Revision: D117721988

meta-codesync · 2026-09-03T18:24:12Z

This pull request has been merged in 392372b.

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/tcp: count data sockets per NIC, and widen the lane hello to match~~ comms/uniflow/tcp: count data sockets per NIC, and widen the lane hello to match (#3922) Sep 1, 2026

cppccppccppc force-pushed the export-D117721988 branch from a2ffa41 to d50bf98 Compare September 1, 2026 07:05

cppccppccppc force-pushed the export-D117721988 branch from d50bf98 to 2299bdf Compare September 1, 2026 07:14

cppccppccppc force-pushed the export-D117721988 branch from 2299bdf to 3101df6 Compare September 1, 2026 15:27

cppccppccppc force-pushed the export-D117721988 branch from 3101df6 to f2f9ef9 Compare September 1, 2026 16:01

cppccppccppc force-pushed the export-D117721988 branch from f2f9ef9 to d32038b Compare September 2, 2026 07:56

cppccppccppc force-pushed the export-D117721988 branch from d32038b to a072939 Compare September 2, 2026 08:08

cppccppccppc force-pushed the export-D117721988 branch from a072939 to ae6e393 Compare September 2, 2026 08:14

cppccppccppc force-pushed the export-D117721988 branch from ae6e393 to 4179c1e Compare September 2, 2026 08:25

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

cppccppccppc force-pushed the export-D117721988 branch from 4179c1e to 57d3a8b Compare September 2, 2026 23:49

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

cppccppccppc force-pushed the export-D117721988 branch from 57d3a8b to 3dbccc5 Compare September 3, 2026 00:08

meta-codesync Bot closed this in 392372b Sep 3, 2026

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

Navigation Menu

Uh oh!

comms/uniflow/tcp: count data sockets per NIC, and widen the lane hello to match (#3922) - #3922

comms/uniflow/tcp: count data sockets per NIC, and widen the lane hello to match (#3922)#3922
cppccppccppc wants to merge 12 commits into
meta-pytorch:mainfrom
cppccppccppc:export-D117721988

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