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?
--
Jens Axboe
