Question about async_wake

Question about async_wake

[Hao Xu]

Hi Jens, Pavel,
I have a question about _async_wake(), would there be cases like
this:
    async_cancel/poll_remove           interrupt
      spin_lock_irq()
list del poll->wait_entry         event happens but irq disabled
                                   so interrupt delays
     spin_unlock_irq()
       generate cqe
                                   async_wake() called and
                                   generate cqe

If it exists, there may be multiple -ECANCELED cqes for one req,
we may do something like this to avoid it:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 30d959416eba..7822b2f9e890 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5023,9 +5023,12 @@ static int __io_async_wake(struct io_kiocb *req, 
struct io_poll_iocb *poll,
         if (mask && !(mask & poll->events))
                 return 0;

-       trace_io_uring_task_add(req->ctx, req->opcode, req->user_data, 
mask);
+       if (list_empty(&poll->wait.entry))
+               return 0;
+       else
+               list_del_init(&poll->wait.entry);

-       list_del_init(&poll->wait.entry);
+       trace_io_uring_task_add(req->ctx, req->opcode, req->user_data, 
mask);

         req->result = mask;
         req->io_task_work.func = func;

Re: Question about async_wake

[Pavel Begunkov]

On 9/8/21 12:18 PM, Hao Xu wrote:
> Hi Jens, Pavel,
> I have a question about _async_wake(), would there be cases like
> this:
>    async_cancel/poll_remove           interrupt
>      spin_lock_irq()
> list del poll->wait_entry         event happens but irq disabled
>                                   so interrupt delays
>     spin_unlock_irq()
>       generate cqe
>                                   async_wake() called and
>                                   generate cqe

wake_up*() looks for queued callbacks under that same spin, so it
won't find that cancelled entry and so won't call io_async_wake().

There might be other similar (or not) problematic cases, though.

> If it exists, there may be multiple -ECANCELED cqes for one req,
> we may do something like this to avoid it:
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 30d959416eba..7822b2f9e890 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5023,9 +5023,12 @@ static int __io_async_wake(struct io_kiocb *req, struct io_poll_iocb *poll,
>         if (mask && !(mask & poll->events))
>                 return 0;
> 
> -       trace_io_uring_task_add(req->ctx, req->opcode, req->user_data, mask);
> +       if (list_empty(&poll->wait.entry))
> +               return 0;
> +       else
> +               list_del_init(&poll->wait.entry);
> 
> -       list_del_init(&poll->wait.entry);
> +       trace_io_uring_task_add(req->ctx, req->opcode, req->user_data, mask);
> 
>         req->result = mask;
>         req->io_task_work.func = func;

-- 
Pavel Begunkov