Add TCP, UDP and Unix socket support #40

LinkTed · 2021-08-07T10:30:57Z

Add TCP, UDP and Unix socket support.

Open to feedback

carllerche

Exposing these ops as async fns like this exposes some issues w/ cancellation. I have a proposal here: https://github.com/tokio-rs/tokio-uring/blob/design-doc/DESIGN.md#byte-streams. You interested in trying that?

carllerche · 2021-08-10T13:48:25Z

src/net/tcp/stream.rs

+}
+
+impl TcpStream {
+    pub async fn connect<A: ToSocketAddrs>(addr: A) -> io::Result<TcpStream> {


I'm wondering if we should just take a SocketAddr here and avoid the lookup_host. A user could call the Tokio lookup_host themselves?

Yes, I think this would be better. It will reduce the complexity and make it clear which IP address the peer is connected.

carllerche · 2021-08-10T13:53:33Z

src/driver/connect.rs

+
+        let op = opcode::Connect::new(
+            types::Fd(connect.socket.as_raw_fd()),
+            connect.os_socket_addr.as_ptr(),


The socket address will need to be stored on the heap to be safe.

Sorry, did not push the current. Yes, there was a use-after free. Maybe we should use a pin instead of Box<T> to not allocated memory on the heap (performance reason)?

LinkTed · 2021-08-10T17:58:30Z

Exposing these ops as async fns like this exposes some issues w/ cancellation. I have a proposal here: https://github.com/tokio-rs/tokio-uring/blob/design-doc/DESIGN.md#byte-streams. You interested in trying that?

Thanks for the feedback. Sure, I will try it on the weekend.

LinkTed · 2021-08-11T15:39:15Z

Exposing these ops as async fns like this exposes some issues w/ cancellation. I have a proposal here: https://github.com/tokio-rs/tokio-uring/blob/design-doc/DESIGN.md#byte-streams. You interested in trying that?
@carllerche
I have some questions about your "proposal":

Should the AsyncBufRead own the buffer or the driver? If yes, then what should happen if the AsyncBufRead drop then?
Should the Write and Read operation be kept or removed?
How big should the internal buffer of the AsyncBufRead be?

src/io/read.rs

johalun · 2021-10-19T22:06:10Z

Any progress on this lately? I'm very interesting in trying it out. Can test if conflicts are resolved.

LinkTed · 2021-10-21T17:01:32Z

@johalun Did not have to work on it, sorry.

LinkTed · 2021-11-01T10:06:18Z

@johalun I continue working on this PR. Currently, there is a bug in the function accept of the TcpListener. The addrlen is not set (https://github.com/tokio-rs/tokio-uring/pull/40/files#diff-b0bd49be618d67cc160c826497fb99a731ecd1ddbefa6e09b8d6c6439981114cR25). If this is fixed then the TCP code can be tested.

Maybe somebody knows why.

Darksonn · 2021-11-23T14:50:30Z

Sorry, I don't know.

dyxushuai · 2021-11-25T05:01:05Z

The accpet doc says that

       The addrlen argument is a value-result argument: the caller must
       initialize it to contain the size (in bytes) of the structure
       pointed to by addr; on return it will contain the actual size of
       the peer address.

LinkTed · 2021-11-28T11:45:41Z

The accpet doc says that

       The addrlen argument is a value-result argument: the caller must
       initialize it to contain the size (in bytes) of the structure
       pointed to by addr; on return it will contain the actual size of
       the peer address.

Yes, I know. I use the addrlen as a value-result argument. But what I am doing wrong?

TiltMeSenpai · 2021-12-09T00:05:29Z

I did a bit of digging, what I think might be happening is Rust is actually copying the Accept struct with sockaddr_storage and addrlen before submitting it to the io_uring queue.

I changed driver/accept.rs so that the Accept struct takes a sockaddr_storage/addrlen pointer instead of holding them itself, and updated driver/socket.rs so that the accept() function owns sockaddr_storage and addrlen. After the Accept future returns, sockaddr_storage and addrlen in accept() are updated with the correct values, however the values in completion.data remain unchanged for some reason.

This is probably a pretty hacky solution, but it does confirm my theory about Rust making random copies of your structs. I hope this helps.

diff --git a/src/driver/accept.rs b/src/driver/accept.rs
index c90db9d..a18a8da 100644
--- a/src/driver/accept.rs
+++ b/src/driver/accept.rs
@@ -4,25 +4,25 @@ use std::io;
 
 pub(crate) struct Accept {
     fd: SharedFd,
-    pub(crate) sockaddr_storage: libc::sockaddr_storage,
-    pub(crate) addrlen: libc::socklen_t,
+    pub(crate) sockaddr_storage: *mut libc::sockaddr_storage,
+    pub(crate) addrlen: *mut libc::socklen_t,
 }
 
 impl Op<Accept> {
-    pub(crate) fn accept(fd: &SharedFd) -> io::Result<Op<Accept>> {
+    pub(crate) fn accept(fd: &SharedFd, sockaddr: &mut libc::sockaddr_storage, addrlen: &mut libc::socklen_t) -> io::Result<Op<Accept>> {
         use io_uring::{opcode, types};
 
         Op::submit_with(
             Accept {
                 fd: fd.clone(),
-                sockaddr_storage: unsafe { std::mem::zeroed() },
-                addrlen: std::mem::size_of::<libc::sockaddr_storage>() as libc::socklen_t,
+                sockaddr_storage: sockaddr,
+                addrlen: addrlen,
             },
             |accept| {
                 opcode::Accept::new(
                     types::Fd(accept.fd.raw_fd()),
-                    &mut accept.sockaddr_storage as *mut _ as *mut _,
-                    &mut accept.addrlen,
+                    accept.sockaddr_storage as *mut _,
+                    accept.addrlen,
                 )
                 .flags(libc::O_CLOEXEC)
                 .build()
diff --git a/src/driver/socket.rs b/src/driver/socket.rs
index 60f80cb..cfad2d4 100644
--- a/src/driver/socket.rs
+++ b/src/driver/socket.rs
@@ -39,15 +39,17 @@ impl Socket {
     }
 
     pub(crate) async fn accept(&self) -> io::Result<(Socket, SocketAddr)> {
-        let op = Op::accept(&self.fd)?;
+        let mut sockaddr: libc::sockaddr_storage = unsafe { std::mem::zeroed() };
+        let mut addrlen:  libc::socklen_t = std::mem::size_of::<libc::sockaddr_storage>() as libc::socklen_t;
+        let op = Op::accept(&self.fd, &mut sockaddr, &mut addrlen)?;
         let completion = op.await;
         let fd = completion.result?;
         let fd = SharedFd::new(fd as i32);
         let socket = Socket { fd };
         let os_socket_addr = unsafe {
             OsSocketAddr::from_raw_parts(
-                &completion.data.sockaddr_storage as *const _ as _,
-                completion.data.addrlen as usize,
+                &sockaddr as *const _ as _,
+                addrlen as usize,
             )
         };
         let socket_addr = os_socket_addr.into_addr().unwrap();

LinkTed · 2021-12-12T18:27:57Z

@TiltMeSenpai Thanks for the patch. Yes, it helps. I will check, how we can fix this.

LinkTed · 2021-12-18T16:35:22Z

@TiltMeSenpai, yes it was the bug. I fix it by using an Box.

Currently, I am working to add the UDP support.

LinkTed · 2021-12-22T18:08:11Z

I will add the Unix support in another PR because I think that this PR would be too big. Therefore, I will clean the current state and remove the draft state, so that the review process can start.

Noah-Kennedy · 2021-12-22T18:37:32Z

I'll review this once you are ready. I think it would be great to get this merged in before I make a new release.

Add the `TcpStream`, `TcpListener` and `UdpSocket` structs.

Noah-Kennedy · 2022-01-06T22:44:11Z

If we don't need a bind operation, can we delete the file?

Noah-Kennedy · 2022-01-06T22:55:23Z

src/driver/op.rs

@@ -21,6 +21,7 @@ pub(crate) struct Op<T: 'static> {
 }

 /// Operation completion. Returns stored state with the result of the operation.
+#[allow(dead_code)]


What dead code are we allowing?

I do not know.

Let's not add these sorts of statements then.

src/driver/open.rs

Noah-Kennedy · 2022-02-13T22:19:30Z

@carllerche can you give this a re-review?

inevity · 2022-02-16T11:56:43Z

examples/mix.rs

 use std::env;

+use tokio_uring::{fs::File, net::TcpListener};
+


Should create a new test ,this test is for tokio type compatile consideration.

The mix.rs example jus show that tokio-uring runtime can use the tokio type from tokio crate,such as tokio::net and tokio::spawn.

inevity · 2022-02-16T12:20:41Z

src/driver/op.rs

@@ -146,7 +145,7 @@ where
                Poll::Ready(Completion {
                    data: me.data.take().expect("unexpected operation state"),
                    result,
-                    flags,
+                    flags: flags,


We can add this back once we decide to use it. For now, let's not.

It is just lint waring.

inevity · 2022-02-16T12:21:34Z

src/driver/op.rs

@@ -63,7 +62,7 @@ impl<T> Op<T> {
    /// the kernel.
    pub(super) fn submit_with<F>(data: T, f: F) -> io::Result<Op<T>>
    where
-        F: FnOnce() -> squeue::Entry,
+        F: FnOnce(&mut T) -> squeue::Entry,


Because submitting certain ops might mutate the stored data.

inevity · 2022-02-17T03:18:31Z

Since we just use the old tokio current runtime to impl the tokio_uring runtime, So is there any benefit from using the tokio:uring TCP socket support? -------> replace the mio and tokio:spawn/await with tokio_uring::driver(get accept and place fd in the epoll) and uring sumbit?

Noah-Kennedy · 2022-02-18T00:01:24Z

@inevity could you please rephrase that statement?

inevity · 2022-02-18T04:50:22Z

@inevity could you please rephrase that statement?

just omit. It maybe have perf improvement.

jon-chuang · 2022-02-26T01:31:24Z

Has this been tested with Hyper with HTTP/2 and its h2 backend?

Its worth noting that monoio's io_uring-based TcpStream is not compatible as a plugin to h2. See https://github.com/bytedance/monoio/tree/master/monoio-compat

LinkTed marked this pull request as draft August 7, 2021 10:31

carllerche requested changes Aug 10, 2021

View reviewed changes

LinkTed force-pushed the master branch from 1eb0b71 to 7e36f0f Compare August 10, 2021 17:50

dyxushuai reviewed Aug 26, 2021

View reviewed changes

src/io/read.rs Outdated Show resolved Hide resolved

LinkTed force-pushed the master branch from 7e36f0f to de94382 Compare September 4, 2021 17:21

LinkTed force-pushed the master branch from de94382 to b3b2f9d Compare October 23, 2021 10:00

LinkTed force-pushed the master branch 3 times, most recently from 0a0485a to 013d6ee Compare November 1, 2021 10:01

LinkTed force-pushed the master branch 3 times, most recently from 6655724 to 23c5a71 Compare December 22, 2021 17:55

net: add TCP and UDP support

1d5635b

Add the `TcpStream`, `TcpListener` and `UdpSocket` structs.

LinkTed force-pushed the master branch from 23c5a71 to 1d5635b Compare December 23, 2021 15:22

LinkTed marked this pull request as ready for review December 23, 2021 15:22

Noah-Kennedy reviewed Jan 6, 2022

View reviewed changes

shrink diff

72412d0

Noah-Kennedy reviewed Jan 24, 2022

View reviewed changes

src/driver/open.rs Outdated Show resolved Hide resolved

Noah-Kennedy added 3 commits January 23, 2022 20:59

fix example

2d1dcf8

shrink diff

2cd23ff

fix warnings

d4f5a22

Noah-Kennedy and others added 4 commits February 13, 2022 16:21

try and fix a merge conflict

cda43a2

Merge branch 'master' into master

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

72233fc

fmt

29fc6f1

fix tests

bed6593

Noah-Kennedy approved these changes Feb 13, 2022

View reviewed changes

Noah-Kennedy requested a review from carllerche February 13, 2022 22:32

fix docs

ea8e883

inevity reviewed Feb 16, 2022

View reviewed changes

Noah-Kennedy added 5 commits February 20, 2022 15:20

fix docs

cda9f47

change signature of Op::open

e99ceb0

add doc test for TCP

ae78120

dedup examples in tcp stream

22c684e

add tests to udp.rs

67a1f6b

carllerche approved these changes Feb 25, 2022

View reviewed changes

Noah-Kennedy merged commit 523dae0 into tokio-rs:master Feb 25, 2022

Noah-Kennedy mentioned this pull request Feb 25, 2022

Add support for basic UDP networking #61

Closed

This was referenced Feb 27, 2022

Update benchmarks with tokio_uring::net bytedance/monoio#59

Open

Add unix stream & listener #74

Merged

Add TCP, UDP and Unix socket support #40

Add TCP, UDP and Unix socket support #40

Conversation

LinkTed commented Aug 7, 2021 • edited Loading

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinkTed commented Aug 10, 2021

LinkTed commented Aug 11, 2021 • edited Loading

johalun commented Oct 19, 2021

LinkTed commented Oct 21, 2021 • edited Loading

LinkTed commented Nov 1, 2021

Darksonn commented Nov 23, 2021

dyxushuai commented Nov 25, 2021

LinkTed commented Nov 28, 2021

TiltMeSenpai commented Dec 9, 2021

LinkTed commented Dec 12, 2021 • edited Loading

LinkTed commented Dec 18, 2021

LinkTed commented Dec 22, 2021

Noah-Kennedy commented Dec 22, 2021

Noah-Kennedy commented Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noah-Kennedy commented Feb 13, 2022

inevity Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inevity commented Feb 17, 2022 • edited Loading

Noah-Kennedy commented Feb 18, 2022

inevity commented Feb 18, 2022

jon-chuang commented Feb 26, 2022

LinkTed commented Aug 7, 2021 •

edited

Loading

LinkTed commented Aug 11, 2021 •

edited

Loading

LinkTed commented Oct 21, 2021 •

edited

Loading

LinkTed commented Dec 12, 2021 •

edited

Loading

Noah-Kennedy commented Jan 6, 2022 •

edited

Loading

inevity Feb 16, 2022 •

edited

Loading

inevity commented Feb 17, 2022 •

edited

Loading