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

Pavel Begunkov
2022-06-19T14:21:26Z

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);
}

--
Pavel Begunkov