quinn-rs / quinn

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

Enable UDP GRO #1350

Closed alessandrod closed 2 years ago

alessandrod commented 2 years ago

Hello! Thanks for such a great lib! 🤗

As a way to learn quinn I started looking into the IO code and noticed that while GSO is supported, GRO isn't yet. This PR is my first stab at implementing it. Initial results seem promising:

No GRO:

Client 0 stats:
Overall download stats:

Transferred 1073741824 bytes on 1 streams in 2.96s (345.49 MiB/s)

Stream download metrics:

      │  Throughput   │ Duration 
──────┼───────────────┼──────────
 AVG  │  345.62 MiB/s │     2.96s
 P0   │  345.50 MiB/s │     2.96s
 P10  │  345.75 MiB/s │     2.96s
 P50  │  345.75 MiB/s │     2.96s
 P90  │  345.75 MiB/s │     2.96s
 P100 │  345.75 MiB/s │     2.96s

With GRO:

 Client 0 stats:
Overall download stats:

Transferred 1073741824 bytes on 1 streams in 2.51s (407.22 MiB/s)

Stream download metrics:

      │  Throughput   │ Duration 
──────┼───────────────┼──────────
 AVG  │  407.38 MiB/s │     2.52s
 P0   │  407.25 MiB/s │     2.51s
 P10  │  407.50 MiB/s │     2.52s
 P50  │  407.50 MiB/s │     2.52s
 P90  │  407.50 MiB/s │     2.52s
 P100 │  407.50 MiB/s │     2.52s

The existing tests pass, so hopefully I'm not benchmarking garbage.

Obviously the PR needs a lot more work - some things that come to mind:

Ralith commented 2 years ago

This is awesome, thanks!

should this be conditionally enabled based on config? (GSO seems to be enabled unconditionally)

GSO is used based on quinn_udp::UdpState::new(), which detects GSO support at runtime. We should do similar here, and the same "create a dummy socket and setsockopt it" pattern should be applicable.

should the GRO size be configurable?

We could experimentally determine a reasonable value, and leave customizing it to future work if anyone ever actually wants that.

I'm testing on a 5.14 kernel, we need to find the minimum kernel version that supports GRO

I think it should be sufficient to see whether the setsockopt succeeds.

alessandrod commented 2 years ago

This is awesome, thanks!

should this be conditionally enabled based on config? (GSO seems to be enabled unconditionally)

GSO is used based on quinn_udp::UdpState::new(), which detects GSO support at runtime. We should do similar here, and the same "create a dummy socket and setsockopt it" pattern should be applicable.

should the GRO size be configurable?

We could experimentally determine a reasonable value, and leave customizing it to future work if anyone ever actually wants that.

I'm testing on a 5.14 kernel, we need to find the minimum kernel version that supports GRO

I think it should be sufficient to see whether the setsockopt succeeds.

Ok I've now implemented this. One thing to notice is that since we do setsockopt in UdpSocket::from_std but UdpState lives at another layer, the current implementation does this:

a) UdpSocket::from_std calls init(&socket) which opportunistically tries to enable UDP_GRO, ignoring the case in which setting the opt fails b) EndpointRef::new sizes the recv buffer using UdpState::gro_segments(). If setting UDP_GRO failed during step a), it'll fail in this step too, therefore GRO will be disabled.

An alternative implementation could be:

Btw I've pushed this as individual commits to make reviewing easier, I'll squash when it's ready

djc commented 2 years ago

Awesome, thanks!