Re: [PATCH v2] io_uring: use TWA_SIGNAL for task_work if the task isn't running

[Jann Horn <jannh@google.com>]

On Fri, Aug 7, 2020 at 6:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> An earlier commit:
>
> b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")
>
> ensured that we didn't get stuck waiting for eventfd reads when it's
> registered with the io_uring ring for event notification, but we still
> have a gap where the task can be waiting on other events in the kernel
> and need a bigger nudge to make forward progress.
>
> Ensure that we use signaled notifications for a task that isn't currently
> running, to be certain the work is seen and processed immediately.
>
> Cc: stable@vger.kernel.org # v5.7+
> Reported-by: Josef <josef.grieb@gmail.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> ---
>
> This isn't perfect, as it'll use TWA_SIGNAL even for cases where we
> don't absolutely need it (like task waiting for completions in
> io_cqring_wait()), but we don't have a good way to tell right now. We
> can probably improve on this in the future, for now I think this is the
> best solution.
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e9b27cdaa735..b4300a61f231 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1720,7 +1720,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
>          */
>         if (ctx->flags & IORING_SETUP_SQPOLL)
>                 notify = 0;
> -       else if (ctx->cq_ev_fd)
> +       else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
>                 notify = TWA_SIGNAL;
>
>         ret = task_work_add(tsk, cb, notify);

I don't get it. Apart from still not understanding the big picture:

What guarantees that the lockless read of tsk->state is in any way
related to the state of the remote process by the time we reach
task_work_add()? And why do we not need to signal in TASK_RUNNING
state (e.g. directly before the remote process switches to
TASK_INTERRUPTIBLE or something like that)?

Even if this is correct, it would still be nice if you could add a big
comment that explains the precise semantics this is attempting to
provide. As far as I understand so far, the goal is to trigger -EINTR
returns from certain syscalls, or something like that? But I don't
understand whether that's indeed what's going on, or which syscalls
precisely this is attempting to make return -EINTR.

(Also, lockless reads of concurrently changing variables should be
written with READ_ONCE().)