quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

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

Closed stormshield-damiend closed 10 months ago

stormshield-damiend commented 1 year ago

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 :

Some possible long term tasks or benefits :

Feel free to comment on this draft.

djc commented 11 months ago

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

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

stormshield-damiend commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

Ralith commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

Rebased to include the new quinn-udp tests, but the cfg gates in quinn-udp/tests/tests.rs will need to be relaxed to validate the new functionality.

Ralith commented 10 months ago

Addressed outstanding comments. Currently failing:

Ralith commented 10 months ago

Dropped ECN CE from the test and fixed the IPv4 ECN cmsg. Should be good to go now.

Ralith commented 10 months ago

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 commented 10 months ago

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