* [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async()
@ 2019-05-13 18:20 Roman Penyaev
2019-05-13 18:25 ` Roman Penyaev
2019-05-13 18:52 ` Jens Axboe
0 siblings, 2 replies; 5+ messages in thread
From: Roman Penyaev @ 2019-05-13 18:20 UTC (permalink / raw)
Cc: Roman Penyaev, Jens Axboe, linux-block
This fixes couple of races which lead to infinite wait of park completion
with the following backtraces:
[20801.303319] Call Trace:
[20801.303321] ? __schedule+0x284/0x650
[20801.303323] schedule+0x33/0xc0
[20801.303324] schedule_timeout+0x1bc/0x210
[20801.303326] ? schedule+0x3d/0xc0
[20801.303327] ? schedule_timeout+0x1bc/0x210
[20801.303329] ? preempt_count_add+0x79/0xb0
[20801.303330] wait_for_completion+0xa5/0x120
[20801.303331] ? wake_up_q+0x70/0x70
[20801.303333] kthread_park+0x48/0x80
[20801.303335] io_finish_async+0x2c/0x70
[20801.303336] io_ring_ctx_wait_and_kill+0x95/0x180
[20801.303338] io_uring_release+0x1c/0x20
[20801.303339] __fput+0xad/0x210
[20801.303341] task_work_run+0x8f/0xb0
[20801.303342] exit_to_usermode_loop+0xa0/0xb0
[20801.303343] do_syscall_64+0xe0/0x100
[20801.303349] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[20801.303380] Call Trace:
[20801.303383] ? __schedule+0x284/0x650
[20801.303384] schedule+0x33/0xc0
[20801.303386] io_sq_thread+0x38a/0x410
[20801.303388] ? __switch_to_asm+0x40/0x70
[20801.303390] ? wait_woken+0x80/0x80
[20801.303392] ? _raw_spin_lock_irqsave+0x17/0x40
[20801.303394] ? io_submit_sqes+0x120/0x120
[20801.303395] kthread+0x112/0x130
[20801.303396] ? kthread_create_on_node+0x60/0x60
[20801.303398] ret_from_fork+0x35/0x40
o kthread_park() waits for park completion, so io_sq_thread() loop
should check kthread_should_park() along with khread_should_stop(),
otherwise if kthread_park() is called before prepare_to_wait()
the following schedule() never returns.
o if the flag ctx->sqo_stop is observed by the io_sq_thread() loop
it is quite possible, that kthread_should_park() check and the
following kthread_parkme() is never called, because kthread_park()
has not been yet called, but few moments later is is called and
waits there for park completion, which never happens, because
kthread has already exited.
It seems that parking here is not needed at all (thread is parked and
then stopped and never unparked), so here in this patch I simply rely
on kthread_should_stop() check and then exit the thread.
Signed-off-by: Roman Penyaev <rpenyaev@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
---
fs/io_uring.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 452e35357865..449c652bb334 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -231,7 +231,6 @@ struct io_ring_ctx {
struct task_struct *sqo_thread; /* if using sq thread polling */
struct mm_struct *sqo_mm;
wait_queue_head_t sqo_wait;
- unsigned sqo_stop;
struct {
/* CQ ring */
@@ -2028,7 +2027,7 @@ static int io_sq_thread(void *data)
set_fs(USER_DS);
timeout = inflight = 0;
- while (!kthread_should_stop() && !ctx->sqo_stop) {
+ while (!kthread_should_stop()) {
bool all_fixed, mm_fault = false;
int i;
@@ -2140,9 +2139,6 @@ static int io_sq_thread(void *data)
mmput(cur_mm);
}
- if (kthread_should_park())
- kthread_parkme();
-
return 0;
}
@@ -2273,9 +2269,6 @@ static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
static void io_sq_thread_stop(struct io_ring_ctx *ctx)
{
if (ctx->sqo_thread) {
- ctx->sqo_stop = 1;
- mb();
- kthread_park(ctx->sqo_thread);
kthread_stop(ctx->sqo_thread);
ctx->sqo_thread = NULL;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async()
2019-05-13 18:20 [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async() Roman Penyaev
@ 2019-05-13 18:25 ` Roman Penyaev
2019-05-13 18:52 ` Jens Axboe
1 sibling, 0 replies; 5+ messages in thread
From: Roman Penyaev @ 2019-05-13 18:25 UTC (permalink / raw)
To: Jens Axboe, linux-block
Hi Jens,
I forgot to mention that dead lock is quite well reproduced
if app is killed, when no IO is performed (i.e. polling thread
is scheduled out).
--
Roman
On 2019-05-13 20:20, Roman Penyaev wrote:
> This fixes couple of races which lead to infinite wait of park
> completion
> with the following backtraces:
>
> [20801.303319] Call Trace:
> [20801.303321] ? __schedule+0x284/0x650
> [20801.303323] schedule+0x33/0xc0
> [20801.303324] schedule_timeout+0x1bc/0x210
> [20801.303326] ? schedule+0x3d/0xc0
> [20801.303327] ? schedule_timeout+0x1bc/0x210
> [20801.303329] ? preempt_count_add+0x79/0xb0
> [20801.303330] wait_for_completion+0xa5/0x120
> [20801.303331] ? wake_up_q+0x70/0x70
> [20801.303333] kthread_park+0x48/0x80
> [20801.303335] io_finish_async+0x2c/0x70
> [20801.303336] io_ring_ctx_wait_and_kill+0x95/0x180
> [20801.303338] io_uring_release+0x1c/0x20
> [20801.303339] __fput+0xad/0x210
> [20801.303341] task_work_run+0x8f/0xb0
> [20801.303342] exit_to_usermode_loop+0xa0/0xb0
> [20801.303343] do_syscall_64+0xe0/0x100
> [20801.303349] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [20801.303380] Call Trace:
> [20801.303383] ? __schedule+0x284/0x650
> [20801.303384] schedule+0x33/0xc0
> [20801.303386] io_sq_thread+0x38a/0x410
> [20801.303388] ? __switch_to_asm+0x40/0x70
> [20801.303390] ? wait_woken+0x80/0x80
> [20801.303392] ? _raw_spin_lock_irqsave+0x17/0x40
> [20801.303394] ? io_submit_sqes+0x120/0x120
> [20801.303395] kthread+0x112/0x130
> [20801.303396] ? kthread_create_on_node+0x60/0x60
> [20801.303398] ret_from_fork+0x35/0x40
>
> o kthread_park() waits for park completion, so io_sq_thread() loop
> should check kthread_should_park() along with khread_should_stop(),
> otherwise if kthread_park() is called before prepare_to_wait()
> the following schedule() never returns.
>
> o if the flag ctx->sqo_stop is observed by the io_sq_thread() loop
> it is quite possible, that kthread_should_park() check and the
> following kthread_parkme() is never called, because kthread_park()
> has not been yet called, but few moments later is is called and
> waits there for park completion, which never happens, because
> kthread has already exited.
>
> It seems that parking here is not needed at all (thread is parked and
> then stopped and never unparked), so here in this patch I simply rely
> on kthread_should_stop() check and then exit the thread.
>
> Signed-off-by: Roman Penyaev <rpenyaev@suse.de>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> ---
> fs/io_uring.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 452e35357865..449c652bb334 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -231,7 +231,6 @@ struct io_ring_ctx {
> struct task_struct *sqo_thread; /* if using sq thread polling */
> struct mm_struct *sqo_mm;
> wait_queue_head_t sqo_wait;
> - unsigned sqo_stop;
>
> struct {
> /* CQ ring */
> @@ -2028,7 +2027,7 @@ static int io_sq_thread(void *data)
> set_fs(USER_DS);
>
> timeout = inflight = 0;
> - while (!kthread_should_stop() && !ctx->sqo_stop) {
> + while (!kthread_should_stop()) {
> bool all_fixed, mm_fault = false;
> int i;
>
> @@ -2140,9 +2139,6 @@ static int io_sq_thread(void *data)
> mmput(cur_mm);
> }
>
> - if (kthread_should_park())
> - kthread_parkme();
> -
> return 0;
> }
>
> @@ -2273,9 +2269,6 @@ static int io_sqe_files_unregister(struct
> io_ring_ctx *ctx)
> static void io_sq_thread_stop(struct io_ring_ctx *ctx)
> {
> if (ctx->sqo_thread) {
> - ctx->sqo_stop = 1;
> - mb();
> - kthread_park(ctx->sqo_thread);
> kthread_stop(ctx->sqo_thread);
> ctx->sqo_thread = NULL;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async()
2019-05-13 18:20 [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async() Roman Penyaev
2019-05-13 18:25 ` Roman Penyaev
@ 2019-05-13 18:52 ` Jens Axboe
2019-05-14 9:17 ` Roman Penyaev
1 sibling, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2019-05-13 18:52 UTC (permalink / raw)
To: Roman Penyaev; +Cc: linux-block
On 5/13/19 12:20 PM, Roman Penyaev wrote:
> This fixes couple of races which lead to infinite wait of park completion
> with the following backtraces:
>
> [20801.303319] Call Trace:
> [20801.303321] ? __schedule+0x284/0x650
> [20801.303323] schedule+0x33/0xc0
> [20801.303324] schedule_timeout+0x1bc/0x210
> [20801.303326] ? schedule+0x3d/0xc0
> [20801.303327] ? schedule_timeout+0x1bc/0x210
> [20801.303329] ? preempt_count_add+0x79/0xb0
> [20801.303330] wait_for_completion+0xa5/0x120
> [20801.303331] ? wake_up_q+0x70/0x70
> [20801.303333] kthread_park+0x48/0x80
> [20801.303335] io_finish_async+0x2c/0x70
> [20801.303336] io_ring_ctx_wait_and_kill+0x95/0x180
> [20801.303338] io_uring_release+0x1c/0x20
> [20801.303339] __fput+0xad/0x210
> [20801.303341] task_work_run+0x8f/0xb0
> [20801.303342] exit_to_usermode_loop+0xa0/0xb0
> [20801.303343] do_syscall_64+0xe0/0x100
> [20801.303349] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [20801.303380] Call Trace:
> [20801.303383] ? __schedule+0x284/0x650
> [20801.303384] schedule+0x33/0xc0
> [20801.303386] io_sq_thread+0x38a/0x410
> [20801.303388] ? __switch_to_asm+0x40/0x70
> [20801.303390] ? wait_woken+0x80/0x80
> [20801.303392] ? _raw_spin_lock_irqsave+0x17/0x40
> [20801.303394] ? io_submit_sqes+0x120/0x120
> [20801.303395] kthread+0x112/0x130
> [20801.303396] ? kthread_create_on_node+0x60/0x60
> [20801.303398] ret_from_fork+0x35/0x40
>
> o kthread_park() waits for park completion, so io_sq_thread() loop
> should check kthread_should_park() along with khread_should_stop(),
> otherwise if kthread_park() is called before prepare_to_wait()
> the following schedule() never returns.
>
> o if the flag ctx->sqo_stop is observed by the io_sq_thread() loop
> it is quite possible, that kthread_should_park() check and the
> following kthread_parkme() is never called, because kthread_park()
> has not been yet called, but few moments later is is called and
> waits there for park completion, which never happens, because
> kthread has already exited.
>
> It seems that parking here is not needed at all (thread is parked and
> then stopped and never unparked), so here in this patch I simply rely
> on kthread_should_stop() check and then exit the thread.
The park is a bit of a work-around, without it we get warning spews on
shutdown with SQPOLL set and affinity set to a single CPU.
Also see:
commit 06058632464845abb1af91521122fd04dd3daaec
Author: Jens Axboe <axboe@kernel.dk>
Date: Sat Apr 13 09:26:03 2019 -0600
io_uring: park SQPOLL thread if it's percpu
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async()
2019-05-13 18:52 ` Jens Axboe
@ 2019-05-14 9:17 ` Roman Penyaev
2019-05-14 9:27 ` Roman Penyaev
0 siblings, 1 reply; 5+ messages in thread
From: Roman Penyaev @ 2019-05-14 9:17 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
On 2019-05-13 20:52, Jens Axboe wrote:
> On 5/13/19 12:20 PM, Roman Penyaev wrote:
>> This fixes couple of races which lead to infinite wait of park
>> completion
>> with the following backtraces:
>>
>> [20801.303319] Call Trace:
>> [20801.303321] ? __schedule+0x284/0x650
>> [20801.303323] schedule+0x33/0xc0
>> [20801.303324] schedule_timeout+0x1bc/0x210
>> [20801.303326] ? schedule+0x3d/0xc0
>> [20801.303327] ? schedule_timeout+0x1bc/0x210
>> [20801.303329] ? preempt_count_add+0x79/0xb0
>> [20801.303330] wait_for_completion+0xa5/0x120
>> [20801.303331] ? wake_up_q+0x70/0x70
>> [20801.303333] kthread_park+0x48/0x80
>> [20801.303335] io_finish_async+0x2c/0x70
>> [20801.303336] io_ring_ctx_wait_and_kill+0x95/0x180
>> [20801.303338] io_uring_release+0x1c/0x20
>> [20801.303339] __fput+0xad/0x210
>> [20801.303341] task_work_run+0x8f/0xb0
>> [20801.303342] exit_to_usermode_loop+0xa0/0xb0
>> [20801.303343] do_syscall_64+0xe0/0x100
>> [20801.303349] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> [20801.303380] Call Trace:
>> [20801.303383] ? __schedule+0x284/0x650
>> [20801.303384] schedule+0x33/0xc0
>> [20801.303386] io_sq_thread+0x38a/0x410
>> [20801.303388] ? __switch_to_asm+0x40/0x70
>> [20801.303390] ? wait_woken+0x80/0x80
>> [20801.303392] ? _raw_spin_lock_irqsave+0x17/0x40
>> [20801.303394] ? io_submit_sqes+0x120/0x120
>> [20801.303395] kthread+0x112/0x130
>> [20801.303396] ? kthread_create_on_node+0x60/0x60
>> [20801.303398] ret_from_fork+0x35/0x40
>>
>> o kthread_park() waits for park completion, so io_sq_thread() loop
>> should check kthread_should_park() along with khread_should_stop(),
>> otherwise if kthread_park() is called before prepare_to_wait()
>> the following schedule() never returns.
>>
>> o if the flag ctx->sqo_stop is observed by the io_sq_thread() loop
>> it is quite possible, that kthread_should_park() check and the
>> following kthread_parkme() is never called, because kthread_park()
>> has not been yet called, but few moments later is is called and
>> waits there for park completion, which never happens, because
>> kthread has already exited.
>>
>> It seems that parking here is not needed at all (thread is parked and
>> then stopped and never unparked), so here in this patch I simply rely
>> on kthread_should_stop() check and then exit the thread.
>
> The park is a bit of a work-around, without it we get warning spews on
> shutdown with SQPOLL set and affinity set to a single CPU.
>
> Also see:
>
> commit 06058632464845abb1af91521122fd04dd3daaec
> Author: Jens Axboe <axboe@kernel.dk>
> Date: Sat Apr 13 09:26:03 2019 -0600
>
> io_uring: park SQPOLL thread if it's percpu
I see. Do you think it makes sense to fix this is kthread? With
something
as the following:
diff --git a/kernel/kthread.c b/kernel/kthread.c
index be4e8795561a..191f8be5c9e0 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -472,6 +472,10 @@ void kthread_unpark(struct task_struct *k)
{
struct kthread *kthread = to_kthread(k);
+ if (!__kthread_should_park(k))
+ /* Nop if thread was never parked */
+ break;
+
Nevertheless, the other way is to fix io_uring: we quit loop by
only kthread_should_park() check (kthread_park() is synchronous,
so kthread_should_stop() is never observed), and we abandon
->sqo_stop flag, because it is racy. At the end we always parkme(),
because we've exited the loop by park flag.
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 452e35357865..9401ac1bbd46 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -231,7 +231,6 @@ struct io_ring_ctx {
struct task_struct *sqo_thread; /* if using sq thread
polling */
struct mm_struct *sqo_mm;
wait_queue_head_t sqo_wait;
- unsigned sqo_stop;
struct {
/* CQ ring */
@@ -2028,7 +2027,7 @@ static int io_sq_thread(void *data)
set_fs(USER_DS);
timeout = inflight = 0;
- while (!kthread_should_stop() && !ctx->sqo_stop) {
+ while (!kthread_should_park()) {
bool all_fixed, mm_fault = false;
int i;
@@ -2090,7 +2089,7 @@ static int io_sq_thread(void *data)
smp_mb();
if (!io_get_sqring(ctx, &sqes[0])) {
- if (kthread_should_stop()) {
+ if (kthread_should_park()) {
finish_wait(&ctx->sqo_wait,
&wait);
break;
}
@@ -2140,8 +2139,7 @@ static int io_sq_thread(void *data)
mmput(cur_mm);
}
- if (kthread_should_park())
- kthread_parkme();
+ kthread_parkme();
return 0;
}
@@ -2273,8 +2271,11 @@ static int io_sqe_files_unregister(struct
io_ring_ctx *ctx)
static void io_sq_thread_stop(struct io_ring_ctx *ctx)
{
if (ctx->sqo_thread) {
- ctx->sqo_stop = 1;
- mb();
+ /*
+ * The park is a bit of a work-around, without it we get
+ * warning spews on shutdown with SQPOLL set and
affinity
+ * set to a single CPU.
+ */
kthread_park(ctx->sqo_thread);
kthread_stop(ctx->sqo_thread);
ctx->sqo_thread = NULL;
Makes sense?
--
Roman
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async()
2019-05-14 9:17 ` Roman Penyaev
@ 2019-05-14 9:27 ` Roman Penyaev
0 siblings, 0 replies; 5+ messages in thread
From: Roman Penyaev @ 2019-05-14 9:27 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-block-owner
On 2019-05-14 11:17, Roman Penyaev wrote:
> I see. Do you think it makes sense to fix this is kthread? With
> something
> as the following:
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index be4e8795561a..191f8be5c9e0 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -472,6 +472,10 @@ void kthread_unpark(struct task_struct *k)
> {
> struct kthread *kthread = to_kthread(k);
>
> + if (!__kthread_should_park(k))
> + /* Nop if thread was never parked */
> + break;
return of course, not break.
--
Roman
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-14 9:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 18:20 [PATCH 1/1] io_uring: fix infinite wait in khread_park() on io_finish_async() Roman Penyaev
2019-05-13 18:25 ` Roman Penyaev
2019-05-13 18:52 ` Jens Axboe
2019-05-14 9:17 ` Roman Penyaev
2019-05-14 9:27 ` Roman Penyaev
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).