* [PATCH] io_uring/net: cache provided buffer group value for multishot receives
@ 2023-01-22 17:13 Jens Axboe
2023-01-23 14:04 ` Dylan Yudaken
0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2023-01-22 17:13 UTC (permalink / raw)
To: io-uring; +Cc: Dylan Yudaken
If we're using ring provided buffers with multishot receive, and we end
up doing an io-wq based issue at some points that also needs to select
a buffer, we'll lose the initially assigned buffer group as
io_ring_buffer_select() correctly clears the buffer group list as the
issue isn't serialized by the ctx uring_lock. This is fine for normal
receives as the request puts the buffer and finishes, but for multishot,
we will re-arm and do further receives. On the next trigger for this
multishot receive, the receive will try and pick from a buffer group
whose value is the same as the buffer ID of the las receive. That is
obviously incorrect, and will result in a premature -ENOUFS error for
the receive even if we had available buffers in the correct group.
Cache the buffer group value at prep time, so we can restore it for
future receives. This only needs doing for the above mentioned case, but
just do it by default to keep it easier to read.
Cc: [email protected]
Fixes: b3fdea6ecb55 ("io_uring: multishot recv")
Fixes: 9bb66906f23e ("io_uring: support multishot in recvmsg")
Cc: Dylan Yudaken <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
diff --git a/io_uring/net.c b/io_uring/net.c
index fbc34a7c2743..07a6aa39ab6f 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -62,6 +62,7 @@ struct io_sr_msg {
u16 flags;
/* initialised and used only by !msg send variants */
u16 addr_len;
+ u16 buf_group;
void __user *addr;
/* used only for send zerocopy */
struct io_kiocb *notif;
@@ -580,6 +581,15 @@ int io_recvmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
if (req->opcode == IORING_OP_RECV && sr->len)
return -EINVAL;
req->flags |= REQ_F_APOLL_MULTISHOT;
+ /*
+ * Store the buffer group for this multishot receive separately,
+ * as if we end up doing an io-wq based issue that selects a
+ * buffer, it has to be committed immediately and that will
+ * clear ->buf_list. This means we lose the link to the buffer
+ * list, and the eventual buffer put on completion then cannot
+ * restore it.
+ */
+ sr->buf_group = req->buf_index;
}
#ifdef CONFIG_COMPAT
@@ -816,8 +826,10 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags)
if (kmsg->msg.msg_inq)
cflags |= IORING_CQE_F_SOCK_NONEMPTY;
- if (!io_recv_finish(req, &ret, cflags, mshot_finished, issue_flags))
+ if (!io_recv_finish(req, &ret, cflags, mshot_finished, issue_flags)) {
+ req->buf_index = sr->buf_group;
goto retry_multishot;
+ }
if (mshot_finished) {
/* fast path, check for non-NULL to avoid function call */
@@ -918,8 +930,10 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
if (msg.msg_inq)
cflags |= IORING_CQE_F_SOCK_NONEMPTY;
- if (!io_recv_finish(req, &ret, cflags, ret <= 0, issue_flags))
+ if (!io_recv_finish(req, &ret, cflags, ret <= 0, issue_flags)) {
+ req->buf_index = sr->buf_group;
goto retry_multishot;
+ }
return ret;
}
--
Jens Axboe
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] io_uring/net: cache provided buffer group value for multishot receives
2023-01-22 17:13 [PATCH] io_uring/net: cache provided buffer group value for multishot receives Jens Axboe
@ 2023-01-23 14:04 ` Dylan Yudaken
2023-01-23 14:06 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Dylan Yudaken @ 2023-01-23 14:04 UTC (permalink / raw)
To: [email protected], [email protected]
On Sun, 2023-01-22 at 10:13 -0700, Jens Axboe wrote:
> If we're using ring provided buffers with multishot receive, and we
> end
> up doing an io-wq based issue at some points that also needs to
> select
> a buffer, we'll lose the initially assigned buffer group as
> io_ring_buffer_select() correctly clears the buffer group list as the
> issue isn't serialized by the ctx uring_lock. This is fine for normal
> receives as the request puts the buffer and finishes, but for
> multishot,
> we will re-arm and do further receives. On the next trigger for this
> multishot receive, the receive will try and pick from a buffer group
> whose value is the same as the buffer ID of the las receive. That is
> obviously incorrect, and will result in a premature -ENOUFS error for
> the receive even if we had available buffers in the correct group.
>
> Cache the buffer group value at prep time, so we can restore it for
> future receives. This only needs doing for the above mentioned case,
> but
> just do it by default to keep it easier to read.
>
> Cc: [email protected]
> Fixes: b3fdea6ecb55 ("io_uring: multishot recv")
> Fixes: 9bb66906f23e ("io_uring: support multishot in recvmsg")
> Cc: Dylan Yudaken <[email protected]>
> Signed-off-by: Jens Axboe <[email protected]>
>
> ---
>
> diff --git a/io_uring/net.c b/io_uring/net.c
> index fbc34a7c2743..07a6aa39ab6f 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -62,6 +62,7 @@ struct io_sr_msg {
> u16 flags;
> /* initialised and used only by !msg send variants */
> u16 addr_len;
> + u16 buf_group;
> void __user *addr;
> /* used only for send zerocopy */
> struct io_kiocb *notif;
> @@ -580,6 +581,15 @@ int io_recvmsg_prep(struct io_kiocb *req, const
> struct io_uring_sqe *sqe)
> if (req->opcode == IORING_OP_RECV && sr->len)
> return -EINVAL;
> req->flags |= REQ_F_APOLL_MULTISHOT;
> + /*
> + * Store the buffer group for this multishot receive
> separately,
> + * as if we end up doing an io-wq based issue that
> selects a
> + * buffer, it has to be committed immediately and
> that will
> + * clear ->buf_list. This means we lose the link to
> the buffer
> + * list, and the eventual buffer put on completion
> then cannot
> + * restore it.
> + */
> + sr->buf_group = req->buf_index;
> }
>
> #ifdef CONFIG_COMPAT
> @@ -816,8 +826,10 @@ int io_recvmsg(struct io_kiocb *req, unsigned
> int issue_flags)
> if (kmsg->msg.msg_inq)
> cflags |= IORING_CQE_F_SOCK_NONEMPTY;
>
> - if (!io_recv_finish(req, &ret, cflags, mshot_finished,
> issue_flags))
> + if (!io_recv_finish(req, &ret, cflags, mshot_finished,
> issue_flags)) {
> + req->buf_index = sr->buf_group;
I think this is better placed in io_recv_prep_retry()? It would remove
the duplicated logic below
> goto retry_multishot;
> + }
>
> if (mshot_finished) {
> /* fast path, check for non-NULL to avoid function
> call */
> @@ -918,8 +930,10 @@ int io_recv(struct io_kiocb *req, unsigned int
> issue_flags)
> if (msg.msg_inq)
> cflags |= IORING_CQE_F_SOCK_NONEMPTY;
>
> - if (!io_recv_finish(req, &ret, cflags, ret <= 0,
> issue_flags))
> + if (!io_recv_finish(req, &ret, cflags, ret <= 0,
> issue_flags)) {
> + req->buf_index = sr->buf_group;
> goto retry_multishot;
> + }
>
> return ret;
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] io_uring/net: cache provided buffer group value for multishot receives
2023-01-23 14:04 ` Dylan Yudaken
@ 2023-01-23 14:06 ` Jens Axboe
0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2023-01-23 14:06 UTC (permalink / raw)
To: Dylan Yudaken, [email protected]
On 1/23/23 7:04 AM, Dylan Yudaken wrote:
> On Sun, 2023-01-22 at 10:13 -0700, Jens Axboe wrote:
>> If we're using ring provided buffers with multishot receive, and we
>> end
>> up doing an io-wq based issue at some points that also needs to
>> select
>> a buffer, we'll lose the initially assigned buffer group as
>> io_ring_buffer_select() correctly clears the buffer group list as the
>> issue isn't serialized by the ctx uring_lock. This is fine for normal
>> receives as the request puts the buffer and finishes, but for
>> multishot,
>> we will re-arm and do further receives. On the next trigger for this
>> multishot receive, the receive will try and pick from a buffer group
>> whose value is the same as the buffer ID of the las receive. That is
>> obviously incorrect, and will result in a premature -ENOUFS error for
>> the receive even if we had available buffers in the correct group.
>>
>> Cache the buffer group value at prep time, so we can restore it for
>> future receives. This only needs doing for the above mentioned case,
>> but
>> just do it by default to keep it easier to read.
>>
>> Cc: [email protected]
>> Fixes: b3fdea6ecb55 ("io_uring: multishot recv")
>> Fixes: 9bb66906f23e ("io_uring: support multishot in recvmsg")
>> Cc: Dylan Yudaken <[email protected]>
>> Signed-off-by: Jens Axboe <[email protected]>
>>
>> ---
>>
>> diff --git a/io_uring/net.c b/io_uring/net.c
>> index fbc34a7c2743..07a6aa39ab6f 100644
>> --- a/io_uring/net.c
>> +++ b/io_uring/net.c
>> @@ -62,6 +62,7 @@ struct io_sr_msg {
>> u16 flags;
>> /* initialised and used only by !msg send variants */
>> u16 addr_len;
>> + u16 buf_group;
>> void __user *addr;
>> /* used only for send zerocopy */
>> struct io_kiocb *notif;
>> @@ -580,6 +581,15 @@ int io_recvmsg_prep(struct io_kiocb *req, const
>> struct io_uring_sqe *sqe)
>> if (req->opcode == IORING_OP_RECV && sr->len)
>> return -EINVAL;
>> req->flags |= REQ_F_APOLL_MULTISHOT;
>> + /*
>> + * Store the buffer group for this multishot receive
>> separately,
>> + * as if we end up doing an io-wq based issue that
>> selects a
>> + * buffer, it has to be committed immediately and
>> that will
>> + * clear ->buf_list. This means we lose the link to
>> the buffer
>> + * list, and the eventual buffer put on completion
>> then cannot
>> + * restore it.
>> + */
>> + sr->buf_group = req->buf_index;
>> }
>>
>> #ifdef CONFIG_COMPAT
>> @@ -816,8 +826,10 @@ int io_recvmsg(struct io_kiocb *req, unsigned
>> int issue_flags)
>> if (kmsg->msg.msg_inq)
>> cflags |= IORING_CQE_F_SOCK_NONEMPTY;
>>
>> - if (!io_recv_finish(req, &ret, cflags, mshot_finished,
>> issue_flags))
>> + if (!io_recv_finish(req, &ret, cflags, mshot_finished,
>> issue_flags)) {
>> + req->buf_index = sr->buf_group;
>
> I think this is better placed in io_recv_prep_retry()? It would remove
> the duplicated logic below
True, let's move it there instead, and then perhaps also add a comment.
I'll make that change.
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-23 14:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-22 17:13 [PATCH] io_uring/net: cache provided buffer group value for multishot receives Jens Axboe
2023-01-23 14:04 ` Dylan Yudaken
2023-01-23 14:06 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox