On 2025-02-20 05:45, Pavel Begunkov wrote:
> On 2/18/25 16:57, David Wei wrote:
>> Currently only multishot recvzc requests are supported, but sometimes
>> there is a need to do a single recv e.g. peeking at some data in the
>> socket. Add single shot recvzc requests where IORING_RECV_MULTISHOT is
>> _not_ set and the sqe->len field is set to the number of bytes to read
>> N.
>>
>> There could be multiple completions containing data, like the multishot
>> case, since N bytes could be split across multiple frags. This is
>> followed by a final completion with res and cflags both set to 0 that
>> indicate the completion of the request, or a -res that indicate an
>> error.
>>
>> Signed-off-by: David Wei <dw@xxxxxxxxxxx>
>> ---
>> io_uring/net.c | 26 ++++++++++++++++++--------
>> io_uring/zcrx.c | 17 ++++++++++++++---
>> io_uring/zcrx.h | 2 +-
>> 3 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/io_uring/net.c b/io_uring/net.c
>> index 000dc70d08d0..d3a9aaa52a13 100644
>> --- a/io_uring/net.c
>> +++ b/io_uring/net.c
>> @@ -94,6 +94,7 @@ struct io_recvzc {
>> struct file *file;
>> unsigned msg_flags;
>> u16 flags;
>> + u32 len;
>
> Something is up with the types, it's u32, for which you use
> UINT_MAX, and later convert to ulong.
Inconsistency is my middle name.
I'll fix it up.
>
>> struct io_zcrx_ifq *ifq;
>> };
>>
> ...
>> @@ -1250,6 +1251,9 @@ int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> zc->ifq = req->ctx->ifq;
>> if (!zc->ifq)
>> return -EINVAL;
>> + zc->len = READ_ONCE(sqe->len);
>> + if (zc->len == UINT_MAX)
>> + return -EINVAL;
>> zc->flags = READ_ONCE(sqe->ioprio);
>> zc->msg_flags = READ_ONCE(sqe->msg_flags);
>> @@ -1257,12 +1261,14 @@ int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> return -EINVAL;
>> if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
>> return -EINVAL;
>> - /* multishot required */
>> - if (!(zc->flags & IORING_RECV_MULTISHOT))
>> - return -EINVAL;
>> - /* All data completions are posted as aux CQEs. */
>> - req->flags |= REQ_F_APOLL_MULTISHOT;
>> -
>> + if (zc->flags & IORING_RECV_MULTISHOT) {
>> + if (zc->len)
>> + return -EINVAL;
>> + /* All data completions are posted as aux CQEs. */
>> + req->flags |= REQ_F_APOLL_MULTISHOT;
>
> If you're posting "aux" cqes you have to set the flag for
> synchronisation reasons. We probably can split out a "I want to post
> aux cqes" flag, but it seems like you don't actually care about
> multishot here but limiting the length, or limiting the length + nowait.
Yeah, it's still "multishot" because there are still aux cqes for data
notifications. The requested N bytes could be in multiple frags. I'll
make sure REQ_F_APOLL_MULTISHOT is set.
>
>> + }
>> + if (!zc->len)
>> + zc->len = UINT_MAX;
>> return 0;
>> }
>> @@ -1281,7 +1287,7 @@ int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>> return -ENOTSOCK;
>> ret = io_zcrx_recv(req, zc->ifq, sock, zc->msg_flags | MSG_DONTWAIT,
>> - issue_flags);
>> + issue_flags, zc->len);
>> if (unlikely(ret <= 0) && ret != -EAGAIN) {
>> if (ret == -ERESTARTSYS)
>> ret = -EINTR;
>> @@ -1296,6 +1302,10 @@ int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>> return IOU_OK;
>> }
>> + if (zc->len != UINT_MAX) {
>> + io_req_set_res(req, ret, 0);
>> + return IOU_OK;
>> + }
>> if (issue_flags & IO_URING_F_MULTISHOT)
>> return IOU_ISSUE_SKIP_COMPLETE;
>> return -EAGAIN;
>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>> index ea099f746599..834c887743c8 100644
>> --- a/io_uring/zcrx.c
>> +++ b/io_uring/zcrx.c
>> @@ -106,6 +106,7 @@ struct io_zcrx_args {
>> struct io_zcrx_ifq *ifq;
>> struct socket *sock;
>> unsigned nr_skbs;
>> + unsigned long len;
>> };
>> static const struct memory_provider_ops io_uring_pp_zc_ops;
>> @@ -826,6 +827,10 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>> int i, copy, end, off;
>> int ret = 0;
>> + if (args->len == 0)
>> + return -EINTR;
>> + len = (args->len != UINT_MAX) ? min_t(size_t, len, args->len) : len;
>
> Just min?
Yes :facepalm:
>
>> +
>> if (unlikely(args->nr_skbs++ > IO_SKBS_PER_CALL_LIMIT))
>> return -EAGAIN;
>> @@ -920,17 +925,21 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>> out:
>> if (offset == start_off)
>> return ret;
>> + args->len -= (offset - start_off);
>
> Doesn't it unconditionally change the magic value UINT_MAX
> you're trying to preserve?
Yes, you're right. I will fix this and add a test case.
>
>> + if (args->len == 0)
>> + desc->count = 0;
>> return offset - start_off;
>> }
>> static int io_zcrx_tcp_recvmsg(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>> struct sock *sk, int flags,
>> - unsigned issue_flags)
>> + unsigned issue_flags, unsigned long len)
>> {
>> struct io_zcrx_args args = {
>> .req = req,
>> .ifq = ifq,
>> .sock = sk->sk_socket,
>> + .len = len,
>> };
>> read_descriptor_t rd_desc = {
>> .count = 1,
>> @@ -956,6 +965,8 @@ static int io_zcrx_tcp_recvmsg(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>> ret = IOU_REQUEUE;
>> } else if (sock_flag(sk, SOCK_DONE)) {
>> /* Make it to retry until it finally gets 0. */
>> + if (len != UINT_MAX)
>> + goto out;
>> if (issue_flags & IO_URING_F_MULTISHOT)
>> ret = IOU_REQUEUE;
>> else
>
