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

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

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.

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

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

-- 
Pavel Begunkov