Re: [PATCH v2 1/2] io_uring/zcrx: add single shot recvzc

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/22/25 00: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.

I think 0 goes better as a special uapi value. It doesn't alter the
uapi, and commonly understood as "no limits", which is the opposite
to the other option, especially since UINT_MAX is not a max value for
an unlimited request, I'd easily expect it to drive more than 4GB.

  
>> @@ -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?
> 
>> @@ -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.

You missed IOU_STOP_MULTISHOT, but even without it separate error
codes for polling is already an utter mess to try be smart about it.
I'll try to clean it up, but that's orthogonal.

...
>> @@ -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

I agree with the statement, but fwiw there are no single shots and
can't be that, the message is misleading.

> 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?

There is no "msg" variant of the request, if that's what you mean.

-- 
Pavel Begunkov