Re: [PATCH 4/5] coroutine-lock: reimplement CoRwLock to fix downgrade bug

[Paolo Bonzini]

On 17/03/21 11:40, David Edmondson wrote:
> Isn't this...
>
>   * ... Also, @qemu_co_rwlock_upgrade
>   * only overrides CoRwlock fairness if there are no concurrent readers, so
>   * another writer might run while @qemu_co_rwlock_upgrade blocks.
>
> ...now incorrect?

Maybe, but for sure the comment was too hard to parse.

> > +    if (lock->owner == 1 && QSIMPLEQ_EMPTY(&lock->tickets)) {
> > +        lock->owner = -1;
> > +        qemu_co_mutex_unlock(&lock->mutex);
> > +    } else {
> > +        lock->owner--;
> > +        qemu_co_rwlock_sleep(false, lock);
>
> Doesn't this need something for the case where lock->owner hits 0?

You're right, we need to call qemu_co_rwlock_maybe_wake_one if lock goes 
to 0.  The "else" branch would have to be

        if (--lock->owner == 0) {
                qemu_co_rwlock_maybe_wake_one(lock);
                qemu_co_mutex_lock(&lock->mutex);
        }
        qemu_co_rwlock_sleep(false, lock);

In the end it's actually simpler to just inline qemu_co_rwlock_sleep, 
which also leads to the following slightly more optimized code for the 
"else" branch:

        CoRwTicket my_ticket = { false, qemu_coroutine_self() };

        lock->owner--;
        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
        qemu_co_rwlock_maybe_wake_one(lock);
        qemu_coroutine_yield();
        assert(lock->owner == -1);

I'll add a testcase, too.  You don't even need two upgraders, for example:

        rdlock
        yield
                        wrlock
        upgrade
        <queued>        <dequeued>
                        unlock
        <dequeued>
        unlock

In fact even for this simple case, the old implementation got it wrong! 
 (The new one also did, but the fix is easy).

There are a couple other improvements that can be made: 
qemu_co_rwlock_unlock can also call qemu_co_rwlock_maybe_wake_one 
unconditionally, the "if" around the call is unnecessary; and I'll 
rename "owner" to "owners".

Anyway, there is nothing that really made you scream, so that's good.

Paolo