Re: [PATCH 5/5] io_uring/zcrx: add support for multiple ifqs

[David Wei <dw@davidwei.uk>]

On 2025-04-18 10:22, Pavel Begunkov wrote:
> On 4/18/25 18:01, David Wei wrote:
>> On 2025-04-16 08:21, Pavel Begunkov wrote:
>>> Allow the user to register multiple ifqs / zcrx contexts. With that we
>>> can use multiple interfaces / interface queues in a single io_uring
>>> instance.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>   include/linux/io_uring_types.h |  5 ++--
>>>   io_uring/io_uring.c            |  3 +-
>>>   io_uring/net.c                 |  8 ++---
>>>   io_uring/zcrx.c                | 53 +++++++++++++++++++++-------------
>>>   4 files changed, 40 insertions(+), 29 deletions(-)
>>>
>> [...]
>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>> index 5f1a519d1fc6..b3a643675ce8 100644
>>> --- a/io_uring/net.c
>>> +++ b/io_uring/net.c
>>> @@ -1185,16 +1185,14 @@ 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->addr3))
>>> +    if (unlikely(sqe->addr2 || sqe->addr || sqe->addr3))
>>>           return -EINVAL;
>>
>> Why remove sqe->file_index?
> 
> it's aliased with ->zcrx_ifq_idx. The ifq_idx check below
> essentially does nothing. And fwiw, userspace was getting
> correct errors, so it's not a fix.
> 

Got it.

>>>       ifq_idx = READ_ONCE(sqe->zcrx_ifq_idx);
>>> -    if (ifq_idx != 0)
>>> -        return -EINVAL;
>>> -    zc->ifq = req->ctx->ifq;
>>> +    zc->ifq = xa_load(&req->ctx->zcrx_ctxs, ifq_idx);
>>>       if (!zc->ifq)
>>>           return -EINVAL;
>>> +
>>>       zc->len = READ_ONCE(sqe->len);
>>>       zc->flags = READ_ONCE(sqe->ioprio);
>>>       zc->msg_flags = READ_ONCE(sqe->msg_flags);
>>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>>> index d56665fd103d..e4ce971b1257 100644
>>> --- a/io_uring/zcrx.c
>>> +++ b/io_uring/zcrx.c
>>> @@ -172,9 +172,6 @@ static int io_allocate_rbuf_ring(struct io_zcrx_ifq *ifq,
>>>     static void io_free_rbuf_ring(struct io_zcrx_ifq *ifq)
>>>   {
>>> -    if (WARN_ON_ONCE(ifq->ctx->ifq))
>>> -        return;
>>> -
>>
>> I think this should stay.
> 
> There is not ctx->ifq anymore. You may look up in the xarray,
> but for that you need to know the index, and it's easier to
> just remove it.

Yeah, and it's rather defensive to check.

> 
>  
>>>       io_free_region(ifq->ctx, &ifq->region);
>>>       ifq->rq_ring = NULL;
>>>       ifq->rqes = NULL;
>> [...]
>>> @@ -440,16 +443,23 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx,
>>>     void io_unregister_zcrx_ifqs(struct io_ring_ctx *ctx)
>>>   {
>>> -    struct io_zcrx_ifq *ifq = ctx->ifq;
>>> +    struct io_zcrx_ifq *ifq;
>>> +    unsigned long id;
>>>         lockdep_assert_held(&ctx->uring_lock);
>>>   -    if (!ifq)
>>> -        return;
>>> +    while (1) {
>>> +        scoped_guard(mutex, &ctx->mmap_lock) {
>>> +            ifq = xa_find(&ctx->zcrx_ctxs, &id, ULONG_MAX, XA_PRESENT);
>>> +            if (ifq)
>>> +                xa_erase(&ctx->zcrx_ctxs, id);
>>> +        }
>>> +        if (!ifq)
>>> +            break;
>>> +        io_zcrx_ifq_free(ifq);
>>> +    }
>>
>> Why not xa_for_each()? Is it weirdness with scoped_guard macro?
> 
> I don't want io_zcrx_ifq_free() to be under mmap_lock, that might
> complicate sync with mmap. It's good enough for now, but I'd like
> to have sth like this in the future:
> 
> struct xarray tmp_onstack_xarray;
> 
> scoped_guard(mutex, &ctx->mmap_lock)
>     xarray_swap(&tmp_onstack_xarray, &ctx->zcrx);
> for_each_xarray(tmp_onstack_xarray)
>     io_zcrx_ifq_free();
> 
> but there is no xarray_swap AFAIK.
> 

Reviewed-by: David Wei <dw@davidwei.uk>