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