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

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

[Stefan Metzmacher]

Hi,

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

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 (6):
  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()
  io-wq: warn about future set_task_comm() overflows.
  io_uring: warn about future set_task_comm() overflows.

 fs/io-wq.c    | 20 ++++++++++++++++----
 fs/io_uring.c | 34 +++++++++++++++++++++++-----------
 kernel/fork.c |  1 +
 3 files changed, 40 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] kernel: always initialize task->pf_io_worker to NULL

[Stefan Metzmacher]

Otherwise io_wq_worker_{running,sleeping}() may dereference an
invalid pointer (in future). Currently all users of create_io_thread()
are fine and get task->pf_io_worker = NULL implicitly from the
wq_manager, which got it either from the userspace thread
of the sq_thread, which explicitly reset it to NULL.

I think it's safer to always reset it in order to avoid future
problems.

Fixes: 3bfe6106693b ("io-wq: fork worker threads from original task")
cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 kernel/fork.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index b81ccb1ca3a7..224c8317df34 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -927,6 +927,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	tsk->splice_pipe = NULL;
 	tsk->task_frag.page = NULL;
 	tsk->wake_q.next = NULL;
+	tsk->pf_io_worker = NULL;
 
 	account_kernel_stack(tsk, 1);
 
-- 
2.25.1

[PATCH v3 2/6] io_uring: io_sq_thread() no longer needs to reset current->pf_io_worker

[Stefan Metzmacher]

This is done by create_io_thread() now.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io_uring.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 58f12bfbfb44..234c4b8a015c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6786,7 +6786,6 @@ static int io_sq_thread(void *data)
 
 	snprintf(buf, sizeof(buf), "iou-sqp-%d", sqd->task_pid);
 	set_task_comm(current, buf);
-	current->pf_io_worker = NULL;
 
 	if (sqd->sq_cpu != -1)
 		set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
-- 
2.25.1

[PATCH v3 3/6] io-wq: call set_task_comm() before wake_up_new_task()

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io-wq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 49def8714083..cd1af924c3d1 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -528,13 +528,9 @@ static int io_wqe_worker(void *data)
 	struct io_worker *worker = data;
 	struct io_wqe *wqe = worker->wqe;
 	struct io_wq *wq = wqe->wq;
-	char buf[TASK_COMM_LEN];
 
 	worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING);
 
-	snprintf(buf, sizeof(buf), "iou-wrk-%d", wq->task->pid);
-	set_task_comm(current, buf);
-
 	while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) {
 		long ret;
 
@@ -620,6 +616,7 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 {
 	struct io_wqe_acct *acct = &wqe->acct[index];
 	struct io_worker *worker;
+	char tsk_comm[TASK_COMM_LEN];
 	struct task_struct *tsk;
 
 	__set_current_state(TASK_RUNNING);
@@ -643,6 +640,9 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		return;
 	}
 
+	snprintf(tsk_comm, sizeof(tsk_comm), "iou-wrk-%d", wq->task->pid);
+	set_task_comm(tsk, tsk_comm);
+
 	tsk->pf_io_worker = worker;
 	worker->task = tsk;
 	set_cpus_allowed_ptr(tsk, cpumask_of_node(wqe->node));
-- 
2.25.1

[PATCH v3 4/6] io_uring: complete sq_thread setup before calling wake_up_new_task()

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io_uring.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 234c4b8a015c..856289b13488 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6781,19 +6781,10 @@ static int io_sq_thread(void *data)
 	struct io_sq_data *sqd = data;
 	struct io_ring_ctx *ctx;
 	unsigned long timeout = 0;
-	char buf[TASK_COMM_LEN];
 	DEFINE_WAIT(wait);
 
-	snprintf(buf, sizeof(buf), "iou-sqp-%d", sqd->task_pid);
-	set_task_comm(current, buf);
-
-	if (sqd->sq_cpu != -1)
-		set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
-	else
-		set_cpus_allowed_ptr(current, cpu_online_mask);
-	current->flags |= PF_NO_SETAFFINITY;
-
 	mutex_lock(&sqd->lock);
+
 	while (!test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state)) {
 		int ret;
 		bool cap_entries, sqt_spin, needs_sched;
@@ -7888,6 +7879,7 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 		fdput(f);
 	}
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
+		char tsk_comm[TASK_COMM_LEN];
 		struct task_struct *tsk;
 		struct io_sq_data *sqd;
 		bool attached;
@@ -7940,6 +7932,15 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
+		snprintf(tsk_comm, sizeof(tsk_comm), "iou-sqp-%d", sqd->task_pid);
+		set_task_comm(tsk, tsk_comm);
+
+		if (sqd->sq_cpu != -1)
+			set_cpus_allowed_ptr(tsk, cpumask_of(sqd->sq_cpu));
+		else
+			set_cpus_allowed_ptr(tsk, cpu_online_mask);
+		tsk->flags |= PF_NO_SETAFFINITY;
+
 		sqd->thread = tsk;
 		ret = io_uring_alloc_task_context(tsk, ctx);
 		wake_up_new_task(tsk);
-- 
2.25.1

[PATCH v3 5/6] io-wq: warn about future set_task_comm() overflows.

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io-wq.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index cd1af924c3d1..b80c5d905127 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -640,7 +640,19 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		return;
 	}
 
-	snprintf(tsk_comm, sizeof(tsk_comm), "iou-wrk-%d", wq->task->pid);
+	/*
+	 * The limit value of pid_max_max/PID_MAX_LIMIT
+	 * is 4 * 1024 * 1024 = 4194304.
+	 *
+	 * TASK_COMM_LEN is 16, so we have 15 chars to fill.
+	 *
+	 * With "iou-wrk-4194304" we just fit into 15 chars.
+	 * If that ever changes we may better add some special
+	 * handling for PF_IO_WORKER in proc_task_name(), as that
+	 * allows up to 63 chars.
+	 */
+	WARN_ON(snprintf(tsk_comm, sizeof(tsk_comm),
+			 "iou-wrk-%d", wq->task->pid) >= sizeof(tsk_comm));
 	set_task_comm(tsk, tsk_comm);
 
 	tsk->pf_io_worker = worker;
-- 
2.25.1

Re: [PATCH v3 5/6] io-wq: warn about future set_task_comm() overflows.

[Pavel Begunkov]

On 4/25/21 12:26 AM, Stefan Metzmacher wrote:
> Signed-off-by: Stefan Metzmacher <metze@samba.org>
> ---
>  fs/io-wq.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index cd1af924c3d1..b80c5d905127 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -640,7 +640,19 @@ static void create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
>  		return;
>  	}
>  
> -	snprintf(tsk_comm, sizeof(tsk_comm), "iou-wrk-%d", wq->task->pid);
> +	/*
> +	 * The limit value of pid_max_max/PID_MAX_LIMIT
> +	 * is 4 * 1024 * 1024 = 4194304.
> +	 *
> +	 * TASK_COMM_LEN is 16, so we have 15 chars to fill.
> +	 *
> +	 * With "iou-wrk-4194304" we just fit into 15 chars.
> +	 * If that ever changes we may better add some special
> +	 * handling for PF_IO_WORKER in proc_task_name(), as that
> +	 * allows up to 63 chars.
> +	 */
> +	WARN_ON(snprintf(tsk_comm, sizeof(tsk_comm),
> +			 "iou-wrk-%d", wq->task->pid) >= sizeof(tsk_comm));


We don't really care, so saner would be to just to leave them and don't
warn. Not see much need but can be something like "iou-wrk-00000*" if
overflowed. Same for 6/6.


>  	set_task_comm(tsk, tsk_comm);
>  
>  	tsk->pf_io_worker = worker;
> 

-- 
Pavel Begunkov

[PATCH v3 6/6] io_uring: warn about future set_task_comm() overflows.

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io_uring.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 856289b13488..dba4ceea80ee 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7932,7 +7932,19 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
-		snprintf(tsk_comm, sizeof(tsk_comm), "iou-sqp-%d", sqd->task_pid);
+		/*
+		 * The limit value of pid_max_max/PID_MAX_LIMIT
+		 * is 4 * 1024 * 1024 = 4194304.
+		 *
+		 * TASK_COMM_LEN is 16, so we have 15 chars to fill.
+		 *
+		 * With "iou-sqp-4194304" we just fit into 15 chars.
+		 * If that ever changes we may better add some special
+		 * handling for PF_IO_WORKER in proc_task_name(), as that
+		 * allows up to 63 chars.
+		 */
+		WARN_ON(snprintf(tsk_comm, sizeof(tsk_comm),
+				 "iou-sqp-%d", sqd->task_pid) >= sizeof(tsk_comm));
 		set_task_comm(tsk, tsk_comm);
 
 		if (sqd->sq_cpu != -1)
-- 
2.25.1

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

[Jens Axboe]

On 4/24/21 5:26 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 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.

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.

-- 
Jens Axboe

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

[Stefan Metzmacher]

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