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

Support UDP GSO and GRO on linux #1209

Merged
merged 1 commit into from
Apr 25, 2020
Merged

Support UDP GSO and GRO on linux #1209

merged 1 commit into from
Apr 25, 2020

Conversation

glebpom
Copy link
Contributor

@glebpom glebpom commented Apr 8, 2020

This PR implements support for UDP GSO and GRO on Linux. It provides the way to send/receive UDP payloads bigger than interface MTU. The goal is to improve UDP performance.

GSO was introduced in Linux 4.18, GRO in 5.3

@asomers
Copy link
Member

asomers commented Apr 12, 2020

Looks like your tests are failing because this feature isn't implemented in Travis. You'll have to figure out how to test for the feature's availability at runtime. We have a few other tests that do that already.

Also, I wonder if it would be worth converting the Linux CI tests to use Cirrus-ci.com, if it can get us more coverage.

@glebpom
Copy link
Contributor Author

glebpom commented Apr 13, 2020

@asomers I've added the macro to skip tests if kernel version doesn't match. What do you think?

@asomers
Copy link
Member

asomers commented Apr 14, 2020

Is there not a more direct way to check for kernel support? Support could be removed from the kernel configuration, for example, in which case this wouldn't work.

@glebpom
Copy link
Contributor Author

glebpom commented Apr 14, 2020

Unfortunately, I didn't find any direct way to check if UDP offload is supported.

Checking the error code on actual call (like ENOSYS) is not the best idea, since it can be a result of the bug, with using wrong constants or something like that.

I think that considering the kernel version is generally a good idea because we typically know at what version the particular call appeared.

Regarding the customization of the kernel, I think it's a pretty common problem for almost any part of nix tests. The good solution would be to check if the particular configuration flag is enabled in kernel config (see here).

src/sys/socket/mod.rs Show resolved Hide resolved
@@ -617,6 +626,9 @@ pub enum ControlMessage<'a> {
))]
AlgSetAeadAssoclen(&'a u32),

///Gso UDP
Copy link
Member

Choose a reason for hiding this comment

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

Please document this enum variant.

test/sys/test_socket.rs Outdated Show resolved Hide resolved
test/sys/test_socket.rs Outdated Show resolved Hide resolved
test/sys/test_socket.rs Show resolved Hide resolved
test/test.rs Show resolved Hide resolved
@glebpom
Copy link
Contributor Author

glebpom commented Apr 22, 2020

More details can be found here and here

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Much better. Just add those comments so I can understand how the tests work. And it looks like you'll need to rebase, too.

test/sys/test_socket.rs Outdated Show resolved Hide resolved
test/sys/test_socket.rs Outdated Show resolved Hide resolved
@glebpom glebpom force-pushed the udp-gso-gro branch 2 times, most recently from c318172 to bec8fb4 Compare April 24, 2020 10:29
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Ok, looks good! Now just add a CHANGELOG message, and you can go ahead and squash your commits.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 25, 2020

Build succeeded:

@bors bors bot merged commit 490e979 into nix-rust:master Apr 25, 2020
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.

2 participants