Re: [PATCH 6/8] io_uring/zcrx: notify user when out of buffers

On 5/19/26 10:04 AM, Pavel Begunkov wrote:
> On 5/19/26 16:43, Jens Axboe wrote:
>> On 5/19/26 9:40 AM, Pavel Begunkov wrote:
>>> On 5/19/26 16:37, Jens Axboe wrote:
>>>> On 5/19/26 9:30 AM, Pavel Begunkov wrote:
>>>>> On 5/19/26 16:26, Jens Axboe wrote:
>>>>>> On 5/19/26 5:44 AM, Pavel Begunkov wrote:
>>>>>>> @@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
>>>>>>>         return allocated;
>>>>>>>     }
>>>>>>>     +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
>>>>>>> +{
>>>>>>> +    struct io_kiocb *req = tw_req.req;
>>>>>>> +    struct io_ring_ctx *ctx = req->ctx;
>>>>>>> +
>>>>>>> +    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
>>>>>>> +    percpu_ref_put(&ctx->refs);
>>>>>>> +    io_poison_req(req);
>>>>>>> +    kmem_cache_free(req_cachep, req);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
>>>>>>> +{
>>>>>>> +    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
>>>>>>> +    u32 type_mask = 1 << type;
>>>>>>> +    struct io_kiocb *req;
>>>>>>> +
>>>>>>> +    if (!(type_mask & ifq->allowed_notif_mask))
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    guard(spinlock_bh)(&ifq->ctx_lock);
>>>>>>> +    if (!ifq->master_ctx)
>>>>>>> +        return;
>>>>>>> +    if (type_mask & ifq->fired_notifs)
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    req = kmem_cache_alloc(req_cachep, gfp);
>>>>>>> +    if (unlikely(!req))
>>>>>>> +        return;
>>>>>>
>>>>>> It'd be nice to avoid an allocation here inside ctx_lock and with bh's
>>>>>> disabled, which looks like is also the only reason why GFP_ATOMIC is
>>>>>> being used here.
>>>>>
>>>>> I thought about it, but it's already bh, it'd need to do pre
>>>>> allocations + caching to be reliable, but that's left out for now.
>>>>
>>>> Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC.
>>>> What's the contract in terms of the notification? If we fail the alloc,
>>>> then userspace can't rely on the notification on the refill failure.
>>>>
>>>> Are we under bh save already here, before doing it ourselves? If so,
>>>> then how does the guard work?
>>>
>>> In 99% of cases it's called from softirq, not sure what you mean
>>> by how it works.
>>
>> Ah ok, I thought you meant it was already called with softirqs disabled.
>> In which case the guard would seem broken, as we'd enable softirqs when
>> exiting. But if we're just inside softirq yeah it's fine, and there's no
>> point shuffling the allocation either.
> 
> Softirqs are run with bh disabled, but bh_disable()/enable() are
> reenterable.

No worries on that then.

>> Question on the contract still stands, in terms of missing a
>> notification. I guess since it's a hint basically it doesn't really
>> matter, just something that should be documented on the userspace side.
> 
> Should rather be improved than documented, I'd say, but it's still

Of course, that's why I was originally asking about what the contract is
here - is it a hint, or is it more than that? In either case, should be
documented what the application can rely on. And might not be too bad to
harden, since it also really doesn't make sense to have more than one of
these inflight at the time anyway.

> better than not getting anything at all. And it's the only place
> where it can in theory be dropped, e.g. CQE overflow handling,
> though different GFP.
> 
>> Do you have test cases for these?
> 
> Clement needs to resend them. Actually, seems I forgot to CC Vish
> and Clement here, my bad.

Sounds good.

-- 
Jens Axboe