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

proto: add a way to configure the minimum MTU change needed to stop the binary search used during the MTU discovery phase. #1702

Merged

Conversation

stormshield-damiend
Copy link
Contributor

Previously the binary search algorithm used during the MTU discovery phase was stopping itself if the discovered MTU was within -20 / +20 of the previous detected one.

This could lead to finding a MTU 40 bytes smaller of the real one in the worst case.

This change allow changing the default minimum change value if more precision is needed.

the binary search used during the MTU discovery phase.
@djc
Copy link
Collaborator

djc commented Nov 3, 2023

What value would you use for this in your project?

@stormshield-damiend
Copy link
Contributor Author

What value would you use for this in your project?

Hi djc, as you know we do packet tunneling with Quic, thus the MTU is quite important for us as it is in direct link with the possible throughput of the tunnel.
With the default value during a real-life test with someone in Spain connecting to an AWS VM, Quinn find the estimated MTU (40 lower than the real one) in 3 search phases. With a value of 1 (so -1 / +1 from real MTU), Quinn find the real MTU with 2 more iterations.

We think that 1 is too much aggressive for the default value, thus we propose this API, which allow changing the default value when needed.

@djc
Copy link
Collaborator

djc commented Nov 6, 2023

Okay, so you intend to configure this all the way to 1?

@stormshield-damiend
Copy link
Contributor Author

Okay, so you intend to configure this all the way to 1?

Yes one or a small value, we need to run more real-life test to see how it goes in term of search convergence.

Copy link
Collaborator

@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.

Okay, I guess this makes sense to have.

@Ralith Ralith merged commit ab280e4 into quinn-rs:main Nov 6, 2023
7 of 8 checks passed
@stormshield-damiend stormshield-damiend deleted the mtud_binary_search_minimum_change branch November 7, 2023 08:19
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