* [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications @ 2020-05-15 16:43 Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 1/5] Add CQ ring 'flags' field Stefano Garzarella ` (4 more replies) 0 siblings, 5 replies; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring This series is based on top of a new IORING_CQ_EVENTFD_DISABLED flag available in the CQ ring flags. I added io_uring_cq_eventfd_enabled() to get the status of eventfd notifications, and io_uring_cq_eventfd_enable() to disable/enabled eventfd notifications. I updated man pages and I added a eventfd-disable.c test case. Stefano Garzarella (5): Add CQ ring 'flags' field man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets Add helpers to set and get eventfd notification status man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description Add test/eventfd-disable.c test case .gitignore | 1 + man/io_uring_register.2 | 8 ++ man/io_uring_setup.2 | 3 +- src/include/liburing.h | 31 +++++++ src/include/liburing/io_uring.h | 11 ++- src/setup.c | 2 + test/Makefile | 4 +- test/eventfd-disable.c | 148 ++++++++++++++++++++++++++++++++ 8 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 test/eventfd-disable.c -- 2.25.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH liburing 1/5] Add CQ ring 'flags' field 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella @ 2020-05-15 16:43 ` Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 2/5] man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets Stefano Garzarella ` (3 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring io_uring provides the new CQ ring 'flags' field if 'cq_off.flags' is not zero. In this case we set the 'cq->kflags' pointer, otherwise it will be NULL. Signed-off-by: Stefano Garzarella <[email protected]> --- src/include/liburing.h | 1 + src/include/liburing/io_uring.h | 4 +++- src/setup.c | 2 ++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/include/liburing.h b/src/include/liburing.h index dd85f7b..ea596f6 100644 --- a/src/include/liburing.h +++ b/src/include/liburing.h @@ -41,6 +41,7 @@ struct io_uring_cq { unsigned *ktail; unsigned *kring_mask; unsigned *kring_entries; + unsigned *kflags; unsigned *koverflow; struct io_uring_cqe *cqes; diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h index e48d746..602bb0e 100644 --- a/src/include/liburing/io_uring.h +++ b/src/include/liburing/io_uring.h @@ -204,7 +204,9 @@ struct io_cqring_offsets { __u32 ring_entries; __u32 overflow; __u32 cqes; - __u64 resv[2]; + __u32 flags; + __u32 resv1; + __u64 resv2; }; /* diff --git a/src/setup.c b/src/setup.c index f783b6a..860c112 100644 --- a/src/setup.c +++ b/src/setup.c @@ -76,6 +76,8 @@ err: cq->kring_entries = cq->ring_ptr + p->cq_off.ring_entries; cq->koverflow = cq->ring_ptr + p->cq_off.overflow; cq->cqes = cq->ring_ptr + p->cq_off.cqes; + if (p->cq_off.flags) + cq->kflags = cq->ring_ptr + p->cq_off.flags; return 0; } -- 2.25.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH liburing 2/5] man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 1/5] Add CQ ring 'flags' field Stefano Garzarella @ 2020-05-15 16:43 ` Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 3/5] Add helpers to set and get eventfd notification status Stefano Garzarella ` (2 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring Signed-off-by: Stefano Garzarella <[email protected]> --- man/io_uring_setup.2 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/man/io_uring_setup.2 b/man/io_uring_setup.2 index d48bb32..c929cb7 100644 --- a/man/io_uring_setup.2 +++ b/man/io_uring_setup.2 @@ -325,7 +325,8 @@ struct io_cqring_offsets { __u32 ring_entries; __u32 overflow; __u32 cqes; - __u32 resv[4]; + __u32 flags; + __u32 resv[3]; }; .EE .in -- 2.25.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 1/5] Add CQ ring 'flags' field Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 2/5] man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets Stefano Garzarella @ 2020-05-15 16:43 ` Stefano Garzarella 2020-05-15 16:53 ` Jens Axboe 2020-05-15 16:43 ` [PATCH liburing 4/5] man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 5/5] Add test/eventfd-disable.c test case Stefano Garzarella 4 siblings, 1 reply; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring This patch adds the new IORING_CQ_EVENTFD_DISABLED flag. It can be used to disable/enable notifications from the kernel when a request is completed and queued to the CQ ring. We also add two helpers function to check if the notifications are enabled and to enable/disable them. If the kernel doesn't provide CQ ring flags, the notifications are always enabled if an eventfd is registered. Signed-off-by: Stefano Garzarella <[email protected]> --- src/include/liburing.h | 30 ++++++++++++++++++++++++++++++ src/include/liburing/io_uring.h | 7 +++++++ 2 files changed, 37 insertions(+) diff --git a/src/include/liburing.h b/src/include/liburing.h index ea596f6..fe03547 100644 --- a/src/include/liburing.h +++ b/src/include/liburing.h @@ -9,7 +9,9 @@ extern "C" { #include <sys/socket.h> #include <sys/uio.h> #include <sys/stat.h> +#include <errno.h> #include <signal.h> +#include <stdbool.h> #include <inttypes.h> #include <time.h> #include "liburing/compat.h" @@ -445,6 +447,34 @@ static inline unsigned io_uring_cq_ready(struct io_uring *ring) return io_uring_smp_load_acquire(ring->cq.ktail) - *ring->cq.khead; } +static inline int io_uring_cq_eventfd_enable(struct io_uring *ring, + bool enabled) +{ + uint32_t flags; + + if (!ring->cq.kflags) + return -ENOTSUP; + + flags = *ring->cq.kflags; + + if (enabled) + flags &= ~IORING_CQ_EVENTFD_DISABLED; + else + flags |= IORING_CQ_EVENTFD_DISABLED; + + IO_URING_WRITE_ONCE(*ring->cq.kflags, flags); + + return 0; +} + +static inline bool io_uring_cq_eventfd_enabled(struct io_uring *ring) +{ + if (!ring->cq.kflags) + return true; + + return !(*ring->cq.kflags & IORING_CQ_EVENTFD_DISABLED); +} + static int __io_uring_peek_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr) { struct io_uring_cqe *cqe; diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h index 602bb0e..8c5775d 100644 --- a/src/include/liburing/io_uring.h +++ b/src/include/liburing/io_uring.h @@ -209,6 +209,13 @@ struct io_cqring_offsets { __u64 resv2; }; +/* + * cq_ring->flags + */ + +/* disable eventfd notifications */ +#define IORING_CQ_EVENTFD_DISABLED (1U << 0) + /* * io_uring_enter(2) flags */ -- 2.25.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-15 16:43 ` [PATCH liburing 3/5] Add helpers to set and get eventfd notification status Stefano Garzarella @ 2020-05-15 16:53 ` Jens Axboe 2020-05-15 17:11 ` Stefano Garzarella 0 siblings, 1 reply; 12+ messages in thread From: Jens Axboe @ 2020-05-15 16:53 UTC (permalink / raw) To: Stefano Garzarella; +Cc: io-uring On 5/15/20 10:43 AM, Stefano Garzarella wrote: > This patch adds the new IORING_CQ_EVENTFD_DISABLED flag. It can be > used to disable/enable notifications from the kernel when a > request is completed and queued to the CQ ring. > > We also add two helpers function to check if the notifications are > enabled and to enable/disable them. > > If the kernel doesn't provide CQ ring flags, the notifications are > always enabled if an eventfd is registered. > > Signed-off-by: Stefano Garzarella <[email protected]> > --- > src/include/liburing.h | 30 ++++++++++++++++++++++++++++++ > src/include/liburing/io_uring.h | 7 +++++++ > 2 files changed, 37 insertions(+) > > diff --git a/src/include/liburing.h b/src/include/liburing.h > index ea596f6..fe03547 100644 > --- a/src/include/liburing.h > +++ b/src/include/liburing.h > @@ -9,7 +9,9 @@ extern "C" { > #include <sys/socket.h> > #include <sys/uio.h> > #include <sys/stat.h> > +#include <errno.h> > #include <signal.h> > +#include <stdbool.h> > #include <inttypes.h> > #include <time.h> > #include "liburing/compat.h" > @@ -445,6 +447,34 @@ static inline unsigned io_uring_cq_ready(struct io_uring *ring) > return io_uring_smp_load_acquire(ring->cq.ktail) - *ring->cq.khead; > } > > +static inline int io_uring_cq_eventfd_enable(struct io_uring *ring, > + bool enabled) > +{ > + uint32_t flags; > + > + if (!ring->cq.kflags) > + return -ENOTSUP; > + > + flags = *ring->cq.kflags; > + > + if (enabled) > + flags &= ~IORING_CQ_EVENTFD_DISABLED; > + else > + flags |= IORING_CQ_EVENTFD_DISABLED; > + > + IO_URING_WRITE_ONCE(*ring->cq.kflags, flags); > + > + return 0; > +} The -ENOTSUP seems a bit odd, I wonder if we should even flag that as an error. The function should probably also be io_uring_cq_eventfd_toggle() or something like that, as it does both enable and disable. Either that, or have two functions, and enable and disable. The bigger question is probably how to handle kernels that don't have this feature. It'll succeed, but we'll still post events. Maybe the kernel side should have a feature flag that we can test? -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-15 16:53 ` Jens Axboe @ 2020-05-15 17:11 ` Stefano Garzarella 2020-05-20 13:12 ` Stefano Garzarella 0 siblings, 1 reply; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 17:11 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring On Fri, May 15, 2020 at 10:53:50AM -0600, Jens Axboe wrote: > On 5/15/20 10:43 AM, Stefano Garzarella wrote: > > This patch adds the new IORING_CQ_EVENTFD_DISABLED flag. It can be > > used to disable/enable notifications from the kernel when a > > request is completed and queued to the CQ ring. > > > > We also add two helpers function to check if the notifications are > > enabled and to enable/disable them. > > > > If the kernel doesn't provide CQ ring flags, the notifications are > > always enabled if an eventfd is registered. > > > > Signed-off-by: Stefano Garzarella <[email protected]> > > --- > > src/include/liburing.h | 30 ++++++++++++++++++++++++++++++ > > src/include/liburing/io_uring.h | 7 +++++++ > > 2 files changed, 37 insertions(+) > > > > diff --git a/src/include/liburing.h b/src/include/liburing.h > > index ea596f6..fe03547 100644 > > --- a/src/include/liburing.h > > +++ b/src/include/liburing.h > > @@ -9,7 +9,9 @@ extern "C" { > > #include <sys/socket.h> > > #include <sys/uio.h> > > #include <sys/stat.h> > > +#include <errno.h> > > #include <signal.h> > > +#include <stdbool.h> > > #include <inttypes.h> > > #include <time.h> > > #include "liburing/compat.h" > > @@ -445,6 +447,34 @@ static inline unsigned io_uring_cq_ready(struct io_uring *ring) > > return io_uring_smp_load_acquire(ring->cq.ktail) - *ring->cq.khead; > > } > > > > +static inline int io_uring_cq_eventfd_enable(struct io_uring *ring, > > + bool enabled) > > +{ > > + uint32_t flags; > > + > > + if (!ring->cq.kflags) > > + return -ENOTSUP; > > + > > + flags = *ring->cq.kflags; > > + > > + if (enabled) > > + flags &= ~IORING_CQ_EVENTFD_DISABLED; > > + else > > + flags |= IORING_CQ_EVENTFD_DISABLED; > > + > > + IO_URING_WRITE_ONCE(*ring->cq.kflags, flags); > > + > > + return 0; > > +} > > The -ENOTSUP seems a bit odd, I wonder if we should even flag that as an > error. Do you think it's better to ignore the enabling/disabling if we don't have the flag field available? > > The function should probably also be io_uring_cq_eventfd_toggle() or > something like that, as it does both enable and disable. > > Either that, or have two functions, and enable and disable. Okay, I'll change it in io_uring_cq_eventfd_toggle(). > > The bigger question is probably how to handle kernels that don't > have this feature. It'll succeed, but we'll still post events. Maybe > the kernel side should have a feature flag that we can test? I thought about that, and initially I added a IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding the CQ 'flags' field together with the eventfd disabling feature. So I supposed that if 'p->cq_off.flags' is not zero, than the kernel supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit. Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE (or something similar)? Thanks, Stefano ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-15 17:11 ` Stefano Garzarella @ 2020-05-20 13:12 ` Stefano Garzarella 2020-05-20 13:43 ` Jens Axboe 0 siblings, 1 reply; 12+ messages in thread From: Stefano Garzarella @ 2020-05-20 13:12 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring On Fri, May 15, 2020 at 07:11:11PM +0200, Stefano Garzarella wrote: > On Fri, May 15, 2020 at 10:53:50AM -0600, Jens Axboe wrote: > > On 5/15/20 10:43 AM, Stefano Garzarella wrote: > > > This patch adds the new IORING_CQ_EVENTFD_DISABLED flag. It can be > > > used to disable/enable notifications from the kernel when a > > > request is completed and queued to the CQ ring. > > > > > > We also add two helpers function to check if the notifications are > > > enabled and to enable/disable them. > > > > > > If the kernel doesn't provide CQ ring flags, the notifications are > > > always enabled if an eventfd is registered. > > > > > > Signed-off-by: Stefano Garzarella <[email protected]> > > > --- > > > src/include/liburing.h | 30 ++++++++++++++++++++++++++++++ > > > src/include/liburing/io_uring.h | 7 +++++++ > > > 2 files changed, 37 insertions(+) > > > > > > diff --git a/src/include/liburing.h b/src/include/liburing.h > > > index ea596f6..fe03547 100644 > > > --- a/src/include/liburing.h > > > +++ b/src/include/liburing.h > > > @@ -9,7 +9,9 @@ extern "C" { > > > #include <sys/socket.h> > > > #include <sys/uio.h> > > > #include <sys/stat.h> > > > +#include <errno.h> > > > #include <signal.h> > > > +#include <stdbool.h> > > > #include <inttypes.h> > > > #include <time.h> > > > #include "liburing/compat.h" > > > @@ -445,6 +447,34 @@ static inline unsigned io_uring_cq_ready(struct io_uring *ring) > > > return io_uring_smp_load_acquire(ring->cq.ktail) - *ring->cq.khead; > > > } > > > > > > +static inline int io_uring_cq_eventfd_enable(struct io_uring *ring, > > > + bool enabled) > > > +{ > > > + uint32_t flags; > > > + > > > + if (!ring->cq.kflags) > > > + return -ENOTSUP; > > > + > > > + flags = *ring->cq.kflags; > > > + > > > + if (enabled) > > > + flags &= ~IORING_CQ_EVENTFD_DISABLED; > > > + else > > > + flags |= IORING_CQ_EVENTFD_DISABLED; > > > + > > > + IO_URING_WRITE_ONCE(*ring->cq.kflags, flags); > > > + > > > + return 0; > > > +} > > > > The -ENOTSUP seems a bit odd, I wonder if we should even flag that as an > > error. > > Do you think it's better to ignore the enabling/disabling if we don't have > the flag field available? > > > > > The function should probably also be io_uring_cq_eventfd_toggle() or > > something like that, as it does both enable and disable. > > > > Either that, or have two functions, and enable and disable. > > Okay, I'll change it in io_uring_cq_eventfd_toggle(). > > > > > The bigger question is probably how to handle kernels that don't > > have this feature. It'll succeed, but we'll still post events. Maybe > > the kernel side should have a feature flag that we can test? > > I thought about that, and initially I added a > IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding > the CQ 'flags' field together with the eventfd disabling feature. > > So I supposed that if 'p->cq_off.flags' is not zero, than the kernel > supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit. > > Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE > (or something similar)? Hi Jens, I'm changing io_uring_cq_eventfd_enable() to io_uring_cq_eventfd_toggle(). Any advice on the error and eventual feature flag? Thank you very much, Stefano ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-20 13:12 ` Stefano Garzarella @ 2020-05-20 13:43 ` Jens Axboe 2020-05-20 15:11 ` Stefano Garzarella 0 siblings, 1 reply; 12+ messages in thread From: Jens Axboe @ 2020-05-20 13:43 UTC (permalink / raw) To: Stefano Garzarella; +Cc: io-uring On 5/20/20 7:12 AM, Stefano Garzarella wrote: >>> The bigger question is probably how to handle kernels that don't >>> have this feature. It'll succeed, but we'll still post events. Maybe >>> the kernel side should have a feature flag that we can test? >> >> I thought about that, and initially I added a >> IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding >> the CQ 'flags' field together with the eventfd disabling feature. >> >> So I supposed that if 'p->cq_off.flags' is not zero, than the kernel >> supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit. >> >> Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE >> (or something similar)? > > Hi Jens, > I'm changing io_uring_cq_eventfd_enable() to io_uring_cq_eventfd_toggle(). Sounds good. > Any advice on the error and eventual feature flag? I guess we can use cq_off.flags != 0 to tell if we have this feature or not, even though it's a bit quirky. But at the same time, probably not worth adding a specific feature flag for. For the error, -EOPNOTSUPP seems fine if we don't have the feature. Just don't flag errors for enabling when already enabled, or vice versa. It's inherently racy in that completions can come in while the app is calling the helper, so we should make the interface relaxed. -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-20 13:43 ` Jens Axboe @ 2020-05-20 15:11 ` Stefano Garzarella 2020-05-20 15:19 ` Jens Axboe 0 siblings, 1 reply; 12+ messages in thread From: Stefano Garzarella @ 2020-05-20 15:11 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring On Wed, May 20, 2020 at 07:43:43AM -0600, Jens Axboe wrote: > On 5/20/20 7:12 AM, Stefano Garzarella wrote: > >>> The bigger question is probably how to handle kernels that don't > >>> have this feature. It'll succeed, but we'll still post events. Maybe > >>> the kernel side should have a feature flag that we can test? > >> > >> I thought about that, and initially I added a > >> IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding > >> the CQ 'flags' field together with the eventfd disabling feature. > >> > >> So I supposed that if 'p->cq_off.flags' is not zero, than the kernel > >> supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit. > >> > >> Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE > >> (or something similar)? > > > > Hi Jens, > > I'm changing io_uring_cq_eventfd_enable() to io_uring_cq_eventfd_toggle(). > > Sounds good. > > > Any advice on the error and eventual feature flag? > > I guess we can use cq_off.flags != 0 to tell if we have this feature or > not, even though it's a bit quirky. But at the same time, probably not > worth adding a specific feature flag for. Agree. > > For the error, -EOPNOTSUPP seems fine if we don't have the feature. Just > don't flag errors for enabling when already enabled, or vice versa. It's Okay. > inherently racy in that completions can come in while the app is calling > the helper, so we should make the interface relaxed. Yes, do you think we should also provide an interface to do double check while re-enabling notifications? Or we can leave this to the application? I mean something like this: bool io_uring_cq_eventfd_safe_enable(struct io_uring *ring) { /* enable notifications */ io_uring_cq_eventfd_toggle(ring, true); /* Do we have any more cqe in the ring? */ if (io_uring_cq_ready(ring)) { io_uring_cq_eventfd_toggle(ring, false); return false; } return true; } Thanks, Stefano ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status 2020-05-20 15:11 ` Stefano Garzarella @ 2020-05-20 15:19 ` Jens Axboe 0 siblings, 0 replies; 12+ messages in thread From: Jens Axboe @ 2020-05-20 15:19 UTC (permalink / raw) To: Stefano Garzarella; +Cc: io-uring On 5/20/20 9:11 AM, Stefano Garzarella wrote: > On Wed, May 20, 2020 at 07:43:43AM -0600, Jens Axboe wrote: >> On 5/20/20 7:12 AM, Stefano Garzarella wrote: >>>>> The bigger question is probably how to handle kernels that don't >>>>> have this feature. It'll succeed, but we'll still post events. Maybe >>>>> the kernel side should have a feature flag that we can test? >>>> >>>> I thought about that, and initially I added a >>>> IORING_FEAT_EVENTFD_DISABLE, but then I realized that we are adding >>>> the CQ 'flags' field together with the eventfd disabling feature. >>>> >>>> So I supposed that if 'p->cq_off.flags' is not zero, than the kernel >>>> supports CQ flags and also the IORING_CQ_EVENTFD_DISABLED bit. >>>> >>>> Do you think that's okay, or should we add IORING_FEAT_EVENTFD_DISABLE >>>> (or something similar)? >>> >>> Hi Jens, >>> I'm changing io_uring_cq_eventfd_enable() to io_uring_cq_eventfd_toggle(). >> >> Sounds good. >> >>> Any advice on the error and eventual feature flag? >> >> I guess we can use cq_off.flags != 0 to tell if we have this feature or >> not, even though it's a bit quirky. But at the same time, probably not >> worth adding a specific feature flag for. > > Agree. > >> >> For the error, -EOPNOTSUPP seems fine if we don't have the feature. Just >> don't flag errors for enabling when already enabled, or vice versa. It's > > Okay. > >> inherently racy in that completions can come in while the app is calling >> the helper, so we should make the interface relaxed. > > Yes, do you think we should also provide an interface to do double > check while re-enabling notifications? > Or we can leave this to the application? > > I mean something like this: > > bool io_uring_cq_eventfd_safe_enable(struct io_uring *ring) > { > /* enable notifications */ > io_uring_cq_eventfd_toggle(ring, true); > > /* Do we have any more cqe in the ring? */ > if (io_uring_cq_ready(ring)) { > io_uring_cq_eventfd_toggle(ring, false); > return false; > } > > return true; > } Let's just leave it for now unless/until there's a clear need for something like that. -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH liburing 4/5] man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella ` (2 preceding siblings ...) 2020-05-15 16:43 ` [PATCH liburing 3/5] Add helpers to set and get eventfd notification status Stefano Garzarella @ 2020-05-15 16:43 ` Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 5/5] Add test/eventfd-disable.c test case Stefano Garzarella 4 siblings, 0 replies; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring Signed-off-by: Stefano Garzarella <[email protected]> --- man/io_uring_register.2 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/man/io_uring_register.2 b/man/io_uring_register.2 index e64f688..5022c03 100644 --- a/man/io_uring_register.2 +++ b/man/io_uring_register.2 @@ -168,6 +168,14 @@ must contain a pointer to the eventfd file descriptor, and .I nr_args must be 1. Available since 5.2. +An application can temporarily disable notifications, coming through the +registered eventfd, by setting the +.B IORING_CQ_EVENTFD_DISABLED +bit in the +.I flags +field of the CQ ring. +Available since 5.8. + .TP .B IORING_REGISTER_EVENTFD_ASYNC This works just like -- 2.25.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH liburing 5/5] Add test/eventfd-disable.c test case 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella ` (3 preceding siblings ...) 2020-05-15 16:43 ` [PATCH liburing 4/5] man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description Stefano Garzarella @ 2020-05-15 16:43 ` Stefano Garzarella 4 siblings, 0 replies; 12+ messages in thread From: Stefano Garzarella @ 2020-05-15 16:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring This new test checks if the mechanism to enable/disable notifications through eventfd when a request is completed works correctly. Signed-off-by: Stefano Garzarella <[email protected]> --- .gitignore | 1 + test/Makefile | 4 +- test/eventfd-disable.c | 148 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 test/eventfd-disable.c diff --git a/.gitignore b/.gitignore index 9784f0b..7f01e6e 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,7 @@ /test/defer /test/eeed8b54e0df-test /test/eventfd +/test/eventfd-disable /test/eventfd-ring /test/fadvise /test/fallocate diff --git a/test/Makefile b/test/Makefile index ff4d4b8..a46e45d 100644 --- a/test/Makefile +++ b/test/Makefile @@ -21,7 +21,7 @@ all_targets += poll poll-cancel ring-leak fsync io_uring_setup io_uring_register file-update accept-reuse poll-v-poll fadvise madvise \ short-read openat2 probe shared-wq personality eventfd \ send_recv eventfd-ring across-fork sq-poll-kthread splice \ - lfs-openat lfs-openat-write + lfs-openat lfs-openat-write eventfd-disable include ../Makefile.quiet @@ -53,7 +53,7 @@ test_srcs := poll.c poll-cancel.c ring-leak.c fsync.c io_uring_setup.c \ file-update.c accept-reuse.c poll-v-poll.c fadvise.c \ madvise.c short-read.c openat2.c probe.c shared-wq.c \ personality.c eventfd.c eventfd-ring.c across-fork.c sq-poll-kthread.c \ - splice.c lfs-openat.c lfs-openat-write.c + splice.c lfs-openat.c lfs-openat-write.c eventfd-disable.c ifdef CONFIG_HAVE_STATX test_srcs += statx.c diff --git a/test/eventfd-disable.c b/test/eventfd-disable.c new file mode 100644 index 0000000..d4fb14e --- /dev/null +++ b/test/eventfd-disable.c @@ -0,0 +1,148 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Description: test disable/enable notifications through eventfd + * + */ +#include <errno.h> +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <string.h> +#include <fcntl.h> +#include <sys/poll.h> +#include <sys/eventfd.h> + +#include "liburing.h" + +int main(int argc, char *argv[]) +{ + struct io_uring_params p = {}; + struct io_uring_sqe *sqe; + struct io_uring_cqe *cqe; + struct io_uring ring; + uint64_t ptr; + struct iovec vec = { + .iov_base = &ptr, + .iov_len = sizeof(ptr) + }; + int ret, evfd, i; + + ret = io_uring_queue_init_params(64, &ring, &p); + if (ret) { + fprintf(stderr, "ring setup failed: %d\n", ret); + return 1; + } + + evfd = eventfd(0, EFD_CLOEXEC); + if (evfd < 0) { + perror("eventfd"); + return 1; + } + + ret = io_uring_register_eventfd(&ring, evfd); + if (ret) { + fprintf(stderr, "failed to register evfd: %d\n", ret); + return 1; + } + + if (!io_uring_cq_eventfd_enabled(&ring)) { + fprintf(stderr, "eventfd disabled\n"); + return 1; + } + + ret = io_uring_cq_eventfd_enable(&ring, false); + if (ret) { + fprintf(stdout, "Skipping, CQ flags not available!\n"); + return 0; + } + + sqe = io_uring_get_sqe(&ring); + io_uring_prep_readv(sqe, evfd, &vec, 1, 0); + sqe->user_data = 1; + + ret = io_uring_submit(&ring); + if (ret != 1) { + fprintf(stderr, "submit: %d\n", ret); + return 1; + } + + for (i = 0; i < 63; i++) { + sqe = io_uring_get_sqe(&ring); + io_uring_prep_nop(sqe); + sqe->user_data = 2; + } + + ret = io_uring_submit(&ring); + if (ret != 63) { + fprintf(stderr, "submit: %d\n", ret); + return 1; + } + + for (i = 0; i < 63; i++) { + ret = io_uring_wait_cqe(&ring, &cqe); + if (ret) { + fprintf(stderr, "wait: %d\n", ret); + return 1; + } + + switch (cqe->user_data) { + case 1: /* eventfd */ + fprintf(stderr, "eventfd unexpected: %d\n", (int)ptr); + return 1; + case 2: + if (cqe->res) { + fprintf(stderr, "nop: %d\n", cqe->res); + return 1; + } + break; + } + io_uring_cqe_seen(&ring, cqe); + } + + ret = io_uring_cq_eventfd_enable(&ring, true); + if (ret) { + fprintf(stderr, "io_uring_cq_eventfd_enable: %d\n", ret); + return 1; + } + + sqe = io_uring_get_sqe(&ring); + io_uring_prep_nop(sqe); + sqe->user_data = 2; + + ret = io_uring_submit(&ring); + if (ret != 1) { + fprintf(stderr, "submit: %d\n", ret); + return 1; + } + + for (i = 0; i < 2; i++) { + ret = io_uring_wait_cqe(&ring, &cqe); + if (ret) { + fprintf(stderr, "wait: %d\n", ret); + return 1; + } + + switch (cqe->user_data) { + case 1: /* eventfd */ + if (cqe->res != sizeof(ptr)) { + fprintf(stderr, "read res: %d\n", cqe->res); + return 1; + } + + if (ptr != 1) { + fprintf(stderr, "eventfd: %d\n", (int)ptr); + return 1; + } + break; + case 2: + if (cqe->res) { + fprintf(stderr, "nop: %d\n", cqe->res); + return 1; + } + break; + } + io_uring_cqe_seen(&ring, cqe); + } + + return 0; +} -- 2.25.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-20 15:21 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-15 16:43 [PATCH liburing 0/5] liburing: add helpers to enable/disable eventfd notifications Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 1/5] Add CQ ring 'flags' field Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 2/5] man/io_uring_setup.2: add 'flags' field in the struct io_cqring_offsets Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 3/5] Add helpers to set and get eventfd notification status Stefano Garzarella 2020-05-15 16:53 ` Jens Axboe 2020-05-15 17:11 ` Stefano Garzarella 2020-05-20 13:12 ` Stefano Garzarella 2020-05-20 13:43 ` Jens Axboe 2020-05-20 15:11 ` Stefano Garzarella 2020-05-20 15:19 ` Jens Axboe 2020-05-15 16:43 ` [PATCH liburing 4/5] man/io_uring_register.2: add IORING_CQ_EVENTFD_DISABLED description Stefano Garzarella 2020-05-15 16:43 ` [PATCH liburing 5/5] Add test/eventfd-disable.c test case Stefano Garzarella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox