Repository navigation
Conversation
zerox80
requested review from
KershawChang,
larseggert,
martinthomson,
mxinden and
omansfeld
as code owners
October 13, 2026 21:59
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Applying the peer limit leaves the pacer using its previous MTU until later ACK or loss processing.
1 open finding
What changed in this PR
Caps ordinary QUIC datagrams by the peer-advertised UDP payload limit while preserving PMTU discovery state.
Changes:
- Applies the peer limit to effective PLPMTU.
- Adds IPv4/IPv6, directional, PMTUD, congestion-control, and pacing tests.
| File | Description |
|---|---|
neqo-transport/src/pmtud.rs |
Caps effective PLPMTU and adds unit coverage. |
neqo-transport/src/sender.rs |
Tests congestion-control and pacer behavior. |
neqo-transport/src/connection/tests/pmtud.rs |
Adds end-to-end payload-limit tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
118
to
119
| pub const fn set_peer_max_udp_payload(&mut self, peer_max_udp_payload: usize) { | ||
| self.peer_max_udp_payload = Some(peer_max_udp_payload); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4081 +/- ##
==========================================
- Coverage 97.19% 97.03% -0.16%
==========================================
Files 113 118 +5
Lines 41742 42189 +447
Branches 41742 42189 +447
==========================================
+ Hits 40570 40940 +370
- Misses 1156 1223 +67
- Partials 16 26 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | CPU Simulation | spend disabled |
12 µs | 12.5 µs | -4.15% |
| ❌ | Walltime | neqo-neqo-cubic |
25.1 ms | 26 ms | -3.49% |
| ⚡ | CPU Simulation | next fast-path |
14.5 µs | 12.8 µs | +13.34% |
| ⚡ | Walltime | walltime/pacing-true/same-seed |
4.4 ms | 4.3 ms | +3.36% |
| ⚡ | Walltime | walltime/pacing-false/same-seed |
3.9 ms | 3.8 ms | +3.17% |
| ⚡ | CPU Simulation | simulated/pacing-false/varying-seeds |
70.5 ms | 68.6 ms | +2.7% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing zerox80:codex/peer-udp-payload-limit (18837d6) with main (a04c990)
larseggert
requested changes
Oct 19, 2026
larseggert
left a comment
larseggert
left a comment
Collaborator
There was a problem hiding this comment.
This can be much more concise. There are also some correctness concerns.
| path.borrow_mut() | ||
| .pmtud_mut() | ||
| .set_peer_max_udp_payload(max_udp_payload); | ||
| path.borrow_mut().set_peer_max_udp_payload(max_udp_payload); |
Collaborator
There was a problem hiding this comment.
Makes the limit only apply at handshake completion, which is too late.
| } | ||
|
|
||
| /// Apply the peer's UDP payload limit and synchronize the pacer's packet size. | ||
| pub fn set_peer_max_udp_payload(&mut self, peer_max_udp_payload: usize) { |
Collaborator
There was a problem hiding this comment.
One more place that has to remember to sync the pacer. Can we route through PacketSender?
| /// Set the peer's `max_udp_payload_size` transport parameter as an upper bound for probing. | ||
| /// Set the peer's `max_udp_payload_size` transport parameter as an upper bound for datagrams | ||
| /// and probing. | ||
| pub const fn set_peer_max_udp_payload(&mut self, peer_max_udp_payload: usize) { |
Collaborator
There was a problem hiding this comment.
The capped plpmtu gets no mtu_updated qlog event.
| p.prime_rtt(primary.borrow().rtt()); | ||
| if let Some(peer_max) = primary.borrow().pmtud().peer_max_udp_payload() { | ||
| p.pmtud_mut().set_peer_max_udp_payload(peer_max); | ||
| p.set_peer_max_udp_payload(peer_max); |
Collaborator
There was a problem hiding this comment.
Paths that exist before the parameters are processed never get the limit
| /// sent in an ordinary datagram, capped by the peer's receive limit when known. PMTU probes | ||
| /// use [`Self::probe_size`] and are separately bounded by the peer's receive limit. | ||
| #[must_use] | ||
| pub const fn plpmtu(&self) -> usize { |
Collaborator
There was a problem hiding this comment.
Can you consolidate the capping with what next does?
| tparams::{TransportParameter, TransportParameterId}, | ||
| }; | ||
|
|
||
| fn connect_with_udp_payload_limits( |
Collaborator
There was a problem hiding this comment.
Reuse/extend the existing helpers please.
| } | ||
|
|
||
| #[test] | ||
| fn peer_max_udp_payload_limits_datagrams() { |
Collaborator
There was a problem hiding this comment.
Lots of duplication between the two tests, be more DRY.
Also, why the loops here? The behavior does not depend on them at all?
Contributor
Author
|
@larseggert Thanks, I’ll simplify this and address the correctness gaps: apply the limit when peer transport parameters become available, propagate it to existing and future paths, and consolidate the sizing logic with pacing and qlog updates. I’ll also reuse the existing test helpers and replace the duplicated cases with focused regression coverage. |
zerox80
force-pushed
the
codex/peer-udp-payload-limit
branch
from
October 19, 2026 20:27
681076f to
badbe6c
Compare
larseggert
requested changes
Oct 20, 2026
larseggert
left a comment
larseggert
left a comment
Collaborator
There was a problem hiding this comment.
This PR is getting more complex and not less. @zerox80, please check your LLM output.
Comment on lines
+55
to
+114
| #[test] | ||
| fn peer_max_udp_payload_limits_datagrams() { | ||
| let params = ConnectionParameters::default() | ||
| .pmtud(true) | ||
| .pmtud_iface_mtu(false); | ||
| let mut client = new_client(params.clone()); | ||
| let mut server = new_server(params); | ||
| client | ||
| .set_local_tparam( | ||
| TransportParameterId::MaxUdpPayloadSize, | ||
| TransportParameter::Integer(1200), | ||
| ) | ||
| .unwrap(); | ||
| connect(&mut client, &mut server); | ||
|
|
||
| check_datagram_sizes(&mut server, &mut client, 1200); | ||
| assert_eq!(server.stats().pmtud_tx, 0); | ||
| let client_mtu = client.plpmtu(); | ||
| assert!(client_mtu > 1200); | ||
| check_datagram_sizes(&mut client, &mut server, client_mtu); | ||
| } | ||
|
|
||
| #[test] | ||
| fn peer_max_udp_payload_applies_during_handshake() { | ||
| let mut client = new_client(ConnectionParameters::default()); | ||
| let mut server = default_server(); | ||
| client | ||
| .set_local_tparam( | ||
| TransportParameterId::MaxUdpPayloadSize, | ||
| TransportParameter::Integer(1200), | ||
| ) | ||
| .unwrap(); | ||
| server.process_input(client.process_output(now()).dgram().unwrap(), now()); | ||
| let response = server | ||
| .process(client.process_output(now()).dgram(), now()) | ||
| .dgram() | ||
| .unwrap(); | ||
| assert_eq!(*server.state(), crate::State::Handshaking); | ||
| assert_eq!(server.plpmtu(), 1200); | ||
| assert_eq!(response.len(), 1200); | ||
| } | ||
|
|
||
| #[test] | ||
| fn peer_max_udp_payload_applies_to_resumption() { | ||
| let mut client = new_client(ConnectionParameters::default()); | ||
| let mut server = default_server(); | ||
| server | ||
| .set_local_tparam( | ||
| TransportParameterId::MaxUdpPayloadSize, | ||
| TransportParameter::Integer(1200), | ||
| ) | ||
| .unwrap(); | ||
| connect(&mut client, &mut server); | ||
| let token = super::exchange_ticket(&mut client, &mut server, now()); | ||
| let mut client = new_client(ConnectionParameters::default()); | ||
| client.enable_resumption(now(), token).unwrap(); | ||
| assert_eq!(client.plpmtu(), 1200); | ||
| assert_eq!(client.process_output(now()).dgram().unwrap().len(), 1200); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
These tests still have a lot of duplication. Don't repeat yourself.
|
|
||
| /// Whether PMTUD is enabled for this connection. | ||
| pmtud: bool, | ||
| peer_max_udp_payload: Option<usize>, |
Collaborator
There was a problem hiding this comment.
Why is this per path? TPs apply to a connection.
| .dgram() | ||
| .unwrap(); | ||
| assert_eq!(datagrams.num_datagrams(), 2); | ||
| // GSO permits a larger probe first, followed by a smaller last datagram. |
Collaborator
There was a problem hiding this comment.
Huh? GSO does not matter here.
The peer's max_udp_payload_size only bounded PMTUD probes, so ordinary datagrams could still use the default 1252/1232-byte PLPMTU when the peer advertised 1200. Cap plpmtu() by the peer limit, sharing the bound with next(). Apply it in set_initial_limits(), so it takes effect as soon as the peer's transport parameters are known (including remembered ones for 0-RTT), on all paths, and log the resulting MTU change. The pacer now picks up PLPMTU changes before each send.
zerox80
force-pushed
the
codex/peer-udp-payload-limit
branch
from
October 20, 2026 16:06
badbe6c to
07c7dee
Compare
Contributor
Author
Sorry about the last round. I used an LLM for parts of it and missed some of the things it added when I went over it. I started over from main and kept only what's needed:
Why per path: Pacer: |
The pacer kept its own copy of the packet size, which every PLPMTU change had to sync. Pass the PLPMTU to `Pacer::next()` instead, so the pacer always uses the current value.
RFC 9002 Section 7.2: if the maximum datagram size decreases, the congestion window SHOULD be set to the new initial congestion window. Route the peer limit through the congestion controller so it can do that, without raising a window that is already smaller. Share an assertion helper across the connection tests, check that a new path inherits the limit, and assert the logged MTU values.
Contributor
Author
|
Follow-up to my comment above: I pushed two more commits. One passes the current PLPMTU directly to the pacer instead of keeping a separate copy, removing the synchronization logic. The other caps cwnd at the new initial window when the peer limit lowers the PLPMTU, following RFC 9002 Section 7.2. I realize this increased the diff again. I'm having trouble finding a smaller approach that still addresses the correctness concerns. Would you prefer keeping these changes together, splitting the pacer refactor and the cwnd adjustment into separate PRs, or taking a different approach? I'd appreciate any pointers on what you'd simplify. |
Contributor
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Contributor
Benchmark resultsNo significant performance differences relative to a8809b4. All resultstransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1500 time: [60.158 ms 60.192 ms 60.229 ms]
thrpt: [1.6214 GiB/s 1.6224 GiB/s 1.6233 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildtransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1500 time: [60.325 ms 60.371 ms 60.416 ms]
thrpt: [1.6164 GiB/s 1.6176 GiB/s 1.6188 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1500 time: [3.5402 ms 3.5409 ms 3.5417 ms]
thrpt: [282.35 B/s 282.41 B/s 282.47 B/s]
Found 9 outliers among 100 measurements (9.00%)
7 (7.00%) high mild
2 (2.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1500 time: [81.000 ms 81.247 ms 81.455 ms]
thrpt: [122.77 Kelem/s 123.08 Kelem/s 123.46 Kelem/s]
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) low severe
2 (2.00%) low mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes time: [12.605 ms 12.608 ms 12.612 ms]
thrpt: [317.16 MiB/s 317.25 MiB/s 317.34 MiB/s]streams-flow-controlled/walltime/10-streams/each-1048576-bytes time: [33.694 ms 33.708 ms 33.721 ms]
thrpt: [296.55 MiB/s 296.67 MiB/s 296.79 MiB/s]
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
2 (2.00%) high mildstreams/walltime/1-streams/each-1000-bytes time: [155.95 µs 156.57 µs 157.36 µs]
thrpt: [6.0605 MiB/s 6.0911 MiB/s 6.1152 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1000-streams/each-1-bytes time: [4.4804 ms 4.4846 ms 4.4888 ms]
thrpt: [217.56 KiB/s 217.76 KiB/s 217.96 KiB/s]
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) low mildstreams/walltime/1000-streams/each-1000-bytes time: [14.806 ms 14.816 ms 14.827 ms]
thrpt: [64.320 MiB/s 64.366 MiB/s 64.413 MiB/s]transfer/walltime/pacing-false/same-seed time: [3.8808 ms 3.8834 ms 3.8866 ms]
thrpt: [1.0051 GiB/s 1.0059 GiB/s 1.0066 GiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/varying-seeds time: [3.8905 ms 3.8922 ms 3.8944 ms]
thrpt: [1.0031 GiB/s 1.0036 GiB/s 1.0040 GiB/s]transfer/walltime/pacing-true/same-seed time: [4.1099 ms 4.1130 ms 4.1166 ms]
thrpt: [971.67 MiB/s 972.53 MiB/s 973.26 MiB/s]transfer/walltime/pacing-true/varying-seeds time: [4.1809 ms 4.1830 ms 4.1854 ms]
thrpt: [955.70 MiB/s 956.26 MiB/s 956.73 MiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) low mildInstructions per cycleCriterion reported no significant timing changes. All benchmarks
Profiles for profiler.firefox.com (64)
Download data for |
Contributor
Client/server transfer resultsPerformance differences relative to a8809b4. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a peer advertises
max_udp_payload_size = 1200, neqo only used it to bound PMTUD probes. Ordinary datagrams still used the default PLPMTU of 1252 (IPv4) or 1232 (IPv6) bytes, which such a peer may drop.Cap
plpmtu()by the peer limit and apply it inset_initial_limits(), so it is in effect as soon as the peer's transport parameters are known, including the remembered ones for 0-RTT, and on all paths.Pacer::next()now takes the current PLPMTU as an argument, like it already takescwnd, instead of keeping a copy that had to be synced on ACK and loss.When the peer limit lowers the PLPMTU, cwnd is reduced to the initial window for the new size (RFC 9002, Section 7.2).
See RFC 9000, Section 18.2.