Re: [PATCH] io_uring: check sqring and iopoll_list before shedule

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

On 4/21/21 4:19 PM, Hao Xu wrote:
> do this to avoid race below:
> 
>          userspace                         kernel
> 
>                                |  check sqring and iopoll_list
> submit sqe                     |
> check IORING_SQ_NEED_WAKEUP    |
> (which is not set)    |        |
>                                |  set IORING_SQ_NEED_WAKEUP
> wait cqe                       |  schedule(never wakeup again)

Agree, the flag should be set first.
Haven't tried it, but the patch looks reasonable

> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
> 
> Hi all,
> I'm doing some work to reduce cpu usage in low IO pression, and I
> removed timeout logic in io_sq_thread() to do some test with fio-3.26,
> I found that fio hangs in getevents, inifinitely trying to get a cqe,
> While sq-thread is sleeping. It seems there is race situation, and it
> is still there even after I fix the issue described above in the commit
> message. I doubt it is something to do with memory barrier logic
> between userspace and kernel, I'm trying to address it, not many clues
> for now.
> I'll send the fio config and kernel modification I did for test in
> following mail soon.
> 
> Thanks,
> Hao
> 
>  fs/io_uring.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index dff34975d86b..042f1149db51 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6802,27 +6802,29 @@ static int io_sq_thread(void *data)
>  			continue;
>  		}
>  
> -		needs_sched = true;
>  		prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
> -		list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> -			if ((ctx->flags & IORING_SETUP_IOPOLL) &&
> -			    !list_empty_careful(&ctx->iopoll_list)) {
> -				needs_sched = false;
> -				break;
> -			}
> -			if (io_sqring_entries(ctx)) {
> -				needs_sched = false;
> -				break;
> -			}
> -		}
> -
> -		if (needs_sched && !test_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state)) {
> +		if (!test_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state)) {
>  			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>  				io_ring_set_wakeup_flag(ctx);
>  
> -			mutex_unlock(&sqd->lock);
> -			schedule();
> -			mutex_lock(&sqd->lock);
> +			needs_sched = true;
> +			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> +				if ((ctx->flags & IORING_SETUP_IOPOLL) &&
> +				    !list_empty_careful(&ctx->iopoll_list)) {
> +					needs_sched = false;
> +					break;
> +				}
> +				if (io_sqring_entries(ctx)) {
> +					needs_sched = false;
> +					break;
> +				}
> +			}
> +
> +			if (needs_sched) {
> +				mutex_unlock(&sqd->lock);
> +				schedule();
> +				mutex_lock(&sqd->lock);
> +			}
>  			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>  				io_ring_clear_wakeup_flag(ctx);
>  		}
> 

-- 
Pavel Begunkov