| Message ID | 20230425182054.2826621-2-shr@devkernel.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | liburing: add api for napi busy poll | expand |
On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: > + > +int io_uring_register_napi(struct io_uring *ring, struct > io_uring_napi *napi) > +{ > + return __sys_io_uring_register(ring->ring_fd, > + IORING_REGISTER_NAPI, napi, 0); > +} > + > +int io_uring_unregister_napi(struct io_uring *ring, struct > io_uring_napi *napi) > +{ > + return __sys_io_uring_register(ring->ring_fd, > + IORING_UNREGISTER_NAPI, napi, 0); > +} my apologies if this is not the latest version of the patch but I think that it is... nr_args should be 1 to match what __io_uring_register() in the current corresponding kernel patch expects: https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0
On 2/2/24 9:41 AM, Olivier Langlois wrote: > On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: >> + >> +int io_uring_register_napi(struct io_uring *ring, struct >> io_uring_napi *napi) >> +{ >> + return __sys_io_uring_register(ring->ring_fd, >> + IORING_REGISTER_NAPI, napi, 0); >> +} >> + >> +int io_uring_unregister_napi(struct io_uring *ring, struct >> io_uring_napi *napi) >> +{ >> + return __sys_io_uring_register(ring->ring_fd, >> + IORING_UNREGISTER_NAPI, napi, 0); >> +} > > my apologies if this is not the latest version of the patch but I think > that it is... > > nr_args should be 1 to match what __io_uring_register() in the current > corresponding kernel patch expects: > > https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0 Can you send a patch I can fold in?
On Fri, 2024-02-02 at 09:42 -0700, Jens Axboe wrote: > On 2/2/24 9:41 AM, Olivier Langlois wrote: > > On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: > > > + > > > +int io_uring_register_napi(struct io_uring *ring, struct > > > io_uring_napi *napi) > > > +{ > > > + return __sys_io_uring_register(ring->ring_fd, > > > + IORING_REGISTER_NAPI, napi, 0); > > > +} > > > + > > > +int io_uring_unregister_napi(struct io_uring *ring, struct > > > io_uring_napi *napi) > > > +{ > > > + return __sys_io_uring_register(ring->ring_fd, > > > + IORING_UNREGISTER_NAPI, napi, > > > 0); > > > +} > > > > my apologies if this is not the latest version of the patch but I > > think > > that it is... > > > > nr_args should be 1 to match what __io_uring_register() in the > > current > > corresponding kernel patch expects: > > > > https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0 > > Can you send a patch I can fold in? > Jens, I am unsure of what you are asking me. You would like me to send a patch to fix a liburing patch that has never been applied AFAIK? if the v9 patch has never been applied. Wouldn't just editing Stefan patch by replacing the nr_args param from 0 to 1 directly do it?
On Feb 2, 2024, at 10:41 AM, Olivier Langlois <olivier@trillion01.com> wrote: > > On Fri, 2024-02-02 at 09:42 -0700, Jens Axboe wrote: >>> On 2/2/24 9:41 AM, Olivier Langlois wrote: >>> On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: >>>> + >>>> +int io_uring_register_napi(struct io_uring *ring, struct >>>> io_uring_napi *napi) >>>> +{ >>>> + return __sys_io_uring_register(ring->ring_fd, >>>> + IORING_REGISTER_NAPI, napi, 0); >>>> +} >>>> + >>>> +int io_uring_unregister_napi(struct io_uring *ring, struct >>>> io_uring_napi *napi) >>>> +{ >>>> + return __sys_io_uring_register(ring->ring_fd, >>>> + IORING_UNREGISTER_NAPI, napi, >>>> 0); >>>> +} >>> >>> my apologies if this is not the latest version of the patch but I >>> think >>> that it is... >>> >>> nr_args should be 1 to match what __io_uring_register() in the >>> current >>> corresponding kernel patch expects: >>> >>> https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0 >> >> Can you send a patch I can fold in? >> > Jens, > > I am unsure of what you are asking me. > > You would like me to send a patch to fix a liburing patch that has > never been applied AFAIK? > > if the v9 patch has never been applied. Wouldn't just editing Stefan > patch by replacing the nr_args param from 0 to 1 directly do it? The current version is what is in that branch you referenced. If you see anything in there that needs changing, send a patch for that. I can send out the series again if needed. — Jens Axboe
On Fri, 2024-02-02 at 10:48 -0700, Jens Axboe wrote: > On Feb 2, 2024, at 10:41 AM, Olivier Langlois > <olivier@trillion01.com> wrote: > > > > On Fri, 2024-02-02 at 09:42 -0700, Jens Axboe wrote: > > > > On 2/2/24 9:41 AM, Olivier Langlois wrote: > > > > On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: > > > > > + > > > > > +int io_uring_register_napi(struct io_uring *ring, struct > > > > > io_uring_napi *napi) > > > > > +{ > > > > > + return __sys_io_uring_register(ring->ring_fd, > > > > > + IORING_REGISTER_NAPI, napi, 0); > > > > > +} > > > > > + > > > > > +int io_uring_unregister_napi(struct io_uring *ring, struct > > > > > io_uring_napi *napi) > > > > > +{ > > > > > + return __sys_io_uring_register(ring->ring_fd, > > > > > + IORING_UNREGISTER_NAPI, napi, > > > > > 0); > > > > > +} > > > > > > > > my apologies if this is not the latest version of the patch but > > > > I > > > > think > > > > that it is... > > > > > > > > nr_args should be 1 to match what __io_uring_register() in the > > > > current > > > > corresponding kernel patch expects: > > > > > > > > https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0 > > > > > > Can you send a patch I can fold in? > > > > > Jens, > > > > I am unsure of what you are asking me. > > > > You would like me to send a patch to fix a liburing patch that has > > never been applied AFAIK? > > > > if the v9 patch has never been applied. Wouldn't just editing > > Stefan > > patch by replacing the nr_args param from 0 to 1 directly do it? > > The current version is what is in that branch you referenced. If you > see anything in there that needs changing, send a patch for that. > > I can send out the series again if needed. > AFAIK, the kernel patch is fine. my comment was referring the liburing patch that is calling __sys_io_uring_register() by passing a nr_args value of 0. This is problematic because on the kernel side, io_uring_register() returns -EINVAL if nr_args is not 1.
On Feb 2, 2024, at 1:11 PM, Olivier Langlois <olivier@trillion01.com> wrote: > > On Fri, 2024-02-02 at 10:48 -0700, Jens Axboe wrote: >>> On Feb 2, 2024, at 10:41 AM, Olivier Langlois >>> <olivier@trillion01.com> wrote: >>> >>> On Fri, 2024-02-02 at 09:42 -0700, Jens Axboe wrote: >>>>> On 2/2/24 9:41 AM, Olivier Langlois wrote: >>>>> On Tue, 2023-04-25 at 11:20 -0700, Stefan Roesch wrote: >>>>>> + >>>>>> +int io_uring_register_napi(struct io_uring *ring, struct >>>>>> io_uring_napi *napi) >>>>>> +{ >>>>>> + return __sys_io_uring_register(ring->ring_fd, >>>>>> + IORING_REGISTER_NAPI, napi, 0); >>>>>> +} >>>>>> + >>>>>> +int io_uring_unregister_napi(struct io_uring *ring, struct >>>>>> io_uring_napi *napi) >>>>>> +{ >>>>>> + return __sys_io_uring_register(ring->ring_fd, >>>>>> + IORING_UNREGISTER_NAPI, napi, >>>>>> 0); >>>>>> +} >>>>> >>>>> my apologies if this is not the latest version of the patch but >>>>> I >>>>> think >>>>> that it is... >>>>> >>>>> nr_args should be 1 to match what __io_uring_register() in the >>>>> current >>>>> corresponding kernel patch expects: >>>>> >>>>> https://git.kernel.dk/cgit/linux/commit/?h=io_uring-napi&id=787d81d3132aaf4eb4a4a5f24ff949e350e537d0 >>>> >>>> Can you send a patch I can fold in? >>>> >>> Jens, >>> >>> I am unsure of what you are asking me. >>> >>> You would like me to send a patch to fix a liburing patch that has >>> never been applied AFAIK? >>> >>> if the v9 patch has never been applied. Wouldn't just editing >>> Stefan >>> patch by replacing the nr_args param from 0 to 1 directly do it? >> >> The current version is what is in that branch you referenced. If you >> see anything in there that needs changing, send a patch for that. >> >> I can send out the series again if needed. >> > AFAIK, the kernel patch is fine. > > my comment was referring the liburing patch that is calling > __sys_io_uring_register() by passing a nr_args value of 0. > > This is problematic because on the kernel side, io_uring_register() > returns -EINVAL if nr_args is not 1. Ah gotcha, yeah that’s odd and could not ever have worked. I wonder how that was tested… I’ll setup a liburing branch as well. — Jens Axboe
On Fri, 2024-02-02 at 13:14 -0700, Jens Axboe wrote: > > Ah gotcha, yeah that’s odd and could not ever have worked. I wonder > how that was tested… > > I’ll setup a liburing branch as well. > It is easy. You omit to check the function return value by telling to yourself that it cannot fail... I caught my mistake on a second pass code review... C++ has a very useful [[nodiscard]] attribute that can help to catch this simple error... I am not sure if there is something similar to the [[nodiscard]] in the ISO C standard...
On 2/2/24 1:23 PM, Olivier Langlois wrote: > On Fri, 2024-02-02 at 13:14 -0700, Jens Axboe wrote: >> >> Ah gotcha, yeah that?s odd and could not ever have worked. I wonder >> how that was tested? >> >> I?ll setup a liburing branch as well. >> > It is easy. You omit to check the function return value by telling to > yourself that it cannot fail... > > I caught my mistake on a second pass code review... Oh I can see how that can happen, but then there should be no functional changes in terms of latency... Which means that it was never tested. The test results were from the original postings, so probably just fine. It's just that later versions would've failed. Looking at the example test case, it doesn't check the return value. > C++ has a very useful [[nodiscard]] attribute that can help to catch > this simple error... I am not sure if there is something similar to the > [[nodiscard]] in the ISO C standard... You can use __attribute__((__warn_unused_result__)) - the kernel does that, for example.
On 2/2/24 3:20 PM, Jens Axboe wrote: > On 2/2/24 1:23 PM, Olivier Langlois wrote: >> On Fri, 2024-02-02 at 13:14 -0700, Jens Axboe wrote: >>> >>> Ah gotcha, yeah that?s odd and could not ever have worked. I wonder >>> how that was tested? >>> >>> I?ll setup a liburing branch as well. >>> >> It is easy. You omit to check the function return value by telling to >> yourself that it cannot fail... >> >> I caught my mistake on a second pass code review... > > Oh I can see how that can happen, but then there should be no functional > changes in terms of latency... Which means that it was never tested. The > test results were from the original postings, so probably just fine. > It's just that later versions would've failed. Looking at the example > test case, it doesn't check the return value. Setup a 'napi' branch with the patches, and some fixes on top. It's a start... I'll try the example ping test here, just need to get off a plane and be able to access test boxes.
On 2/2/24 3:46 PM, Jens Axboe wrote: > On 2/2/24 3:20 PM, Jens Axboe wrote: >> On 2/2/24 1:23 PM, Olivier Langlois wrote: >>> On Fri, 2024-02-02 at 13:14 -0700, Jens Axboe wrote: >>>> >>>> Ah gotcha, yeah that?s odd and could not ever have worked. I wonder >>>> how that was tested? >>>> >>>> I?ll setup a liburing branch as well. >>>> >>> It is easy. You omit to check the function return value by telling to >>> yourself that it cannot fail... >>> >>> I caught my mistake on a second pass code review... >> >> Oh I can see how that can happen, but then there should be no functional >> changes in terms of latency... Which means that it was never tested. The >> test results were from the original postings, so probably just fine. >> It's just that later versions would've failed. Looking at the example >> test case, it doesn't check the return value. > > Setup a 'napi' branch with the patches, and some fixes on top. It's a > start... I'll try the example ping test here, just need to get off a > plane and be able to access test boxes. Added ipv4 support to the napi example code, and here's what I get. Stock settings, no NAPI, 100k packets: rtt(us) min/avg/max/mdev = 31.730/37.006/87.960/0.497 and with -t10 -b enabled: rtt(us) min/avg/max/mdev = 23.250/29.795/63.511/1.203 So it seems to work fine as-is, with the current branch. I've added some tweaks on the liburing napi branch, but mostly just little fixes.
diff --git a/src/include/liburing.h b/src/include/liburing.h index 70c1774..add17a0 100644 --- a/src/include/liburing.h +++ b/src/include/liburing.h @@ -241,6 +241,9 @@ int io_uring_register_sync_cancel(struct io_uring *ring, int io_uring_register_file_alloc_range(struct io_uring *ring, unsigned off, unsigned len); +int io_uring_register_napi(struct io_uring *ring, struct io_uring_napi *napi); +int io_uring_unregister_napi(struct io_uring *ring, struct io_uring_napi *napi); + int io_uring_get_events(struct io_uring *ring); int io_uring_submit_and_get_events(struct io_uring *ring); diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h index 3d3a63b..0e6e0bd 100644 --- a/src/include/liburing/io_uring.h +++ b/src/include/liburing/io_uring.h @@ -523,6 +523,10 @@ enum { /* register a range of fixed file slots for automatic slot allocation */ IORING_REGISTER_FILE_ALLOC_RANGE = 25, + /* set/clear busy poll settings */ + IORING_REGISTER_NAPI = 26, + IORING_UNREGISTER_NAPI = 27, + /* this goes last */ IORING_REGISTER_LAST, @@ -662,6 +666,14 @@ struct io_uring_buf_reg { __u64 resv[3]; }; +/* argument for IORING_(UN)REGISTER_NAPI */ +struct io_uring_napi { + __u32 busy_poll_to; + __u8 prefer_busy_poll; + __u8 pad[3]; + __u64 resv; +}; + /* * io_uring_restriction->opcode values */ diff --git a/src/liburing-ffi.map b/src/liburing-ffi.map index 0a5e12c..2d9ab9f 100644 --- a/src/liburing-ffi.map +++ b/src/liburing-ffi.map @@ -171,6 +171,8 @@ LIBURING_2.4 { io_uring_prep_msg_ring_fd; io_uring_prep_msg_ring_fd_alloc; io_uring_prep_sendto; + io_uring_register_napi; + io_uring_unregister_napi; local: *; }; diff --git a/src/liburing.map b/src/liburing.map index 3b37022..c775b61 100644 --- a/src/liburing.map +++ b/src/liburing.map @@ -79,4 +79,6 @@ LIBURING_2.4 { io_uring_register_restrictions; io_uring_setup_buf_ring; io_uring_free_buf_ring; + io_uring_register_napi; + io_uring_unregister_napi; } LIBURING_2.3; diff --git a/src/register.c b/src/register.c index a3fcc78..d494be7 100644 --- a/src/register.c +++ b/src/register.c @@ -337,3 +337,15 @@ int io_uring_register_file_alloc_range(struct io_uring *ring, return do_register(ring, IORING_REGISTER_FILE_ALLOC_RANGE, &range, 0); } + +int io_uring_register_napi(struct io_uring *ring, struct io_uring_napi *napi) +{ + return __sys_io_uring_register(ring->ring_fd, + IORING_REGISTER_NAPI, napi, 0); +} + +int io_uring_unregister_napi(struct io_uring *ring, struct io_uring_napi *napi) +{ + return __sys_io_uring_register(ring->ring_fd, + IORING_UNREGISTER_NAPI, napi, 0); +}