Re: [PATCH 4/5] io_uring/zcrx: move zcrx region to struct io_zcrx_ifq

[David Wei <dw@davidwei.uk>]

On 2025-04-18 09:22, Pavel Begunkov wrote:
> On 4/18/25 17:05, David Wei wrote:
>> On 2025-04-16 08:21, Pavel Begunkov wrote:
>>> Refill queue region is a part of zcrx and should stay in struct
>>> io_zcrx_ifq. We can't have multiple queues without it.
>>>
>>> Note: ctx->ifq assignments are now protected by mmap_lock as it's in
>>> the mmap region look up path.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>   include/linux/io_uring_types.h |  2 --
>>>   io_uring/zcrx.c                | 20 ++++++++++++--------
>>>   io_uring/zcrx.h                |  1 +
>>>   3 files changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>>> index 3b467879bca8..06d722289fc5 100644
>>> --- a/include/linux/io_uring_types.h
>>> +++ b/include/linux/io_uring_types.h
>>> @@ -448,8 +448,6 @@ struct io_ring_ctx {
>>>       struct io_mapped_region        ring_region;
>>>       /* used for optimised request parameter and wait argument passing  */
>>>       struct io_mapped_region        param_region;
>>> -    /* just one zcrx per ring for now, will move to io_zcrx_ifq eventually */
>>> -    struct io_mapped_region        zcrx_region;
>>>   };
>>>     /*
>>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>>> index 652daff0eb8d..d56665fd103d 100644
>>> --- a/io_uring/zcrx.c
>>> +++ b/io_uring/zcrx.c
>>> @@ -160,12 +160,11 @@ static int io_allocate_rbuf_ring(struct io_zcrx_ifq *ifq,
>>>       if (size > rd->size)
>>>           return -EINVAL;
>>>   -    ret = io_create_region_mmap_safe(ifq->ctx, &ifq->ctx->zcrx_region, rd,
>>> -                     IORING_MAP_OFF_ZCRX_REGION);
>>> +    ret = io_create_region(ifq->ctx, &ifq->region, rd, IORING_MAP_OFF_ZCRX_REGION);
>>
>> Why is this changed to io_create_region()? I don't see the caller of
>> io_allocate_rbuf_ring() changing or taking mmap_lock.
> 
> We only care about mmap seeing a consistent region. The mmap holds
> the ctx->mmap_lock, and *mmap_safe was making sure the region is
> updated atomically from its perspective.
> 
> Now, instead of protecting the region itself, this patch is protecting
> how ifq and subsequently the region is published:
> 
> -    ctx->ifq = ifq;
> +    scoped_guard(mutex, &ctx->mmap_lock)
> +        ctx->ifq = ifq;
> 
> And io_zcrx_get_region() is either sees NULL or ifq with a
> correct region.
> 

Ah I see, thanks, make sense.

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