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

Enable UDP GRO #1350

Merged
merged 4 commits into from May 5, 2022
Merged

Enable UDP GRO #1350

merged 4 commits into from May 5, 2022

Conversation

alessandrod
Copy link
Contributor

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:

  • should this be conditionally enabled based on config? (GSO seems to be enabled unconditionally)
  • should the GRO size be configurable? If the read buffers aren't large enough GRO is actually less performant than no GRO
  • I'm testing on a 5.14 kernel, we need to find the minimum kernel version that supports GRO

quinn-udp/src/lib.rs Outdated Show resolved Hide resolved
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented May 5, 2022

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 alessandrod changed the title WIP: enable GRO Enable UDP GRO May 5, 2022
@alessandrod alessandrod marked this pull request as ready for review May 5, 2022 10:49
@alessandrod
Copy link
Contributor Author

alessandrod commented May 5, 2022

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:

  • get rid of step a)
  • keep step b), and from there do: if udp_state.gro_segments() > 1 { socket.<new method that enables gro>() }

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

@alessandrod alessandrod requested a review from Ralith May 5, 2022 10:56
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.

I imagined we'd pass down a &UdpState to init, but this seems fine too.

@djc djc merged commit 29d3932 into quinn-rs:main May 5, 2022
@djc
Copy link
Collaborator

djc commented May 5, 2022

Awesome, thanks!

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

3 participants