public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: save repeated issue_flags
@ 2023-12-05 21:55 Keith Busch
  2023-12-05 22:00 ` Jens Axboe
  0 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2023-12-05 21:55 UTC (permalink / raw)
  To: io-uring; +Cc: asml.silence, Keith Busch, Jens Axboe

From: Keith Busch <[email protected]>

No need to rebuild the issue_flags on every IO: they're always the same.

Suggested-by: Jens Axboe <[email protected]>
Signed-off-by: Keith Busch <[email protected]>
---
 include/linux/io_uring_types.h |  1 +
 io_uring/io_uring.c            | 15 ++++++++++++---
 io_uring/uring_cmd.c           |  8 +-------
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index bebab36abce89..dd192d828f463 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -228,6 +228,7 @@ struct io_ring_ctx {
 	/* const or read-mostly hot data */
 	struct {
 		unsigned int		flags;
+		unsigned int		issue_flags;
 		unsigned int		drain_next: 1;
 		unsigned int		restricted: 1;
 		unsigned int		off_timeout_used: 1;
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 1d254f2c997de..a338e3660ecb8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3975,11 +3975,20 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
 	 * polling again, they can rely on io_sq_thread to do polling
 	 * work, which can reduce cpu usage and uring_lock contention.
 	 */
-	if (ctx->flags & IORING_SETUP_IOPOLL &&
-	    !(ctx->flags & IORING_SETUP_SQPOLL))
-		ctx->syscall_iopoll = 1;
+	if (ctx->flags & IORING_SETUP_IOPOLL) {
+		ctx->issue_flags |= IO_URING_F_SQE128;
+		if (!(ctx->flags & IORING_SETUP_SQPOLL))
+			ctx->syscall_iopoll = 1;
+	}
 
 	ctx->compat = in_compat_syscall();
+	if (ctx->compat)
+		ctx->issue_flags |= IO_URING_F_COMPAT;
+	if (ctx->flags & IORING_SETUP_SQE128)
+		ctx->issue_flags |= IO_URING_F_SQE128;
+	if (ctx->flags & IORING_SETUP_CQE32)
+		ctx->issue_flags |= IO_URING_F_CQE32;
+
 	if (!ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
 		ctx->user = get_uid(current_user());
 
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 8a38b9f75d841..dbc0bfbfd0f05 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 	if (ret)
 		return ret;
 
-	if (ctx->flags & IORING_SETUP_SQE128)
-		issue_flags |= IO_URING_F_SQE128;
-	if (ctx->flags & IORING_SETUP_CQE32)
-		issue_flags |= IO_URING_F_CQE32;
-	if (ctx->compat)
-		issue_flags |= IO_URING_F_COMPAT;
 	if (ctx->flags & IORING_SETUP_IOPOLL) {
 		if (!file->f_op->uring_cmd_iopoll)
 			return -EOPNOTSUPP;
-		issue_flags |= IO_URING_F_IOPOLL;
 		req->iopoll_completed = 0;
 	}
 
+	issue_flags |= ctx->issue_flags;
 	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
 	if (ret == -EAGAIN) {
 		if (!req_has_async_data(req)) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-05 21:55 [PATCH] io_uring: save repeated issue_flags Keith Busch
@ 2023-12-05 22:00 ` Jens Axboe
  2023-12-05 23:02   ` Keith Busch
  2023-12-06  1:26   ` Pavel Begunkov
  0 siblings, 2 replies; 8+ messages in thread
From: Jens Axboe @ 2023-12-05 22:00 UTC (permalink / raw)
  To: Keith Busch, io-uring; +Cc: asml.silence, Keith Busch

On 12/5/23 2:55 PM, Keith Busch wrote:
> From: Keith Busch <[email protected]>
> 
> No need to rebuild the issue_flags on every IO: they're always the same.
> 
> Suggested-by: Jens Axboe <[email protected]>
> Signed-off-by: Keith Busch <[email protected]>
> ---
>  include/linux/io_uring_types.h |  1 +
>  io_uring/io_uring.c            | 15 ++++++++++++---
>  io_uring/uring_cmd.c           |  8 +-------
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index bebab36abce89..dd192d828f463 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -228,6 +228,7 @@ struct io_ring_ctx {
>  	/* const or read-mostly hot data */
>  	struct {
>  		unsigned int		flags;
> +		unsigned int		issue_flags;
>  		unsigned int		drain_next: 1;
>  		unsigned int		restricted: 1;
>  		unsigned int		off_timeout_used: 1;
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 1d254f2c997de..a338e3660ecb8 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3975,11 +3975,20 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
>  	 * polling again, they can rely on io_sq_thread to do polling
>  	 * work, which can reduce cpu usage and uring_lock contention.
>  	 */
> -	if (ctx->flags & IORING_SETUP_IOPOLL &&
> -	    !(ctx->flags & IORING_SETUP_SQPOLL))
> -		ctx->syscall_iopoll = 1;
> +	if (ctx->flags & IORING_SETUP_IOPOLL) {
> +		ctx->issue_flags |= IO_URING_F_SQE128;
> +		if (!(ctx->flags & IORING_SETUP_SQPOLL))
> +			ctx->syscall_iopoll = 1;
> +	}

This does not look correct?

>  	ctx->compat = in_compat_syscall();
> +	if (ctx->compat)
> +		ctx->issue_flags |= IO_URING_F_COMPAT;
> +	if (ctx->flags & IORING_SETUP_SQE128)
> +		ctx->issue_flags |= IO_URING_F_SQE128;
> +	if (ctx->flags & IORING_SETUP_CQE32)
> +		ctx->issue_flags |= IO_URING_F_CQE32;
> +
>  	if (!ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
>  		ctx->user = get_uid(current_user());
>  
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 8a38b9f75d841..dbc0bfbfd0f05 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>  	if (ret)
>  		return ret;
>  
> -	if (ctx->flags & IORING_SETUP_SQE128)
> -		issue_flags |= IO_URING_F_SQE128;
> -	if (ctx->flags & IORING_SETUP_CQE32)
> -		issue_flags |= IO_URING_F_CQE32;
> -	if (ctx->compat)
> -		issue_flags |= IO_URING_F_COMPAT;
>  	if (ctx->flags & IORING_SETUP_IOPOLL) {
>  		if (!file->f_op->uring_cmd_iopoll)
>  			return -EOPNOTSUPP;
> -		issue_flags |= IO_URING_F_IOPOLL;
>  		req->iopoll_completed = 0;
>  	}
>  
> +	issue_flags |= ctx->issue_flags;
>  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>  	if (ret == -EAGAIN) {
>  		if (!req_has_async_data(req)) {

I obviously like this idea, but it should be accompanied by getting rid
of ->compat and ->syscall_iopoll in the ctx as well?

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-05 22:00 ` Jens Axboe
@ 2023-12-05 23:02   ` Keith Busch
  2023-12-05 23:11     ` Jens Axboe
  2023-12-06  1:26   ` Pavel Begunkov
  1 sibling, 1 reply; 8+ messages in thread
From: Keith Busch @ 2023-12-05 23:02 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Keith Busch, io-uring, asml.silence

On Tue, Dec 05, 2023 at 03:00:52PM -0700, Jens Axboe wrote:
> >  		if (!file->f_op->uring_cmd_iopoll)
> >  			return -EOPNOTSUPP;
> > -		issue_flags |= IO_URING_F_IOPOLL;
> >  		req->iopoll_completed = 0;
> >  	}
> >  
> > +	issue_flags |= ctx->issue_flags;
> >  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
> >  	if (ret == -EAGAIN) {
> >  		if (!req_has_async_data(req)) {
> 
> I obviously like this idea, but it should be accompanied by getting rid
> of ->compat and ->syscall_iopoll in the ctx as well?

Yeah, I considered that, and can incorporate it here. Below is a snippet
of what I had earlier to make that happen, but felt the purpose for the
"issue_flags" was uring_cmd specific and disconnected from everyone
else. Maybe I'm overthinking it...

diff --git a/io_uring/net.c b/io_uring/net.c
index 75d494dad7e2c..c11313e77495c 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -549,7 +549,7 @@ static int io_recvmsg_copy_hdr(struct io_kiocb *req,
 	iomsg->msg.msg_iter.nr_segs = 0;

 #ifdef CONFIG_COMPAT
-	if (req->ctx->compat)
+	if (req->ctx->issue_flags & IO_URING_F_COMPAT)
 		return __io_compat_recvmsg_copy_hdr(req, iomsg);
 #endif

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-05 23:02   ` Keith Busch
@ 2023-12-05 23:11     ` Jens Axboe
  2023-12-06  1:31       ` Pavel Begunkov
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2023-12-05 23:11 UTC (permalink / raw)
  To: Keith Busch; +Cc: Keith Busch, io-uring, asml.silence

On 12/5/23 4:02 PM, Keith Busch wrote:
> On Tue, Dec 05, 2023 at 03:00:52PM -0700, Jens Axboe wrote:
>>>  		if (!file->f_op->uring_cmd_iopoll)
>>>  			return -EOPNOTSUPP;
>>> -		issue_flags |= IO_URING_F_IOPOLL;
>>>  		req->iopoll_completed = 0;
>>>  	}
>>>  
>>> +	issue_flags |= ctx->issue_flags;
>>>  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>>  	if (ret == -EAGAIN) {
>>>  		if (!req_has_async_data(req)) {
>>
>> I obviously like this idea, but it should be accompanied by getting rid
>> of ->compat and ->syscall_iopoll in the ctx as well?
> 
> Yeah, I considered that, and can incorporate it here. Below is a snippet
> of what I had earlier to make that happen, but felt the purpose for the
> "issue_flags" was uring_cmd specific and disconnected from everyone
> else. Maybe I'm overthinking it...

I'd just do a patch 2 that does compat and syscall_iopoll. And then if
we ever have a new issue flags (as your other series), then it'd become
natural to add that flag too.

It's not a hard requirement, but it's somewhat ugly to have the same
state in two spots. Which is why I'd prefer if we got rid of the actual
compat/syscall_iopoll members as well, after the conversion is done.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-05 22:00 ` Jens Axboe
  2023-12-05 23:02   ` Keith Busch
@ 2023-12-06  1:26   ` Pavel Begunkov
  2023-12-06  1:41     ` Jens Axboe
  1 sibling, 1 reply; 8+ messages in thread
From: Pavel Begunkov @ 2023-12-06  1:26 UTC (permalink / raw)
  To: Jens Axboe, Keith Busch, io-uring; +Cc: Keith Busch

On 12/5/23 22:00, Jens Axboe wrote:
> On 12/5/23 2:55 PM, Keith Busch wrote:
>> From: Keith Busch <[email protected]>
>>
>> No need to rebuild the issue_flags on every IO: they're always the same.
>>
>> Suggested-by: Jens Axboe <[email protected]>
>> Signed-off-by: Keith Busch <[email protected]>
>> ---
[...]
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index 8a38b9f75d841..dbc0bfbfd0f05 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>   	if (ret)
>>   		return ret;
>>   
>> -	if (ctx->flags & IORING_SETUP_SQE128)
>> -		issue_flags |= IO_URING_F_SQE128;
>> -	if (ctx->flags & IORING_SETUP_CQE32)
>> -		issue_flags |= IO_URING_F_CQE32;
>> -	if (ctx->compat)
>> -		issue_flags |= IO_URING_F_COMPAT;
>>   	if (ctx->flags & IORING_SETUP_IOPOLL) {
>>   		if (!file->f_op->uring_cmd_iopoll)
>>   			return -EOPNOTSUPP;
>> -		issue_flags |= IO_URING_F_IOPOLL;
>>   		req->iopoll_completed = 0;
>>   	}
>>   
>> +	issue_flags |= ctx->issue_flags;
>>   	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>   	if (ret == -EAGAIN) {
>>   		if (!req_has_async_data(req)) {
> 
> I obviously like this idea, but it should be accompanied by getting rid
> of ->compat and ->syscall_iopoll in the ctx as well?

This all piggy backing cmd specific bits onto core io_uring issue_flags
business is pretty nasty. Apart from that, it mixes constant io_uring
flags and "execution context" issue_flags. And we're dancing around it
not really addressing the problem.

IMHO, cmds should be testing for IORING_SETUP flags directly via
helpers, not renaming them and abusing core io_uring flags. E.g. I had
a patch like below but didn't care enough to send:


diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 909377068a87..1a82a0633f16 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
  
  	ublk_ctrl_cmd_dump(cmd);
  
-	if (!(issue_flags & IO_URING_F_SQE128))
+	if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
  		goto out;
  
  	ret = ublk_check_cmd_op(cmd_op);
diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index d69b4038aa3e..8a18a705ff31 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
  	return cmd_to_io_kiocb(cmd)->task;
  }
  
+static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
+{
+	struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
+
+	return ctx->flags;
+}
+
  #endif /* _LINUX_IO_URING_CMD_H */

-- 
Pavel Begunkov

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-05 23:11     ` Jens Axboe
@ 2023-12-06  1:31       ` Pavel Begunkov
  0 siblings, 0 replies; 8+ messages in thread
From: Pavel Begunkov @ 2023-12-06  1:31 UTC (permalink / raw)
  To: Jens Axboe, Keith Busch; +Cc: Keith Busch, io-uring

On 12/5/23 23:11, Jens Axboe wrote:
> On 12/5/23 4:02 PM, Keith Busch wrote:
>> On Tue, Dec 05, 2023 at 03:00:52PM -0700, Jens Axboe wrote:
>>>>   		if (!file->f_op->uring_cmd_iopoll)
>>>>   			return -EOPNOTSUPP;
>>>> -		issue_flags |= IO_URING_F_IOPOLL;
>>>>   		req->iopoll_completed = 0;
>>>>   	}
>>>>   
>>>> +	issue_flags |= ctx->issue_flags;
>>>>   	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>>>   	if (ret == -EAGAIN) {
>>>>   		if (!req_has_async_data(req)) {
>>>
>>> I obviously like this idea, but it should be accompanied by getting rid
>>> of ->compat and ->syscall_iopoll in the ctx as well?
>>
>> Yeah, I considered that, and can incorporate it here. Below is a snippet
>> of what I had earlier to make that happen, but felt the purpose for the
>> "issue_flags" was uring_cmd specific and disconnected from everyone
>> else. Maybe I'm overthinking it...
> 
> I'd just do a patch 2 that does compat and syscall_iopoll. And then if

I don't understand why and how you'd get rid of syscall_iopoll,
considering it's not exposed to cmds, and if it is, then we have
a bigger problem. It's a bit sleazily defined and is not just
SETUP_IOPOLL.

> we ever have a new issue flags (as your other series), then it'd become
> natural to add that flag too.
> 
> It's not a hard requirement, but it's somewhat ugly to have the same
> state in two spots. Which is why I'd prefer if we got rid of the actual
> compat/syscall_iopoll members as well, after the conversion is done.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-06  1:26   ` Pavel Begunkov
@ 2023-12-06  1:41     ` Jens Axboe
  2023-12-06  1:51       ` Pavel Begunkov
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2023-12-06  1:41 UTC (permalink / raw)
  To: Pavel Begunkov, Keith Busch, io-uring; +Cc: Keith Busch

On 12/5/23 6:26 PM, Pavel Begunkov wrote:
> On 12/5/23 22:00, Jens Axboe wrote:
>> On 12/5/23 2:55 PM, Keith Busch wrote:
>>> From: Keith Busch <[email protected]>
>>>
>>> No need to rebuild the issue_flags on every IO: they're always the same.
>>>
>>> Suggested-by: Jens Axboe <[email protected]>
>>> Signed-off-by: Keith Busch <[email protected]>
>>> ---
> [...]
>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>> index 8a38b9f75d841..dbc0bfbfd0f05 100644
>>> --- a/io_uring/uring_cmd.c
>>> +++ b/io_uring/uring_cmd.c
>>> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>       if (ret)
>>>           return ret;
>>>   -    if (ctx->flags & IORING_SETUP_SQE128)
>>> -        issue_flags |= IO_URING_F_SQE128;
>>> -    if (ctx->flags & IORING_SETUP_CQE32)
>>> -        issue_flags |= IO_URING_F_CQE32;
>>> -    if (ctx->compat)
>>> -        issue_flags |= IO_URING_F_COMPAT;
>>>       if (ctx->flags & IORING_SETUP_IOPOLL) {
>>>           if (!file->f_op->uring_cmd_iopoll)
>>>               return -EOPNOTSUPP;
>>> -        issue_flags |= IO_URING_F_IOPOLL;
>>>           req->iopoll_completed = 0;
>>>       }
>>>   +    issue_flags |= ctx->issue_flags;
>>>       ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>>       if (ret == -EAGAIN) {
>>>           if (!req_has_async_data(req)) {
>>
>> I obviously like this idea, but it should be accompanied by getting rid
>> of ->compat and ->syscall_iopoll in the ctx as well?
> 
> This all piggy backing cmd specific bits onto core io_uring issue_flags
> business is pretty nasty. Apart from that, it mixes constant io_uring
> flags and "execution context" issue_flags. And we're dancing around it
> not really addressing the problem.
> 
> IMHO, cmds should be testing for IORING_SETUP flags directly via
> helpers, not renaming them and abusing core io_uring flags. E.g. I had
> a patch like below but didn't care enough to send:
> 
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 909377068a87..1a82a0633f16 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
>  
>      ublk_ctrl_cmd_dump(cmd);
>  
> -    if (!(issue_flags & IO_URING_F_SQE128))
> +    if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
>          goto out;
>  
>      ret = ublk_check_cmd_op(cmd_op);
> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> index d69b4038aa3e..8a18a705ff31 100644
> --- a/include/linux/io_uring/cmd.h
> +++ b/include/linux/io_uring/cmd.h
> @@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
>      return cmd_to_io_kiocb(cmd)->task;
>  }
>  
> +static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
> +{
> +    struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
> +
> +    return ctx->flags;
> +}
> +
>  #endif /* _LINUX_IO_URING_CMD_H */

Yeah this is fine too, I just don't like our current scheme of having to
mirror state in issue flags. Consolidating one way or another would be
really nice.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_uring: save repeated issue_flags
  2023-12-06  1:41     ` Jens Axboe
@ 2023-12-06  1:51       ` Pavel Begunkov
  0 siblings, 0 replies; 8+ messages in thread
From: Pavel Begunkov @ 2023-12-06  1:51 UTC (permalink / raw)
  To: Jens Axboe, Keith Busch, io-uring; +Cc: Keith Busch

On 12/6/23 01:41, Jens Axboe wrote:
> On 12/5/23 6:26 PM, Pavel Begunkov wrote:
>> On 12/5/23 22:00, Jens Axboe wrote:
>>> On 12/5/23 2:55 PM, Keith Busch wrote:
>>>> From: Keith Busch <[email protected]>
>>>>
>>>> No need to rebuild the issue_flags on every IO: they're always the same.
>>>>
>>>> Suggested-by: Jens Axboe <[email protected]>
>>>> Signed-off-by: Keith Busch <[email protected]>
>>>> ---
>> [...]
>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>> index 8a38b9f75d841..dbc0bfbfd0f05 100644
>>>> --- a/io_uring/uring_cmd.c
>>>> +++ b/io_uring/uring_cmd.c
>>>> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>        if (ret)
>>>>            return ret;
>>>>    -    if (ctx->flags & IORING_SETUP_SQE128)
>>>> -        issue_flags |= IO_URING_F_SQE128;
>>>> -    if (ctx->flags & IORING_SETUP_CQE32)
>>>> -        issue_flags |= IO_URING_F_CQE32;
>>>> -    if (ctx->compat)
>>>> -        issue_flags |= IO_URING_F_COMPAT;
>>>>        if (ctx->flags & IORING_SETUP_IOPOLL) {
>>>>            if (!file->f_op->uring_cmd_iopoll)
>>>>                return -EOPNOTSUPP;
>>>> -        issue_flags |= IO_URING_F_IOPOLL;
>>>>            req->iopoll_completed = 0;
>>>>        }
>>>>    +    issue_flags |= ctx->issue_flags;
>>>>        ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>>>        if (ret == -EAGAIN) {
>>>>            if (!req_has_async_data(req)) {
>>>
>>> I obviously like this idea, but it should be accompanied by getting rid
>>> of ->compat and ->syscall_iopoll in the ctx as well?
>>
>> This all piggy backing cmd specific bits onto core io_uring issue_flags
>> business is pretty nasty. Apart from that, it mixes constant io_uring
>> flags and "execution context" issue_flags. And we're dancing around it
>> not really addressing the problem.
>>
>> IMHO, cmds should be testing for IORING_SETUP flags directly via
>> helpers, not renaming them and abusing core io_uring flags. E.g. I had
>> a patch like below but didn't care enough to send:
>>
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 909377068a87..1a82a0633f16 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
>>   
>>       ublk_ctrl_cmd_dump(cmd);
>>   
>> -    if (!(issue_flags & IO_URING_F_SQE128))
>> +    if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
>>           goto out;
>>   
>>       ret = ublk_check_cmd_op(cmd_op);
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index d69b4038aa3e..8a18a705ff31 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
>>       return cmd_to_io_kiocb(cmd)->task;
>>   }
>>   
>> +static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
>> +{
>> +    struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
>> +
>> +    return ctx->flags;
>> +}
>> +
>>   #endif /* _LINUX_IO_URING_CMD_H */
> 
> Yeah this is fine too, I just don't like our current scheme of having to
> mirror state in issue flags. Consolidating one way or another would be
> really nice.

Just hiding ->compat into the cache won't help it, most of the cmd
flags mirror IORING_SETUP_*, so unless it checks IORING_SETUP_* directly
there will be this duplication.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-12-06  1:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-05 21:55 [PATCH] io_uring: save repeated issue_flags Keith Busch
2023-12-05 22:00 ` Jens Axboe
2023-12-05 23:02   ` Keith Busch
2023-12-05 23:11     ` Jens Axboe
2023-12-06  1:31       ` Pavel Begunkov
2023-12-06  1:26   ` Pavel Begunkov
2023-12-06  1:41     ` Jens Axboe
2023-12-06  1:51       ` Pavel Begunkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox