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

[Jens Axboe <axboe@kernel.dk>]

On 10/9/24 12:51 PM, Pavel Begunkov wrote:
> 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.

Right

>>> +    /* 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.

Maybe make it more like the others and require that
IORING_RECV_MULTISHOT is set then, and set it based on that?

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

Yeah can't disagree with that. Not a big deal, doesn't really matter,
can stay as-is.

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

Then let's please just kill it, should not need a check for that then.

-- 
Jens Axboe