All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 <sgarzare@redhat.com>
---
 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 <sgarzare@redhat.com>
---
 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 <sgarzare@redhat.com>
---
 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

* [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 <sgarzare@redhat.com>
---
 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 <sgarzare@redhat.com>
---
 .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

* 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 <sgarzare@redhat.com>
> ---
>  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 <sgarzare@redhat.com>
> > ---
> >  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 <sgarzare@redhat.com>
> > > ---
> > >  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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.