Re: [PATCH 2/4] io_uring: move locking inside overflow posting

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

On 5/14/25 20:25, Jens Axboe wrote:
> On 5/14/25 11:18 AM, Pavel Begunkov wrote:
>> On 5/14/25 17:42, Jens Axboe wrote:
>>> On 5/14/25 2:07 AM, Pavel Begunkov wrote:
>> ...>> +static bool io_cqring_event_overflow(struct io_ring_ctx *ctx, bool locked,
>>>> +                     u64 user_data, s32 res, u32 cflags,
>>>> +                     u64 extra1, u64 extra2)
>>>> +{
>>>> +    bool queued;
>>>> +
>>>> +    if (locked) {
>>>> +        queued = __io_cqring_event_overflow(ctx, user_data, res, cflags,
>>>> +                            extra1, extra2);
>>>> +    } else {
>>>> +        spin_lock(&ctx->completion_lock);
>>>> +        queued = __io_cqring_event_overflow(ctx, user_data, res, cflags,
>>>> +                            extra1, extra2);
>>>> +        spin_unlock(&ctx->completion_lock);
>>>> +    }
>>>> +    return queued;
>>>> +}
>>>
>>> Really not a fan of passing in locking state and having a split helper
>>> like that.
>>
>> I'd agree in general, but it's the same thing that's already in
>> __io_submit_flush_completions(), just with a named argument
>> instead of backflipping on ctx->lockless_cq.
> 
> And I honestly _greatly_ prefer that to passing in some random bool
> where you have to go find the function to even see what it does...

I see, it's much worse than having it somewhat abstracted,
trying to be more reliable and self checking to prevent bugs,
I give up on trying to do anything with it.

>>> It's also pretty unwieldy with 7 arguments.
>>
>> It's 6 vs 7, not much difference, and the real problem here is
>> passing all cqe parts as separate arguments, which this series
>> doesn't touch.
> 
> It's 6 you're passing on, it's 7 for the overflow helper.

I don't get what you're saying.

>>> Overall, why do we care about atomic vs non-atomic allocations for
>>> overflows? If you're hitting overflow, you've messed up... And if it's
>>
>> Not really, it's not far fetched to overflow with multishots unless
>> you're overallocating CQ quite a bit, especially if traffic is not so
>> homogeneous and has spikes. Expensive and should be minimised b/c of
>> that, but it's still reasonably possible in a well structured app.
> 
> It's still possible, but it should be a fairly rare occurence after all.

That's what I'm saying. And handling that "rare" well is the difference
between having reliable software and terminally screwing userspace.

>>> about cost, gfp atomic alloc will be predictable, vs GFP_KERNEL which
>>> can be a lot more involved.
>>
>> Not the cost but reliability. If overflowing fails, the userspace can't
>> reasonably do anything to recover, so either terminate or leak. And
>> it's taking the atomic reserves from those who actually need it
>> like irq.
> 
> I don't really disagree on that part, GFP_ATOMIC is only really used
> because of the locking, as this series deals with. Just saying that
> it'll make overflow jitter more pronounced, potentially, though not

It's like saying that the allocation makes heavier so let's just
drop the CQE on the floor. It should do reclaim only when the
situation with memory is already bad enough and you're risking
failing atomic allocations (even if not exactly that).

> something I'm worried about. I'd much rather handle that sanely instead
> of passing in locking state. At least a gfp_t argument is immediately
> apparent what it does.

-- 
Pavel Begunkov