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

udp: add support for ECN, local addrs, GSO, and GRO on Windows #1701

Merged
merged 12 commits into from
Jan 4, 2024

Conversation

stormshield-damiend
Copy link
Contributor

@stormshield-damiend stormshield-damiend commented Nov 3, 2023

This adds support for ECN on Windows using WSARecvMsg() and WSASendMsg() from WinSock API.

This solves #765.

Some points need to be discussed or done :

  • WSARecvMSG() cannot be accessed directly, we need to retrieve its function pointer using a dedicated call to WSAIoctl(). This detection may fail on old versions of windows (it seems to have been added as an extension in Windows XP). Do we keep the old API as a fallback (as currently done in the draft PR) ?
  • If we keep the fallback API, do we still send ECN if we cannot receive them ?
  • The previous detection is also done at every creation of a socket. Should this be done once on a dummy UDP socket in a lazy way (using OnceCell or OnceLock for example) ?
  • All unsafe code block must be checked and have a # Safety : documentation
  • wsa_cmsg.rs must be reviewed carefully
  • see if we could merge cmsg.rs and wsa_cmsg.rs
  • dual stack test must be done

Some possible long term tasks or benefits :

  • We could try to merge cmsg and wsa_cmsg into socket2, as at the moment, there is no API to read or write socket control messages
  • Using the new API gives us access to some other control messages / features, for example we could use URO

Feel free to comment on this draft.

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 for digging into this!

quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/wsa_cmsg.rs Outdated Show resolved Hide resolved
quinn-udp/src/cmsg/mod.rs Outdated Show resolved Hide resolved
quinn-udp/src/cmsg/mod.rs Outdated Show resolved Hide resolved
quinn-udp/src/cmsg/mod.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Dec 19, 2023

I do think this should not be just one commit. I'd suggest at least some commits like this:

  • Move cmsg.rs to cmsg/mod.rs
  • Further splitting of cmsg into multiple modules without any other changes
  • Change cmsg to new structure on the Unix side that can accomodate the changes for Windows
  • Add changes for Windows

Instead of depending on once_cell, could we use OnceLock instead? We'd have to bump the MSRV a bunch...

@stormshield-damiend
Copy link
Contributor Author

I do think this should not be just one commit. I'd suggest at least some commits like this:

* Move `cmsg.rs` to `cmsg/mod.rs`

Ok i can do that

* Further splitting of `cmsg` into multiple modules without any other changes

Something like encoder.rs / decoder.rs / iterator.rs ?

* Change `cmsg` to new structure on the Unix side that can accomodate the changes for Windows

Sure i can do that

* Add changes for Windows

In one commit ?

Instead of depending on once_cell, could we use OnceLock instead? We'd have to bump the MSRV a bunch...

Yes i can do that, but this is your and @Ralith calls.

@djc
Copy link
Member

djc commented Dec 19, 2023

I do think this should not be just one commit. I'd suggest at least some commits like this:

* Move `cmsg.rs` to `cmsg/mod.rs`

Ok i can do that

* Further splitting of `cmsg` into multiple modules without any other changes

Something like encoder.rs / decoder.rs / iterator.rs ?

Sorry, I meant splitting along the lines of the split you have now (so in this commit, just splitting unix.rs out of mod.rs).

* Change `cmsg` to new structure on the Unix side that can accomodate the changes for Windows

Sure i can do that

* Add changes for Windows

In one commit?

If you think there are multiple stages that can each compile and pass tests, that might be nice?

Instead of depending on once_cell, could we use OnceLock instead? We'd have to bump the MSRV a bunch...

Yes i can do that, but this is your and @Ralith calls.

Let's see what @Ralith thinks.

@stormshield-damiend
Copy link
Contributor Author

I have taken into account @djc remarks. I still need to add Safety comments where needed.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Looking pretty good!

quinn-udp/src/cmsg/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/cmsg/mod.rs Show resolved Hide resolved
quinn-udp/src/cmsg/mod.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented Dec 19, 2023

Instead of depending on once_cell, could we use OnceLock instead? We'd have to bump the MSRV a bunch...

Our rule of thumb is 6 months, right? 1.70 is just about there, so I'm fine with it. Tokio's still on 1.63, but probably they just haven't had any need to change, and there's no sense letting that hold us back arbitrarily. It's also a really trivial update, though, so I'm also happy to judge it non-blocking and handle it personally in a followup if @stormshield-damiend wants to stay focused.

@stormshield-damiend
Copy link
Contributor Author

Instead of depending on once_cell, could we use OnceLock instead? We'd have to bump the MSRV a bunch...

Our rule of thumb is 6 months, right? 1.70 is just about there, so I'm fine with it. Tokio's still on 1.63, but probably they just haven't had any need to change, and there's no sense letting that hold us back arbitrarily. It's also a really trivial update, though, so I'm also happy to judge it non-blocking and handle it personally in a followup if @stormshield-damiend wants to stay focused.

Fine with me, lets keep the once_cell dependency and remove it later.

@Ralith
Copy link
Collaborator

Ralith commented Dec 31, 2023

Added GSO and GRO support while I was at it. I wasn't able to observe GRO working in loopback tests, but at least it isn't hurting.

Bulk benchmark on Windows has ~doubled performance with GSO.

@Ralith Ralith force-pushed the windows_ecn branch 2 times, most recently from 26aa6c6 to 8880b0d Compare December 31, 2023 07:14
@Ralith Ralith changed the title udp: add support for ECN on Windows udp: add support for ECN, local addrs, GSO, and GRO on Windows Dec 31, 2023
quinn-udp/tests/tests.rs Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
quinn-udp/src/windows.rs Show resolved Hide resolved
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

(Not a full review, just reviewing for semver incompatibility changes -- I didn't spot any.)

quinn-udp/src/windows.rs Show resolved Hide resolved
quinn-udp/src/windows.rs Outdated Show resolved Hide resolved
@Ralith Ralith merged commit 6e3d108 into quinn-rs:main Jan 4, 2024
8 checks passed
@Ralith
Copy link
Collaborator

Ralith commented Jan 4, 2024

Thanks for taking care of this! This is a huge and long overdue improvement to our performance and reliability on Windows.

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