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

[Jens Axboe <axboe@kernel.dk>]

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

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

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

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

-- 
Jens Axboe