[PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API

[PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API

[Olivier Langlois]

before commit 0ea4ccd1c0e4 ("src/queue: don't flush SQ ring for new wait interface"),
io_uring_wait_cqes() was serving the purpose of submit sqe and wait for cqe up to a certain timeout value.

Since the commit, a new function is needed to fill this gap.

Fixes: https://github.com/axboe/liburing/issues/440
Signed-off-by: Olivier Langlois <olivier@trillion01.com>
---
 src/include/liburing.h |  5 +++++
 src/liburing.map       |  5 +++++
 src/queue.c            | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/src/include/liburing.h b/src/include/liburing.h
index 0c2c5c2..fe8bfbe 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -122,6 +122,11 @@ int io_uring_wait_cqe_timeout(struct io_uring *ring,
 			      struct __kernel_timespec *ts);
 int io_uring_submit(struct io_uring *ring);
 int io_uring_submit_and_wait(struct io_uring *ring, unsigned wait_nr);
+int io_uring_submit_and_wait_timout(struct io_uring *ring,
+				    struct io_uring_cqe **cqe_ptr,
+				    unsigned wait_nr,
+				    struct __kernel_timespec *ts,
+				    sigset_t *sigmask);
 struct io_uring_sqe *io_uring_get_sqe(struct io_uring *ring);
 
 int io_uring_register_buffers(struct io_uring *ring, const struct iovec *iovecs,
diff --git a/src/liburing.map b/src/liburing.map
index 6692a3b..09f4275 100644
--- a/src/liburing.map
+++ b/src/liburing.map
@@ -44,3 +44,8 @@ LIBURING_2.1 {
 		io_uring_unregister_iowq_aff;
 		io_uring_register_iowq_max_workers;
 } LIBURING_2.0;
+
+LIBURING_2.2 {
+	global:
+		io_uring_submit_and_wait_timout;
+} LIBURING_2.1;
diff --git a/src/queue.c b/src/queue.c
index 31aa17c..9ac9fe5 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -305,6 +305,39 @@ int io_uring_wait_cqes(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
 	return __io_uring_get_cqe(ring, cqe_ptr, to_submit, wait_nr, sigmask);
 }
 
+int io_uring_submit_and_wait_timout(struct io_uring *ring,
+				    struct io_uring_cqe **cqe_ptr,
+				    unsigned wait_nr,
+				    struct __kernel_timespec *ts,
+				    sigset_t *sigmask)
+{
+	if (uring_likely(ts)) {
+		if (uring_unlikely(!(ring->features & IORING_FEAT_EXT_ARG)))
+			return io_uring_wait_cqes(ring, cqe_ptr, wait_nr,
+						  ts, sigmask);
+		else {
+			struct io_uring_getevents_arg arg = {
+				.sigmask	= (unsigned long) sigmask,
+				.sigmask_sz	= _NSIG / 8,
+				.ts		= (unsigned long) ts
+			};
+			struct get_data data = {
+				.submit		= __io_uring_flush_sq(ring),
+				.wait_nr	= wait_nr,
+				.get_flags	= IORING_ENTER_EXT_ARG,
+				.sz		= sizeof(arg),
+				.arg		= &arg
+			};
+
+			return _io_uring_get_cqe(ring, cqe_ptr, &data);
+		}
+	}
+	else
+		return __io_uring_get_cqe(ring, cqe_ptr,
+					  __io_uring_flush_sq(ring),
+					  wait_nr, sigmask);
+}
+
 /*
  * See io_uring_wait_cqes() - this function is the same, it just always uses
  * '1' as the wait_nr.
-- 
2.33.0

Re: [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API

[Jens Axboe]

On 10/4/21 7:31 AM, Olivier Langlois wrote:
> before commit 0ea4ccd1c0e4 ("src/queue: don't flush SQ ring for new wait interface"),
> io_uring_wait_cqes() was serving the purpose of submit sqe and wait for cqe up to a certain timeout value.
> 
> Since the commit, a new function is needed to fill this gap.
> 
> Fixes: https://github.com/axboe/liburing/issues/440
> Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> ---
>  src/include/liburing.h |  5 +++++
>  src/liburing.map       |  5 +++++
>  src/queue.c            | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/src/include/liburing.h b/src/include/liburing.h
> index 0c2c5c2..fe8bfbe 100644
> --- a/src/include/liburing.h
> +++ b/src/include/liburing.h
> @@ -122,6 +122,11 @@ int io_uring_wait_cqe_timeout(struct io_uring *ring,
>  			      struct __kernel_timespec *ts);
>  int io_uring_submit(struct io_uring *ring);
>  int io_uring_submit_and_wait(struct io_uring *ring, unsigned wait_nr);
> +int io_uring_submit_and_wait_timout(struct io_uring *ring,
> +				    struct io_uring_cqe **cqe_ptr,
> +				    unsigned wait_nr,
> +				    struct __kernel_timespec *ts,
> +				    sigset_t *sigmask);
>  struct io_uring_sqe *io_uring_get_sqe(struct io_uring *ring);
>  
>  int io_uring_register_buffers(struct io_uring *ring, const struct iovec *iovecs,
> diff --git a/src/liburing.map b/src/liburing.map
> index 6692a3b..09f4275 100644
> --- a/src/liburing.map
> +++ b/src/liburing.map
> @@ -44,3 +44,8 @@ LIBURING_2.1 {
>  		io_uring_unregister_iowq_aff;
>  		io_uring_register_iowq_max_workers;
>  } LIBURING_2.0;
> +
> +LIBURING_2.2 {
> +	global:
> +		io_uring_submit_and_wait_timout;
> +} LIBURING_2.1;
> diff --git a/src/queue.c b/src/queue.c
> index 31aa17c..9ac9fe5 100644
> --- a/src/queue.c
> +++ b/src/queue.c
> @@ -305,6 +305,39 @@ int io_uring_wait_cqes(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
>  	return __io_uring_get_cqe(ring, cqe_ptr, to_submit, wait_nr, sigmask);
>  }
>  
> +int io_uring_submit_and_wait_timout(struct io_uring *ring,
> +				    struct io_uring_cqe **cqe_ptr,
> +				    unsigned wait_nr,
> +				    struct __kernel_timespec *ts,
> +				    sigset_t *sigmask)
> +{
> +	if (uring_likely(ts)) {
> +		if (uring_unlikely(!(ring->features & IORING_FEAT_EXT_ARG)))
> +			return io_uring_wait_cqes(ring, cqe_ptr, wait_nr,
> +						  ts, sigmask);
> +		else {
> +			struct io_uring_getevents_arg arg = {
> +				.sigmask	= (unsigned long) sigmask,
> +				.sigmask_sz	= _NSIG / 8,
> +				.ts		= (unsigned long) ts
> +			};
> +			struct get_data data = {
> +				.submit		= __io_uring_flush_sq(ring),
> +				.wait_nr	= wait_nr,
> +				.get_flags	= IORING_ENTER_EXT_ARG,
> +				.sz		= sizeof(arg),
> +				.arg		= &arg
> +			};
> +
> +			return _io_uring_get_cqe(ring, cqe_ptr, &data);
> +		}
> +	}
> +	else
> +		return __io_uring_get_cqe(ring, cqe_ptr,
> +					  __io_uring_flush_sq(ring),
> +					  wait_nr, sigmask);
> +}

I'd get rid of the likely/unlikely, imho it just hinders readability
and for some cases they may end up being wrong. You also don't need an
else when there's a return, and if you use braces on one condition, use
it for all. IOW, something like:

	if (ts) {
		if (ring->features & IORING_FEAT_EXT_ARG) {
			struct io_uring_getevents_arg arg = {
				.sigmask	= (unsigned long) sigmask,
				.sigmask_sz	= _NSIG / 8,
				.ts		= (unsigned long) ts
			};
			struct get_data data = {
				.submit		= __io_uring_flush_sq(ring),
				.wait_nr	= wait_nr,
				.get_flags	= IORING_ENTER_EXT_ARG,
				.sz		= sizeof(arg),
				.arg		= &arg
			};

			return _io_uring_get_cqe(ring, cqe_ptr, &data);
		}
		return io_uring_wait_cqes(ring, cqe_ptr, wait_nr, ts, sigmask);
	}

	return __io_uring_get_cqe(ring, cqe_ptr, __io_uring_flush_sq(ring),
					  wait_nr, sigmask);

which is a lot more readable too.

-- 
Jens Axboe

Re: [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API

[Olivier Langlois]

On Mon, 2021-10-04 at 08:38 -0600, Jens Axboe wrote:
> 
> I'd get rid of the likely/unlikely, imho it just hinders readability
> and for some cases they may end up being wrong. You also don't need
> an
> else when there's a return, and if you use braces on one condition,
> use
> it for all. IOW, something like:
> 
>         if (ts) {
>                 if (ring->features & IORING_FEAT_EXT_ARG) {
>                         struct io_uring_getevents_arg arg = {
>                                 .sigmask        = (unsigned long)
> sigmask,
>                                 .sigmask_sz     = _NSIG / 8,
>                                 .ts             = (unsigned long) ts
>                         };
>                         struct get_data data = {
>                                 .submit         =
> __io_uring_flush_sq(ring),
>                                 .wait_nr        = wait_nr,
>                                 .get_flags      =
> IORING_ENTER_EXT_ARG,
>                                 .sz             = sizeof(arg),
>                                 .arg            = &arg
>                         };
> 
>                         return _io_uring_get_cqe(ring, cqe_ptr,
> &data);
>                 }
>                 return io_uring_wait_cqes(ring, cqe_ptr, wait_nr, ts,
> sigmask);
>         }
> 
>         return __io_uring_get_cqe(ring, cqe_ptr,
> __io_uring_flush_sq(ring),
>                                           wait_nr, sigmask);
> 
> which is a lot more readable too.
> 

ok. I will do as you suggest. I agree with the readability argument.

However, I believe that the likely events were correctly identified as
if ts is NULL, you should be calling io_uring_submit_and_wait() instead
of this new function.

and the second condition as no other choice to become more and more
likely as the time pass.

That being said, out of my old memories, I remember that if you want to
optimize code manually to help x86 branch prediction algo, you can do
so by crafting the conditional expression to be true for the most
likely case so what you propose still keep the same flow implicitly
(with better readability).

Since submission, I got another improvement idea that I will integrate
in v2.

I am leveraging io_uring_wait_cqes() code for the case where the kernel
is old and timeout must be passed as an extra sqe.

1. It is brittle. If someone change that part of io_uring_wait_cqes(),
he could unknowingly break io_uring_submit_and_wait_timeout().
2. The intent behind calling io_uring_wait_cqes from
io_uring_submit_and_wait_timeout isn't 100% clear IMHO.

I am thinking factoring out the common code into a static function.
Considering that running over an old kernel should, hopefully, become a
rare event, the extra function call shouldn't be a too big issue to
reach a better readability.