diff mbox series

[for-next,v2,21/25] io_uring: add IORING_SETUP_SINGLE_ISSUER

Message ID d6235e12830a225584b90cf3b29f09a0681acc95.1655213915.git.asml.silence@gmail.com (mailing list archive)
State New
Headers show
Series 5.20 cleanups and poll optimisations | expand

Commit Message

Pavel Begunkov June 14, 2022, 2:37 p.m. UTC 
Add a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part
of it, i.e. put limitations of submitters. Also, don't allow it together
with IOPOLL as we're not going to put it to good use.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/uapi/linux/io_uring.h |  5 ++++-
 io_uring/io_uring.c           |  7 +++++--
 io_uring/io_uring_types.h     |  1 +
 io_uring/tctx.c               | 27 ++++++++++++++++++++++++---
 io_uring/tctx.h               |  4 ++--
 5 files changed, 36 insertions(+), 8 deletions(-)

Comments

Hao Xu June 15, 2022, 9:34 a.m. UTC | #1 
On 6/14/22 22:37, Pavel Begunkov wrote:
> @@ -228,7 +249,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);
> +	ret = __io_uring_add_tctx_node(ctx, false);

An question unrelated with this patch: why do we need this, since anyway
we will do it in later io_uring_enter() this task really submits reqs.

>   	mutex_lock(&ctx->uring_lock);
>   	if (ret)
>   		return ret;
Hao Xu June 15, 2022, 9:41 a.m. UTC | #2 
On 6/14/22 22:37, Pavel Begunkov wrote:
> Add a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part
> of it, i.e. put limitations of submitters. Also, don't allow it together
> with IOPOLL as we're not going to put it to good use.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   include/uapi/linux/io_uring.h |  5 ++++-
>   io_uring/io_uring.c           |  7 +++++--
>   io_uring/io_uring_types.h     |  1 +
>   io_uring/tctx.c               | 27 ++++++++++++++++++++++++---
>   io_uring/tctx.h               |  4 ++--
>   5 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index a41ddb8c5e1f..a3a691340d3e 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -138,9 +138,12 @@ enum {
>    * IORING_SQ_TASKRUN in the sq ring flags. Not valid with COOP_TASKRUN.
>    */
>   #define IORING_SETUP_TASKRUN_FLAG	(1U << 9)
> -
>   #define IORING_SETUP_SQE128		(1U << 10) /* SQEs are 128 byte */
>   #define IORING_SETUP_CQE32		(1U << 11) /* CQEs are 32 byte */
> +/*
> + * Only one task is allowed to submit requests
> + */
> +#define IORING_SETUP_SINGLE_ISSUER	(1U << 12)
>   
>   enum io_uring_op {
>   	IORING_OP_NOP,
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 15d209f334eb..4b90439808e3 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3020,6 +3020,8 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
>   	io_destroy_buffers(ctx);
>   	if (ctx->sq_creds)
>   		put_cred(ctx->sq_creds);
> +	if (ctx->submitter_task)
> +		put_task_struct(ctx->submitter_task);
>   
>   	/* there are no registered resources left, nobody uses it */
>   	if (ctx->rsrc_node)
> @@ -3752,7 +3754,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);
> +	ret = __io_uring_add_tctx_node(ctx, false);
>   	if (ret) {
>   		put_unused_fd(fd);
>   		return ret;
> @@ -3972,7 +3974,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
>   			IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ |
>   			IORING_SETUP_R_DISABLED | IORING_SETUP_SUBMIT_ALL |
>   			IORING_SETUP_COOP_TASKRUN | IORING_SETUP_TASKRUN_FLAG |
> -			IORING_SETUP_SQE128 | IORING_SETUP_CQE32))
> +			IORING_SETUP_SQE128 | IORING_SETUP_CQE32 |
> +			IORING_SETUP_SINGLE_ISSUER))
>   		return -EINVAL;
>   
>   	return io_uring_create(entries, &p, params);
> diff --git a/io_uring/io_uring_types.h b/io_uring/io_uring_types.h
> index aba0f8cd6f49..f6d0ad25f377 100644
> --- a/io_uring/io_uring_types.h
> +++ b/io_uring/io_uring_types.h
> @@ -241,6 +241,7 @@ struct io_ring_ctx {
>   	/* Keep this last, we don't need it for the fast path */
>   
>   	struct io_restriction		restrictions;
> +	struct task_struct		*submitter_task;
>   
>   	/* slow path rsrc auxilary data, used by update/register */
>   	struct io_rsrc_node		*rsrc_backup_node;
> diff --git a/io_uring/tctx.c b/io_uring/tctx.c
> index 6adf659687f8..012be261dc50 100644
> --- a/io_uring/tctx.c
> +++ b/io_uring/tctx.c
> @@ -81,12 +81,32 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,
>   	return 0;
>   }
>   
> -int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
> +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;
> +}

Seems we don't need this uring_lock:
When we create a ring, we setup ctx->submitter_task before uring fd is
installed so at that time nobody else can enter this code.
when we enter this code later in io_uring_enter, we just read it.
Pavel Begunkov June 15, 2022, 10:20 a.m. UTC | #3 
On 6/15/22 10:34, Hao Xu wrote:
> On 6/14/22 22:37, Pavel Begunkov wrote:
>> @@ -228,7 +249,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);
>> +    ret = __io_uring_add_tctx_node(ctx, false);
> 
> An question unrelated with this patch: why do we need this, since anyway
> we will do it in later io_uring_enter() this task really submits reqs.

At least we need to allocate a tctx as the files are stored in there


>>       mutex_lock(&ctx->uring_lock);
>>       if (ret)
>>           return ret;
Pavel Begunkov June 15, 2022, 10:26 a.m. UTC | #4 
On 6/15/22 10:41, Hao Xu wrote:
> On 6/14/22 22:37, Pavel Begunkov wrote:
>> Add a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part
>> of it, i.e. put limitations of submitters. Also, don't allow it together
>> with IOPOLL as we're not going to put it to good use.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   include/uapi/linux/io_uring.h |  5 ++++-
>>   io_uring/io_uring.c           |  7 +++++--
>>   io_uring/io_uring_types.h     |  1 +
>>   io_uring/tctx.c               | 27 ++++++++++++++++++++++++---
>>   io_uring/tctx.h               |  4 ++--
>>   5 files changed, 36 insertions(+), 8 deletions(-)
>>
[...]
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index 15d209f334eb..4b90439808e3 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -3020,6 +3020,8 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
>>       io_destroy_buffers(ctx);
>>       if (ctx->sq_creds)
>>           put_cred(ctx->sq_creds);
>> +    if (ctx->submitter_task)
>> +        put_task_struct(ctx->submitter_task);
>>       /* there are no registered resources left, nobody uses it */
>>       if (ctx->rsrc_node)
>> @@ -3752,7 +3754,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);
>> +    ret = __io_uring_add_tctx_node(ctx, false);

                                             ^^^^^^

Note this one


>>       if (ret) {
>>           put_unused_fd(fd);
>>           return ret;
>> @@ -3972,7 +3974,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
>>               IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ |
>>               IORING_SETUP_R_DISABLED | IORING_SETUP_SUBMIT_ALL |
>>               IORING_SETUP_COOP_TASKRUN | IORING_SETUP_TASKRUN_FLAG |
>> -            IORING_SETUP_SQE128 | IORING_SETUP_CQE32))
>> +            IORING_SETUP_SQE128 | IORING_SETUP_CQE32 |
>> +            IORING_SETUP_SINGLE_ISSUER))
>>           return -EINVAL;
>>       return io_uring_create(entries, &p, params);
>> diff --git a/io_uring/io_uring_types.h b/io_uring/io_uring_types.h
>> index aba0f8cd6f49..f6d0ad25f377 100644
>> --- a/io_uring/io_uring_types.h
>> +++ b/io_uring/io_uring_types.h
>> @@ -241,6 +241,7 @@ struct io_ring_ctx {
>>       /* Keep this last, we don't need it for the fast path */
>>       struct io_restriction        restrictions;
>> +    struct task_struct        *submitter_task;
>>       /* slow path rsrc auxilary data, used by update/register */
>>       struct io_rsrc_node        *rsrc_backup_node;
>> diff --git a/io_uring/tctx.c b/io_uring/tctx.c
>> index 6adf659687f8..012be261dc50 100644
>> --- a/io_uring/tctx.c
>> +++ b/io_uring/tctx.c
>> @@ -81,12 +81,32 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,
>>       return 0;
>>   }
>> -int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
>> +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;
>> +}
> 
> Seems we don't need this uring_lock:
> When we create a ring, we setup ctx->submitter_task before uring fd is
> installed so at that time nobody else can enter this code.
> when we enter this code later in io_uring_enter, we just read it.

Not really, we specifically don't set it just to the ring's
creator but to the first submitter. That's needed to be able to
create a ring in one task and pass it over to another.
Hao Xu June 15, 2022, 11:08 a.m. UTC | #5 
On 6/15/22 18:26, Pavel Begunkov wrote:
> On 6/15/22 10:41, Hao Xu wrote:
>> On 6/14/22 22:37, Pavel Begunkov wrote:
>>> Add a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part
>>> of it, i.e. put limitations of submitters. Also, don't allow it together
>>> with IOPOLL as we're not going to put it to good use.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>   include/uapi/linux/io_uring.h |  5 ++++-
>>>   io_uring/io_uring.c           |  7 +++++--
>>>   io_uring/io_uring_types.h     |  1 +
>>>   io_uring/tctx.c               | 27 ++++++++++++++++++++++++---
>>>   io_uring/tctx.h               |  4 ++--
>>>   5 files changed, 36 insertions(+), 8 deletions(-)
>>>
> [...]
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index 15d209f334eb..4b90439808e3 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -3020,6 +3020,8 @@ static __cold void io_ring_ctx_free(struct 
>>> io_ring_ctx *ctx)
>>>       io_destroy_buffers(ctx);
>>>       if (ctx->sq_creds)
>>>           put_cred(ctx->sq_creds);
>>> +    if (ctx->submitter_task)
>>> +        put_task_struct(ctx->submitter_task);
>>>       /* there are no registered resources left, nobody uses it */
>>>       if (ctx->rsrc_node)
>>> @@ -3752,7 +3754,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);
>>> +    ret = __io_uring_add_tctx_node(ctx, false);
> 
>                                              ^^^^^^
> 
> Note this one

My bad, I read it wrong...
diff mbox series

Patch

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index a41ddb8c5e1f..a3a691340d3e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -138,9 +138,12 @@  enum {
  * IORING_SQ_TASKRUN in the sq ring flags. Not valid with COOP_TASKRUN.
  */
 #define IORING_SETUP_TASKRUN_FLAG	(1U << 9)
-
 #define IORING_SETUP_SQE128		(1U << 10) /* SQEs are 128 byte */
 #define IORING_SETUP_CQE32		(1U << 11) /* CQEs are 32 byte */
+/*
+ * Only one task is allowed to submit requests
+ */
+#define IORING_SETUP_SINGLE_ISSUER	(1U << 12)
 
 enum io_uring_op {
 	IORING_OP_NOP,
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 15d209f334eb..4b90439808e3 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3020,6 +3020,8 @@  static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	io_destroy_buffers(ctx);
 	if (ctx->sq_creds)
 		put_cred(ctx->sq_creds);
+	if (ctx->submitter_task)
+		put_task_struct(ctx->submitter_task);
 
 	/* there are no registered resources left, nobody uses it */
 	if (ctx->rsrc_node)
@@ -3752,7 +3754,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);
+	ret = __io_uring_add_tctx_node(ctx, false);
 	if (ret) {
 		put_unused_fd(fd);
 		return ret;
@@ -3972,7 +3974,8 @@  static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
 			IORING_SETUP_CLAMP | IORING_SETUP_ATTACH_WQ |
 			IORING_SETUP_R_DISABLED | IORING_SETUP_SUBMIT_ALL |
 			IORING_SETUP_COOP_TASKRUN | IORING_SETUP_TASKRUN_FLAG |
-			IORING_SETUP_SQE128 | IORING_SETUP_CQE32))
+			IORING_SETUP_SQE128 | IORING_SETUP_CQE32 |
+			IORING_SETUP_SINGLE_ISSUER))
 		return -EINVAL;
 
 	return io_uring_create(entries, &p, params);
diff --git a/io_uring/io_uring_types.h b/io_uring/io_uring_types.h
index aba0f8cd6f49..f6d0ad25f377 100644
--- a/io_uring/io_uring_types.h
+++ b/io_uring/io_uring_types.h
@@ -241,6 +241,7 @@  struct io_ring_ctx {
 	/* Keep this last, we don't need it for the fast path */
 
 	struct io_restriction		restrictions;
+	struct task_struct		*submitter_task;
 
 	/* slow path rsrc auxilary data, used by update/register */
 	struct io_rsrc_node		*rsrc_backup_node;
diff --git a/io_uring/tctx.c b/io_uring/tctx.c
index 6adf659687f8..012be261dc50 100644
--- a/io_uring/tctx.c
+++ b/io_uring/tctx.c
@@ -81,12 +81,32 @@  __cold int io_uring_alloc_task_context(struct task_struct *task,
 	return 0;
 }
 
-int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
+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, bool submitter)
 {
 	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))
@@ -120,7 +140,8 @@  int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
 		list_add(&node->ctx_node, &ctx->tctx_list);
 		mutex_unlock(&ctx->uring_lock);
 	}
-	tctx->last = ctx;
+	if (submitter)
+		tctx->last = ctx;
 	return 0;
 }
 
@@ -228,7 +249,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);
+	ret = __io_uring_add_tctx_node(ctx, false);
 	mutex_lock(&ctx->uring_lock);
 	if (ret)
 		return ret;
diff --git a/io_uring/tctx.h b/io_uring/tctx.h
index 7684713e950f..dde82ce4d8e2 100644
--- a/io_uring/tctx.h
+++ b/io_uring/tctx.h
@@ -34,7 +34,7 @@  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);
+int __io_uring_add_tctx_node(struct io_ring_ctx *ctx, bool submitter);
 void io_uring_clean_tctx(struct io_uring_task *tctx);
 
 void io_uring_unreg_ringfd(void);
@@ -52,5 +52,5 @@  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);
+	return __io_uring_add_tctx_node(ctx, true);
 }