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

Also reset the MTU when the network paths has changed #15

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

flub
Copy link
Collaborator

@flub flub commented Oct 23, 2024

The existing mechanism to reset the congestion controller and RTT estimator is insufficient: the MTU should also start anew when the path has changed. This adds this to the same method and renames the functions to make their use clearer.

@flub
Copy link
Collaborator Author

flub commented Oct 23, 2024

I still need to test this with iroh that it behaves as expected.

@flub
Copy link
Collaborator Author

flub commented Oct 23, 2024

confirmed this works, as in, I can see the MTU go back to 1200 when the path changes and the MTUD probes start again to up it, correctly growing the MTU of the direct connection.

@flub flub marked this pull request as ready for review October 23, 2024 13:46
@flub
Copy link
Collaborator Author

flub commented Oct 23, 2024

The CI failure is not because of this PR, it occurs on the 0.11.x branch as well and is fixed in upstream main branch. I'll update the 0.11.x branch to the latest release and then rebase this PR, which should fix it.

In the mean time should not block reviewing this PR.

Copy link

@ramfox ramfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable!

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
The existing mechanism to reset the congestion controller and RTT
estimator is insufficient: the MTU should also start anew when the
path has changed.  This adds this to the same method and renames the
functions to make their use clearer.
@flub flub merged commit 02f3b33 into iroh-0.11.x Oct 24, 2024
13 checks passed
@flub flub deleted the flub/mtu-reset branch October 24, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants