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

[Jens Axboe <axboe@kernel.dk>]

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