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

[PATCH v2 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. The only catch is users that might want to move the
ring after creation but before submission. For these users allow them to
create the ring with IORING_SETUP_R_DISABLED and then enable it on the
submission task.

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

v2:
 - add the IORING_SETUP_R_DISABLED logic

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 |  9 ++++++++-
 io_uring/tctx.c     | 42 ++++++++++++++++++------------------------
 io_uring/tctx.h     |  6 ++++--
 3 files changed, 30 insertions(+), 27 deletions(-)


base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
-- 
2.30.2

[PATCH v2 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 or in the case of disabled (IORING_SETUP_R_DISABLED) the
enabling task.

This approach allows a lot of simplification of the logic here. This
removes init logic from the submission path, which can always be a bit
confusing, but also removes the need for locking to write (or read) the
submitter_task.

Users that want to move a ring before submitting can create the ring
disabled and then enable it on the submitting task.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2965b354efc8..242d896c00f3 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3357,6 +3357,10 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
 		goto err;
 	}
 
+	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER
+	    && !(ctx->flags & IORING_SETUP_R_DISABLED))
+		ctx->submitter_task = get_task_struct(current);
+
 	file = io_uring_get_file(ctx);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);
@@ -3548,6 +3552,9 @@ static int io_register_enable_rings(struct io_ring_ctx *ctx)
 	if (!(ctx->flags & IORING_SETUP_R_DISABLED))
 		return -EBADFD;
 
+	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER && !ctx->submitter_task)
+		ctx->submitter_task = get_task_struct(current);
+
 	if (ctx->restrictions.registered)
 		ctx->restricted = 1;
 
-- 
2.30.2

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

[Pavel Begunkov]

On 9/26/22 18:09, Dylan Yudaken wrote:
> Instead of picking the task from the first submitter task, rather use the
> creator task or in the case of disabled (IORING_SETUP_R_DISABLED) the
> enabling task.
> 
> This approach allows a lot of simplification of the logic here. This
> removes init logic from the submission path, which can always be a bit
> confusing, but also removes the need for locking to write (or read) the
> submitter_task.
> 
> Users that want to move a ring before submitting can create the ring
> disabled and then enable it on the submitting task.

I think Dylan briefly mentioned before that it might be a good
idea to task limit registration as well. I can't think of a use
case at the moment but I agree we may find some in the future.


diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 242d896c00f3..60a471e43fd9 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3706,6 +3706,9 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
  	if (WARN_ON_ONCE(percpu_ref_is_dying(&ctx->refs)))
  		return -ENXIO;
  
+	if (ctx->submitter_task && ctx->submitter_task != current)
+		return -EEXIST;
+
  	if (ctx->restricted) {
  		if (opcode >= IORING_REGISTER_LAST)
  			return -EINVAL;


-- 
Pavel Begunkov

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

[Jens Axboe]

On 9/26/22 1:12 PM, Pavel Begunkov wrote:
> On 9/26/22 18:09, Dylan Yudaken wrote:
>> Instead of picking the task from the first submitter task, rather use the
>> creator task or in the case of disabled (IORING_SETUP_R_DISABLED) the
>> enabling task.
>>
>> This approach allows a lot of simplification of the logic here. This
>> removes init logic from the submission path, which can always be a bit
>> confusing, but also removes the need for locking to write (or read) the
>> submitter_task.
>>
>> Users that want to move a ring before submitting can create the ring
>> disabled and then enable it on the submitting task.
> 
> I think Dylan briefly mentioned before that it might be a good
> idea to task limit registration as well. I can't think of a use
> case at the moment but I agree we may find some in the future.
> 
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 242d896c00f3..60a471e43fd9 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3706,6 +3706,9 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>      if (WARN_ON_ONCE(percpu_ref_is_dying(&ctx->refs)))
>          return -ENXIO;
>  
> +    if (ctx->submitter_task && ctx->submitter_task != current)
> +        return -EEXIST;
> +
>      if (ctx->restricted) {
>          if (opcode >= IORING_REGISTER_LAST)
>              return -EINVAL;

Yes, I don't see any reason why not to enforce this for registration
too. Don't think there's currently a need to do so, but it'd be easy
to miss once we do add that. Let's queue that up for 6.1?

-- 
Jens Axboe

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

[Pavel Begunkov]

On 9/26/22 20:40, Jens Axboe wrote:
> On 9/26/22 1:12 PM, Pavel Begunkov wrote:
>> On 9/26/22 18:09, Dylan Yudaken wrote:
>>> Instead of picking the task from the first submitter task, rather use the
>>> creator task or in the case of disabled (IORING_SETUP_R_DISABLED) the
>>> enabling task.
>>>
>>> This approach allows a lot of simplification of the logic here. This
>>> removes init logic from the submission path, which can always be a bit
>>> confusing, but also removes the need for locking to write (or read) the
>>> submitter_task.
>>>
>>> Users that want to move a ring before submitting can create the ring
>>> disabled and then enable it on the submitting task.
>>
>> I think Dylan briefly mentioned before that it might be a good
>> idea to task limit registration as well. I can't think of a use
>> case at the moment but I agree we may find some in the future.
>>
>>
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index 242d896c00f3..60a471e43fd9 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -3706,6 +3706,9 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>>       if (WARN_ON_ONCE(percpu_ref_is_dying(&ctx->refs)))
>>           return -ENXIO;
>>   
>> +    if (ctx->submitter_task && ctx->submitter_task != current)
>> +        return -EEXIST;
>> +
>>       if (ctx->restricted) {
>>           if (opcode >= IORING_REGISTER_LAST)
>>               return -EINVAL;
> 
> Yes, I don't see any reason why not to enforce this for registration
> too. Don't think there's currently a need to do so, but it'd be easy
> to miss once we do add that. Let's queue that up for 6.1?

6.1 + stable sounds ok, I don't have an opinion on how to how
to merge it.

-- 
Pavel Begunkov

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

[Jens Axboe]

On 9/26/22 2:29 PM, Pavel Begunkov wrote:
> On 9/26/22 20:40, Jens Axboe wrote:
>> On 9/26/22 1:12 PM, Pavel Begunkov wrote:
>>> On 9/26/22 18:09, Dylan Yudaken wrote:
>>>> Instead of picking the task from the first submitter task, rather use the
>>>> creator task or in the case of disabled (IORING_SETUP_R_DISABLED) the
>>>> enabling task.
>>>>
>>>> This approach allows a lot of simplification of the logic here. This
>>>> removes init logic from the submission path, which can always be a bit
>>>> confusing, but also removes the need for locking to write (or read) the
>>>> submitter_task.
>>>>
>>>> Users that want to move a ring before submitting can create the ring
>>>> disabled and then enable it on the submitting task.
>>>
>>> I think Dylan briefly mentioned before that it might be a good
>>> idea to task limit registration as well. I can't think of a use
>>> case at the moment but I agree we may find some in the future.
>>>
>>>
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index 242d896c00f3..60a471e43fd9 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -3706,6 +3706,9 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>>>       if (WARN_ON_ONCE(percpu_ref_is_dying(&ctx->refs)))
>>>           return -ENXIO;
>>>   +    if (ctx->submitter_task && ctx->submitter_task != current)
>>> +        return -EEXIST;
>>> +
>>>       if (ctx->restricted) {
>>>           if (opcode >= IORING_REGISTER_LAST)
>>>               return -EINVAL;
>>
>> Yes, I don't see any reason why not to enforce this for registration
>> too. Don't think there's currently a need to do so, but it'd be easy
>> to miss once we do add that. Let's queue that up for 6.1?
> 
> 6.1 + stable sounds ok, I don't have an opinion on how to how
> to merge it.

That's the plan. If you can just send it out as a separate commit,
I'll stage it up behind the two others from Dylan.

-- 
Jens Axboe

[PATCH v2 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 242d896c00f3..a4024d56240f 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 v2 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 v2 0/3] io_uring: register single issuer task at creation

[Jens Axboe]

On 9/26/22 11:09 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. The only catch is users that might want to move the
> ring after creation but before submission. For these users allow them to
> create the ring with IORING_SETUP_R_DISABLED and then enable it on the
> submission task.
> 
> 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

Thanks, I like 1/3 a lot better now. Will provide applications with an
easy path to use SINGLE_ISSUER, even if they currently setup the ring
from a different thread/task than they end up using it from.

I've updated the 6.0 and 6.1 repos to reflect this.

-- 
Jens Axboe