On 2025-02-21 16:08, Jens Axboe wrote:
> Just a few minor drive-by nits.
>
>> @@ -1250,6 +1251,12 @@ 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;
>> + /* UINT_MAX means no limit on readlen */
>> + if (!zc->len)
>> + zc->len = UINT_MAX;
>
> Why not just make UINT_MAX allowed, meaning no limit? Would avoid two
> branches here, and as far as I can tell not really change anything in
> terms of API niceness.
>
>> @@ -1269,6 +1276,7 @@ int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>> {
>> struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
>> + bool limit = zc->len != UINT_MAX;
>> struct socket *sock;
>> int ret;
>
> Doesn't seem to be used?
Oops, I'll remove it.
>
>> @@ -1296,6 +1304,13 @@ int io_recvzc(struct io_kiocb *req, unsigned int issue_flags)
>> return IOU_OK;
>> }
>>
>> + if (zc->len == 0) {
>> + io_req_set_res(req, 0, 0);
>> +
>> + if (issue_flags & IO_URING_F_MULTISHOT)
>> + return IOU_STOP_MULTISHOT;
>> + return IOU_OK;
>> + }
>> if (issue_flags & IO_URING_F_MULTISHOT)
>> return IOU_ISSUE_SKIP_COMPLETE;
>> return -EAGAIN;
>
> Might be cleaner as:
>
> ret = -EAGAIN;
> if (!zc->len) {
> io_req_set_res(req, 0, 0);
> ret = -IOU_OK;
> }
> if (issue_flags & IO_URING_F_MULTISHOT)
> return IOU_ISSUE_SKIP_COMPLETE;
> return ret;
>
> rather than duplicate the flag checking.
>
>
>> 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 int *outlen)
>> {
>> + unsigned int len = *outlen;
>> + bool limit = len != UINT_MAX;
>> struct io_zcrx_args args = {
>> .req = req,
>> .ifq = ifq,
>> .sock = sk->sk_socket,
>> };
>> read_descriptor_t rd_desc = {
>> - .count = 1,
>> + .count = len,
>> .arg.data = &args,
>> };
>> int ret;
>>
>> lock_sock(sk);
>> ret = tcp_read_sock(sk, &rd_desc, io_zcrx_recv_skb);
>> + if (limit && ret)
>> + *outlen = len - ret;
>> if (ret <= 0) {
>> if (ret < 0 || sock_flag(sk, SOCK_DONE))
>> goto out;
>> @@ -930,7 +937,7 @@ 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 (issue_flags & IO_URING_F_MULTISHOT)
>> + if (!limit && (issue_flags & IO_URING_F_MULTISHOT))
>> ret = IOU_REQUEUE;
>> else
>> ret = -EAGAIN;
>
> In the two above hunks, the limit checking feels a bit hackish. For
> example, why is it related to multishot or not? I think the patch would
> benefit from being split into separate patches for singleshot and length
> support. Eg one that adds singleshot support, and then one that adds
> length capping on top. That would make it much easier to reason about
> hunks like the above one.
>
>> @@ -942,7 +949,7 @@ static int io_zcrx_tcp_recvmsg(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>>
>> int io_zcrx_recv(struct io_kiocb *req, struct io_zcrx_ifq *ifq,
>> struct socket *sock, unsigned int flags,
>> - unsigned issue_flags)
>> + unsigned issue_flags, unsigned int *len)
>> {
>> struct sock *sk = sock->sk;
>> const struct proto *prot = READ_ONCE(sk->sk_prot);
>
> Shouldn't recv support it too?
>
