[PATCH 0/3] iopoll improvements

[PATCH 0/3] iopoll improvements

[Pavel Begunkov]

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(-)

-- 
2.24.0

[PATCH 1/3] io_uring: don't delay iopoll'ed req completion

[Pavel Begunkov]

->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().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a2459504b371..50f9260eea9b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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;
-- 
2.24.0

[PATCH 2/3] io_uring: fix stopping iopoll'ing too early

[Pavel Begunkov]

Nobody 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().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 50f9260eea9b..020944a193d0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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;
-- 
2.24.0

[PATCH 3/3] io_uring: briefly loose locks while reaping events

[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 <asml.silence@gmail.com>
---
 fs/io_uring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 020944a193d0..568e25bcadd6 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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);
 }
-- 
2.24.0

Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events

[Jens Axboe]

On 7/6/20 8:14 AM, Pavel Begunkov wrote:
> 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 <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 020944a193d0..568e25bcadd6 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -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);
>  }

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.

-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events

[Pavel Begunkov]

On 06/07/2020 17:31, Jens Axboe wrote:
> On 7/6/20 8:14 AM, Pavel Begunkov wrote:
>> 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 <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 020944a193d0..568e25bcadd6 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -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);
>>  }
> 
> 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.

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

-- 
Pavel Begunkov

Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events

[Pavel Begunkov]

On 06/07/2020 17:42, Pavel Begunkov wrote:
> On 06/07/2020 17:31, Jens Axboe wrote:
>> On 7/6/20 8:14 AM, Pavel Begunkov wrote:
>>> 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 <asml.silence@gmail.com>
>>> ---
>>>  fs/io_uring.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 020944a193d0..568e25bcadd6 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -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);
>>>  }
>>
>> 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.
> 
> 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

Also, io_iopoll_getevents() already does need_resched(). Hmm, do you wan't
me to resend?

-- 
Pavel Begunkov

Re: [PATCH 3/3] io_uring: briefly loose locks while reaping events

[Jens Axboe]

On 7/6/20 8:45 AM, Pavel Begunkov wrote:
> 
> 
> On 06/07/2020 17:42, Pavel Begunkov wrote:
>> On 06/07/2020 17:31, Jens Axboe wrote:
>>> On 7/6/20 8:14 AM, Pavel Begunkov wrote:
>>>> 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 <asml.silence@gmail.com>
>>>> ---
>>>>  fs/io_uring.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 020944a193d0..568e25bcadd6 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -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);
>>>>  }
>>>
>>> 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.
>>
>> 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
> 
> Also, io_iopoll_getevents() already does need_resched(). Hmm, do you wan't
> me to resend?

I missed this was the "reap on dead" path. But yes, please resend.

-- 
Jens Axboe