Re: [PATCH for-next 6/7] io_uring: introduce locking helpers for CQE posting

[Jens Axboe <axboe@kernel.dk>]

On 6/19/22 8:20 AM, Pavel Begunkov wrote:
> On 6/19/22 14:30, Jens Axboe wrote:
>> On 6/19/22 5:26 AM, Pavel Begunkov wrote:
>>> spin_lock(&ctx->completion_lock);
>>> /* post CQEs */
>>> io_commit_cqring(ctx);
>>> spin_unlock(&ctx->completion_lock);
>>> io_cqring_ev_posted(ctx);
>>>
>>> We have many places repeating this sequence, and the three function
>>> unlock section is not perfect from the maintainance perspective and also
>>> makes harder to add new locking/sync trick.
>>>
>>> Introduce to helpers. io_cq_lock(), which is simple and only grabs
>>> ->completion_lock, and io_cq_unlock_post() encapsulating the three call
>>> section.
>>
>> I'm a bit split on this one, since I generally hate helpers that are
>> just wrapping something trivial:
>>
>> static inline void io_cq_lock(struct io_ring_ctx *ctx)
>>     __acquires(ctx->completion_lock)
>> {
>>     spin_lock(&ctx->completion_lock);
>> }
>>
>> The problem imho is that when I see spin_lock(ctx->lock) in the code I
>> know exactly what it does, if I see io_cq_lock(ctx) I have a good guess,
>> but I don't know for a fact until I become familiar with that new
>> helper.
>>
>> I can see why you're doing it as it gives us symmetry with the unlock
>> helper, which does indeed make more sense. But I do wonder if we
>> shouldn't just keep the spin_lock() part the same, and just have the
>> unlock helper?
> 
> That what I was doing first, but it's too ugly, that's the main
> reason. And if we find that removing locking with SINGLE_ISSUER
> is worth it, it'd need modification on the locking side:
> 
> cq_lock() {
>     if (!(ctx->flags & SINGLE_ISSUER))
>         lock(compl_lock);
> }
> 
> cq_unlock() {
>     ...
>     if (!(ctx->flags & SINGLE_ISSUER))
>         unlock(compl_lock);
> }

OK, that makes sense, if the helper will grow further changes.

-- 
Jens Axboe