On Tue, Mar 11, 2025 at 01:08:14PM +0000, Pavel Begunkov wrote:
> On 3/11/25 11:40, Sidong Yang wrote:
> > io_uring_cmd_import_fixed_vec() could be used for using multiple
> > fixed buffer in uring_cmd callback.
> >
> > Signed-off-by: Sidong Yang <sidong.yang@xxxxxxxxxx>
> > ---
> > include/linux/io_uring/cmd.h | 14 ++++++++++++++
> > io_uring/uring_cmd.c | 29 +++++++++++++++++++++++++++++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> > index 598cacda4aa3..75cf25c1e730 100644
> > --- a/include/linux/io_uring/cmd.h
> > +++ b/include/linux/io_uring/cmd.h
> > @@ -44,6 +44,13 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
> > struct io_uring_cmd *ioucmd,
> > unsigned int issue_flags);
> > +int io_uring_cmd_import_fixed_vec(const struct iovec __user *uiovec,
> > + unsigned long nr_segs, int rw,
> > + struct iov_iter *iter,
> > + struct io_uring_cmd *ioucmd,
>
> nit: it's better to be the first arg
Thanks for tip.
>
> > + struct iou_vec *iou_vec, bool compat,
>
> Same comment, iou_vec should not be exposed. And why do we
> need to pass compat here? Instead of io_is_compat() inside
> the helper.
Actually I don't know about io_is_compat(). Thanks.
>
> > + unsigned int issue_flags);
> > +
> > /*
> > * Completes the request, i.e. posts an io_uring CQE and deallocates @ioucmd
> > * and the corresponding io_uring request.
> > @@ -76,6 +83,13 @@ io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
> > {
> > return -EOPNOTSUPP;
> > }
> > +int io_uring_cmd_import_fixed_vec(int rw, struct iov_iter *iter,
> > + struct io_uring_cmd *ioucmd,
> > + struct iou_vec *vec, unsigned nr_iovs,
> > + unsigned iovec_off, unsigned int issue_flags)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > static inline void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret,
> > u64 ret2, unsigned issue_flags)
> > {
> > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > index de39b602aa82..58e2932f29e7 100644
> > --- a/io_uring/uring_cmd.c
> > +++ b/io_uring/uring_cmd.c
> > @@ -255,6 +255,35 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
> > }
> > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed);
> > +int io_uring_cmd_import_fixed_vec(const struct iovec __user *uiovec,
> > + unsigned long nr_segs, int rw,
> > + struct iov_iter *iter,
> > + struct io_uring_cmd *ioucmd,
> > + struct iou_vec *iou_vec, bool compat,
> > + unsigned int issue_flags)
> > +{
> > + struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
> > + struct iovec *iov;
> > + int ret;
> > +
> > + iov = iovec_from_user(uiovec, nr_segs, 0, NULL, compat);
> > + if (IS_ERR(iov))
> > + return PTR_ERR(iov);
>
> That's one allocation
>
> > +
> > + ret = io_vec_realloc(iou_vec, nr_segs);
>
> That's a second one
>
> > + if (ret) {
> > + kfree(iov);
> > + return ret;
> > + }
> > + memcpy(iou_vec->iovec, iov, sizeof(*iov) * nr_segs);
> > + kfree(iov);
> > +
> > + ret = io_import_reg_vec(rw, iter, req, iou_vec, iou_vec->nr, 0,
>
> It's slightly out of date, the import side should use io_prep_reg_iovec(),
> it abstracts from iovec placement questions.
>
> > + issue_flags);
>
> And there will likely be a 3rd one. That's pretty likely why
> performance is not up to expectations, unlike the rw/net
> side which cache it to eventually 0 realloctions.
>
> The first one can be easily removed, but it'll need better
> abstractions for cmds not to expose iou_vec. Let me think
> what would be a good approach here.
Totally agreed, There is too many allocation for this. It should be done
allocation.
Thanks,
Sidong
>
> --
> Pavel Begunkov
>
