iopoll improvements
cover iopoll improvements
iopoll'ing should be more responsive with these.
Pavel Begunkov (3):
io_uring: don't delay iopoll'ed req completion
io_uring: fix stopping iopoll'ing too early
io_uring: briefly loose locks while reaping events
fs/io_uring.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
Signature · 1 line
2.24.0
PATCH 1/3 io_uring: don't delay iopoll'ed req completion
->iopoll() may have completed current request, but instead of reaping it, io_do_iopoll() just continues with the next request in the list. As a result it can leave just polled and completed request in the list up until next syscall. Even outer loop in io_iopoll_getevents() doesn't help the situation.
E.g. poll_list: req0 -> req1
If req0->iopoll() completed both requests, and @min<=1, then @req0 will be left behind.
Check whether a req was completed after ->iopoll().
--- fs/io_uring.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/io_uring.c +4 -0
@@ -2008,6 +2008,10 @@ static int io_do_iopoll(struct io_ring_ctx *ctx, unsigned int *nr_events,if (ret <0 )break ; +/* iopoll may have completed current req */ +if (READ_ONCE (req->iopoll_completed)) +list_move_tail (&req->list, &done); +if (ret && spin) spin = false; ret =0 ;
Signature · 1 line
2.24.0
PATCH 2/3 io_uring: fix stopping iopoll'ing too early
In reply to Pavel BegunkovNobody adjusts *nr_events (number of completed requests) before calling io_iopoll_getevents(), so the passed @min shouldn't be adjusted as well. Othewise it can return less than initially asked @min without hitting need_resched().
--- fs/io_uring.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
fs/io_uring.c +2 -7
@@ -2037,7 +2037,7 @@ static int io_iopoll_getevents(struct io_ring_ctx *ctx, unsigned int *nr_events, ret =io_do_iopoll (ctx, nr_events, min);if (ret <0 )return ret; -if (!min || *nr_events >= min) +if (*nr_events >= min)return 0 ; }
@@ -2080,8 +2080,6 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, unsigned *nr_events,*/ mutex_lock (&ctx->uring_lock); do { - int tmin = 0; - /* * Don't enter poll loop if we already have events pending. * If we do, we can potentially be spinning for commands that
@@ -2106,10 +2104,7 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, unsigned *nr_events,mutex_lock (&ctx->uring_lock); } -if (*nr_events < min) - tmin = min - *nr_events; - - ret =io_iopoll_getevents (ctx, nr_events, tmin); + ret =io_iopoll_getevents (ctx, nr_events, min);if (ret <=0 )break ; ret =0 ;
Signature · 1 line
2.24.0
PATCH 3/3 io_uring: briefly loose locks while reaping events
In reply to Pavel BegunkovIt's not nice to hold @uring_lock for too long io_iopoll_reap_events(). For instance, the lock is needed to publish requests to @poll_list, and that locks out tasks doing that for no good reason. Loose it occasionally.
--- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)
fs/io_uring.c +3 -0
@@ -2059,11 +2059,14 @@ static void io_iopoll_reap_events(struct io_ring_ctx *ctx)io_iopoll_getevents (ctx, &nr_events,1 ); +/* task_work takes the lock to publish a req to @poll_list */ +mutex_unlock (&ctx->uring_lock);/* * Ensure we allow local-to-the-cpu processing to take place, * in this case we need to ensure that we reap all events. */ cond_resched (); +mutex_lock (&ctx->uring_lock); }mutex_unlock (&ctx->uring_lock); }
Signature · 1 line
2.24.0
Jens Axboe
maintainerRe: [PATCH 3/3] io_uring: briefly loose locks while reaping events
Quoted 19 lines · Pavel Begunkov
It's not nice to hold @uring_lock for too long
io_iopoll_reap_events(). For instance, the lock is needed to publish requests to @poll_list, and that locks out tasks doing that for no good reason. Loose it occasionally.Signed-off-by Pavel Begunkov <[email protected]>--- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)fs/io_uring.c +1
-0@@ -2059,11 +2059,14 @@ static void io_iopoll_reap_events(struct io_ring_ctx *ctx)io_iopoll_getevents (ctx, &nr_events,1 ); +/* task_work takes the lock to publish a req to @poll_list */
fs/io_uring.c static void io_iopoll_reap_events(struct io_ring_ctx *ctx) · line 2072
Quoted 10 lines
fs/io_uring.c +2
-0+mutex_unlock (&ctx->uring_lock);/* * Ensure we allow local-to-the-cpu processing to take place, * in this case we need to ensure that we reap all events. */ cond_resched (); +mutex_lock (&ctx->uring_lock); }mutex_unlock (&ctx->uring_lock); }
This would be much better written as:
if (need_resched()) {
mutex_unlock(&ctx->uring_lock);
cond_resched();
mutex_lock(&ctx->uring_lock);
}to avoid shuffling the lock when not needed to. Every cycle counts for polled IO.
Signature · 1 line
Jens Axboe
Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events
Quoted 33 lines · Jens Axboe
Quoted 19 lines · Pavel Begunkov
It's not nice to hold @uring_lock for too long
io_iopoll_reap_events(). For instance, the lock is needed to publish requests to @poll_list, and that locks out tasks doing that for no good reason. Loose it occasionally.Signed-off-by Pavel Begunkov <[email protected]>--- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)fs/io_uring.c +1
-0@@ -2059,11 +2059,14 @@ static void io_iopoll_reap_events(struct io_ring_ctx *ctx)io_iopoll_getevents (ctx, &nr_events,1 ); +/* task_work takes the lock to publish a req to @poll_list */ fs/io_uring.c static void io_iopoll_reap_events(struct io_ring_ctx *ctx) · line 2072
Quoted 10 lines
fs/io_uring.c +2
-0+mutex_unlock (&ctx->uring_lock);/* * Ensure we allow local-to-the-cpu processing to take place, * in this case we need to ensure that we reap all events. */ cond_resched (); +mutex_lock (&ctx->uring_lock); }mutex_unlock (&ctx->uring_lock); }This would be much better written as:
Quoted 8 lines
if (need_resched()) { mutex_unlock(&ctx->uring_lock); cond_resched(); mutex_lock(&ctx->uring_lock); }to avoid shuffling the lock when not needed to. Every cycle counts for polled IO.
It happens only when io_uring is being killed, can't imagine any sane app trying to catch CQEs after doing that. I'll resend
Signature · 1 line
Pavel Begunkov
Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events
Quoted 40 lines · Pavel Begunkov
Quoted 39 lines · Jens Axboe
Quoted 19 lines · Pavel Begunkov
It's not nice to hold @uring_lock for too long
io_iopoll_reap_events(). For instance, the lock is needed to publish requests to @poll_list, and that locks out tasks doing that for no good reason. Loose it occasionally.Signed-off-by Pavel Begunkov <[email protected]>--- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)fs/io_uring.c +1
-0@@ -2059,11 +2059,14 @@ static void io_iopoll_reap_events(struct io_ring_ctx *ctx)io_iopoll_getevents (ctx, &nr_events,1 ); +/* task_work takes the lock to publish a req to @poll_list */ fs/io_uring.c static void io_iopoll_reap_events(struct io_ring_ctx *ctx) · line 2072
Quoted 10 lines
fs/io_uring.c +2
-0+mutex_unlock (&ctx->uring_lock);/* * Ensure we allow local-to-the-cpu processing to take place, * in this case we need to ensure that we reap all events. */ cond_resched (); +mutex_lock (&ctx->uring_lock); }mutex_unlock (&ctx->uring_lock); }This would be much better written as:
if (need_resched()) { mutex_unlock(&ctx->uring_lock); cond_resched(); mutex_lock(&ctx->uring_lock); }
Quoted 5 lines
Quoted 2 lines
to avoid shuffling the lock when not needed to. Every cycle counts for polled IO.
It happens only when
io_uringis being killed, can't imagine any sane app trying to catch CQEs after doing that. I'll resend
Also, io_iopoll_getevents() already does need_resched(). Hmm, do you wan't me to resend?
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerRe: [PATCH 3/3] io_uring: briefly loose locks while reaping events
Quoted 43 lines · Pavel Begunkov
Quoted 40 lines · Pavel Begunkov
Quoted 39 lines · Jens Axboe
Quoted 19 lines · Pavel Begunkov
It's not nice to hold @uring_lock for too long
io_iopoll_reap_events(). For instance, the lock is needed to publish requests to @poll_list, and that locks out tasks doing that for no good reason. Loose it occasionally.Signed-off-by Pavel Begunkov <[email protected]>--- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)fs/io_uring.c +1
-0@@ -2059,11 +2059,14 @@ static void io_iopoll_reap_events(struct io_ring_ctx *ctx)io_iopoll_getevents (ctx, &nr_events,1 ); +/* task_work takes the lock to publish a req to @poll_list */ fs/io_uring.c static void io_iopoll_reap_events(struct io_ring_ctx *ctx) · line 2072
Quoted 10 lines
fs/io_uring.c +2
-0+mutex_unlock (&ctx->uring_lock);/* * Ensure we allow local-to-the-cpu processing to take place, * in this case we need to ensure that we reap all events. */ cond_resched (); +mutex_lock (&ctx->uring_lock); }mutex_unlock (&ctx->uring_lock); }This would be much better written as:
if (need_resched()) { mutex_unlock(&ctx->uring_lock); cond_resched(); mutex_lock(&ctx->uring_lock); }
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
to avoid shuffling the lock when not needed to. Every cycle counts for polled IO.
It happens only when
io_uringis being killed, can't imagine any sane app trying to catch CQEs after doing that. I'll resendAlso,
io_iopoll_getevents()already doesneed_resched(). Hmm, do you wan't me to resend?
I missed this was the "reap on dead" path. But yes, please resend.
Signature · 1 line
Jens Axboe