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

[Olivier Langlois <olivier@trillion01.com>]

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.