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

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

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.

-- 
Pavel Begunkov