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

Add support for zero copy sendmsg. #212

Merged
merged 30 commits into from
Jan 20, 2023
Merged

Conversation

MA-ETL
Copy link
Contributor

@MA-ETL MA-ETL commented Jan 17, 2023

This PR adds support for the IORING_OP_SENDMSG_ZC opcode added in Linux 6.1 with a new sendmsg_zc function for UDP sockets.

Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

Nice. Biggest question is why ownership of the list of buffers isn't passed in. Seems to break the convention established for all other operations.

Cargo.toml Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/net/udp.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

Looking good.

Would you mind also pasting some example code showing this works into a comment in this PR so if we get around to testing on a 6.1 release, we could more easily throw a test together?

src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

Looking really good. With no tests, I'm assuming you are seeing this work in your own tree?

src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

Nice. Probably two outstanding questions on my part. Thanks for this. I think it's time we asked @Noah-Kennedy for a review too. (I haven't run this through CI because I'm assuming it compiles and there are no new tests.)

src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
src/io/sendmsg_zc.rs Outdated Show resolved Hide resolved
@ollie-etl
Copy link
Contributor

@FrankReh Are all of your concerns addressed?

ollie-etl added a commit to ollie-etl/tokio-uring that referenced this pull request Jan 20, 2023
Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

Thanks. Looks very good. I wish we have a test case or some sample of a test case attached to this PR so when we were ready to test with a 6.1 kernel, it would be easy for somebody new to know what to add. But it is ready to merge anyway. Just going to give @Noah-Kennedy a chance at feedback before I merge in a few hours. I realize it doesn't break or change any existing functionality anyway.

@FrankReh FrankReh merged commit d2f1494 into tokio-rs:master Jan 20, 2023
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