[PATCH 0/2] Minor SQPOLL thread fix and improvement

[PATCH 0/2] Minor SQPOLL thread fix and improvement

[Olivier Langlois, Olivier Langlois]

I have been investigated a deadlock situation in my io_uring usage where
the SQPOLL thread was going to sleep and my user threads were waiting
inside io_uring_enter() for completions.

https://github.com/axboe/liburing/issues/367

This patch serie is the result of my investigation.

Olivier Langlois (2):
  io_uring: Fix race condition when sqp thread goes to sleep
  io_uring: Create define to modify a SQPOLL parameter

 fs/io_uring.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.32.0

[PATCH 1/2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

If an asynchronous completion happens before the task is preparing
itself to wait and set its state to TASK_INTERRUPTABLE, the completion
will not wake up the sqp thread.

Signed-off-by: Olivier Langlois <olivier@trillion01.com>
---
 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index fc8637f591a6..02f789e07d4c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6902,7 +6902,7 @@ static int io_sq_thread(void *data)
 		}
 
 		prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
-		if (!io_sqd_events_pending(sqd)) {
+		if (!io_sqd_events_pending(sqd) && !current->task_works) {
 			needs_sched = true;
 			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
 				io_ring_set_wakeup_flag(ctx);
-- 
2.32.0

[PATCH 2/2] io_uring: Create define to modify a SQPOLL parameter

[Olivier Langlois]

The magic number used to cap the number of entries extracted from an
io_uring instance SQ before moving to the other instances is an
interesting parameter to experiment with.

A define has been created to make it easy to change its value from a
single location.

Signed-off-by: Olivier Langlois <olivier@trillion01.com>
---
 fs/io_uring.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 02f789e07d4c..3f271bd7726b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -89,6 +89,7 @@
 
 #define IORING_MAX_ENTRIES	32768
 #define IORING_MAX_CQ_ENTRIES	(2 * IORING_MAX_ENTRIES)
+#define IORING_SQPOLL_CAP_ENTRIES_VALUE 8
 
 /*
  * Shift of 9 is 512 entries, or exactly one page on 64-bit archs
@@ -6797,8 +6798,8 @@ static int __io_sq_thread(struct io_ring_ctx *ctx, bool cap_entries)
 
 	to_submit = io_sqring_entries(ctx);
 	/* if we're handling multiple rings, cap submit size for fairness */
-	if (cap_entries && to_submit > 8)
-		to_submit = 8;
+	if (cap_entries && to_submit > IORING_SQPOLL_CAP_ENTRIES_VALUE)
+		to_submit = IORING_SQPOLL_CAP_ENTRIES_VALUE;
 
 	if (!list_empty(&ctx->iopoll_list) || to_submit) {
 		unsigned nr_events = 0;
-- 
2.32.0

Re: [PATCH 1/2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

On Tue, 2021-06-22 at 11:45 -0700, Olivier Langlois wrote:
> If an asynchronous completion happens before the task is preparing
> itself to wait and set its state to TASK_INTERRUPTABLE, the
> completion
> will not wake up the sqp thread.
> 
I have just noticed that I made a typo in the description. I will send
a v2 of that patch.

Sorry about that. I was too excited to share my discovery...

Re: [PATCH 1/2] io_uring: Fix race condition when sqp thread goes to sleep

[Pavel Begunkov]

On 6/22/21 7:53 PM, Olivier Langlois wrote:
> On Tue, 2021-06-22 at 11:45 -0700, Olivier Langlois wrote:
>> If an asynchronous completion happens before the task is preparing
>> itself to wait and set its state to TASK_INTERRUPTABLE, the
>> completion
>> will not wake up the sqp thread.
>>
> I have just noticed that I made a typo in the description. I will send
> a v2 of that patch.
> 
> Sorry about that. I was too excited to share my discovery...

git format-patch --cover-letter --thread=shallow ...

would be even better, but the fix looks right

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

On Tue, 2021-06-22 at 19:55 +0100, Pavel Begunkov wrote:
> On 6/22/21 7:53 PM, Olivier Langlois wrote:
> > On Tue, 2021-06-22 at 11:45 -0700, Olivier Langlois wrote:
> > > If an asynchronous completion happens before the task is
> > > preparing
> > > itself to wait and set its state to TASK_INTERRUPTABLE, the
> > > completion
> > > will not wake up the sqp thread.
> > > 
> > I have just noticed that I made a typo in the description. I will
> > send
> > a v2 of that patch.
> > 
> > Sorry about that. I was too excited to share my discovery...
> 
> git format-patch --cover-letter --thread=shallow ...
> 
> would be even better, but the fix looks right
> 
You are too good... I thought that I could get away from hacking
manually the patch file for such a minor change...

It seems that I got caught...

Let me know if you need me to redo it the right way...

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Pavel Begunkov]

On 6/22/21 7:55 PM, Olivier Langlois wrote:
> If an asynchronous completion happens before the task is preparing
> itself to wait and set its state to TASK_INTERRUPTIBLE, the completion
> will not wake up the sqp thread.
> 
> Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> ---
>  fs/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index fc8637f591a6..02f789e07d4c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6902,7 +6902,7 @@ static int io_sq_thread(void *data)
>  		}
>  
>  		prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
> -		if (!io_sqd_events_pending(sqd)) {
> +		if (!io_sqd_events_pending(sqd) && !current->task_works) {

Agree that it should be here, but we also lack a good enough
task_work_run() around, and that may send the task burn CPU
for a while in some cases. Let's do

if (!io_sqd_events_pending(sqd) && !io_run_task_work())
   ...

fwiw, no need to worry about TASK_INTERRUPTIBLE as
io_run_task_work() sets it to TASK_RUNNING.

>  			needs_sched = true;
>  			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
>  				io_ring_set_wakeup_flag(ctx);
> 

-- 
Pavel Begunkov

Re: [PATCH 2/2] io_uring: Create define to modify a SQPOLL parameter

[Pavel Begunkov]

On 6/22/21 7:45 PM, Olivier Langlois wrote:
> The magic number used to cap the number of entries extracted from an
> io_uring instance SQ before moving to the other instances is an
> interesting parameter to experiment with.

Not particularly related to this patch, but the problem with this
capping is that there is no reliable way to do request linking
using shared sqpoll. It may break a link in half (or in N parts for
long links) and submit them separately and in parallel.


> A define has been created to make it easy to change its value from a
> single location.
> 
> Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> ---
>  fs/io_uring.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 02f789e07d4c..3f271bd7726b 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -89,6 +89,7 @@
>  
>  #define IORING_MAX_ENTRIES	32768
>  #define IORING_MAX_CQ_ENTRIES	(2 * IORING_MAX_ENTRIES)
> +#define IORING_SQPOLL_CAP_ENTRIES_VALUE 8
>  
>  /*
>   * Shift of 9 is 512 entries, or exactly one page on 64-bit archs
> @@ -6797,8 +6798,8 @@ static int __io_sq_thread(struct io_ring_ctx *ctx, bool cap_entries)
>  
>  	to_submit = io_sqring_entries(ctx);
>  	/* if we're handling multiple rings, cap submit size for fairness */
> -	if (cap_entries && to_submit > 8)
> -		to_submit = 8;
> +	if (cap_entries && to_submit > IORING_SQPOLL_CAP_ENTRIES_VALUE)
> +		to_submit = IORING_SQPOLL_CAP_ENTRIES_VALUE;
>  
>  	if (!list_empty(&ctx->iopoll_list) || to_submit) {
>  		unsigned nr_events = 0;
> 

-- 
Pavel Begunkov

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
> On 6/22/21 7:55 PM, Olivier Langlois wrote:
> > If an asynchronous completion happens before the task is preparing
> > itself to wait and set its state to TASK_INTERRUPTIBLE, the
> > completion
> > will not wake up the sqp thread.
> > 
> > Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> > ---
> >  fs/io_uring.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index fc8637f591a6..02f789e07d4c 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -6902,7 +6902,7 @@ static int io_sq_thread(void *data)
> >                 }
> >  
> >                 prepare_to_wait(&sqd->wait, &wait,
> > TASK_INTERRUPTIBLE);
> > -               if (!io_sqd_events_pending(sqd)) {
> > +               if (!io_sqd_events_pending(sqd) && !current-
> > >task_works) {
> 
> Agree that it should be here, but we also lack a good enough
> task_work_run() around, and that may send the task burn CPU
> for a while in some cases. Let's do
> 
> if (!io_sqd_events_pending(sqd) && !io_run_task_work())
>    ...

I can do that if you want but considering that the function is inline
and the race condition is a relatively rare occurence, is the cost
coming with inline expansion really worth it in this case?
> 
> fwiw, no need to worry about TASK_INTERRUPTIBLE as
> io_run_task_work() sets it to TASK_RUNNING.

I wasn't worried about that as I believe that finish_wait() is taking
care the state as well.

What I wasn't sure about was if the patch was sufficient to totally
eliminate the race condition.

I had to educate myself about how schedule() works to appreciate its
design and convince myself that the patch was good.
> 
> >                         needs_sched = true;
> >                         list_for_each_entry(ctx, &sqd->ctx_list,
> > sqd_list) {
> >                                 io_ring_set_wakeup_flag(ctx);
> > 
> 

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

On Tue, 2021-06-22 at 18:37 -0400, Olivier Langlois wrote:
> On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
> > On 6/22/21 7:55 PM, Olivier Langlois wrote:
> > > If an asynchronous completion happens before the task is
> > > preparing
> > > itself to wait and set its state to TASK_INTERRUPTIBLE, the
> > > completion
> > > will not wake up the sqp thread.
> > > 
> > > Signed-off-by: Olivier Langlois <olivier@trillion01.com>
> > > ---
> > >  fs/io_uring.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > > index fc8637f591a6..02f789e07d4c 100644
> > > --- a/fs/io_uring.c
> > > +++ b/fs/io_uring.c
> > > @@ -6902,7 +6902,7 @@ static int io_sq_thread(void *data)
> > >                 }
> > >  
> > >                 prepare_to_wait(&sqd->wait, &wait,
> > > TASK_INTERRUPTIBLE);
> > > -               if (!io_sqd_events_pending(sqd)) {
> > > +               if (!io_sqd_events_pending(sqd) && !current-
> > > > task_works) {
> > 
> > Agree that it should be here, but we also lack a good enough
> > task_work_run() around, and that may send the task burn CPU
> > for a while in some cases. Let's do
> > 
> > if (!io_sqd_events_pending(sqd) && !io_run_task_work())
> >    ...
> 
> I can do that if you want but considering that the function is inline
> and the race condition is a relatively rare occurence, is the cost
> coming with inline expansion really worth it in this case?
> > 
On hand, there is the inline expansion concern.

OTOH, the benefit of going with your suggestion is that completions
generally precedes new submissions so yes, it might be better that way.

I'm really unsure about this. I'm just raising the concern and I'll let
you make the final decision...

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Pavel Begunkov]

On 6/22/21 11:42 PM, Olivier Langlois wrote:
> On Tue, 2021-06-22 at 18:37 -0400, Olivier Langlois wrote:
>> On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
>>> On 6/22/21 7:55 PM, Olivier Langlois wrote:
>>>> If an asynchronous completion happens before the task is
>>>> preparing
>>>> itself to wait and set its state to TASK_INTERRUPTIBLE, the
>>>> completion
>>>> will not wake up the sqp thread.
>>>>
>>>> Signed-off-by: Olivier Langlois <olivier@trillion01.com>
>>>> ---
>>>>  fs/io_uring.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index fc8637f591a6..02f789e07d4c 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -6902,7 +6902,7 @@ static int io_sq_thread(void *data)
>>>>                 }
>>>>  
>>>>                 prepare_to_wait(&sqd->wait, &wait,
>>>> TASK_INTERRUPTIBLE);
>>>> -               if (!io_sqd_events_pending(sqd)) {
>>>> +               if (!io_sqd_events_pending(sqd) && !current-
>>>>> task_works) {
>>>
>>> Agree that it should be here, but we also lack a good enough
>>> task_work_run() around, and that may send the task burn CPU
>>> for a while in some cases. Let's do
>>>
>>> if (!io_sqd_events_pending(sqd) && !io_run_task_work())
>>>    ...
>>
>> I can do that if you want but considering that the function is inline
>> and the race condition is a relatively rare occurence, is the cost
>> coming with inline expansion really worth it in this case?
>>>
> On hand, there is the inline expansion concern.
> 
> OTOH, the benefit of going with your suggestion is that completions
> generally precedes new submissions so yes, it might be better that way.
> 
> I'm really unsure about this. I'm just raising the concern and I'll let
> you make the final decision...

It seems it may actually loop infinitely until it gets a signal,
so yes. And even if not, rare stalls are nasty, they will ruin
some 9s of latency and hard to catch.

That part is quite cold anyway, would generate some extra cold
instructions, meh

-- 
Pavel Begunkov

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Olivier Langlois]

On Wed, 2021-06-23 at 00:03 +0100, Pavel Begunkov wrote:
> On 6/22/21 11:42 PM, Olivier Langlois wrote:
> > On Tue, 2021-06-22 at 18:37 -0400, Olivier Langlois wrote:
> > > On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
> > > 
> > > 
> > > I can do that if you want but considering that the function is
> > > inline
> > > and the race condition is a relatively rare occurence, is the
> > > cost
> > > coming with inline expansion really worth it in this case?
> > > > 
> > On hand, there is the inline expansion concern.
> > 
> > OTOH, the benefit of going with your suggestion is that completions
> > generally precedes new submissions so yes, it might be better that
> > way.
> > 
> > I'm really unsure about this. I'm just raising the concern and I'll
> > let
> > you make the final decision...
> 
> It seems it may actually loop infinitely until it gets a signal,
> so yes. And even if not, rare stalls are nasty, they will ruin
> some 9s of latency and hard to catch.
> 
> That part is quite cold anyway, would generate some extra cold
> instructions, meh
> 
I'm not 100% sure to see the infinite loop possibility but I guess that
with some badly placed preemptions, it could take few iterations before
entering the block:

		if (sqt_spin || !time_after(jiffies, timeout)) {

So I will go ahead with your suggestion.

I'll retest the new patch version (it should be a formality) and I'll
resend an update once done.

Greetings,

Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes to sleep

[Pavel Begunkov]

On 6/23/21 2:52 PM, Olivier Langlois wrote:
> On Wed, 2021-06-23 at 00:03 +0100, Pavel Begunkov wrote:
>> On 6/22/21 11:42 PM, Olivier Langlois wrote:
>>> On Tue, 2021-06-22 at 18:37 -0400, Olivier Langlois wrote:
>>>> On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
>>>>
>>>>
>>>> I can do that if you want but considering that the function is
>>>> inline
>>>> and the race condition is a relatively rare occurence, is the
>>>> cost
>>>> coming with inline expansion really worth it in this case?
>>>>>
>>> On hand, there is the inline expansion concern.
>>>
>>> OTOH, the benefit of going with your suggestion is that completions
>>> generally precedes new submissions so yes, it might be better that
>>> way.
>>>
>>> I'm really unsure about this. I'm just raising the concern and I'll
>>> let
>>> you make the final decision...
>>
>> It seems it may actually loop infinitely until it gets a signal,
>> so yes. And even if not, rare stalls are nasty, they will ruin
>> some 9s of latency and hard to catch.
>>
>> That part is quite cold anyway, would generate some extra cold
>> instructions, meh
>>
> I'm not 100% sure to see the infinite loop possibility but I guess that
> with some badly placed preemptions, it could take few iterations before
> entering the block:
> 
> 		if (sqt_spin || !time_after(jiffies, timeout)) {

Had a case in mind, but looking through the branches it can't
really happen. Agree that won't be infinite in real life, until
we start using (and there was an RFC) finer grained timeouts.

In any case for several reasons think it's the right thing to do.

> So I will go ahead with your suggestion.
> 
> I'll retest the new patch version (it should be a formality) and I'll
> resend an update once done.

Perfect

-- 
Pavel Begunkov