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@xxxxxxxxx/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@xxxxxxxxx/T/#u I think we should also take that hunk... What do you think? metze
