Problems with io_threads

Problems with io_threads

[Stefan Metzmacher]

Hi Jens,

as said before I found some problems related to
the new io_threads together with signals.

I applied the diff (at the end) to examples/io_uring-cp.c
in order to run endless in order to give me time to
look at /proc/...

Trying to attach gdb --pid to the pid of the main process (thread group)
it goes into an endless loop because it can't attach to the io_threads.

Sending kill -STOP to the main pid causes the io_threads to spin cpu
at 100%.

Can you try to reproduce and fix it? Maybe same_thread_group() should not match?

Thanks!
metze

--- a/examples/io_uring-cp.c
+++ b/examples/io_uring-cp.c
@@ -116,17 +116,18 @@ static void queue_write(struct io_uring *ring, struct io_data *data)
        io_uring_submit(ring);
 }

-static int copy_file(struct io_uring *ring, off_t insize)
+static int copy_file(struct io_uring *ring, off_t _insize)
 {
        unsigned long reads, writes;
        struct io_uring_cqe *cqe;
        off_t write_left, offset;
        int ret;

-       write_left = insize;
+       write_left = _insize;
        writes = reads = offset = 0;

-       while (insize || write_left) {
+       while (_insize || write_left) {
+               off_t insize = _insize;
                int had_reads, got_comp;

                /*
@@ -219,6 +220,10 @@ static int copy_file(struct io_uring *ring, off_t insize)
                        }
                        io_uring_cqe_seen(ring, cqe);
                }
+
+               sleep(1);
+               write_left = _insize;
+               writes = reads = offset = 0;
        }

        return 0;

[RFC PATCH 00/10] Complete setup before calling wake_up_new_task() and improve task->comm

[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 things up before calling
wake_up_new_task().

There're also some improvements around task->comm:
- We return 0 bytes for /proc/<pid>/cmdline
- 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()

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?

I've compiled but didn't test, I hope there's something useful...

Stefan Metzmacher (10):
  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: protect against future set_task_comm() overflows.
  io_uring: protect against future set_task_comm() overflows.
  fs/proc: hide PF_IO_WORKER in get_task_cmdline()
  fs/proc: protect /proc/<pid>/[task/<tid>]/comm for PF_IO_WORKER
  io-wq: add io_wq_worker_comm() helper function for dynamic
    proc_task_comm() generation
  fs/proc: make use of io_wq_worker_comm() for PF_IO_WORKER threads

 fs/io-wq.c      | 118 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/io-wq.h      |   4 ++
 fs/io_uring.c   |  33 +++++++++-----
 fs/proc/array.c |   3 ++
 fs/proc/base.c  |  18 ++++++--
 kernel/fork.c   |   1 +
 6 files changed, 154 insertions(+), 23 deletions(-)

-- 
2.25.1

[RFC PATCH 01/10] 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 d3171e8e88e5..0b7068c55552 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

[RFC PATCH 02/10] 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 f048c42d8b8f..059bf8b9eb72 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6695,7 +6695,6 @@ static int io_sq_thread(void *data)
 
 	sprintf(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

[RFC PATCH 03/10] 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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index e05f996d088f..d6b15a627f9a 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -479,14 +479,10 @@ 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);
 	io_wqe_inc_running(worker);
 
-	sprintf(buf, "iou-wrk-%d", wq->task_pid);
-	set_task_comm(current, buf);
-
 	while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) {
 		long ret;
 
@@ -567,6 +563,7 @@ static bool 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);
@@ -591,6 +588,9 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		return false;
 	}
 
+	sprintf(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));
@@ -702,12 +702,8 @@ static void io_wq_cancel_pending(struct io_wq *wq)
 static int io_wq_manager(void *data)
 {
 	struct io_wq *wq = data;
-	char buf[TASK_COMM_LEN];
 	int node;
 
-	sprintf(buf, "iou-mgr-%d", wq->task_pid);
-	set_task_comm(current, buf);
-
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		io_wq_check_workers(wq);
@@ -782,6 +778,11 @@ static int io_wq_fork_manager(struct io_wq *wq)
 	atomic_set(&wq->worker_refs, 1);
 	tsk = create_io_thread(io_wq_manager, wq, NUMA_NO_NODE);
 	if (!IS_ERR(tsk)) {
+		char tsk_comm[TASK_COMM_LEN];
+
+		sprintf(tsk_comm, "iou-mgr-%d", wq->task_pid);
+		set_task_comm(tsk, tsk_comm);
+
 		wq->manager = get_task_struct(tsk);
 		wake_up_new_task(tsk);
 		return 0;
-- 
2.25.1

[RFC PATCH 04/10] 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 059bf8b9eb72..e7e2d87cd9c1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6690,18 +6690,8 @@ 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);
 
-	sprintf(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;
-
 	down_read(&sqd->rw_lock);
 
 	while (!test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state)) {
@@ -7853,6 +7843,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;
@@ -7914,6 +7905,15 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
+		sprintf(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

[RFC PATCH 05/10] io-wq: protect against future set_task_comm() overflows.

[Stefan Metzmacher]

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

diff --git a/fs/io-wq.c b/fs/io-wq.c
index d6b15a627f9a..5c7d2a8c112e 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -588,7 +588,19 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		return false;
 	}
 
-	sprintf(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;
@@ -780,7 +792,19 @@ static int io_wq_fork_manager(struct io_wq *wq)
 	if (!IS_ERR(tsk)) {
 		char tsk_comm[TASK_COMM_LEN];
 
-		sprintf(tsk_comm, "iou-mgr-%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-mgr-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-mgr-%d", wq->task_pid) >= sizeof(tsk_comm));
 		set_task_comm(tsk, tsk_comm);
 
 		wq->manager = get_task_struct(tsk);
-- 
2.25.1

[RFC PATCH 06/10] io_uring: protect against 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 e7e2d87cd9c1..e88d9f95d0aa 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7905,7 +7905,19 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
-		sprintf(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

[RFC PATCH 07/10] fs/proc: hide PF_IO_WORKER in get_task_cmdline()

[Stefan Metzmacher]

We should not return the userspace cmdline for io_worker threads.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/proc/base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3851bfcdba56..6e04278de582 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -344,6 +344,9 @@ static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,
 	struct mm_struct *mm;
 	ssize_t ret;
 
+	if (tsk->flags & PF_IO_WORKER)
+		return 0;
+
 	mm = get_task_mm(tsk);
 	if (!mm)
 		return 0;
-- 
2.25.1

[RFC PATCH 08/10] fs/proc: protect /proc/<pid>/[task/<tid>]/comm for PF_IO_WORKER

[Stefan Metzmacher]

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

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6e04278de582..7177e92790c4 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,6 +1665,7 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
 				size_t count, loff_t *offset)
 {
 	struct inode *inode = file_inode(file);
+	bool is_same_tgroup = false;
 	struct task_struct *p;
 	char buffer[TASK_COMM_LEN];
 	const size_t maxlen = sizeof(buffer) - 1;
@@ -1677,7 +1678,10 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
 	if (!p)
 		return -ESRCH;
 
-	if (same_thread_group(current, p))
+	if (!(p->flags & PF_IO_WORKER))
+		is_same_tgroup = same_thread_group(current, p);
+
+	if (is_same_tgroup)
 		set_task_comm(p, buffer);
 	else
 		count = -EINVAL;
@@ -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;
@@ -3478,19 +3482,22 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 static int proc_tid_comm_permission(struct user_namespace *mnt_userns,
 				    struct inode *inode, int mask)
 {
-	bool is_same_tgroup;
+	bool is_same_tgroup = false;
 	struct task_struct *task;
 
 	task = get_proc_task(inode);
 	if (!task)
 		return -ESRCH;
-	is_same_tgroup = same_thread_group(current, task);
+	if (!(task->flags & PF_IO_WORKER))
+		is_same_tgroup = same_thread_group(current, task);
 	put_task_struct(task);
 
 	if (likely(is_same_tgroup && !(mask & MAY_EXEC))) {
 		/* This file (/proc/<pid>/task/<tid>/comm) can always be
 		 * read or written by the members of the corresponding
 		 * thread group.
+		 *
+		 * But we exclude PF_IO_WORKER kernel threads.
 		 */
 		return 0;
 	}
-- 
2.25.1

[RFC PATCH 09/10] io-wq: add io_wq_worker_comm() helper function for dynamic proc_task_comm() generation

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/io-wq.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/io-wq.h |  4 +++
 2 files changed, 81 insertions(+)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index 5c7d2a8c112e..1267171f6388 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -559,6 +559,83 @@ void io_wq_worker_sleeping(struct task_struct *tsk)
 	raw_spin_unlock_irq(&worker->wqe->lock);
 }
 
+/* used to show worker information through /proc/PID/{comm,stat,status} */
+void io_wq_worker_comm(char *buf, size_t size, struct task_struct *tsk)
+{
+	struct io_worker *worker = tsk->pf_io_worker;
+	bool working = false;
+	unsigned flags = 0;
+	unsigned int cpu;
+	int node;
+	int wqe_node = NUMA_NO_NODE;
+	int off;
+
+	BUG_ON(!(tsk->flags & PF_IO_WORKER));
+
+	/* prepend iothread/ to the actual comm */
+	off = scnprintf(buf, size, "iothread/%s", tsk->comm);
+	if (off < 0)
+		return;
+
+	cpu = task_cpu(tsk);
+	node = cpu_to_node(cpu);
+
+	if (worker && io_worker_get(worker)) {
+		spin_lock_irq(&worker->lock);
+		flags = worker->flags;
+		working = worker->cur_work != NULL;
+		wqe_node = worker->wqe->node;
+		spin_unlock_irq(&worker->lock);
+		io_worker_release(worker);
+	}
+
+	/*
+	 * It may or may not run on the desired node
+	 */
+	if (node == wqe_node)
+		off += scnprintf(buf + off, size - off, "/+numa%u", node);
+	else
+		off += scnprintf(buf + off, size - off, "/-numa%u", node);
+	if (off < 0)
+		return;
+
+	/*
+	 * It maybe created via create_io_thread(), but not
+	 * via the create_io_worker() wrapper.
+	 */
+	if (worker == NULL)
+		return;
+
+	if (!(flags & IO_WORKER_F_UP))
+		off += scnprintf(buf + off, size - off, "-down");
+	else if (flags & IO_WORKER_F_FREE)
+		off += scnprintf(buf + off, size - off, "-free");
+	else if (flags & IO_WORKER_F_RUNNING) {
+		if (working)
+			off += scnprintf(buf + off, size - off, "+working");
+		else
+			off += scnprintf(buf + off, size - off, "+running");
+		if (off < 0)
+			return;
+	}
+	if (off < 0)
+		return;
+
+	if (flags & IO_WORKER_F_BOUND)
+		off += scnprintf(buf + off, size - off, "+bound");
+	else
+		off += scnprintf(buf + off, size - off, "-unbound");
+	if (off < 0)
+		return;
+
+	if (flags & IO_WORKER_F_FIXED)
+		off += scnprintf(buf + off, size - off, "+fixed");
+	if (off < 0)
+		return;
+
+	return;
+}
+
 static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 {
 	struct io_wqe_acct *acct = &wqe->acct[index];
diff --git a/fs/io-wq.h b/fs/io-wq.h
index 80d590564ff9..470f854256d1 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -140,6 +140,7 @@ enum io_wq_cancel io_wq_cancel_cb(struct io_wq *wq, work_cancel_fn *cancel,
 #if defined(CONFIG_IO_WQ)
 extern void io_wq_worker_sleeping(struct task_struct *);
 extern void io_wq_worker_running(struct task_struct *);
+extern void io_wq_worker_comm(char *buf, size_t size, struct task_struct *tsk);
 #else
 static inline void io_wq_worker_sleeping(struct task_struct *tsk)
 {
@@ -147,6 +148,9 @@ static inline void io_wq_worker_sleeping(struct task_struct *tsk)
 static inline void io_wq_worker_running(struct task_struct *tsk)
 {
 }
+static inline void io_wq_worker_comm(char *buf, size_t size, struct task_struct *tsk)
+{
+}
 #endif
 
 static inline bool io_wq_current_is_worker(void)
-- 
2.25.1

[RFC PATCH 10/10] fs/proc: make use of io_wq_worker_comm() for PF_IO_WORKER threads

[Stefan Metzmacher]

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/proc/array.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index bb87e4d89cd8..e1d40d124385 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -92,6 +92,7 @@
 #include <linux/string_helpers.h>
 #include <linux/user_namespace.h>
 #include <linux/fs_struct.h>
+#include "../io-wq.h"
 
 #include <asm/processor.h>
 #include "internal.h"
@@ -105,6 +106,8 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
 
 	if (p->flags & PF_WQ_WORKER)
 		wq_worker_comm(tcomm, sizeof(tcomm), p);
+	else if (p->flags & PF_IO_WORKER)
+		io_wq_worker_comm(tcomm, sizeof(tcomm), p);
 	else
 		__get_task_comm(tcomm, sizeof(tcomm), p);
 
-- 
2.25.1

Re: [RFC PATCH 00/10] Complete setup before calling wake_up_new_task() and improve task->comm

[Jens Axboe]

On 3/15/21 11:01 AM, 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
> - 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()
> 
> 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?
> 
> I've compiled but didn't test, I hope there's something useful...

Looks pretty good to me. Can I talk you into splitting this into
a series for 5.12, and then a 5.13 on top? It looks a bit mixed
right now. For 5.12, basically just things we absolutely need for
release. Any cleanups or improvements on top should go to 5.13.

-- 
Jens Axboe

Re: [RFC PATCH 00/10] Complete setup before calling wake_up_new_task() and improve task->comm

[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 things up before calling
>> wake_up_new_task().
>>
>> There're also some improvements around task->comm:
>> - We return 0 bytes for /proc/<pid>/cmdline
>> - 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()
>>
>> 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?

Can you comment more deeply here and recheck this in the code
I just noticed possible problems by reading the code and playing
with git grep. I don't have time right now to build my own 5.12 kernel
and play with it. (I'm planing to do some livepath tricks to inject
backported io_uring into an older kernel...).

For a lot of things regarding PF_KTHREAD v. PF_IO_WORKER and
same_thread_group() I'm just unsure what the correct behavior would be.

It would help if you could post dumps of things like:
ps axf -o user,pid,tid,comm,cmd
ls -laR /proc/$pid/

Currently I can only guess how things will look like.

>> I've compiled but didn't test, I hope there's something useful...
> 
> Looks pretty good to me. Can I talk you into splitting this into
> a series for 5.12, and then a 5.13 on top? It looks a bit mixed
> right now. For 5.12, basically just things we absolutely need for
> release. Any cleanups or improvements on top should go to 5.13.

I'll rebase tomorrow. Actually I'd like to see all of them in 5.12
because it would means that do the admin visible change only once.

The WARN_ON() fixes are not strictly needed, but for me it would be
strange to defer them.
io_wq_worker_comm() patches are not strictly required,
but they would make the new design more consistent and
avoid changing things again in 5.13.

But I'll let you decide...

Thanks!
metze

Re: [RFC PATCH 00/10] Complete setup before calling wake_up_new_task() and improve task->comm

[Jens Axboe]

On 3/17/21 5:06 PM, Stefan Metzmacher wrote:
> 
> Hi Jens,
> 
>>> 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
>>> - 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()
>>>
>>> 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?
> 
> Can you comment more deeply here and recheck this in the code
> I just noticed possible problems by reading the code and playing
> with git grep. I don't have time right now to build my own 5.12 kernel
> and play with it. (I'm planing to do some livepath tricks to inject
> backported io_uring into an older kernel...).
> 
> For a lot of things regarding PF_KTHREAD v. PF_IO_WORKER and
> same_thread_group() I'm just unsure what the correct behavior would be.

FWIW, I do agree that we should probably have an umbrella that covers
PF_KTHREAD and PF_IO_WORKER, though not in all cases would that be
useful. But we have had a few, so definitely useful.

> It would help if you could post dumps of things like:
> ps axf -o user,pid,tid,comm,cmd
> ls -laR /proc/$pid/
> 
> Currently I can only guess how things will look like.

I'm not too worried about the comm side, and in fact, I'd prefer
deferring that to 5.13 and we can just stable backport it instead.
Trying to keep the amount of churn down to bare necessities at this
point.

>>> I've compiled but didn't test, I hope there's something useful...
>>
>> Looks pretty good to me. Can I talk you into splitting this into
>> a series for 5.12, and then a 5.13 on top? It looks a bit mixed
>> right now. For 5.12, basically just things we absolutely need for
>> release. Any cleanups or improvements on top should go to 5.13.
> 
> I'll rebase tomorrow. Actually I'd like to see all of them in 5.12
> because it would means that do the admin visible change only once.
> 
> The WARN_ON() fixes are not strictly needed, but for me it would be
> strange to defer them.
> io_wq_worker_comm() patches are not strictly required,
> but they would make the new design more consistent and
> avoid changing things again in 5.13.

Right, hence I'd prefer to push comm changes, and anything that isn't
strictly a bug.

-- 
Jens Axboe

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

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

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(-)

-- 
2.25.1

[PATCH v2 1/5] 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 d3171e8e88e5..0b7068c55552 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 v2 2/5] 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 ea2d3e120555..cf46b890a36f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6726,7 +6726,6 @@ static int io_sq_thread(void *data)
 
 	sprintf(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 v2 3/5] 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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index e05f996d088f..d6b15a627f9a 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -479,14 +479,10 @@ 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);
 	io_wqe_inc_running(worker);
 
-	sprintf(buf, "iou-wrk-%d", wq->task_pid);
-	set_task_comm(current, buf);
-
 	while (!test_bit(IO_WQ_BIT_EXIT, &wq->state)) {
 		long ret;
 
@@ -567,6 +563,7 @@ static bool 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);
@@ -591,6 +588,9 @@ static bool create_io_worker(struct io_wq *wq, struct io_wqe *wqe, int index)
 		return false;
 	}
 
+	sprintf(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));
@@ -702,12 +702,8 @@ static void io_wq_cancel_pending(struct io_wq *wq)
 static int io_wq_manager(void *data)
 {
 	struct io_wq *wq = data;
-	char buf[TASK_COMM_LEN];
 	int node;
 
-	sprintf(buf, "iou-mgr-%d", wq->task_pid);
-	set_task_comm(current, buf);
-
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		io_wq_check_workers(wq);
@@ -782,6 +778,11 @@ static int io_wq_fork_manager(struct io_wq *wq)
 	atomic_set(&wq->worker_refs, 1);
 	tsk = create_io_thread(io_wq_manager, wq, NUMA_NO_NODE);
 	if (!IS_ERR(tsk)) {
+		char tsk_comm[TASK_COMM_LEN];
+
+		sprintf(tsk_comm, "iou-mgr-%d", wq->task_pid);
+		set_task_comm(tsk, tsk_comm);
+
 		wq->manager = get_task_struct(tsk);
 		wake_up_new_task(tsk);
 		return 0;
-- 
2.25.1

[PATCH v2 4/5] 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 cf46b890a36f..320672930d87 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6721,19 +6721,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);
 
-	sprintf(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;
@@ -7887,6 +7878,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;
@@ -7943,6 +7935,15 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
+		sprintf(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 v2 5/5] fs/proc: hide PF_IO_WORKER in get_task_cmdline()

[Stefan Metzmacher]

We should not return the userspace cmdline for io_worker threads.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/proc/base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3851bfcdba56..6e04278de582 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -344,6 +344,9 @@ static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,
 	struct mm_struct *mm;
 	ssize_t ret;
 
+	if (tsk->flags & PF_IO_WORKER)
+		return 0;
+
 	mm = get_task_mm(tsk);
 	if (!mm)
 		return 0;
-- 
2.25.1

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

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

> 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

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

[Stefan Metzmacher]

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

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

[Jens Axboe]

On 3/20/21 1:22 PM, Stefan Metzmacher wrote:
> 
> 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.

Gotcha

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

I think it'll still show up there, as they are still linked.

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

I think it'll show them as a thread group still.

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

Maybe?

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

That one makes sense, to keep it consistent with earlier to some extent,
and not to have 5.12 be the odd one out compared to later kernels as
well.

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

I'll have to look into that, on the face of it it seems wrong. Why just
assign global root uid/gid for the io worker? It's using the same
credentials as the original task.

-- 
Jens Axboe

Re: Problems with io_threads

[Jens Axboe]

On Mar 19, 2021, at 5:27 PM, Stefan Metzmacher <metze@samba.org> wrote:
> 
> Hi Jens,
> 
> as said before I found some problems related to
> the new io_threads together with signals.
> 
> I applied the diff (at the end) to examples/io_uring-cp.c
> in order to run endless in order to give me time to
> look at /proc/...
> 
> Trying to attach gdb --pid to the pid of the main process (thread group)
> it goes into an endless loop because it can't attach to the io_threads.
> 
> Sending kill -STOP to the main pid causes the io_threads to spin cpu
> at 100%.
> 
> Can you try to reproduce and fix it? Maybe same_thread_group() should not match?

Definitely, I’ll go over this shortly and make sure we handle (and ignore) signals correctly. 

— 
Jens Axboe

Re: Problems with io_threads

[Stefan Metzmacher]

Am 20.03.21 um 00:46 schrieb Jens Axboe:
> On Mar 19, 2021, at 5:27 PM, Stefan Metzmacher <metze@samba.org> wrote:
>>
>> Hi Jens,
>>
>> as said before I found some problems related to
>> the new io_threads together with signals.
>>
>> I applied the diff (at the end) to examples/io_uring-cp.c
>> in order to run endless in order to give me time to
>> look at /proc/...
>>
>> Trying to attach gdb --pid to the pid of the main process (thread group)
>> it goes into an endless loop because it can't attach to the io_threads.
>>
>> Sending kill -STOP to the main pid causes the io_threads to spin cpu
>> at 100%.
>>
>> Can you try to reproduce and fix it? Maybe same_thread_group() should not match?
> 
> Definitely, I’ll go over this shortly and make sure we handle (and ignore) signals correctly. 

Thanks! Also a kill -9 to a io_thread kills the application.

metze

Re: Problems with io_threads

[Jens Axboe]

On 3/19/21 6:25 PM, Stefan Metzmacher wrote:
> Am 20.03.21 um 00:46 schrieb Jens Axboe:
>> On Mar 19, 2021, at 5:27 PM, Stefan Metzmacher <metze@samba.org> wrote:
>>>
>>> Hi Jens,
>>>
>>> as said before I found some problems related to
>>> the new io_threads together with signals.
>>>
>>> I applied the diff (at the end) to examples/io_uring-cp.c
>>> in order to run endless in order to give me time to
>>> look at /proc/...
>>>
>>> Trying to attach gdb --pid to the pid of the main process (thread group)
>>> it goes into an endless loop because it can't attach to the io_threads.
>>>
>>> Sending kill -STOP to the main pid causes the io_threads to spin cpu
>>> at 100%.
>>>
>>> Can you try to reproduce and fix it? Maybe same_thread_group() should not match?
>>
>> Definitely, I’ll go over this shortly and make sure we handle (and ignore) signals correctly. 
> 
> Thanks! Also a kill -9 to a io_thread kills the application.

OK, this I believe should take care of it - ignore STOP specifically for
PF_IO_WORKER, and disallow any signal sent to a worker.

diff --git a/kernel/signal.c b/kernel/signal.c
index ba4d1ef39a9e..b113bf647fb4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -833,6 +833,9 @@ static int check_kill_permission(int sig, struct kernel_siginfo *info,
 
 	if (!valid_signal(sig))
 		return -EINVAL;
+	/* PF_IO_WORKER threads don't take any signals */
+	if (t->flags & PF_IO_WORKER)
+		return -EPERM;
 
 	if (!si_fromuser(info))
 		return 0;
@@ -2346,6 +2349,10 @@ static bool do_signal_stop(int signr)
 
 		t = current;
 		while_each_thread(current, t) {
+			/* don't STOP PF_IO_WORKER threads */
+			if (t->flags & PF_IO_WORKER)
+				continue;
+
 			/*
 			 * Setting state to TASK_STOPPED for a group
 			 * stop is always done with the siglock held,

-- 
Jens Axboe