[PATCH 0/3] io_uring: register single issuer task at creation

[PATCH 0/3] io_uring: register single issuer task at creation

[Dylan Yudaken]

Registering the single issuer task from the first submit adds unnecesary
complications to the API as well as the implementation. Where simply
registering it at creation should not impose any barriers to getting the
same performance wins.

There is another problem in 6.1, with IORING_SETUP_DEFER_TASKRUN. That
would like to check the submitter_task from unlocked contexts, which would
be racy. If upfront the submitter_task is set at creation time it will
simplify the logic there and probably increase performance (though this is
unmeasured).

Patch 1 registers the task at creation of the io_uring, this works
standalone in case you want to only merge this part for 6.0

Patch 2/3 cleans up the code from the old style

Dylan Yudaken (3):
  io_uring: register single issuer task at creation
  io_uring: simplify __io_uring_add_tctx_node
  io_uring: remove io_register_submitter

 io_uring/io_uring.c |  5 ++++-
 io_uring/tctx.c     | 42 ++++++++++++++++++------------------------
 io_uring/tctx.h     |  6 ++++--
 3 files changed, 26 insertions(+), 27 deletions(-)


base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
-- 
2.30.2

[PATCH 1/3] io_uring: register single issuer task at creation

[Dylan Yudaken]

Instead of picking the task from the first submitter task, rather use the
creator task.
The downside of this is that users are unable to move tasks before
submitting.

However this simplifies the logic here. For example CQEs are able to be
posted by io_uring_register, which could also be from a separate
task. This could be confusing as this does not count as a submit, and so
would not be the registered task.

Additionally this removes init logic from the submission path, which can
always be a bit confusing.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 io_uring/io_uring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2965b354efc8..3f40defd721d 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3357,6 +3357,9 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
 		goto err;
 	}
 
+	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER)
+		ctx->submitter_task = get_task_struct(current);
+
 	file = io_uring_get_file(ctx);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);
-- 
2.30.2

[PATCH 2/3] io_uring: simplify __io_uring_add_tctx_node

[Dylan Yudaken]

Remove submitter parameter from __io_uring_add_tctx_node.

It was only called from one place, and we can do that logic in that one
place.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 io_uring/io_uring.c |  2 +-
 io_uring/tctx.c     | 30 ++++++++++++++++++++----------
 io_uring/tctx.h     |  6 ++++--
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 3f40defd721d..57315a49b85c 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3183,7 +3183,7 @@ static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
 	if (fd < 0)
 		return fd;
 
-	ret = __io_uring_add_tctx_node(ctx, false);
+	ret = __io_uring_add_tctx_node(ctx);
 	if (ret) {
 		put_unused_fd(fd);
 		return ret;
diff --git a/io_uring/tctx.c b/io_uring/tctx.c
index 7f97d97fef0a..dd0205fcdb13 100644
--- a/io_uring/tctx.c
+++ b/io_uring/tctx.c
@@ -105,18 +105,12 @@ static int io_register_submitter(struct io_ring_ctx *ctx)
 	return ret;
 }
 
-int __io_uring_add_tctx_node(struct io_ring_ctx *ctx, bool submitter)
+int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
 {
 	struct io_uring_task *tctx = current->io_uring;
 	struct io_tctx_node *node;
 	int ret;
 
-	if ((ctx->flags & IORING_SETUP_SINGLE_ISSUER) && submitter) {
-		ret = io_register_submitter(ctx);
-		if (ret)
-			return ret;
-	}
-
 	if (unlikely(!tctx)) {
 		ret = io_uring_alloc_task_context(current, ctx);
 		if (unlikely(ret))
@@ -150,8 +144,24 @@ int __io_uring_add_tctx_node(struct io_ring_ctx *ctx, bool submitter)
 		list_add(&node->ctx_node, &ctx->tctx_list);
 		mutex_unlock(&ctx->uring_lock);
 	}
-	if (submitter)
-		tctx->last = ctx;
+	return 0;
+}
+
+int __io_uring_add_tctx_node_from_submit(struct io_ring_ctx *ctx)
+{
+	int ret;
+
+	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER) {
+		ret = io_register_submitter(ctx);
+		if (ret)
+			return ret;
+	}
+
+	ret = __io_uring_add_tctx_node(ctx);
+	if (ret)
+		return ret;
+
+	current->io_uring->last = ctx;
 	return 0;
 }
 
@@ -259,7 +269,7 @@ int io_ringfd_register(struct io_ring_ctx *ctx, void __user *__arg,
 		return -EINVAL;
 
 	mutex_unlock(&ctx->uring_lock);
-	ret = __io_uring_add_tctx_node(ctx, false);
+	ret = __io_uring_add_tctx_node(ctx);
 	mutex_lock(&ctx->uring_lock);
 	if (ret)
 		return ret;
diff --git a/io_uring/tctx.h b/io_uring/tctx.h
index 25974beed4d6..608e96de70a2 100644
--- a/io_uring/tctx.h
+++ b/io_uring/tctx.h
@@ -9,7 +9,8 @@ struct io_tctx_node {
 int io_uring_alloc_task_context(struct task_struct *task,
 				struct io_ring_ctx *ctx);
 void io_uring_del_tctx_node(unsigned long index);
-int __io_uring_add_tctx_node(struct io_ring_ctx *ctx, bool submitter);
+int __io_uring_add_tctx_node(struct io_ring_ctx *ctx);
+int __io_uring_add_tctx_node_from_submit(struct io_ring_ctx *ctx);
 void io_uring_clean_tctx(struct io_uring_task *tctx);
 
 void io_uring_unreg_ringfd(void);
@@ -27,5 +28,6 @@ static inline int io_uring_add_tctx_node(struct io_ring_ctx *ctx)
 
 	if (likely(tctx && tctx->last == ctx))
 		return 0;
-	return __io_uring_add_tctx_node(ctx, true);
+
+	return __io_uring_add_tctx_node_from_submit(ctx);
 }
-- 
2.30.2

[PATCH 3/3] io_uring: remove io_register_submitter

[Dylan Yudaken]

this is no longer needed, as submitter_task is set at creation time.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 io_uring/tctx.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/io_uring/tctx.c b/io_uring/tctx.c
index dd0205fcdb13..4324b1cf1f6a 100644
--- a/io_uring/tctx.c
+++ b/io_uring/tctx.c
@@ -91,20 +91,6 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,
 	return 0;
 }
 
-static int io_register_submitter(struct io_ring_ctx *ctx)
-{
-	int ret = 0;
-
-	mutex_lock(&ctx->uring_lock);
-	if (!ctx->submitter_task)
-		ctx->submitter_task = get_task_struct(current);
-	else if (ctx->submitter_task != current)
-		ret = -EEXIST;
-	mutex_unlock(&ctx->uring_lock);
-
-	return ret;
-}
-
 int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
 {
 	struct io_uring_task *tctx = current->io_uring;
@@ -151,11 +137,9 @@ int __io_uring_add_tctx_node_from_submit(struct io_ring_ctx *ctx)
 {
 	int ret;
 
-	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER) {
-		ret = io_register_submitter(ctx);
-		if (ret)
-			return ret;
-	}
+	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER
+	    && ctx->submitter_task != current)
+		return -EEXIST;
 
 	ret = __io_uring_add_tctx_node(ctx);
 	if (ret)
-- 
2.30.2

Re: [PATCH 0/3] io_uring: register single issuer task at creation

[Jens Axboe]

On 9/26/22 8:03 AM, Dylan Yudaken wrote:
> Registering the single issuer task from the first submit adds unnecesary
> complications to the API as well as the implementation. Where simply
> registering it at creation should not impose any barriers to getting the
> same performance wins.
> 
> There is another problem in 6.1, with IORING_SETUP_DEFER_TASKRUN. That
> would like to check the submitter_task from unlocked contexts, which would
> be racy. If upfront the submitter_task is set at creation time it will
> simplify the logic there and probably increase performance (though this is
> unmeasured).
> 
> Patch 1 registers the task at creation of the io_uring, this works
> standalone in case you want to only merge this part for 6.0
> 
> Patch 2/3 cleans up the code from the old style

Applied 1/3 for 6.0, and then created a new branch for 6.1 that holds
2-3/3. Thanks!

-- 
Jens Axboe