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 4:17 PM, Pavel Begunkov wrote:
> On 11/3/24 22:51, Jens Axboe wrote:
>> On 11/3/24 3:47 PM, Pavel Begunkov wrote:
>>> On 11/3/24 22:40, Jens Axboe wrote:
> ...
>>>> 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 ;-)
> 
> I don't think whack-a-mole'ing all cases is a good thing,
> but at least it can get moved into a helper and be reused in
> all other places.
> 
> if (io_tw_should_terminate(req, tw))
>     fail;
> 
> should be more readable

There's only 3 spots, but yeah we can add a helper for this with a bit
more of a fulfilling comment. Will do that.

-- 
Jens Axboe