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

[Pavel Begunkov <asml.silence@gmail.com>]

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