Skip to content

Bounding UDP payload size to 1200 and renaming - #1156

Merged
djc merged 2 commits into
quinn-rs:mainfrom
BiagioFesta:wip/bfesta/min-mtu-as-dgram-size
Jul 7, 2021
Merged

Bounding UDP payload size to 1200 and renaming#1156
djc merged 2 commits into
quinn-rs:mainfrom
BiagioFesta:wip/bfesta/min-mtu-as-dgram-size

Conversation

@BiagioFesta

Copy link
Copy Markdown
Contributor

As mentioned in #1154, it seems quinn sends UDP datagrams with a payload 1232 bytes by default.

This is easily reproducible with the client.rs/server.rs quinn example, making a GET request and serve a reasonable big file.

cargo run --example client https:/server:1234/README.md
[...]
Jul 06 02:59:45.746 TRACE drive{id=0}: quinn_proto::connection: sending 1232 bytes in 1 datagrams
[...]

Looking at the QUIC spec, (quic-draft-32 #14) , it seems the protocol should supports networks where the maximum UDP payload is 1200 bytes.
Any larger value should be discovered using PMTUD.

This PR contains two changes:

  • Renaming the const MIN_MTU: u16 = 1232; into MAX_INITIAL_UDP_PAYLOAD_SIZE. This change has no impact from the semantic point of view. The rationale behind this change is because that value seems to be used across the code to compute what in QUIC is defined as "the maximum datagram size" (i.e., the UDP datagram payload).
  • Changing this value from 1232 to 1200 and avoid quinn assembles packets (UDP datagrams) with a payload greater than this value.

Ralith
Ralith previously requested changes Jul 7, 2021

@Ralith Ralith left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread quinn-proto/src/lib.rs Outdated
@BiagioFesta
BiagioFesta force-pushed the wip/bfesta/min-mtu-as-dgram-size branch from cdc87ec to 52a42c8 Compare July 7, 2021 08:45
@djc

djc commented Jul 7, 2021

Copy link
Copy Markdown
Member

Just need to fix up the formatting a bit, then I'm happy to merge this.

@BiagioFesta

Copy link
Copy Markdown
Contributor Author

Just need to fix up the formatting a bit, then I'm happy to merge this.

I guess a cargo fmt should be enough even if I don't see a rustfmt.toml

@djc

djc commented Jul 7, 2021

Copy link
Copy Markdown
Member

Would you mind squashing the formatting fixes into the first commit? Should be easy with an interactive rebase.

@BiagioFesta
BiagioFesta force-pushed the wip/bfesta/min-mtu-as-dgram-size branch from b34e0f8 to 2543c47 Compare July 7, 2021 10:18
@djc
djc dismissed Ralith’s stale review July 7, 2021 11:25

Obsolete

@djc
djc merged commit 5a9d362 into quinn-rs:main Jul 7, 2021
@Ralith Ralith mentioned this pull request Jul 7, 2021
@BiagioFesta
BiagioFesta deleted the wip/bfesta/min-mtu-as-dgram-size branch July 8, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants