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

Match GSO segment size to the first datagram, not the MTU #1837

Merged
merged 6 commits into from
May 1, 2024
Merged

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Apr 27, 2024

Fixes #1832.

Still working on how to handle the case where a datagram is queued but won't fit with the current segment size. We need to either detect this in advance and finish the GSO batch before beginning another packet, or gracefully abandon packet assembly after failing to fit any frames. Currently leaning towards the latter. This will need careful testing.

Also still TODO: End the GSO batch early if the amount of padding in the current packet would be excessive.

@Ralith Ralith force-pushed the compact-gso branch 6 times, most recently from e7c47c4 to a1add92 Compare April 27, 2024 03:03
@Ralith
Copy link
Collaborator Author

Ralith commented Apr 27, 2024

On second thought, maybe judging whether a datagram fits into a certain segment size in advance of beginning packet assembly isn't so bad. We already have the logic to make a conservative estimate in max_datagram_size, and (as of the preceding PR) drop datagrams that exceed that limit. Refactoring packet assembly to be safe to abandon is interesting too, and might allow us to simplify or even abandon some can_send logic in the long run, but it's a lot more churn and we can pursue it separately if desired.

@Ralith Ralith force-pushed the compact-gso branch 2 times, most recently from cbe6295 to ce7f2f3 Compare April 29, 2024 02:08
@Ralith
Copy link
Collaborator Author

Ralith commented Apr 29, 2024

This is now feature-complete, but still needs testing.

@Ralith Ralith force-pushed the compact-gso branch 3 times, most recently from 60c8054 to 28360a3 Compare April 29, 2024 05:00
@Ralith
Copy link
Collaborator Author

Ralith commented Apr 29, 2024

Went ahead and banged out the tests, finding and fixing a number of bugs in the process.

@Ralith Ralith marked this pull request as ready for review April 29, 2024 05:01
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.

Initial round, got interrupted by some bored kids.

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented May 1, 2024

Good stuff!

@Ralith Ralith force-pushed the compact-gso branch 2 times, most recently from 37a5fd9 to aee7545 Compare May 1, 2024 16:33
Base automatically changed from datagram-cleanup to main May 1, 2024 16:33
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.

GSO padding has high overhead for application datagrams larger than half MTU
2 participants