public inbox for [email protected]
 help / color / mirror / Atom feed
* [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled
@ 2020-04-13  7:19 Xiaoguang Wang
  2020-04-13 14:18 ` Jens Axboe
  0 siblings, 1 reply; 5+ messages in thread
From: Xiaoguang Wang @ 2020-04-13  7:19 UTC (permalink / raw)
  To: io-uring; +Cc: axboe, joseph.qi, Xiaoguang Wang

Indeed I'm not sure this patch is necessary, robust applications
should not call io_uring_submit when there are not sqes to submmit.
But still try to add this check, I have seen some applications which
call io_uring_submit(), but there are not sqes to submit.

Signed-off-by: Xiaoguang Wang <[email protected]>
---
 src/queue.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/queue.c b/src/queue.c
index e563775..75d707d 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -19,9 +19,10 @@
  * or if IORING_SQ_NEED_WAKEUP is set, so submit thread must be explicitly
  * awakened. For the latter case, we set the thread wakeup flag.
  */
-static inline bool sq_ring_needs_enter(struct io_uring *ring, unsigned *flags)
+static inline bool sq_ring_needs_enter(struct io_uring *ring,
+				unsigned submitted, unsigned *flags)
 {
-	if (!(ring->flags & IORING_SETUP_SQPOLL))
+	if (!(ring->flags & IORING_SETUP_SQPOLL) && submitted)
 		return true;
 	if (IO_URING_READ_ONCE(*ring->sq.kflags) & IORING_SQ_NEED_WAKEUP) {
 		*flags |= IORING_ENTER_SQ_WAKEUP;
@@ -51,7 +52,7 @@ int __io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
 		if (wait_nr)
 			flags = IORING_ENTER_GETEVENTS;
 		if (submit)
-			sq_ring_needs_enter(ring, &flags);
+			sq_ring_needs_enter(ring, submit, &flags);
 		if (wait_nr || submit)
 			ret = __sys_io_uring_enter(ring->ring_fd, submit,
 						   wait_nr, flags, sigmask);
@@ -197,7 +198,7 @@ static int __io_uring_submit(struct io_uring *ring, unsigned submitted,
 	int ret;
 
 	flags = 0;
-	if (sq_ring_needs_enter(ring, &flags) || wait_nr) {
+	if (sq_ring_needs_enter(ring, submitted, &flags) || wait_nr) {
 		if (wait_nr || (ring->flags & IORING_SETUP_IOPOLL))
 			flags |= IORING_ENTER_GETEVENTS;
 
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled
  2020-04-13  7:19 [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled Xiaoguang Wang
@ 2020-04-13 14:18 ` Jens Axboe
  2020-04-13 15:46   ` Hrvoje Zeba
  2020-04-13 22:27   ` Jens Axboe
  0 siblings, 2 replies; 5+ messages in thread
From: Jens Axboe @ 2020-04-13 14:18 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: joseph.qi

On 4/13/20 1:19 AM, Xiaoguang Wang wrote:
> Indeed I'm not sure this patch is necessary, robust applications
> should not call io_uring_submit when there are not sqes to submmit.
> But still try to add this check, I have seen some applications which
> call io_uring_submit(), but there are not sqes to submit.

Hmm, not sure it's worth complicating the submit path for that case.
A high performant application should not call io_uring_submit() if
it didn't queue anything new. Is this a common case you've seen?

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled
  2020-04-13 14:18 ` Jens Axboe
@ 2020-04-13 15:46   ` Hrvoje Zeba
  2020-04-13 15:59     ` Jens Axboe
  2020-04-13 22:27   ` Jens Axboe
  1 sibling, 1 reply; 5+ messages in thread
From: Hrvoje Zeba @ 2020-04-13 15:46 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Xiaoguang Wang, io-uring, joseph.qi

On Mon, Apr 13, 2020 at 11:29 AM Jens Axboe <[email protected]> wrote:
>
> On 4/13/20 1:19 AM, Xiaoguang Wang wrote:
> > Indeed I'm not sure this patch is necessary, robust applications
> > should not call io_uring_submit when there are not sqes to submmit.
> > But still try to add this check, I have seen some applications which
> > call io_uring_submit(), but there are not sqes to submit.
>
> Hmm, not sure it's worth complicating the submit path for that case.
> A high performant application should not call io_uring_submit() if
> it didn't queue anything new. Is this a common case you've seen?
>

My code calls io_uring_submit() even if there are no sqes to submit to
avoid spinning if there's nothing to do:

...
uint32_t sleep = (gt::user_contexts_waiting() > 0) ? 0 : 1;
auto res = io_uring_submit_and_wait(&m_io_uring, sleep);
...

-- 
I doubt, therefore I might be.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled
  2020-04-13 15:46   ` Hrvoje Zeba
@ 2020-04-13 15:59     ` Jens Axboe
  0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2020-04-13 15:59 UTC (permalink / raw)
  To: Hrvoje Zeba; +Cc: Xiaoguang Wang, io-uring, joseph.qi

On 4/13/20 9:46 AM, Hrvoje Zeba wrote:
> On Mon, Apr 13, 2020 at 11:29 AM Jens Axboe <[email protected]> wrote:
>>
>> On 4/13/20 1:19 AM, Xiaoguang Wang wrote:
>>> Indeed I'm not sure this patch is necessary, robust applications
>>> should not call io_uring_submit when there are not sqes to submmit.
>>> But still try to add this check, I have seen some applications which
>>> call io_uring_submit(), but there are not sqes to submit.
>>
>> Hmm, not sure it's worth complicating the submit path for that case.
>> A high performant application should not call io_uring_submit() if
>> it didn't queue anything new. Is this a common case you've seen?
>>
> 
> My code calls io_uring_submit() even if there are no sqes to submit to
> avoid spinning if there's nothing to do:
> 
> ...
> uint32_t sleep = (gt::user_contexts_waiting() > 0) ? 0 : 1;
> auto res = io_uring_submit_and_wait(&m_io_uring, sleep);

If you're calling with wait, then an sqe will be submitted with a
timeout operation. So that use case is fine. Or waiting in general.
But calling with {0,0} for {submit,wait} would be kind of silly.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled
  2020-04-13 14:18 ` Jens Axboe
  2020-04-13 15:46   ` Hrvoje Zeba
@ 2020-04-13 22:27   ` Jens Axboe
  1 sibling, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2020-04-13 22:27 UTC (permalink / raw)
  To: Xiaoguang Wang, io-uring; +Cc: joseph.qi

On 4/13/20 8:18 AM, Jens Axboe wrote:
> On 4/13/20 1:19 AM, Xiaoguang Wang wrote:
>> Indeed I'm not sure this patch is necessary, robust applications
>> should not call io_uring_submit when there are not sqes to submmit.
>> But still try to add this check, I have seen some applications which
>> call io_uring_submit(), but there are not sqes to submit.
> 
> Hmm, not sure it's worth complicating the submit path for that case.
> A high performant application should not call io_uring_submit() if
> it didn't queue anything new. Is this a common case you've seen?

Looking at it again, it actually seems quite reasonable. I've queued
it up, thanks!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-13 22:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-13  7:19 [LIBURING PATCH] sq_ring_needs_enter: check whether there are sqes when SQPOLL is not enabled Xiaoguang Wang
2020-04-13 14:18 ` Jens Axboe
2020-04-13 15:46   ` Hrvoje Zeba
2020-04-13 15:59     ` Jens Axboe
2020-04-13 22:27   ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox