| Message ID | 20250325143943.1226467-1-csander@purestorage.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] io_uring/net: use REQ_F_IMPORT_BUFFER for send_zc | expand |
On 3/25/25 14:39, 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. It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise looks good. Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> > Suggested-by: Pavel Begunkov <asml.silence@gmail.com> Note for the future, it's a good practice to put your sob last.
On Wed, Mar 26, 2025 at 2:59 AM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 3/25/25 14:39, 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. > > It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise > looks good. It looks like Jens dropped my earlier patch "io_uring/net: import send_zc fixed buffer before going async": https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u . Not sure why it was dropped. But this change is independent, I can rebase it onto the current for-6.15/io_uring-reg-vec if desired. > > Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> Thanks! > > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> > > Suggested-by: Pavel Begunkov <asml.silence@gmail.com> > > Note for the future, it's a good practice to put your sob last. Okay. Is the preferred order of tags documented anywhere? I ran scripts/checkpatch.pl, but it didn't have any complaints. Best, Caleb
On 3/26/25 11:01 AM, Caleb Sander Mateos wrote: > On Wed, Mar 26, 2025 at 2:59?AM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 3/25/25 14:39, 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. >> >> It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise >> looks good. > > It looks like Jens dropped my earlier patch "io_uring/net: import > send_zc fixed buffer before going async": > https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u > . > Not sure why it was dropped. But this change is independent, I can > rebase it onto the current for-6.15/io_uring-reg-vec if desired. Mostly just around the discussion on what we want to guarantee here. I do think that patch makes sense, fwiw! >> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > > Thanks! > >> >>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> >>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com> >> >> Note for the future, it's a good practice to put your sob last. > > Okay. Is the preferred order of tags documented anywhere? I ran > scripts/checkpatch.pl, but it didn't have any complaints. I think that one is minor, as it's not reordering with another SOB. Eg mine would go below it anyway. But you definitely should always include a list of what changed since v1 when posting v2, and so forth. Otherwise you need to find the old patch and compare them to see what changed. Just put it below the --- line in the email.
On Tue, 25 Mar 2025 08:39:42 -0600, 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. > > Applied, thanks! [1/1] io_uring/net: use REQ_F_IMPORT_BUFFER for send_zc commit: 73b6dacb1c6feae8ca4a6ff120848430aeb57fbd Best regards,
On Wed, Mar 26, 2025 at 10:05 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 3/26/25 11:01 AM, Caleb Sander Mateos wrote: > > On Wed, Mar 26, 2025 at 2:59?AM Pavel Begunkov <asml.silence@gmail.com> wrote: > >> > >> On 3/25/25 14:39, 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. > >> > >> It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise > >> looks good. > > > > It looks like Jens dropped my earlier patch "io_uring/net: import > > send_zc fixed buffer before going async": > > https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u > > . > > Not sure why it was dropped. But this change is independent, I can > > rebase it onto the current for-6.15/io_uring-reg-vec if desired. > > Mostly just around the discussion on what we want to guarantee here. I > do think that patch makes sense, fwiw! I hope the approach I took for the revised NVMe passthru patch [1] is an acceptable compromise: the order in which io_uring issues operations isn't guaranteed, but userspace may opportunistically submit operations in parallel with a fallback path in case of failure. Viewed this way, I think it makes sense for the kernel to allow the operation using the fixed buffer to succeed even if it goes async, provided that it doesn't impose any burden on the io_uring implementation. I dropped the "Fixes" tag and added a paragraph to the commit message clarifying that io_uring doesn't guarantee this behavior, it's just an optimization. [1]: https://lore.kernel.org/io-uring/20250324200540.910962-4-csander@purestorage.com/T/#u > > >> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > > > > Thanks! > > > >> > >>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> > >>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com> > >> > >> Note for the future, it's a good practice to put your sob last. > > > > Okay. Is the preferred order of tags documented anywhere? I ran > > scripts/checkpatch.pl, but it didn't have any complaints. > > I think that one is minor, as it's not reordering with another SOB. Eg > mine would go below it anyway. But you definitely should always include > a list of what changed since v1 when posting v2, and so forth. Otherwise > you need to find the old patch and compare them to see what changed. > Just put it below the --- line in the email. Sorry about that, it slipped my mind. Will try to remember next time! Thanks, Caleb
On 3/26/25 11:23 AM, Caleb Sander Mateos wrote: > On Wed, Mar 26, 2025 at 10:05?AM Jens Axboe <axboe@kernel.dk> wrote: >> >> On 3/26/25 11:01 AM, Caleb Sander Mateos wrote: >>> On Wed, Mar 26, 2025 at 2:59?AM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>> >>>> On 3/25/25 14:39, 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. >>>> >>>> It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise >>>> looks good. >>> >>> It looks like Jens dropped my earlier patch "io_uring/net: import >>> send_zc fixed buffer before going async": >>> https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u >>> . >>> Not sure why it was dropped. But this change is independent, I can >>> rebase it onto the current for-6.15/io_uring-reg-vec if desired. >> >> Mostly just around the discussion on what we want to guarantee here. I >> do think that patch makes sense, fwiw! > > I hope the approach I took for the revised NVMe passthru patch [1] is > an acceptable compromise: the order in which io_uring issues > operations isn't guaranteed, but userspace may opportunistically > submit operations in parallel with a fallback path in case of failure. > Viewed this way, I think it makes sense for the kernel to allow the > operation using the fixed buffer to succeed even if it goes async, > provided that it doesn't impose any burden on the io_uring > implementation. I dropped the "Fixes" tag and added a paragraph to the > commit message clarifying that io_uring doesn't guarantee this > behavior, it's just an optimization. > > [1]: https://lore.kernel.org/io-uring/20250324200540.910962-4-csander@purestorage.com/T/#u It is, I already signed off on that one, I think it's just waiting for Keith to get queued up. Always a bit tricky during the merge window, particularly when it ends up depending on multiple branches. But should go in for 6.15. When you have time, resending the net one would be useful. I do think that one makes sense too.
On 3/26/25 17:05, Jens Axboe wrote: > On 3/26/25 11:01 AM, Caleb Sander Mateos wrote: >> On Wed, Mar 26, 2025 at 2:59?AM Pavel Begunkov <asml.silence@gmail.com> wrote: >>> >>> On 3/25/25 14:39, 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. >>> >>> It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise >>> looks good. >> >> It looks like Jens dropped my earlier patch "io_uring/net: import >> send_zc fixed buffer before going async": >> https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u >> . >> Not sure why it was dropped. But this change is independent, I can >> rebase it onto the current for-6.15/io_uring-reg-vec if desired. > > Mostly just around the discussion on what we want to guarantee here. I > do think that patch makes sense, fwiw! > >>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> >> >> Thanks! >> >>> >>>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> >>>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com> >>> >>> Note for the future, it's a good practice to put your sob last. >> >> Okay. Is the preferred order of tags documented anywhere? I ran >> scripts/checkpatch.pl, but it didn't have any complaints. > > I think that one is minor, as it's not reordering with another SOB. Eg > mine would go below it anyway. But you definitely should always include > a list of what changed since v1 when posting v2, and so forth. Otherwise > you need to find the old patch and compare them to see what changed. > Just put it below the --- line in the email. Minor, yes. But to answer why, because it's normally chronological. By default I read it as Suggested-by was added later and not by the patch author, which nobody cares too much about, but that's why Jens mentions ordering b/w sob of other people.
On 3/26/25 17:31, Jens Axboe wrote: > On 3/26/25 11:23 AM, Caleb Sander Mateos wrote: >> On Wed, Mar 26, 2025 at 10:05?AM Jens Axboe <axboe@kernel.dk> wrote: >>> >>> On 3/26/25 11:01 AM, Caleb Sander Mateos wrote: >>>> On Wed, Mar 26, 2025 at 2:59?AM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>>> >>>>> On 3/25/25 14:39, 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. >>>>> >>>>> It didn't apply cleanly to for-6.15/io_uring-reg-vec, but otherwise >>>>> looks good. >>>> >>>> It looks like Jens dropped my earlier patch "io_uring/net: import >>>> send_zc fixed buffer before going async": >>>> https://lore.kernel.org/io-uring/20250321184819.3847386-3-csander@purestorage.com/T/#u >>>> . >>>> Not sure why it was dropped. But this change is independent, I can >>>> rebase it onto the current for-6.15/io_uring-reg-vec if desired. >>> >>> Mostly just around the discussion on what we want to guarantee here. I >>> do think that patch makes sense, fwiw! >> >> I hope the approach I took for the revised NVMe passthru patch [1] is >> an acceptable compromise: the order in which io_uring issues >> operations isn't guaranteed, but userspace may opportunistically >> submit operations in parallel with a fallback path in case of failure. >> Viewed this way, I think it makes sense for the kernel to allow the >> operation using the fixed buffer to succeed even if it goes async, >> provided that it doesn't impose any burden on the io_uring >> implementation. I dropped the "Fixes" tag and added a paragraph to the >> commit message clarifying that io_uring doesn't guarantee this >> behavior, it's just an optimization. >> >> [1]: https://lore.kernel.org/io-uring/20250324200540.910962-4-csander@purestorage.com/T/#u > > It is, I already signed off on that one, I think it's just waiting for > Keith to get queued up. Always a bit tricky during the merge window, > particularly when it ends up depending on multiple branches. But should > go in for 6.15. > > When you have time, resending the net one would be useful. I do think > that one makes sense too. If that's about "io_uring/net: import send_zc fixed buffer before going async" please don't, because the next second you'll be arguing that it's a regression to change it and so it's essentially uapi, and we end up with 2 step prep with semantics nobody ever will be able to sanely describe, not without listing all the cases where it can fail.
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..a7b3e2688689 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,11 +1304,10 @@ 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; if (unlikely(READ_ONCE(sqe->__pad2[0]) || READ_ONCE(sqe->addr3))) return -EINVAL; /* we don't support IOSQE_CQE_SKIP_SUCCESS just yet */ @@ -1351,12 +1349,14 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (io_is_compat(req->ctx)) zc->msg_flags |= MSG_CMSG_COMPAT; if (unlikely(!io_msg_alloc_async(req))) return -ENOMEM; - if (req->opcode != IORING_OP_SENDMSG_ZC) + if (req->opcode == IORING_OP_SEND_ZC) { + req->flags |= REQ_F_IMPORT_BUFFER; return io_send_setup(req, sqe); + } return io_sendmsg_zc_setup(req, sqe); } static int io_sg_from_iter_iovec(struct sk_buff *skb, struct iov_iter *from, size_t length) @@ -1447,12 +1447,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 | 10 +++++----- 2 files changed, 9 insertions(+), 6 deletions(-)