Re: [PATCH v3 0/6] Complete setup before calling wake_up_new_task()

[Stefan Metzmacher <metze@samba.org>]

Hi Jens,

>> now that we have an explicit wake_up_new_task() in order to start the
>> result from create_io_thread(), we should set things up before calling
>> wake_up_new_task().
>>
>> Changes in v3:
>>  - rebased on for-5.13/io_uring.
>>  - I dropped this:
>>   fs/proc: hide PF_IO_WORKER in get_task_cmdline()
>>  - I added:
>>   set_task_comm() overflow checks
> 
> Looks good to me, a few comments:
> 
> 1) I agree with Pavel that the WARN on overflow is kinda silly,
>    it doesn't matter that much. So I'd rather drop those for now.

I think the overflow matters, the last time, it went unnoticed in
commit c5def4ab849494d3c97f6c9fc84b2ddb868fe78c

        worker->task = kthread_create_on_node(io_wqe_worker, worker, wqe->node,
-                                               "io_wqe_worker-%d", wqe->node);
+                               "io_wqe_worker-%d/%d", index, wqe->node);

With that "io_wqe_worker-0" or "io_wqe_worker-1" are still (up to 5.11)
reported to userspace. And between 5.3 and 5.4 the meaning changed,
the shown value is now unbound vs. bound, while it was the numa node before.

While I was debugging numa related problems, that took a long time to
figure out.

Now that we have the pid encoded in the name, it should not be truncated,
otherwise it will make it impossible to debug problems.

If the userspace application has 10 threads (with pids which would all cause
on overflow) and each uses io_uring (64 io-workers each), then we may have
640 io-workers all with the same name, which are all in the same userspace
process, and it's not possible to find which workers belong to which userspace
thread.

Currently we can ignore as there's no problem, so I'm fine with dropping
5-6 for now.

Maybe a better assertion would be BUILD_BUG_ON(PID_MAX_LIMIT > 9999999);
(or something similar) in order to prevent this from happening.

> 2) Would really love it to see some decent commit messages, quite
>    a few of them are empty. In general some reasoning is nice in
>    the commit message, when you don't have the context available.
> 
> Do you want to do a v4 with 5-6/6 dropped for now, and 3-4 having
> some reasoning? I can also just apply as-is and write some commit
> message myself, let me know. I'll add 1-2 for now.

I'm currently busy with other stuff, it would be great if you could
expand the commit messages!

Thanks!
metze