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

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

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.

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

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

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

-- 
Pavel Begunkov