| Message ID | 20250324151123.726124-1-csander@purestorage.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring/net: use REQ_F_IMPORT_BUFFER for send_zc | expand |
On 3/24/25 15:11, Caleb Sander Mateos wrote: > Instead of a bool field in struct io_sr_msg, use REQ_F_IMPORT_BUFFER to > track whether io_send_zc() has already imported the buffer. This flag > already serves a similar purpose for sendmsg_zc and {read,write}v_fixed. > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> > Suggested-by: Pavel Begunkov <asml.silence@gmail.com> > --- > include/linux/io_uring_types.h | 5 ++++- > io_uring/net.c | 8 +++----- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h > index c17d2eedf478..699e2c0895ae 100644 > --- a/include/linux/io_uring_types.h > +++ b/include/linux/io_uring_types.h > @@ -583,11 +583,14 @@ enum { > REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT), > /* buf node is valid */ > REQ_F_BUF_NODE = IO_REQ_FLAG(REQ_F_BUF_NODE_BIT), > /* request has read/write metadata assigned */ > REQ_F_HAS_METADATA = IO_REQ_FLAG(REQ_F_HAS_METADATA_BIT), > - /* resolve padded iovec to registered buffers */ > + /* > + * For vectored fixed buffers, resolve iovec to registered buffers. > + * For SEND_ZC, whether to import buffers (i.e. the first issue). > + */ > REQ_F_IMPORT_BUFFER = IO_REQ_FLAG(REQ_F_IMPORT_BUFFER_BIT), > }; > > typedef void (*io_req_tw_func_t)(struct io_kiocb *req, io_tw_token_t tw); > > diff --git a/io_uring/net.c b/io_uring/net.c > index c87af980b98e..b221abe2600e 100644 > --- a/io_uring/net.c > +++ b/io_uring/net.c > @@ -75,11 +75,10 @@ struct io_sr_msg { > unsigned nr_multishot_loops; > u16 flags; > /* initialised and used only by !msg send variants */ > u16 buf_group; > bool retry; > - bool imported; /* only for io_send_zc */ > void __user *msg_control; > /* used only for send zerocopy */ > struct io_kiocb *notif; > }; > > @@ -1305,12 +1304,11 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > struct io_ring_ctx *ctx = req->ctx; > struct io_kiocb *notif; > > zc->done_io = 0; > zc->retry = false; > - zc->imported = false; > - req->flags |= REQ_F_POLL_NO_LAZY; > + req->flags |= REQ_F_POLL_NO_LAZY | REQ_F_IMPORT_BUFFER; This function is shared with sendmsg_zc, so if we set it here, it'll trigger io_import_reg_vec() in io_sendmsg_zc() even for non register buffer request. > > if (unlikely(READ_ONCE(sqe->__pad2[0]) || READ_ONCE(sqe->addr3))) > return -EINVAL; > /* we don't support IOSQE_CQE_SKIP_SUCCESS just yet */ > if (req->flags & REQ_F_CQE_SKIP) > @@ -1447,12 +1445,12 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags) > if (unlikely(!sock)) > return -ENOTSOCK; > if (!test_bit(SOCK_SUPPORT_ZC, &sock->flags)) > return -EOPNOTSUPP; > > - if (!zc->imported) { > - zc->imported = true; > + if (req->flags & REQ_F_IMPORT_BUFFER) { > + req->flags &= ~REQ_F_IMPORT_BUFFER; > ret = io_send_zc_import(req, issue_flags); > if (unlikely(ret)) > return ret; > } >
On Tue, Mar 25, 2025 at 6:30 AM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 3/24/25 15:11, Caleb Sander Mateos wrote: > > Instead of a bool field in struct io_sr_msg, use REQ_F_IMPORT_BUFFER to > > track whether io_send_zc() has already imported the buffer. This flag > > already serves a similar purpose for sendmsg_zc and {read,write}v_fixed. > > > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> > > Suggested-by: Pavel Begunkov <asml.silence@gmail.com> > > --- > > include/linux/io_uring_types.h | 5 ++++- > > io_uring/net.c | 8 +++----- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h > > index c17d2eedf478..699e2c0895ae 100644 > > --- a/include/linux/io_uring_types.h > > +++ b/include/linux/io_uring_types.h > > @@ -583,11 +583,14 @@ enum { > > REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT), > > /* buf node is valid */ > > REQ_F_BUF_NODE = IO_REQ_FLAG(REQ_F_BUF_NODE_BIT), > > /* request has read/write metadata assigned */ > > REQ_F_HAS_METADATA = IO_REQ_FLAG(REQ_F_HAS_METADATA_BIT), > > - /* resolve padded iovec to registered buffers */ > > + /* > > + * For vectored fixed buffers, resolve iovec to registered buffers. > > + * For SEND_ZC, whether to import buffers (i.e. the first issue). > > + */ > > REQ_F_IMPORT_BUFFER = IO_REQ_FLAG(REQ_F_IMPORT_BUFFER_BIT), > > }; > > > > typedef void (*io_req_tw_func_t)(struct io_kiocb *req, io_tw_token_t tw); > > > > diff --git a/io_uring/net.c b/io_uring/net.c > > index c87af980b98e..b221abe2600e 100644 > > --- a/io_uring/net.c > > +++ b/io_uring/net.c > > @@ -75,11 +75,10 @@ struct io_sr_msg { > > unsigned nr_multishot_loops; > > u16 flags; > > /* initialised and used only by !msg send variants */ > > u16 buf_group; > > bool retry; > > - bool imported; /* only for io_send_zc */ > > void __user *msg_control; > > /* used only for send zerocopy */ > > struct io_kiocb *notif; > > }; > > > > @@ -1305,12 +1304,11 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > > struct io_ring_ctx *ctx = req->ctx; > > struct io_kiocb *notif; > > > > zc->done_io = 0; > > zc->retry = false; > > - zc->imported = false; > > - req->flags |= REQ_F_POLL_NO_LAZY; > > + req->flags |= REQ_F_POLL_NO_LAZY | REQ_F_IMPORT_BUFFER; > > This function is shared with sendmsg_zc, so if we set it here, > it'll trigger io_import_reg_vec() in io_sendmsg_zc() even for > non register buffer request. Good catch. I keep forgetting which prep and issue functions are shared between which opcodes. Thanks, Caleb
On 3/25/25 14:39, Caleb Sander Mateos wrote: > On Tue, Mar 25, 2025 at 6:30 AM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 3/24/25 15:11, Caleb Sander Mateos wrote: >>> Instead of a bool field in struct io_sr_msg, use REQ_F_IMPORT_BUFFER to >>> track whether io_send_zc() has already imported the buffer. This flag >>> already serves a similar purpose for sendmsg_zc and {read,write}v_fixed. >>> >>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> >>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> include/linux/io_uring_types.h | 5 ++++- >>> io_uring/net.c | 8 +++----- >>> 2 files changed, 7 insertions(+), 6 deletions(-) ... >>> @@ -1305,12 +1304,11 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> struct io_ring_ctx *ctx = req->ctx; >>> struct io_kiocb *notif; >>> >>> zc->done_io = 0; >>> zc->retry = false; >>> - zc->imported = false; >>> - req->flags |= REQ_F_POLL_NO_LAZY; >>> + req->flags |= REQ_F_POLL_NO_LAZY | REQ_F_IMPORT_BUFFER; >> >> This function is shared with sendmsg_zc, so if we set it here, >> it'll trigger io_import_reg_vec() in io_sendmsg_zc() even for >> non register buffer request. > > Good catch. I keep forgetting which prep and issue functions are > shared between which opcodes. No worries, can happen. I'd recommend to run liburing tests, they're very useful for catching such things. I run it out of curiosity, it crashes on send-zerocopy.c pretty fast.
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h index c17d2eedf478..699e2c0895ae 100644 --- a/include/linux/io_uring_types.h +++ b/include/linux/io_uring_types.h @@ -583,11 +583,14 @@ enum { REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT), /* buf node is valid */ REQ_F_BUF_NODE = IO_REQ_FLAG(REQ_F_BUF_NODE_BIT), /* request has read/write metadata assigned */ REQ_F_HAS_METADATA = IO_REQ_FLAG(REQ_F_HAS_METADATA_BIT), - /* resolve padded iovec to registered buffers */ + /* + * For vectored fixed buffers, resolve iovec to registered buffers. + * For SEND_ZC, whether to import buffers (i.e. the first issue). + */ REQ_F_IMPORT_BUFFER = IO_REQ_FLAG(REQ_F_IMPORT_BUFFER_BIT), }; typedef void (*io_req_tw_func_t)(struct io_kiocb *req, io_tw_token_t tw); diff --git a/io_uring/net.c b/io_uring/net.c index c87af980b98e..b221abe2600e 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -75,11 +75,10 @@ struct io_sr_msg { unsigned nr_multishot_loops; u16 flags; /* initialised and used only by !msg send variants */ u16 buf_group; bool retry; - bool imported; /* only for io_send_zc */ void __user *msg_control; /* used only for send zerocopy */ struct io_kiocb *notif; }; @@ -1305,12 +1304,11 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) struct io_ring_ctx *ctx = req->ctx; struct io_kiocb *notif; zc->done_io = 0; zc->retry = false; - zc->imported = false; - req->flags |= REQ_F_POLL_NO_LAZY; + req->flags |= REQ_F_POLL_NO_LAZY | REQ_F_IMPORT_BUFFER; if (unlikely(READ_ONCE(sqe->__pad2[0]) || READ_ONCE(sqe->addr3))) return -EINVAL; /* we don't support IOSQE_CQE_SKIP_SUCCESS just yet */ if (req->flags & REQ_F_CQE_SKIP) @@ -1447,12 +1445,12 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags) if (unlikely(!sock)) return -ENOTSOCK; if (!test_bit(SOCK_SUPPORT_ZC, &sock->flags)) return -EOPNOTSUPP; - if (!zc->imported) { - zc->imported = true; + if (req->flags & REQ_F_IMPORT_BUFFER) { + req->flags &= ~REQ_F_IMPORT_BUFFER; ret = io_send_zc_import(req, issue_flags); if (unlikely(ret)) return ret; }
Instead of a bool field in struct io_sr_msg, use REQ_F_IMPORT_BUFFER to track whether io_send_zc() has already imported the buffer. This flag already serves a similar purpose for sendmsg_zc and {read,write}v_fixed. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Suggested-by: Pavel Begunkov <asml.silence@gmail.com> --- include/linux/io_uring_types.h | 5 ++++- io_uring/net.c | 8 +++----- 2 files changed, 7 insertions(+), 6 deletions(-)