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

[Jens Axboe <axboe@kernel.dk>]

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?

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

-- 
Jens Axboe