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