[PATCH 1/1] io_uring: break iopolling on signal

[PATCH 1/1] io_uring: break iopolling on signal

[Pavel Begunkov]

Don't keep spinning iopoll with a signal set. It'll eventually return
back, e.g. by virtue of need_resched(), but it's not a nice user
experience.

Cc: stable@vger.kernel.org
Fixes: def596e9557c9 ("io_uring: support for IO polling")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index f32092d90960..1810cf719a02 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1684,6 +1684,9 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 			break;
 		nr_events += ret;
 		ret = 0;
+
+		if (task_sigpending(current))
+			return -EINTR;
 	} while (nr_events < min && !need_resched());
 
 	return ret;
-- 
2.41.0

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Jens Axboe]

On 8/9/23 9:20 AM, Pavel Begunkov wrote:
> Don't keep spinning iopoll with a signal set. It'll eventually return
> back, e.g. by virtue of need_resched(), but it's not a nice user
> experience.

I wonder if we shouldn't clean it up a bit while at it, the ret clearing
is kind of odd and only used in that one loop? Makes the break
conditions easier to read too, and makes it clear that we're returning
0/-error rather than zero-or-positive/-error as well.

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8681bde70716..ec575f663a82 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1637,7 +1637,6 @@ static __cold void io_iopoll_try_reap_events(struct io_ring_ctx *ctx)
 static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 {
 	unsigned int nr_events = 0;
-	int ret = 0;
 	unsigned long check_cq;
 
 	if (!io_allowed_run_tw(ctx))
@@ -1663,6 +1662,8 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 		return 0;
 
 	do {
+		int ret = 0;
+
 		/*
 		 * If a submit got punted to a workqueue, we can have the
 		 * application entering polling for a command before it gets
@@ -1692,12 +1693,16 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 		}
 		ret = io_do_iopoll(ctx, !min);
 		if (ret < 0)
-			break;
+			return ret;
 		nr_events += ret;
-		ret = 0;
-	} while (nr_events < min && !need_resched());
 
-	return ret;
+		if (task_sigpending(current))
+			return -EINTR;
+		if (need_resched())
+			break;
+	} while (nr_events < min);
+
+	return 0;
 }
 
 void io_req_task_complete(struct io_kiocb *req, struct io_tw_state *ts)

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Pavel Begunkov]

On 8/9/23 16:30, Jens Axboe wrote:
> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>> Don't keep spinning iopoll with a signal set. It'll eventually return
>> back, e.g. by virtue of need_resched(), but it's not a nice user
>> experience.
> 
> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
> is kind of odd and only used in that one loop? Makes the break
> conditions easier to read too, and makes it clear that we're returning
> 0/-error rather than zero-or-positive/-error as well.

We can, but if we're backporting, which I suggest, let's better keep
it simple and do all that as a follow up.

fwiw, this function was responsible for initial uring_lock locking
back in the day I believe, that's why it is how it is.


> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 8681bde70716..ec575f663a82 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -1637,7 +1637,6 @@ static __cold void io_iopoll_try_reap_events(struct io_ring_ctx *ctx)
>   static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
>   {
>   	unsigned int nr_events = 0;
> -	int ret = 0;
>   	unsigned long check_cq;
>   
>   	if (!io_allowed_run_tw(ctx))
> @@ -1663,6 +1662,8 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
>   		return 0;
>   
>   	do {
> +		int ret = 0;
> +
>   		/*
>   		 * If a submit got punted to a workqueue, we can have the
>   		 * application entering polling for a command before it gets
> @@ -1692,12 +1693,16 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
>   		}
>   		ret = io_do_iopoll(ctx, !min);
>   		if (ret < 0)
> -			break;
> +			return ret;
>   		nr_events += ret;
> -		ret = 0;
> -	} while (nr_events < min && !need_resched());
>   
> -	return ret;
> +		if (task_sigpending(current))
> +			return -EINTR;
> +		if (need_resched())
> +			break;
> +	} while (nr_events < min);
> +
> +	return 0;
>   }
>   
>   void io_req_task_complete(struct io_kiocb *req, struct io_tw_state *ts)
> 

-- 
Pavel Begunkov

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Jens Axboe]

On 8/9/23 9:38 AM, Pavel Begunkov wrote:
> On 8/9/23 16:30, Jens Axboe wrote:
>> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>>> Don't keep spinning iopoll with a signal set. It'll eventually return
>>> back, e.g. by virtue of need_resched(), but it's not a nice user
>>> experience.
>>
>> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
>> is kind of odd and only used in that one loop? Makes the break
>> conditions easier to read too, and makes it clear that we're returning
>> 0/-error rather than zero-or-positive/-error as well.
> 
> We can, but if we're backporting, which I suggest, let's better keep
> it simple and do all that as a follow up.

Sure, that's fine too. But can you turn it into a series of 2 then, with
the cleanup following?

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Pavel Begunkov]

On 8/9/23 16:50, Jens Axboe wrote:
> On 8/9/23 9:38 AM, Pavel Begunkov wrote:
>> On 8/9/23 16:30, Jens Axboe wrote:
>>> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>>>> Don't keep spinning iopoll with a signal set. It'll eventually return
>>>> back, e.g. by virtue of need_resched(), but it's not a nice user
>>>> experience.
>>>
>>> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
>>> is kind of odd and only used in that one loop? Makes the break
>>> conditions easier to read too, and makes it clear that we're returning
>>> 0/-error rather than zero-or-positive/-error as well.
>>
>> We can, but if we're backporting, which I suggest, let's better keep
>> it simple and do all that as a follow up.
> 
> Sure, that's fine too. But can you turn it into a series of 2 then, with
> the cleanup following?

Is there a master plan why it has to be in a patchset? I would prefer to
apply now if there are not concerns and send the second one later with
other cleanups, e.g. with the dummy_ubuf series.

But I can do a series if it has to be this way, I don't really care much.

-- 
Pavel Begunkov

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Jens Axboe]

On 8/9/23 9:58 AM, Pavel Begunkov wrote:
> On 8/9/23 16:50, Jens Axboe wrote:
>> On 8/9/23 9:38 AM, Pavel Begunkov wrote:
>>> On 8/9/23 16:30, Jens Axboe wrote:
>>>> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>>>>> Don't keep spinning iopoll with a signal set. It'll eventually return
>>>>> back, e.g. by virtue of need_resched(), but it's not a nice user
>>>>> experience.
>>>>
>>>> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
>>>> is kind of odd and only used in that one loop? Makes the break
>>>> conditions easier to read too, and makes it clear that we're returning
>>>> 0/-error rather than zero-or-positive/-error as well.
>>>
>>> We can, but if we're backporting, which I suggest, let's better keep
>>> it simple and do all that as a follow up.
>>
>> Sure, that's fine too. But can you turn it into a series of 2 then, with
>> the cleanup following?
> 
> Is there a master plan why it has to be in a patchset? I would prefer to
> apply now if there are not concerns and send the second one later with
> other cleanups, e.g. with the dummy_ubuf series.
> 
> But I can do a series if it has to be this way, I don't really care much.

No reason other than so we don't forget. But I can just do it on top of
this one.

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Pavel Begunkov]

On 8/9/23 17:01, Jens Axboe wrote:
> On 8/9/23 9:58 AM, Pavel Begunkov wrote:
>> On 8/9/23 16:50, Jens Axboe wrote:
>>> On 8/9/23 9:38 AM, Pavel Begunkov wrote:
>>>> On 8/9/23 16:30, Jens Axboe wrote:
>>>>> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>>>>>> Don't keep spinning iopoll with a signal set. It'll eventually return
>>>>>> back, e.g. by virtue of need_resched(), but it's not a nice user
>>>>>> experience.
>>>>>
>>>>> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
>>>>> is kind of odd and only used in that one loop? Makes the break
>>>>> conditions easier to read too, and makes it clear that we're returning
>>>>> 0/-error rather than zero-or-positive/-error as well.
>>>>
>>>> We can, but if we're backporting, which I suggest, let's better keep
>>>> it simple and do all that as a follow up.
>>>
>>> Sure, that's fine too. But can you turn it into a series of 2 then, with
>>> the cleanup following?
>>
>> Is there a master plan why it has to be in a patchset? I would prefer to
>> apply now if there are not concerns and send the second one later with
>> other cleanups, e.g. with the dummy_ubuf series.
>>
>> But I can do a series if it has to be this way, I don't really care much.
> 
> No reason other than so we don't forget. But I can just do it on top of
> this one.

Let me know whichever way you decide to take, or I'll just pull
and see when I get back to it.

-- 
Pavel Begunkov

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Jens Axboe]

On 8/9/23 10:01 AM, Pavel Begunkov wrote:
> On 8/9/23 17:01, Jens Axboe wrote:
>> On 8/9/23 9:58 AM, Pavel Begunkov wrote:
>>> On 8/9/23 16:50, Jens Axboe wrote:
>>>> On 8/9/23 9:38 AM, Pavel Begunkov wrote:
>>>>> On 8/9/23 16:30, Jens Axboe wrote:
>>>>>> On 8/9/23 9:20 AM, Pavel Begunkov wrote:
>>>>>>> Don't keep spinning iopoll with a signal set. It'll eventually return
>>>>>>> back, e.g. by virtue of need_resched(), but it's not a nice user
>>>>>>> experience.
>>>>>>
>>>>>> I wonder if we shouldn't clean it up a bit while at it, the ret clearing
>>>>>> is kind of odd and only used in that one loop? Makes the break
>>>>>> conditions easier to read too, and makes it clear that we're returning
>>>>>> 0/-error rather than zero-or-positive/-error as well.
>>>>>
>>>>> We can, but if we're backporting, which I suggest, let's better keep
>>>>> it simple and do all that as a follow up.
>>>>
>>>> Sure, that's fine too. But can you turn it into a series of 2 then, with
>>>> the cleanup following?
>>>
>>> Is there a master plan why it has to be in a patchset? I would prefer to
>>> apply now if there are not concerns and send the second one later with
>>> other cleanups, e.g. with the dummy_ubuf series.
>>>
>>> But I can do a series if it has to be this way, I don't really care much.
>>
>> No reason other than so we don't forget. But I can just do it on top of
>> this one.
> 
> Let me know whichever way you decide to take, or I'll just pull
> and see when I get back to it.

I applied yours and did the cleanup on top, running it through the usual
testing and will send it out. So all good on this one.

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: break iopolling on signal

[Jens Axboe]

On Wed, 09 Aug 2023 16:20:21 +0100, Pavel Begunkov wrote:
> Don't keep spinning iopoll with a signal set. It'll eventually return
> back, e.g. by virtue of need_resched(), but it's not a nice user
> experience.
> 
> 

Applied, thanks!

[1/1] io_uring: break iopolling on signal
      commit: 7f9a6d082585718f0f053b9cb8c2a94691b45e28

Best regards,
-- 
Jens Axboe