Integrate io_uring transport into 4.2 #14045

normanmaurer · 2024-05-10T14:12:37Z

Motivation:

We should include our io_uring transport as part of 4.2

Modifications:

Add io_uring transport based on our incubator version and main version

Result:

normanmaurer · 2024-05-10T14:24:57Z

This "version" is a combination of what we had in netty-incubator-transport-native-io_uring and our main branch.
You might wonder why not pick one or the other, the reason is that both had its pros and cons and so I tried to merge these to also work with our new IoHandler design.

The good news is that with this implementation it should be trivial to support things like file IO, as all it would take is to write a FileIoHandle that you can register and then use the returned IOUringIoRegistration to submit the relevant IOUringIoOps and react to the IOUringIoEvent.

Beside this I also want to take another stab on the IOUringDatagramChannel implementation and use a blocking fd there as well with fastpoll. I will work on this while already give you some chance to have a look at the code.

We should also remove all the methods on the SubmissionQueue that are now not used anymore as Channel implementations are using the IOUringIoRegistration to submit work to the SubmissionQueue.

transport-classes-io_uring/src/main/java/io/netty/channel/uring/ObjectRing.java

normanmaurer · 2024-05-10T14:54:22Z

Also in the future we should look into using provided buffers: https://github.com/axboe/liburing/wiki/io_uring-and-networking-in-2023#provided-buffers

transport-classes-io_uring/src/main/java/io/netty/channel/uring/SubmissionQueue.java

normanmaurer · 2024-05-10T14:55:23Z

And I think we also should move from IOUring* to IoUring* for class / interface names. This is more inline with others like Nio etc.

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringDatagramChannel.java

normanmaurer · 2024-05-10T15:05:08Z

And before I forget shout out to @axboe for all the help in answering my questions and also for working on future changes for io_uring based on some of my feedback. OSS rocks :)

normanmaurer · 2024-05-10T15:17:33Z

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringIoOps.java

+     * @param data      the data
+     * @return          ops.
+     */
+    public static IOUringIoOps newAsyncCancel(int fd, int flags, long udata, short data) {


We could basically add a method for any OP_ that io_uring supports. I just focused on the ones that we need. At the end people can just construct the IOUringIoOps themselves. These methods just make things easier the same way as liburing makes things easier compared to use the raw io_uring interface

franz1981

First round seems ok, but wanna run it and see if I am convinced the same :P

Related the dead code, that's why is not yet approved: need to delete quite a few no longer used methods!

normanmaurer · 2024-05-11T10:53:28Z

First round seems ok, but wanna run it and see if I am convinced the same :P

For sure :) Keep me posted.

Related the dead code, that's why is not yet approved: need to delete quite a few no longer used methods!

Yes will do more cleanup etc. Just wanted to show something first

Motivation: We should include our io_uring transport as part of 4.2 Modifications: Add io_uring transport based on our incubator version and main version Result: Fixes #14010

normanmaurer · 2024-05-11T17:29:43Z

Need to investigate why I see some AssertionError on the CI while I never see it here...

…bmit that ca used assert error

normanmaurer · 2024-05-11T18:32:17Z

Need to investigate why I see some AssertionError on the CI while I never see it here...

Nevermind I did mess up while cleanup ... should be fixed now

…mentations

…tion detail

normanmaurer · 2024-05-13T10:34:42Z

@franz1981 @chrisvest I think this is ready to go now.

normanmaurer · 2024-05-13T10:35:33Z

I will rename everything from IOUring* to IoUring* before pulling it in. I just wanted to keep the name like it is for now to make it easier to compare to main and the incubator version.

chrisvest · 2024-05-13T21:38:48Z

It's a lot to review, but I'm having a look now.

chrisvest

Had some comments. Mostly small stuff.

transport-classes-io_uring/src/main/java/io/netty/channel/uring/Native.java

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringIoOps.java

transport-classes-io_uring/src/main/java/io/netty/channel/uring/AbstractIOUringChannel.java

transport-classes-io_uring/src/main/java/io/netty/channel/uring/LinuxSocket.java

transport-classes-io_uring/src/main/java/io/netty/channel/uring/SockaddrIn.java

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringServerSocketChannel.java

Co-authored-by: Chris Vest <christianvest_hansen@apple.com>

…make this an implementation detail

normanmaurer · 2024-05-14T11:20:50Z

Had some comments. Mostly small stuff.

@chrisvest addressed and also did some more simplification... PTAL again

franz1981

I need to give another round because I see there's some new changes

transport-classes-io_uring/src/main/java/io/netty/channel/uring/AbstractIOUringChannel.java

franz1981 · 2024-05-14T08:30:45Z

...port-classes-io_uring/src/main/java/io/netty/channel/uring/AbstractIOUringStreamChannel.java

+        if (shutdownOutputFuture.isDone()) {
+            shutdownOutputDone(shutdownOutputFuture, promise);
+        } else {
+            shutdownOutputFuture.addListener(new ChannelFutureListener() {


we probably can find a way to turn these in labmdas too, where it makes sense: personally not super happy because lambdas still put pressure on lambda metafactory, but still...they make the code more succint

yeah we can for sure. It's just what we had in the incubator version so I didn't touch it yet.

franz1981 · 2024-05-14T08:55:09Z

...port-classes-io_uring/src/main/java/io/netty/channel/uring/AbstractIOUringStreamChannel.java

+            assert iovArray == null;
+            assert writeId == 0;
+            int numElements = Math.min(in.size(), Limits.IOV_MAX);
+            ByteBuf iovArrayBuffer = alloc().directBuffer(numElements * IovArray.IOV_SIZE);


we could pool the IovArray to save netty/netty-incubator-transport-io_uring#154
on the instance and just let it go if we could reuse it, right after

Yes but like you said we can optimize later. Let's get this in first.

transport-classes-io_uring/src/main/java/io/netty/channel/uring/CompletionQueue.java

franz1981 · 2024-05-14T12:28:11Z

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringDatagramChannel.java

+
+    // These buffers are used for msghdr, iov, sockaddr_in / sockaddr_in6 when doing recvmsg / sendmsg
+    //
+    // TODO: Alternative we could also allocate these everytime from the ByteBufAllocator or we could use


better have an issue for this

It is existing code that we also have in the incubator version. it is just a copy and paste and we can decide later if we want an issue or not imho.

franz1981 · 2024-05-14T12:30:17Z

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringDatagramChannel.java

+
+    @Override
+    protected void doDisconnect() throws Exception {
+        // TODO: use io_uring for this too...


as ditto before: better to have an issue, unless closed in this pr

again... same as we have in the incubator version. We can create an issue once this is in.

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringIoHandler.java

franz1981

Right now just minor things, really.
After fixing wht @chrisvest asked for, I'm +100 to get this in, and well done @normanmaurer !

chrisvest

I think this is good for this PR.

normanmaurer · 2024-05-15T08:24:01Z

Thanks a lot for everyone involved!

normanmaurer requested review from chrisvest and franz1981 May 10, 2024 14:12

normanmaurer added this to the 4.2.0.Final milestone May 10, 2024

normanmaurer requested a review from bryce-anderson May 10, 2024 14:50

normanmaurer commented May 10, 2024

View reviewed changes

transport-classes-io_uring/src/main/java/io/netty/channel/uring/ObjectRing.java Outdated Show resolved Hide resolved

normanmaurer commented May 10, 2024

View reviewed changes

transport-classes-io_uring/src/main/java/io/netty/channel/uring/SubmissionQueue.java Outdated Show resolved Hide resolved

normanmaurer force-pushed the io_uring_42_new branch from 07e51df to e165296 Compare May 10, 2024 14:56

normanmaurer commented May 10, 2024

View reviewed changes

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringDatagramChannel.java Outdated Show resolved Hide resolved

normanmaurer commented May 10, 2024

View reviewed changes

franz1981 reviewed May 11, 2024

View reviewed changes

normanmaurer added 2 commits May 11, 2024 10:54

Integrate io_uring transport into 4.2

2859a17

Motivation: We should include our io_uring transport as part of 4.2 Modifications: Add io_uring transport based on our incubator version and main version Result: Fixes #14010

Rebase and make it work on latest 4.2 branch

6a3d1ad

normanmaurer force-pushed the io_uring_42_new branch from e165296 to 6a3d1ad Compare May 11, 2024 11:00

Cleanup

256154a

Correctly fix sendmsg used for TCP_FAST_OPEN and also fix a double su…

68db728

…bmit that ca used assert error

normanmaurer added 4 commits May 13, 2024 06:38

Remove unused methods / classes and reduce visibility of config imple…

ac34ec2

…mentations

finish impl for datagram

eb7111e

Ensure io_uring can also be used when we dont have a fd yet

1bf4b82

move ops id generation to AbstractIoUringChannel as its an implementa…

f42a571

…tion detail

normanmaurer force-pushed the io_uring_42_new branch from 283b099 to 56a7dc2 Compare May 13, 2024 10:36

Let IoRegistration.submit(...) return a long

ea08054

normanmaurer force-pushed the io_uring_42_new branch from 56a7dc2 to ea08054 Compare May 13, 2024 10:42

normanmaurer added 5 commits May 13, 2024 10:44

Fix typo in comment

e55f0cd

Use IORING_CQE_F_SOCK_NONEMPTY with datagrams as well

77e462e

cleanup

91d0a34

remove link method

71a19fa

Remove unused property

c4aacb9

chrisvest reviewed May 14, 2024

View reviewed changes

normanmaurer commented May 14, 2024

View reviewed changes

transport-classes-io_uring/src/main/java/io/netty/channel/uring/IOUringServerSocketChannel.java Outdated Show resolved Hide resolved

normanmaurer and others added 8 commits May 14, 2024 08:46

Apply suggestions from code review

3921b02

Co-authored-by: Chris Vest <christianvest_hansen@apple.com>

Add comments

79ef788

Use handle

011d651

Remove id from IoUringIoOps and IOUringIoRegistration as we can just …

7ea599b

…make this an implementation detail

Add asserts to clarify

09795ba

Tighten up visibility

8bcc55a

More asserts for the win

25b04e4

Fix copyright year

a03788e

franz1981 requested changes May 14, 2024

View reviewed changes

franz1981 approved these changes May 14, 2024

View reviewed changes

chrisvest approved these changes May 14, 2024

View reviewed changes

normanmaurer added 2 commits May 15, 2024 08:09

Last cleanup

5071548

Rename to be more inline with other namings

5f398a0

normanmaurer merged commit 028394a into 4.2 May 15, 2024
3 checks passed

normanmaurer deleted the io_uring_42_new branch May 15, 2024 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate io_uring transport into 4.2 #14045

Integrate io_uring transport into 4.2 #14045

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer May 10, 2024

franz1981 left a comment

normanmaurer commented May 11, 2024

normanmaurer commented May 11, 2024

normanmaurer commented May 11, 2024

normanmaurer commented May 13, 2024

normanmaurer commented May 13, 2024

chrisvest commented May 13, 2024

chrisvest left a comment

normanmaurer commented May 14, 2024

franz1981 left a comment

franz1981 May 14, 2024

normanmaurer May 14, 2024

franz1981 May 14, 2024

normanmaurer May 14, 2024

franz1981 May 14, 2024

normanmaurer May 14, 2024

franz1981 May 14, 2024

normanmaurer May 14, 2024

franz1981 left a comment

chrisvest left a comment

normanmaurer commented May 15, 2024

Integrate io_uring transport into 4.2 #14045

Integrate io_uring transport into 4.2 #14045

Conversation

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

normanmaurer commented May 10, 2024

Choose a reason for hiding this comment

franz1981 left a comment

Choose a reason for hiding this comment

normanmaurer commented May 11, 2024

normanmaurer commented May 11, 2024

normanmaurer commented May 11, 2024

normanmaurer commented May 13, 2024

normanmaurer commented May 13, 2024

chrisvest commented May 13, 2024

chrisvest left a comment

Choose a reason for hiding this comment

normanmaurer commented May 14, 2024

franz1981 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

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

franz1981 left a comment

Choose a reason for hiding this comment

chrisvest left a comment

Choose a reason for hiding this comment

normanmaurer commented May 15, 2024