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): do not enable URO on Windows on ARM #2071

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Nov 28, 2024

On Windows on ARM with Windows Subsystem for Linux (WSL) WSA_RECVMSG does not return the segment size of coalesced UDP datagrams. See #2041 for details.

While lacking a fix for the root cause, don't enable URO, i.e. don't coalesce UDP datagrams, on Windows on ARM.

@mxinden mxinden force-pushed the no-uro-on-arm branch 4 times, most recently from 70ffc68 to c958ab5 Compare November 28, 2024 12:22
@mxinden mxinden marked this pull request as ready for review November 28, 2024 12:27
@mxinden mxinden requested review from djc and Ralith as code owners November 28, 2024 12:27
@mxinden
Copy link
Collaborator Author

mxinden commented Nov 28, 2024

Given your involvement on #2041, @thomaseizinger @stormshield-damiend do you have thoughts?

@valenting since you had the idea of using IsWow64Process2 ( 🙏 ), want to give this a review?

Copy link

@valenting valenting left a comment

Choose a reason for hiding this comment

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

I'm glad this works!

@stormshield-damiend
Copy link
Contributor

Given your involvement on #2041, @thomaseizinger @stormshield-damiend do you have thoughts?

@valenting since you had the idea of using IsWow64Process2 ( 🙏 ), want to give this a review?

i am ok with this approach, this prevent issue for people that uses Windows ARM + WSL and gives us some time to understand the root cause.

On Windows on ARM with Windows Subsystem for Linux (WSL) `WSA_RECVMSG` does not
return the segment size of coalesced UDP datagrams. See
quinn-rs#2041 for details.

While lacking a fix for the root cause, don't enable URO, i.e. don't coalesce
UDP datagrams on Windows on ARM.
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I'll have to double check but I think the user that ran into this with Firezone was on x64.

Happy for this to merge regardless.

We could also consider adding a config option, defaulting to disabled and with a warning that it may not work on all systems.

I think I'd rather disable GRO completely for Firezone on Windows until we get to the bottom of this. Or add some kind of compatibility switch.

@mxinden
Copy link
Collaborator Author

mxinden commented Dec 3, 2024

@Ralith as the main author of windows.rs, any objections?

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!

@Ralith Ralith added this pull request to the merge queue Dec 6, 2024
Merged via the queue into quinn-rs:main with commit 204b147 Dec 6, 2024
17 checks passed
@mxinden
Copy link
Collaborator Author

mxinden commented Dec 7, 2024

@djc
Copy link
Member

djc commented Dec 7, 2024

Will do it on Monday at the latest.

@djc
Copy link
Member

djc commented Dec 9, 2024

  • Published quinn-udp v0.5.8 at registry crates-io
  • [new tag] quinn-udp-0.5.8 -> quinn-udp-0.5.8
  • Release notes

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.

None yet

6 participants