Repository navigation
fix(uring): match tokio's MTU and honor quic.mtu_discovery - #6414
Conversation
Quest-Claim: b9932b2c63486756
Every send error but ECONNREFUSED, an unreachable destination, and the GSO fallback failed the io_uring socket for good, and with it every connection its worker serves. EPERM is the realistic trigger: a netfilter drop or a full conntrack table, which a spoofed-source flood can fill. Classify by errno: unreachable stays per destination; EBADF, ENOTSOCK and EFAULT, which mean the socket itself is unusable, still fail it; everything else, a short send included, is loss, logged at most once a minute (ECONNREFUSED and EMSGSIZE at debug). Receives also tolerate ECONNRESET and EMSGSIZE, as tokio does. The ring's sockets now set IP(V6)_PMTUDISC_PROBE and IPV6_DONTFRAG like moq-sock's, so a forged ICMP fragmentation-needed cannot shrink a path below a GSO train. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeyiEy5FWz3z44DYZVo7vU
TxBuf::send still promised every failed send surfaced on the next pool acquire. Only an unusable socket, or an unrouted unreachable send, does now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ring pinned its initial and minimum MTU at 1350 bytes and advertised a 1350-byte max_udp_payload_size, so a path carrying fewer bytes (an IPv6 path at its 1280-byte minimum, a tunnel) black-holed the handshake. Drop the overrides so the ring starts at moq-quic's 1200-byte floor like tokio, add `quic::Transport::mtu_discovery` (off by default), allow discovery on the endpoint now that every socket refuses to fragment, and size each GSO train from the connection's current MTU. The relay maps `quic.mtu_discovery` instead of refusing it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeyiEy5FWz3z44DYZVo7vU
kixelated
changed the title
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Outcome: the quest is complete and the PR is marked ready. No decisions are open. Two judgment calls, both open to challenge:
Sequencing: merge #6397 first, then rebase this branch to drop its cherry-picked commits. (Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Merging. Matches tokio's MTU defaults on io_uring and honors quic.mtu_discovery, as the maintainer chose. CI is green, the current head has a non-Grok review, and (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/README.md # quest/m1/uring-local-addr.md # quest/m1/uring-mtu.md # quest/m2/relay-io-uring-package.md # rs/moq-uring/src/udp.rs # rs/moq-uring/tests/integration/main.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged
Checks: (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/README.md
|
Merged (Written by Claude Opus 5.5) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f4937569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // moq-quic probes by default; the 1200-byte floor reaches every path. | ||
| if !config.mtu_discovery { | ||
| transport.mtu_discovery_config(None); | ||
| } |
There was a problem hiding this comment.
When a direct moq-uring consumer enables discovery with a valid smaller udp::Config::tx_buffer_len, such as 1350 bytes, the handshake succeeds but discovery eventually terminates the connection. The default discovery upper bound is 1452 bytes, and moq_quic::Connection::poll_transmit emits individual probes larger than current_mtu() regardless of the calculated segment count. udp::TxBuf::send rejects these probes because their length exceeds the configured buffer ceiling, and flush_one treats that rejection as a terminal I/O error. Bound discovery's upper limit by the socket's transmit-buffer size so enabling discovery does not break connections using smaller buffers.
Useful? React with 👍 / 👎.
Completes
quest/m1/uring-mtu.md.Problem
The io_uring QUIC driver pinned its initial and minimum MTU at 1350 bytes, turned discovery off, and advertised a 1350-byte
max_udp_payload_size. moq-quic pads the client Initial to the current MTU, so any path that cannot carry 1350 bytes (an IPv6 path at its 1280-byte link minimum, a tunnel) black-holed the handshake with no fallback. Tokio starts at the 1200-byte floor. The relay also refusedquic.mtu_discoveryunder--runtime-io-uring.Reproduced first: a userspace hop that drops anything above 1232 bytes (IPv6 minimum less headers) timed the handshake out, and with no cap the ring sent 1350-byte datagrams with discovery "off".
Changes
rs/moq-uring/src/quic/driver/mod.rs: drop theinitial_mtu/min_mtupins and themax_udp_payload_sizeoverride, so the ring uses moq-quic's defaults like tokio (1200 floor, 1472 advertised). Discovery is turned off unlessTransport::mtu_discoveryis set, mirroringmoq_tokio::quic::backend::apply_transport.endpoint_configcan no longer fail, so it returns the config directly.rs/moq-uring/src/quic/mod.rs: deleteSEGMENT; addTransport::mtu_discovery: bool(defaultfalse).rs/moq-uring/src/quic/driver/endpoint.rs: passallow_mtud = true. This is sound because everyudp::Socketnow setsIP_PMTUDISC_PROBEandIPV6_DONTFRAG(from moq-dev/moq#6397), so an oversized probe is lost rather than fragmented. Without it, discovery would stay off whatever the setting.rs/moq-uring/src/quic/driver/connection.rs: size each GSO train ascapacity / current_mtu, notcapacity / 1350. At a discovered 1452 the old formula gives 48 segments, 69,696 bytes, which overflows the 64 KiB transmit buffer.DATAGRAM_WINDOWwas64 * SEGMENT(86,400 bytes). It is now a plain 96 KiB, about 64 datagrams at the largest MTU discovery reaches.rs/moq-relay/src/uring.rs: mapquic.mtu_discoveryonto the workers instead of refusing it, with a unit test that the default and the set value both arrive.doc/bin/relay/config.mdno longer says io_uring refuses it.quest/m1/uring-mtu.mdand its links.quest/m2/perf/uring-profile.mdkeeps its GSO-on-jumbo-paths item, with the dead link removed.Tests
rs/moq-uring/tests/integration/mtu.rsputs two ring endpoints behind a userspace hop that silently drops datagrams above an MTU you can change at runtime. The tests run on the wall clock because they cross real sockets, and they take 0.1 to 0.6 s each:a_path_at_the_ipv6_minimum_connects: a 1232-byte path connects and carries 1 MiB each way. Fails before this PR (handshake timeout).discovery_stays_off_by_default: no datagram either way exceeds 1200. Fails before this PR (1350).discovery_finds_a_larger_mtu: with discovery on over a 1472-byte path, most of a 1 MiB transfer rides above the floor, and GSO trains still batch (workertx_datagramsabove twicetx_sends; measured about 18x). It fails if trains are sized with the old 1350 or ifallow_mtudstaysfalse(both mutations checked).a_shrinking_path_falls_back_to_the_floor: after discovery the path drops to 1232 mid-connection, and 1 MiB each way still completes once black-hole detection falls back.Stress run: 30/30 iterations of the four tests passed. Like the other moq-uring integration tests, these skip below the kernel floor and run in CI on
ubuntu-24.04-arm(release gate and nightlyrs uring) underMOQ_STRICT=1.Public API / wire
mtu_discovery: boolfield on the#[non_exhaustive]moq_uring::quic::Transport, named aftermoq_tokio::quic::Resolved::mtu_discovery. The crate-privateSEGMENTis removed. Behavior change for directmoq-uringusers: connections start at 1200 bytes instead of 1350 and advertise a 1472-byte receive limit.quic.mtu_discoverynow works under--runtime-io-uringinstead of being refused at startup.Checks
just check: passed (748 + 223 tests, all modules).cargo nextest run -p moq-relay --features io-uring --lib uring::andcargo clippy -p moq-relay --features io-uring --all-targets -D warnings: passed.just checkbuilds default features, which leave out the relay's io_uring module.just test interop --all: not run (no wire ormoq-ffichange).🤖 Generated with Claude Code
https://claude.ai/code/session_01EeyiEy5FWz3z44DYZVo7vU
(Written by Claude Opus 5.5)