Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(udp): use IPV6_PMTUDISC_PROBE instead of IP_PMTUDISC_PROBE on v6 #2072

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Nov 29, 2024

Both resolve to 3, thus not an actual bug.

Caught thanks to @marten-seemann's DF draft.

Both resolve to `3`, thus not an actual bug.
@mxinden mxinden requested review from djc and Ralith as code owners November 29, 2024 10:51
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks! Never know when we might hit a weird environment where these differ.

@Ralith Ralith added this pull request to the merge queue Nov 29, 2024
Merged via the queue into quinn-rs:main with commit 7551282 Nov 29, 2024
18 checks passed
@mxinden
Copy link
Collaborator Author

mxinden commented Nov 29, 2024

Never know when we might hit a weird environment where these differ.

Agreed. The more complexity we can offload to other crates (here libc) the better.

@mstyura
Copy link
Contributor

mstyura commented Nov 29, 2024

Sorry for late comment, maybe it worth adding comment explaining why IP_PMTUDISC_PROBE is chosen over IP_PMTUDISC_DO

Path MTU discovery value   Meaning
              IP_PMTUDISC_WANT           Use per-route settings.
              IP_PMTUDISC_DONT           Never do Path MTU Discovery.
              IP_PMTUDISC_DO             Always do Path MTU Discovery.
              IP_PMTUDISC_PROBE          Set DF but ignore Path MTU.

E.g. Chromium uses later option in context when DF flag is desired, WebRTC also. So there is should be something important which probably deserves explanation why quinn uses IP_PMTUDISC_PROBE rather than IP_PMTUDISC_DO.

@Ralith
Copy link
Collaborator

Ralith commented Nov 30, 2024

It seems pretty self-explanatory to me? We do our own MTUD, so we don't want the kernel's built-in MTUD confusing things.

@mstyura
Copy link
Contributor

mstyura commented Nov 30, 2024

We do our own MTUD, so we don't want the kernel's built-in MTUD confusing things.

Thanks, this is exact explanation I missed! WebRTC uses IP_PMTUDISC_DO option probably due to by default conservative packet size 1200 and lack of it's own MTU discovery. And why Chromium don't use IP_PMTUDISC_PROBE in it's code also unclear even so it obviously has QUIC implementation.

@mxinden
Copy link
Collaborator Author

mxinden commented Dec 2, 2024

@mstyura related discussion marten-seemann/draft-seemann-tsvwg-udp-fragmentation#13. Might be worth moving the Chromium discussion there.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 11, 2024
…chain-reviewers

`quinn-udp` `v0.5.8` contains an intermediary fix for Bug 1916558, see
quinn-rs/quinn#2071.

In addition `quinn-udp` `v0.5.8` Includes the following bugfixes:

- quinn-rs/quinn#2072
- quinn-rs/quinn#2073
- quinn-rs/quinn#2074
- quinn-rs/quinn#2050
- quinn-rs/quinn#2047

Differential Revision: https://phabricator.services.mozilla.com/D231505
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Dec 12, 2024
…chain-reviewers

`quinn-udp` `v0.5.8` contains an intermediary fix for Bug 1916558, see
quinn-rs/quinn#2071.

In addition `quinn-udp` `v0.5.8` Includes the following bugfixes:

- quinn-rs/quinn#2072
- quinn-rs/quinn#2073
- quinn-rs/quinn#2074
- quinn-rs/quinn#2050
- quinn-rs/quinn#2047

Differential Revision: https://phabricator.services.mozilla.com/D231505
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Dec 13, 2024
…chain-reviewers

`quinn-udp` `v0.5.8` contains an intermediary fix for Bug 1916558, see
quinn-rs/quinn#2071.

In addition `quinn-udp` `v0.5.8` Includes the following bugfixes:

- quinn-rs/quinn#2072
- quinn-rs/quinn#2073
- quinn-rs/quinn#2074
- quinn-rs/quinn#2050
- quinn-rs/quinn#2047

Differential Revision: https://phabricator.services.mozilla.com/D231505

UltraBlame original commit: a8cd98601fdff821fe9cc516ddb61f54c104fb58
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Dec 13, 2024
…chain-reviewers

`quinn-udp` `v0.5.8` contains an intermediary fix for Bug 1916558, see
quinn-rs/quinn#2071.

In addition `quinn-udp` `v0.5.8` Includes the following bugfixes:

- quinn-rs/quinn#2072
- quinn-rs/quinn#2073
- quinn-rs/quinn#2074
- quinn-rs/quinn#2050
- quinn-rs/quinn#2047

Differential Revision: https://phabricator.services.mozilla.com/D231505

UltraBlame original commit: a8cd98601fdff821fe9cc516ddb61f54c104fb58
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Dec 13, 2024
…chain-reviewers

`quinn-udp` `v0.5.8` contains an intermediary fix for Bug 1916558, see
quinn-rs/quinn#2071.

In addition `quinn-udp` `v0.5.8` Includes the following bugfixes:

- quinn-rs/quinn#2072
- quinn-rs/quinn#2073
- quinn-rs/quinn#2074
- quinn-rs/quinn#2050
- quinn-rs/quinn#2047

Differential Revision: https://phabricator.services.mozilla.com/D231505

UltraBlame original commit: a8cd98601fdff821fe9cc516ddb61f54c104fb58
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.

4 participants