Re: [PATCH 3/3] io_uring: move struct io_kiocb from task_struct to io_uring_task

[Jens Axboe <axboe@kernel.dk>]

On 11/3/24 3:47 PM, Pavel Begunkov wrote:
> On 11/3/24 22:40, Jens Axboe wrote:
>> On 11/3/24 3:36 PM, Pavel Begunkov wrote:
>>> On 11/3/24 22:18, Jens Axboe wrote:
>>>> On 11/3/24 3:05 PM, Pavel Begunkov wrote:
>>>>> On 11/3/24 21:54, Jens Axboe wrote:
>>>>>> On 11/3/24 2:47 PM, Pavel Begunkov wrote:
>>>>>>> On 11/3/24 17:49, Jens Axboe wrote:
>>>>>>> ...
>>>>>>>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>>>>>>> ...
>>>>>>>>          nd->head = prev_nd->head;
>>>>>>>> @@ -115,7 +115,7 @@ struct io_kiocb *io_alloc_notif(struct io_ring_ctx *ctx)
>>>>>>>>          notif->opcode = IORING_OP_NOP;
>>>>>>>>          notif->flags = 0;
>>>>>>>>          notif->file = NULL;
>>>>>>>> -    notif->task = current;
>>>>>>>> +    notif->tctx = current->io_uring;
>>>>>>>>          io_get_task_refs(1);
>>>>>>>>          notif->file_node = NULL;
>>>>>>>>          notif->buf_node = NULL;
>>>>>>>> diff --git a/io_uring/poll.c b/io_uring/poll.c
>>>>>>>> index 7db3010b5733..56332893a4b0 100644
>>>>>>>> --- a/io_uring/poll.c
>>>>>>>> +++ b/io_uring/poll.c
>>>>>>>> @@ -224,8 +224,7 @@ static int io_poll_check_events(struct io_kiocb *req, struct io_tw_state *ts)
>>>>>>>>      {
>>>>>>>>          int v;
>>>>>>>>      -    /* req->task == current here, checking PF_EXITING is safe */
>>>>>>>> -    if (unlikely(req->task->flags & PF_EXITING))
>>>>>>>> +    if (unlikely(current->flags & PF_EXITING))
>>>>>>>>              return -ECANCELED
>>>>>>>
>>>>>>> Unlike what the comment says, req->task doesn't have to match current,
>>>>>>> in which case the new check does nothing and it'll break in many very
>>>>>>> interesting ways.
>>>>>>
>>>>>> In which cases does it not outside of fallback?
>>>>>
>>>>> I think it can only be fallback path
>>>>
>>>> I think so too, that's what I was getting at. Hence I think we should just
>>>> change these PF_EXITING checks to be PF_KTHREAD instead. If we're invoked
>>>> from that kind of context, cancel.
>>>
>>> Replacing with just a PF_KTHREAD check won't be right, you can
>>> get here with the right task but after it has been half killed and
>>> marked PF_EXITING.
>>
>> Right, but:
>>
>> if (current->flags & (PF_EXITING | PF_KTHREAD))
>>     ...
>>
>> should be fine as it'll catch both cases with the single check.
> 
> Was thinking to mention it, it should be fine buf feels wrong. Instead
> of directly checking what we want, i.e. whether the task we want to run
> the request from is dead, we are now doing "let's check if the task
> is dead. Ah yes, let's also see if it's PF_KTHREAD which indirectly
> implies that the task is dead because of implementation details."
> 
> Should be fine to leave that, but why not just leave the check
> how it was? Even if it now requires an extra deref through tctx.

I think it'd be better with a comment, I added one that says:

/* exiting original task or fallback work, cancel */

We can retain the original check, but it's actually a data race to check
->flags from a different task. Yes for this case we're in fallback work
and the value should be long since stable, but seems prudent to just
check for the two criteria we care about. At least the comment will be
correct now ;-)

The extra deref mostly doesn't matter here, only potentially for
io_req_task_submit().

-- 
Jens Axboe