Re: [RFC PATCH 00/10] Complete setup before calling wake_up_new_task() and improve task->comm

[Jens Axboe <axboe@kernel.dk>]

On 3/17/21 5:06 PM, Stefan Metzmacher wrote:
> 
> Hi Jens,
> 
>>> now that we have an explicit wake_up_new_task() in order to start the
>>> result from create_io_thread(), we should things up before calling
>>> wake_up_new_task().
>>>
>>> There're also some improvements around task->comm:
>>> - We return 0 bytes for /proc/<pid>/cmdline
>>> - We no longer allow a userspace process to change
>>>   /proc/<pid>/[task/<tid>]/comm
>>> - We dynamically generate comm names (up to 63 chars)
>>>   via io_wq_worker_comm(), similar to wq_worker_comm()
>>>
>>> While doing this I noticed a few places we check for
>>> PF_KTHREAD, but not PF_IO_WORKER, maybe we should
>>> have something like a PS_IS_KERNEL_THREAD_MASK() macro
>>> that should be used in generic places and only
>>> explicitly use PF_IO_WORKER or PF_KTHREAD checks where the
>>> difference matters.
>>>
>>> There are also quite a number of cases where we use
>>> same_thread_group(), I guess these need to be checked.
>>> Should that return true if userspace threads and their iothreds
>>> are compared?
> 
> Can you comment more deeply here and recheck this in the code
> I just noticed possible problems by reading the code and playing
> with git grep. I don't have time right now to build my own 5.12 kernel
> and play with it. (I'm planing to do some livepath tricks to inject
> backported io_uring into an older kernel...).
> 
> For a lot of things regarding PF_KTHREAD v. PF_IO_WORKER and
> same_thread_group() I'm just unsure what the correct behavior would be.

FWIW, I do agree that we should probably have an umbrella that covers
PF_KTHREAD and PF_IO_WORKER, though not in all cases would that be
useful. But we have had a few, so definitely useful.

> It would help if you could post dumps of things like:
> ps axf -o user,pid,tid,comm,cmd
> ls -laR /proc/$pid/
> 
> Currently I can only guess how things will look like.

I'm not too worried about the comm side, and in fact, I'd prefer
deferring that to 5.13 and we can just stable backport it instead.
Trying to keep the amount of churn down to bare necessities at this
point.

>>> I've compiled but didn't test, I hope there's something useful...
>>
>> Looks pretty good to me. Can I talk you into splitting this into
>> a series for 5.12, and then a 5.13 on top? It looks a bit mixed
>> right now. For 5.12, basically just things we absolutely need for
>> release. Any cleanups or improvements on top should go to 5.13.
> 
> I'll rebase tomorrow. Actually I'd like to see all of them in 5.12
> because it would means that do the admin visible change only once.
> 
> The WARN_ON() fixes are not strictly needed, but for me it would be
> strange to defer them.
> io_wq_worker_comm() patches are not strictly required,
> but they would make the new design more consistent and
> avoid changing things again in 5.13.

Right, hence I'd prefer to push comm changes, and anything that isn't
strictly a bug.

-- 
Jens Axboe