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

On 5/19/26 17:04, 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.

Better to say they're counting nesting

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

-- 
Pavel Begunkov