| Message ID | 20231205215553.2954630-1-kbusch@meta.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: save repeated issue_flags | expand |
On 12/5/23 2:55 PM, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > No need to rebuild the issue_flags on every IO: they're always the same. > > Suggested-by: Jens Axboe <axboe@kernel.dk> > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > 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?
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
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.
On 12/5/23 22:00, Jens Axboe wrote: > On 12/5/23 2:55 PM, Keith Busch wrote: >> From: Keith Busch <kbusch@kernel.org> >> >> No need to rebuild the issue_flags on every IO: they're always the same. >> >> Suggested-by: Jens Axboe <axboe@kernel.dk> >> Signed-off-by: Keith Busch <kbusch@kernel.org> >> --- [...] >> 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 */
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.
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 <kbusch@kernel.org> >>> >>> No need to rebuild the issue_flags on every IO: they're always the same. >>> >>> Suggested-by: Jens Axboe <axboe@kernel.dk> >>> Signed-off-by: Keith Busch <kbusch@kernel.org> >>> --- > [...] >>> 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.
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 <kbusch@kernel.org> >>>> >>>> No need to rebuild the issue_flags on every IO: they're always the same. >>>> >>>> Suggested-by: Jens Axboe <axboe@kernel.dk> >>>> Signed-off-by: Keith Busch <kbusch@kernel.org> >>>> --- >> [...] >>>> 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.
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)) {