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

[Pavel Begunkov <asml.silence@gmail.com>]

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