Re: [PATCH v2 0/5] Complete setup before calling wake_up_new_task() and improve task->comm

[Jens Axboe <axboe@kernel.dk>]

On 3/19/21 6:00 PM, Stefan Metzmacher wrote:
> Hi,
> 
> 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
> 
> 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?

Any particular ones you are worried about here?

> I did some basic testing and found the problems I explained here:
> https://lore.kernel.org/io-uring/F3B6EA77-99D1-4424-85AC-CFFED7DC6A4B@kernel.dk/T/#t
> They appear with and without my changes.
> 
> Changes in v2:
> 
> - I dropped/deferred these changes:
>   - 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()
> 
> Stefan Metzmacher (5):
>   kernel: always initialize task->pf_io_worker to NULL
>   io_uring: io_sq_thread() no longer needs to reset
>     current->pf_io_worker
>   io-wq: call set_task_comm() before wake_up_new_task()
>   io_uring: complete sq_thread setup before calling wake_up_new_task()
>   fs/proc: hide PF_IO_WORKER in get_task_cmdline()
> 
>  fs/io-wq.c     | 17 +++++++++--------
>  fs/io_uring.c  | 22 +++++++++++-----------
>  fs/proc/base.c |  3 +++
>  kernel/fork.c  |  1 +
>  4 files changed, 24 insertions(+), 19 deletions(-)

I don't disagree with any of this, but view them more as cleanups than
fixes. In which case I think 5.13 is fine, and that's where they should
go. That seems true for both the first two fixes, and the comm related
ones too.

If you don't agree, can you detail why? The comm changes seem fine, but
doesn't change the visible name. We can make it wider, sure, but any
reason to?

-- 
Jens Axboe