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
