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

[David Wei <dw@davidwei.uk>]

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