Re: [PATCH v1 12/15] io_uring/zcrx: add io_recvzc request

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

On 10/9/24 19:28, Jens Axboe wrote:
>> diff --git a/io_uring/net.c b/io_uring/net.c
>> index d08abcca89cc..482e138d2994 100644
>> --- a/io_uring/net.c
>> +++ b/io_uring/net.c
>> @@ -1193,6 +1201,76 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>   	return ret;
>>   }
>>   
>> +int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> +{
>> +	struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
>> +	unsigned ifq_idx;
>> +
>> +	if (unlikely(sqe->file_index || sqe->addr2 || sqe->addr ||
>> +		     sqe->len || sqe->addr3))
>> +		return -EINVAL;
>> +
>> +	ifq_idx = READ_ONCE(sqe->zcrx_ifq_idx);
>> +	if (ifq_idx != 0)
>> +		return -EINVAL;
>> +	zc->ifq = req->ctx->ifq;
>> +	if (!zc->ifq)
>> +		return -EINVAL;
> 
> This is read and assigned to 'zc' here, but then the issue handler does
> it again? I'm assuming that at some point we'll have ifq selection here,
> and then the issue handler will just use zc->ifq. So this part should
> probably remain, and the issue side just use zc->ifq?

Yep, fairly overlooked. It's not a real problem, but should
only be fetched and checked here.

>> +	/* All data completions are posted as aux CQEs. */
>> +	req->flags |= REQ_F_APOLL_MULTISHOT;
> 
> This puzzles me a bit...

Well, it's a multishot request. And that flag protects from cq
locking rules violations, i.e. avoiding multishot reqs from
posting from io-wq.

>> +	zc->flags = READ_ONCE(sqe->ioprio);
>> +	zc->msg_flags = READ_ONCE(sqe->msg_flags);
>> +	if (zc->msg_flags)
>> +		return -EINVAL;
> 
> Maybe allow MSG_DONTWAIT at least? You already pass that in anyway.

What would the semantics be? The io_uring nowait has always
been a pure mess because it's not even clear what it supposed
to mean for async requests.


>> +	if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
>> +		return -EINVAL;
>> +
>> +
>> +#ifdef CONFIG_COMPAT
>> +	if (req->ctx->compat)
>> +		zc->msg_flags |= MSG_CMSG_COMPAT;
>> +#endif
>> +	return 0;
>> +}
> 
> Heh, we could probably just return -EINVAL for that case, but since this
> is all we need, fine.

Well, there is no msghdr, cmsg nor iovec there, so doesn't even
make sense to set it. Can fail as well, I don't anyone would care.

-- 
Pavel Begunkov