Re: [PATCH liburing 3/5] Add helpers to set and get eventfd notification status

[Jens Axboe <axboe@kernel.dk>]

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