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

[Stefan Metzmacher <metze@samba.org>]

Am 20.03.21 um 02:24 schrieb Jens Axboe:
> 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?

The signal problems and it's used to allow certain modifications
between threads in the same group.

With your same_thread_group_account() change it should be all fixed
magically. I guess the thread also doesn't appear in /proc/pid/tasks/
any more, correct?

Would 'top' still hide them with the thread group
and only show them with 'H' (which show the individual threads)?

In future we may want to have /proc/pid/iothreads/ instead...

>> 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?

Ok, I guess we want to take only 'fs/proc: hide PF_IO_WORKER in get_task_cmdline()'
so that ps and top show them as '[iou_mgr_12345]' instead of showing the userspace
cmd.

And with your same_thread_group_account() change we only need this hunk:

@@ -1822,7 +1826,7 @@ void task_dump_owner(struct task_struct *task, umode_t mode,
        kuid_t uid;
        kgid_t gid;

-       if (unlikely(task->flags & PF_KTHREAD)) {
+       if (unlikely(task->flags & (PF_KTHREAD | PF_IO_WORKER))) {
                *ruid = GLOBAL_ROOT_UID;
                *rgid = GLOBAL_ROOT_GID;
                return;

From here:
https://lore.kernel.org/io-uring/97ad63bef490139bb4996e75dea408af1e78fa47.1615826736.git.metze@samba.org/T/#u

I think we should also take that hunk...

What do you think?

metze