Re: [RFC PATCH 1/2] io_uring: cmd: introduce io_uring_cmd_import_fixed_vec

[Sidong Yang <sidong.yang@furiosa.ai>]

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@furiosa.ai>
> > ---
> >   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
>