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 zerocopy write to the TCP socket #237

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Feb 13, 2023

Intorduce zero copy version of TcpStream::write using write_zc name.

It behaves almost the same, except returned future will be complete when remotes side ACKs receipt of the buffer content, unlike TcpStream::write which completes when data copied into local TCP socket kernel buffers.

@redbaron
Copy link
Contributor Author

hm, cargo test --doc completes on my kernel 6.1.9 system:

test result: ok. 65 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 11.17s

and cargo test fails on fallocate only, which is unchanged in this PR:

---- basic_fallocate stdout ----
thread 'basic_fallocate' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 95, kind: Uncategorized, message: "Operation not supported" }', tests/fs_file.rs:295:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    basic_fallocate

test result: FAILED. 14 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

/// ```no_run
/// use std::net::SocketAddr;
/// use tokio_uring::net::TcpListener;
/// ```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll let you figure out why your test and the CI test are different. But this seems to hijack a documentation example for write_all with one for write_zc. That doesn't seem to fly.

/// > at writes over around 10 KB.
///
/// Note: Using fixed buffers [#54](https://github.com/tokio-rs/tokio-uring/pull/54), avoids the page-pinning overhead
pub async fn write_zc<T: BoundedBuf>(&self, buf: T) -> crate::BufResult<usize, T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of calling this write_zc? Where does that come from? Does the std or tokio libraries have a name like that for a TCP connection? If not, at first glance it seems better to stick with the names of either the io_uring interface or maybe the io-uring crate API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

write_zc is zero copy version of existing write, similar to how UdpSocket::send_zc is zero copy version of UdpSocket::send.

Choose a reason for hiding this comment

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

Tokio doesn't have this, but Linux sockets do, and it's a very useful optimisation. Offloading data copy from your CPU to your NIC can vastly reduce CPU overhead

@FrankReh
Copy link
Collaborator

I believe it unusual not to provide a high level description of what is going on with the PR. That's what the top description is for. A reviewer, or anyone else, should not have to read through the code diffs to figure out the intent and the way the intent is being handled. If you read through the git log for this repo, you will see the level of detail that is usually given.

And unless you are absolutely sure of yourself. It is safer to create an Issue first talking about what you think is missing and what you would propose doing about it.

@FrankReh
Copy link
Collaborator

And if you do create a PR but you didn't think it was ready for review and submission, please make it a draft.

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