| Message ID | 20250311114053.216359-2-sidong.yang@furiosa.ai (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce io_uring_cmd_import_fixed_vec | expand |
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 > + 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. > + 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.
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 >
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, + struct iou_vec *iou_vec, bool compat, + 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); + + ret = io_vec_realloc(iou_vec, nr_segs); + 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, + issue_flags); + return ret; +} +EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed_vec); + void io_uring_cmd_issue_blocking(struct io_uring_cmd *ioucmd) { struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
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(+)