* [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API
@ 2021-10-04 13:31 Olivier Langlois
2021-10-04 14:38 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Olivier Langlois @ 2021-10-04 13:31 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov, io-uring
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API 2021-10-04 13:31 [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API Olivier Langlois @ 2021-10-04 14:38 ` Jens Axboe 2021-10-04 15:07 ` Olivier Langlois 0 siblings, 1 reply; 3+ messages in thread From: Jens Axboe @ 2021-10-04 14:38 UTC (permalink / raw) To: Olivier Langlois, Pavel Begunkov, io-uring 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API 2021-10-04 14:38 ` Jens Axboe @ 2021-10-04 15:07 ` Olivier Langlois 0 siblings, 0 replies; 3+ messages in thread From: Olivier Langlois @ 2021-10-04 15:07 UTC (permalink / raw) To: Jens Axboe, Pavel Begunkov, io-uring 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-04 15:07 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-04 13:31 [PATCH] liburing: Add io_uring_submit_and_wait_timeout function in API Olivier Langlois 2021-10-04 14:38 ` Jens Axboe 2021-10-04 15:07 ` Olivier Langlois
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).