[PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Stefano Garzarella]

The first patch adds the new 'cq_flags' field for the CQ ring. It
should be written by the application and read by the kernel.

The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
used by the application to enable/disable eventfd notifications.

I'm not sure the name is the best one, an alternative could be
IORING_CQ_NEED_EVENT.

This feature can be useful if the application are using eventfd to be
notified when requests are completed, but they don't want a notification
for every request.
Of course the application can already remove the eventfd from the event
loop, but as soon as it adds the eventfd again, it will be notified,
even if it has already handled all the completed requests.

The most important use case is when the registered eventfd is used to
notify a KVM guest through irqfd and we want a mechanism to
enable/disable interrupts.

I also extended liburing API and added a test case here:
https://github.com/stefano-garzarella/liburing/tree/eventfd-disable

Stefano Garzarella (2):
  io_uring: add 'cq_flags' field for the CQ ring
  io_uring: add IORING_CQ_NEED_WAKEUP to the CQ ring flags

 fs/io_uring.c                 | 17 ++++++++++++++++-
 include/uapi/linux/io_uring.h |  9 ++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCH 1/2] io_uring: add 'cq_flags' field for the CQ ring

[Stefano Garzarella]

This patch adds the new 'cq_flags' field that should be written by
the application and read by the kernel.

This new field is available to the userspace application through
'cq_off.flags'.
We are using 4-bytes previously reserved and set to zero. This means
that if the application finds this field to zero, then the new
functionality is not supported.

In the next patch we will introduce the first flag available.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 fs/io_uring.c                 | 10 +++++++++-
 include/uapi/linux/io_uring.h |  4 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 979d9f977409..6e8158269f3c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -142,7 +142,7 @@ struct io_rings {
 	 */
 	u32			sq_dropped;
 	/*
-	 * Runtime flags
+	 * Runtime SQ flags
 	 *
 	 * Written by the kernel, shouldn't be modified by the
 	 * application.
@@ -151,6 +151,13 @@ struct io_rings {
 	 * for IORING_SQ_NEED_WAKEUP after updating the sq tail.
 	 */
 	u32			sq_flags;
+	/*
+	 * Runtime CQ flags
+	 *
+	 * Written by the application, shouldn't be modified by the
+	 * kernel.
+	 */
+	u32                     cq_flags;
 	/*
 	 * Number of completion events lost because the queue was full;
 	 * this should be avoided by the application by making sure
@@ -7834,6 +7841,7 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p,
 	p->cq_off.ring_entries = offsetof(struct io_rings, cq_ring_entries);
 	p->cq_off.overflow = offsetof(struct io_rings, cq_overflow);
 	p->cq_off.cqes = offsetof(struct io_rings, cqes);
+	p->cq_off.flags = offsetof(struct io_rings, cq_flags);
 
 	p->features = IORING_FEAT_SINGLE_MMAP | IORING_FEAT_NODROP |
 			IORING_FEAT_SUBMIT_STABLE | IORING_FEAT_RW_CUR_POS |
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index e48d746b8e2a..602bb0ece607 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/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;
 };
 
 /*
-- 
2.25.4

[PATCH 2/2] io_uring: add IORING_CQ_NEED_WAKEUP to the CQ ring flags

[Stefano Garzarella]

This new flag should be set/clear from the application to
enable/disable eventfd notifications when a request is completed
and queued to the CQ ring.

Before this patch, notifications were always sent if an eventfd is
registered, so to remain backwards compatible we enable them during
initialization.

It will be up to the application to disable them after initialization if
no notifications are required at the beginning.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 fs/io_uring.c                 | 7 +++++++
 include/uapi/linux/io_uring.h | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6e8158269f3c..7c9486ea5aa3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1152,6 +1152,8 @@ static inline bool io_should_trigger_evfd(struct io_ring_ctx *ctx)
 {
 	if (!ctx->cq_ev_fd)
 		return false;
+	if (!(READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_NEED_WAKEUP))
+		return false;
 	if (!ctx->eventfd_async)
 		return true;
 	return io_wq_current_is_worker();
@@ -7684,6 +7686,11 @@ static int io_allocate_scq_urings(struct io_ring_ctx *ctx,
 	rings->cq_ring_mask = p->cq_entries - 1;
 	rings->sq_ring_entries = p->sq_entries;
 	rings->cq_ring_entries = p->cq_entries;
+	/*
+	 * For backward compatibility we start with eventfd notification
+	 * enabled.
+	 */
+	rings->cq_flags |= IORING_CQ_NEED_WAKEUP;
 	ctx->sq_mask = rings->sq_ring_mask;
 	ctx->cq_mask = rings->cq_ring_mask;
 	ctx->sq_entries = rings->sq_ring_entries;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 602bb0ece607..1d20dc61779e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -209,6 +209,11 @@ struct io_cqring_offsets {
 	__u64 resv2;
 };
 
+/*
+ * cq_ring->flags
+ */
+#define IORING_CQ_NEED_WAKEUP	(1U << 0) /* needs wakeup through eventfd */
+
 /*
  * io_uring_enter(2) flags
  */
-- 
2.25.4

Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Jens Axboe]

On 5/15/20 4:54 AM, Stefano Garzarella wrote:
> The first patch adds the new 'cq_flags' field for the CQ ring. It
> should be written by the application and read by the kernel.
> 
> The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
> used by the application to enable/disable eventfd notifications.
> 
> I'm not sure the name is the best one, an alternative could be
> IORING_CQ_NEED_EVENT.
> 
> This feature can be useful if the application are using eventfd to be
> notified when requests are completed, but they don't want a notification
> for every request.
> Of course the application can already remove the eventfd from the event
> loop, but as soon as it adds the eventfd again, it will be notified,
> even if it has already handled all the completed requests.
> 
> The most important use case is when the registered eventfd is used to
> notify a KVM guest through irqfd and we want a mechanism to
> enable/disable interrupts.
> 
> I also extended liburing API and added a test case here:
> https://github.com/stefano-garzarella/liburing/tree/eventfd-disable

Don't mind the feature, and I think the patches look fine. But the name
is really horrible, I'd have no idea what that flag does without looking
at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
don't have to muck with the default value either. The app would set the
flag to disable eventfd, temporarily, and clear it again when it wants
notifications again.

-- 
Jens Axboe

Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Stefano Garzarella]

On Fri, May 15, 2020 at 08:24:58AM -0600, Jens Axboe wrote:
> On 5/15/20 4:54 AM, Stefano Garzarella wrote:
> > The first patch adds the new 'cq_flags' field for the CQ ring. It
> > should be written by the application and read by the kernel.
> > 
> > The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
> > used by the application to enable/disable eventfd notifications.
> > 
> > I'm not sure the name is the best one, an alternative could be
> > IORING_CQ_NEED_EVENT.
> > 
> > This feature can be useful if the application are using eventfd to be
> > notified when requests are completed, but they don't want a notification
> > for every request.
> > Of course the application can already remove the eventfd from the event
> > loop, but as soon as it adds the eventfd again, it will be notified,
> > even if it has already handled all the completed requests.
> > 
> > The most important use case is when the registered eventfd is used to
> > notify a KVM guest through irqfd and we want a mechanism to
> > enable/disable interrupts.
> > 
> > I also extended liburing API and added a test case here:
> > https://github.com/stefano-garzarella/liburing/tree/eventfd-disable
> 
> Don't mind the feature, and I think the patches look fine. But the name
> is really horrible, I'd have no idea what that flag does without looking
> at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
> something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
> don't have to muck with the default value either. The app would set the
> flag to disable eventfd, temporarily, and clear it again when it wants
> notifications again.

You're clearly right! :-) The name was horrible.

I agree that IORING_CQ_EVENTFD_DISABLED should be the best.
I'll send a v2 changing the name and removing the default value.

Thanks,
Stefano

Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Jens Axboe]

On 5/15/20 8:34 AM, Stefano Garzarella wrote:
> On Fri, May 15, 2020 at 08:24:58AM -0600, Jens Axboe wrote:
>> On 5/15/20 4:54 AM, Stefano Garzarella wrote:
>>> The first patch adds the new 'cq_flags' field for the CQ ring. It
>>> should be written by the application and read by the kernel.
>>>
>>> The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
>>> used by the application to enable/disable eventfd notifications.
>>>
>>> I'm not sure the name is the best one, an alternative could be
>>> IORING_CQ_NEED_EVENT.
>>>
>>> This feature can be useful if the application are using eventfd to be
>>> notified when requests are completed, but they don't want a notification
>>> for every request.
>>> Of course the application can already remove the eventfd from the event
>>> loop, but as soon as it adds the eventfd again, it will be notified,
>>> even if it has already handled all the completed requests.
>>>
>>> The most important use case is when the registered eventfd is used to
>>> notify a KVM guest through irqfd and we want a mechanism to
>>> enable/disable interrupts.
>>>
>>> I also extended liburing API and added a test case here:
>>> https://github.com/stefano-garzarella/liburing/tree/eventfd-disable
>>
>> Don't mind the feature, and I think the patches look fine. But the name
>> is really horrible, I'd have no idea what that flag does without looking
>> at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
>> something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
>> don't have to muck with the default value either. The app would set the
>> flag to disable eventfd, temporarily, and clear it again when it wants
>> notifications again.
> 
> You're clearly right! :-) The name was horrible.

Sometimes you go down that path on naming and just can't think of
the right one. I think we've all been there.

> I agree that IORING_CQ_EVENTFD_DISABLED should be the best.
> I'll send a v2 changing the name and removing the default value.

Great thanks, and please do queue a pull for the liburing side too.

-- 
Jens Axboe

Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Stefano Garzarella]

On Fri, May 15, 2020 at 09:13:33AM -0600, Jens Axboe wrote:
> On 5/15/20 8:34 AM, Stefano Garzarella wrote:
> > On Fri, May 15, 2020 at 08:24:58AM -0600, Jens Axboe wrote:
> >> On 5/15/20 4:54 AM, Stefano Garzarella wrote:
> >>> The first patch adds the new 'cq_flags' field for the CQ ring. It
> >>> should be written by the application and read by the kernel.
> >>>
> >>> The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
> >>> used by the application to enable/disable eventfd notifications.
> >>>
> >>> I'm not sure the name is the best one, an alternative could be
> >>> IORING_CQ_NEED_EVENT.
> >>>
> >>> This feature can be useful if the application are using eventfd to be
> >>> notified when requests are completed, but they don't want a notification
> >>> for every request.
> >>> Of course the application can already remove the eventfd from the event
> >>> loop, but as soon as it adds the eventfd again, it will be notified,
> >>> even if it has already handled all the completed requests.
> >>>
> >>> The most important use case is when the registered eventfd is used to
> >>> notify a KVM guest through irqfd and we want a mechanism to
> >>> enable/disable interrupts.
> >>>
> >>> I also extended liburing API and added a test case here:
> >>> https://github.com/stefano-garzarella/liburing/tree/eventfd-disable
> >>
> >> Don't mind the feature, and I think the patches look fine. But the name
> >> is really horrible, I'd have no idea what that flag does without looking
> >> at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
> >> something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
> >> don't have to muck with the default value either. The app would set the
> >> flag to disable eventfd, temporarily, and clear it again when it wants
> >> notifications again.
> > 
> > You're clearly right! :-) The name was horrible.
> 
> Sometimes you go down that path on naming and just can't think of
> the right one. I think we've all been there.

:-)

> 
> > I agree that IORING_CQ_EVENTFD_DISABLED should be the best.
> > I'll send a v2 changing the name and removing the default value.
> 
> Great thanks, and please do queue a pull for the liburing side too.

For the liburing side do you prefer a PR on github or posting the
patches on io-uring@vger.kernel.org with 'liburing' tag?

Thanks,
Stefano

Re: [PATCH 0/2] io_uring: add a CQ ring flag to enable/disable eventfd notification

[Jens Axboe]

On 5/15/20 9:24 AM, Stefano Garzarella wrote:
> On Fri, May 15, 2020 at 09:13:33AM -0600, Jens Axboe wrote:
>> On 5/15/20 8:34 AM, Stefano Garzarella wrote:
>>> On Fri, May 15, 2020 at 08:24:58AM -0600, Jens Axboe wrote:
>>>> On 5/15/20 4:54 AM, Stefano Garzarella wrote:
>>>>> The first patch adds the new 'cq_flags' field for the CQ ring. It
>>>>> should be written by the application and read by the kernel.
>>>>>
>>>>> The second patch adds a new IORING_CQ_NEED_WAKEUP flag that can be
>>>>> used by the application to enable/disable eventfd notifications.
>>>>>
>>>>> I'm not sure the name is the best one, an alternative could be
>>>>> IORING_CQ_NEED_EVENT.
>>>>>
>>>>> This feature can be useful if the application are using eventfd to be
>>>>> notified when requests are completed, but they don't want a notification
>>>>> for every request.
>>>>> Of course the application can already remove the eventfd from the event
>>>>> loop, but as soon as it adds the eventfd again, it will be notified,
>>>>> even if it has already handled all the completed requests.
>>>>>
>>>>> The most important use case is when the registered eventfd is used to
>>>>> notify a KVM guest through irqfd and we want a mechanism to
>>>>> enable/disable interrupts.
>>>>>
>>>>> I also extended liburing API and added a test case here:
>>>>> https://github.com/stefano-garzarella/liburing/tree/eventfd-disable
>>>>
>>>> Don't mind the feature, and I think the patches look fine. But the name
>>>> is really horrible, I'd have no idea what that flag does without looking
>>>> at the code or a man page. Why not call it IORING_CQ_EVENTFD_ENABLED or
>>>> something like that? Or maybe IORING_CQ_EVENTFD_DISABLED, and then you
>>>> don't have to muck with the default value either. The app would set the
>>>> flag to disable eventfd, temporarily, and clear it again when it wants
>>>> notifications again.
>>>
>>> You're clearly right! :-) The name was horrible.
>>
>> Sometimes you go down that path on naming and just can't think of
>> the right one. I think we've all been there.
> 
> :-)
> 
>>
>>> I agree that IORING_CQ_EVENTFD_DISABLED should be the best.
>>> I'll send a v2 changing the name and removing the default value.
>>
>> Great thanks, and please do queue a pull for the liburing side too.
> 
> For the liburing side do you prefer a PR on github or posting the
> patches on io-uring@vger.kernel.org with 'liburing' tag?

Either one is fine. I tend to prefer patches, but that's mostly because
various contributors on the liburing side don't have the same kind of
patch writing etiquette that we do on the kernel side. Hence they need
massaging in terms of commit messages, and patches are then easier. But
for you, just do what you prefer. I never use the github merge features,
always do it manually myself anyway.

-- 
Jens Axboe