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

Tie up PLPMTUD's loose ends #1529

Merged
merged 5 commits into from
May 3, 2023
Merged

Tie up PLPMTUD's loose ends #1529

merged 5 commits into from
May 3, 2023

Conversation

aochagavia
Copy link
Contributor

This PR introduces a few self-contained improvements to make PLPMTUD work better.

We will probably want to discuss the API impact of the change to the congestion controller code:

  • New on_mtu_update function on the Controller trait
  • Removed max_datagram_size from all controller configs (the current MTU is used now instead)
  • Removed minimum_window from all controller configs (it is now calculated on demand based on the current MTU)

With all this in place, running the top-level bench crate with a MTUD upper bound of 9000 results in sending rates of ~900 MiB/s (on my machine ™).

@aochagavia aochagavia force-pushed the plpmtud-2 branch 2 times, most recently from 45455a0 to d6f320a Compare April 5, 2023 14:28
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Show resolved Hide resolved
quinn-proto/src/congestion/bbr/mod.rs Outdated Show resolved Hide resolved
@aochagavia
Copy link
Contributor Author

I just pushed updated commits with the changes you suggested 😉

@aochagavia
Copy link
Contributor Author

aochagavia commented Apr 17, 2023

@djc @Ralith Here's a reminder just in case this slipped through the cracks. I'll need these changes for my work on ACK frequency (not yet blocked, though).

@Ralith
Copy link
Collaborator

Ralith commented Apr 18, 2023

Still high on my list 👍

@aochagavia
Copy link
Contributor Author

Just a heads up: I'm working on an additional commit to close #1540

@aochagavia
Copy link
Contributor Author

aochagavia commented Apr 19, 2023

The last commit adds a base_udp_payload_size to TransportConfig, meant to configure the payload size that is guaranteed to be supported by the network. Adding it to MtuDiscoveryConfig, as @Ralith suggested, doesn't work here, because this is a property of the black hole detector (i.e. we want to use it even if MTU discovery is is disabled).

By the way, I felt a bit limited by the fact that we aren't using the builder pattern for TransportConfig, with a build function or some such where to do validation. To keep things user-friendly, I decided to automatically raise the initial_max_udp_payload_size when the base_udp_payload_size is raised (instead of crashing, which would force the user to set the values in a particular order). Have you thought about biting the bullet and introducing a builder? There is otherwise quite some implicit stuff going on, like values < 1200 being ignored when passed to initial_max_udp_payload_size.

@djc djc mentioned this pull request Apr 19, 2023
3 tasks
quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/congestion.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Show resolved Hide resolved
@aochagavia aochagavia force-pushed the plpmtud-2 branch 3 times, most recently from 50e440b to 0f4e307 Compare April 20, 2023 12:24
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, overall this is looking great!

quinn-proto/src/connection/mtud.rs Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Show resolved Hide resolved
quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn-proto/src/config.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
@aochagavia aochagavia force-pushed the plpmtud-2 branch 2 times, most recently from 93202b7 to a066d61 Compare April 25, 2023 08:32
@aochagavia
Copy link
Contributor Author

Thanks for the review! Just pushed changes addressing all comments 😉

quinn-proto/src/config.rs Outdated Show resolved Hide resolved
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.

LGTM, thanks!

@aochagavia
Copy link
Contributor Author

aochagavia commented May 1, 2023

Just FYI, I'd like to fininsh the ACK frequency PR no later than next week (it is not 100% sure yet whether funding will be extended after that period), so it would be helpful if this can get merged soon (the changes related to congestion control are necessary for ACK frequency).

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.

Approved with nits.

Sorry for the slow review, I'm on vacation.

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mtud.rs Outdated Show resolved Hide resolved
quinn-proto/src/congestion.rs Outdated Show resolved Hide resolved
quinn-proto/src/congestion/bbr/mod.rs Outdated Show resolved Hide resolved
@aochagavia
Copy link
Contributor Author

Just amended the relevant commits to apply @djc's suggestions

@djc
Copy link
Member

djc commented May 3, 2023

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.

3 participants