* [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
@ 2020-05-13 12:37 Xiaoguang Wang
2020-05-13 15:25 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Xiaoguang Wang @ 2020-05-13 12:37 UTC (permalink / raw)
To: io-uring; +Cc: axboe, joseph.qi, Xiaoguang Wang
When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
for sq thread to idle by busy loop:
while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
cond_resched();
Above codes are not friendly, indeed I think this busy loop will introduce a
cpu burst in current cpu, though it maybe short.
In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
sqes anymore, just discard leftover sqes.
Signed-off-by: Xiaoguang Wang <[email protected]>
---
fs/io_uring.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index ec0aa6957882..6e51140a5722 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5980,7 +5980,8 @@ static int io_sq_thread(void *data)
* If submit got -EBUSY, flag us as needing the application
* to enter the kernel to reap and flush events.
*/
- if (!to_submit || ret == -EBUSY) {
+ if (!to_submit || ret == -EBUSY ||
+ percpu_ref_is_dying(&ctx->refs)) {
/*
* Drop cur_mm before scheduling, we can't hold it for
* long periods (or over schedule()). Do this before
@@ -6027,7 +6028,8 @@ static int io_sq_thread(void *data)
smp_mb();
to_submit = io_sqring_entries(ctx);
- if (!to_submit || ret == -EBUSY) {
+ if (!to_submit || ret == -EBUSY ||
+ percpu_ref_is_dying(&ctx->refs)) {
if (kthread_should_park()) {
finish_wait(&ctx->sqo_wait, &wait);
break;
@@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
}
mutex_lock(&ctx->uring_lock);
- ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
+ if (likely(!percpu_ref_is_dying(&ctx->refs)))
+ ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
mutex_unlock(&ctx->uring_lock);
timeout = jiffies + ctx->sq_thread_idle;
}
@@ -7323,16 +7326,6 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)
percpu_ref_kill(&ctx->refs);
mutex_unlock(&ctx->uring_lock);
- /*
- * Wait for sq thread to idle, if we have one. It won't spin on new
- * work after we've killed the ctx ref above. This is important to do
- * before we cancel existing commands, as the thread could otherwise
- * be queueing new work post that. If that's work we need to cancel,
- * it could cause shutdown to hang.
- */
- while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
- cond_resched();
-
io_kill_timeouts(ctx);
io_poll_remove_all(ctx);
--
2.17.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
2020-05-13 12:37 [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying Xiaoguang Wang
@ 2020-05-13 15:25 ` Jens Axboe
2020-05-13 15:36 ` Xiaoguang Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-05-13 15:25 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: joseph.qi
On 5/13/20 6:37 AM, Xiaoguang Wang wrote:
> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
> for sq thread to idle by busy loop:
> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
> cond_resched();
> Above codes are not friendly, indeed I think this busy loop will introduce a
> cpu burst in current cpu, though it maybe short.
>
> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
> sqes anymore, just discard leftover sqes.
I don't think this really changes anything. What happens if:
> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
> }
>
> mutex_lock(&ctx->uring_lock);
> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
> + if (likely(!percpu_ref_is_dying(&ctx->refs)))
> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
> mutex_unlock(&ctx->uring_lock);
> timeout = jiffies + ctx->sq_thread_idle;
You check for dying here, but that could change basically while you're
checking it. So you're still submitting sqes with a ref that's going
away. You've only reduced the window, you haven't eliminated it.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
2020-05-13 15:25 ` Jens Axboe
@ 2020-05-13 15:36 ` Xiaoguang Wang
2020-05-16 9:23 ` Xiaoguang Wang
0 siblings, 1 reply; 6+ messages in thread
From: Xiaoguang Wang @ 2020-05-13 15:36 UTC (permalink / raw)
To: Jens Axboe, io-uring; +Cc: joseph.qi
hi,
> On 5/13/20 6:37 AM, Xiaoguang Wang wrote:
>> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
>> for sq thread to idle by busy loop:
>> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
>> cond_resched();
>> Above codes are not friendly, indeed I think this busy loop will introduce a
>> cpu burst in current cpu, though it maybe short.
>>
>> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
>> sqes anymore, just discard leftover sqes.
>
> I don't think this really changes anything. What happens if:
>
>> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
>> }
>>
>> mutex_lock(&ctx->uring_lock);
>> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>> + if (likely(!percpu_ref_is_dying(&ctx->refs)))
>> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>> mutex_unlock(&ctx->uring_lock);
>> timeout = jiffies + ctx->sq_thread_idle;
>
> You check for dying here, but that could change basically while you're
> checking it. So you're still submitting sqes with a ref that's going
> away. You've only reduced the window, you haven't eliminated it.
Look at codes, we call percpu_ref_kill() under uring_lock, so isn't it safe
to check the refs' dying status? Thanks.
Regards,
Xiaouang Wang
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
2020-05-13 15:36 ` Xiaoguang Wang
@ 2020-05-16 9:23 ` Xiaoguang Wang
2020-05-19 15:45 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Xiaoguang Wang @ 2020-05-16 9:23 UTC (permalink / raw)
To: Jens Axboe, io-uring; +Cc: joseph.qi
hi,
> hi,
>
>> On 5/13/20 6:37 AM, Xiaoguang Wang wrote:
>>> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
>>> for sq thread to idle by busy loop:
>>> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
>>> cond_resched();
>>> Above codes are not friendly, indeed I think this busy loop will introduce a
>>> cpu burst in current cpu, though it maybe short.
>>>
>>> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
>>> sqes anymore, just discard leftover sqes.
>>
>> I don't think this really changes anything. What happens if:
>>
>>> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
>>> }
>>> mutex_lock(&ctx->uring_lock);
>>> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>> + if (likely(!percpu_ref_is_dying(&ctx->refs)))
>>> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>> mutex_unlock(&ctx->uring_lock);
>>> timeout = jiffies + ctx->sq_thread_idle;
>>
>> You check for dying here, but that could change basically while you're
>> checking it. So you're still submitting sqes with a ref that's going
>> away. You've only reduced the window, you haven't eliminated it.
> Look at codes, we call percpu_ref_kill() under uring_lock, so isn't it safe
> to check the refs' dying status? Thanks.
Cloud you please have a look at my explanation again? Thanks.
Regards,
Xiaoguang Wang
>
> Regards,
> Xiaouang Wang
>
>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
2020-05-16 9:23 ` Xiaoguang Wang
@ 2020-05-19 15:45 ` Jens Axboe
2020-05-20 3:22 ` Xiaoguang Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-05-19 15:45 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: joseph.qi
On 5/16/20 3:23 AM, Xiaoguang Wang wrote:
> hi,
>
>> hi,
>>
>>> On 5/13/20 6:37 AM, Xiaoguang Wang wrote:
>>>> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
>>>> for sq thread to idle by busy loop:
>>>> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
>>>> cond_resched();
>>>> Above codes are not friendly, indeed I think this busy loop will introduce a
>>>> cpu burst in current cpu, though it maybe short.
>>>>
>>>> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
>>>> sqes anymore, just discard leftover sqes.
>>>
>>> I don't think this really changes anything. What happens if:
>>>
>>>> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
>>>> }
>>>> mutex_lock(&ctx->uring_lock);
>>>> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>>> + if (likely(!percpu_ref_is_dying(&ctx->refs)))
>>>> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>>> mutex_unlock(&ctx->uring_lock);
>>>> timeout = jiffies + ctx->sq_thread_idle;
>>>
>>> You check for dying here, but that could change basically while you're
>>> checking it. So you're still submitting sqes with a ref that's going
>>> away. You've only reduced the window, you haven't eliminated it.
>> Look at codes, we call percpu_ref_kill() under uring_lock, so isn't it safe
>> to check the refs' dying status? Thanks.
> Cloud you please have a look at my explanation again? Thanks.
Sorry for the delay - you are right, we only kill it inside the ring
mutex, so should be safe to check. Can we get by with just the very last
check instead of checking all of the cases?
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying
2020-05-19 15:45 ` Jens Axboe
@ 2020-05-20 3:22 ` Xiaoguang Wang
0 siblings, 0 replies; 6+ messages in thread
From: Xiaoguang Wang @ 2020-05-20 3:22 UTC (permalink / raw)
To: Jens Axboe, io-uring; +Cc: joseph.qi
hi,
> On 5/16/20 3:23 AM, Xiaoguang Wang wrote:
>> hi,
>>
>>> hi,
>>>
>>>> On 5/13/20 6:37 AM, Xiaoguang Wang wrote:
>>>>> When IORING_SETUP_SQPOLL is enabled, io_ring_ctx_wait_and_kill() will wait
>>>>> for sq thread to idle by busy loop:
>>>>> while (ctx->sqo_thread && !wq_has_sleeper(&ctx->sqo_wait))
>>>>> cond_resched();
>>>>> Above codes are not friendly, indeed I think this busy loop will introduce a
>>>>> cpu burst in current cpu, though it maybe short.
>>>>>
>>>>> In this patch, if ctx->refs is dying, we forbids sq_thread from submitting
>>>>> sqes anymore, just discard leftover sqes.
>>>>
>>>> I don't think this really changes anything. What happens if:
>>>>
>>>>> @@ -6051,7 +6053,8 @@ static int io_sq_thread(void *data)
>>>>> }
>>>>> mutex_lock(&ctx->uring_lock);
>>>>> - ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>>>> + if (likely(!percpu_ref_is_dying(&ctx->refs)))
>>>>> + ret = io_submit_sqes(ctx, to_submit, NULL, -1, true);
>>>>> mutex_unlock(&ctx->uring_lock);
>>>>> timeout = jiffies + ctx->sq_thread_idle;
>>>>
>>>> You check for dying here, but that could change basically while you're
>>>> checking it. So you're still submitting sqes with a ref that's going
>>>> away. You've only reduced the window, you haven't eliminated it.
>>> Look at codes, we call percpu_ref_kill() under uring_lock, so isn't it safe
>>> to check the refs' dying status? Thanks.
>> Cloud you please have a look at my explanation again? Thanks.
>
> Sorry for the delay - you are right, we only kill it inside the ring
> mutex, so should be safe to check. Can we get by with just the very last
> check instead of checking all of the cases?
Sure, I'll send V2 soon, and thanks for reviewing, I need this patch because I'm
writing a prototype to implement idea in this url:
https://lore.kernel.org/io-uring/[email protected]/T/#t
which needs above patch.
Regards,
Xiaoguang Wang
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-20 3:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-13 12:37 [RFC PATCH] io_uring: don't submit sqes when ctx->refs is dying Xiaoguang Wang
2020-05-13 15:25 ` Jens Axboe
2020-05-13 15:36 ` Xiaoguang Wang
2020-05-16 9:23 ` Xiaoguang Wang
2020-05-19 15:45 ` Jens Axboe
2020-05-20 3:22 ` Xiaoguang Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox