Re: [PATCH 4/6] coroutine-lock: reimplement CoRwlock to fix downgrade bug

[Paolo Bonzini]

On 24/03/21 17:15, Stefan Hajnoczi wrote:
> On Wed, Mar 17, 2021 at 07:00:11PM +0100, Paolo Bonzini wrote:
> > +static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
> > +{
> > +    CoRwTicket *tkt = QSIMPLEQ_FIRST(&lock->tickets);
> > +    Coroutine *co = NULL;
> > +
> > +    /*
> > +     * Setting lock->owners here prevents rdlock and wrlock from
> > +     * sneaking in between unlock and wake.
> > +     */
> > +
> > +    if (tkt) {
> > +        if (tkt->read) {
> > +            if (lock->owners >= 0) {
> > +                lock->owners++;
> > +                co = tkt->co;
> > +            }
> > +        } else {
> > +            if (lock->owners == 0) {
> > +                lock->owners = -1;
> > +                co = tkt->co;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (co) {
> > +        QSIMPLEQ_REMOVE_HEAD(&lock->tickets, next);
> > +        qemu_co_mutex_unlock(&lock->mutex);
> > +        aio_co_wake(co);
>
> I find it hard to reason about QSIMPLEQ_EMPTY(&lock->tickets) callers
> that execute before co is entered. They see an empty queue even though a
> coroutine is about to run. Updating owners above ensures that the code
> correctly tracks the state of the rwlock, but I'm not 100% confident
> about this aspect of the code.

Good point.  The invariant when lock->mutex is released should be 
clarified; a better way to phrase the comment above "if (tkt)" is:

The invariant when lock->mutex is released is that every ticket is 
tracked in either lock->owners or lock->tickets.  By updating 
lock->owners here, rdlock/wrlock/upgrade will block even if they execute 
between qemu_co_mutex_unlock and aio_co_wake.

> > -    self->locks_held--;
> > +        lock->owners--;
> > +        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
> > +        qemu_co_rwlock_maybe_wake_one(lock);
> > +        qemu_coroutine_yield();
> > +        assert(lock->owners == -1);
>
> lock->owners is read outside lock->mutex here. Not sure if this can
> cause problems.

True.  It is okay though because lock->owners cannot change until this 
coroutine unlocks.  A worse occurrence of the issue is in rdlock:

        assert(lock->owners >= 1);

        /* Possibly wake another reader, which will wake the next in 
line.  */
        qemu_co_mutex_lock(&lock->mutex);

where the assert should be moved after taking the lock, or possibly 
changed to use qatomic_read.  (I prefer the former).

> locks_held is kept unchanged across qemu_coroutine_yield() even though
> the read lock has been released. rdlock() and wrlock() only increment
> locks_held after acquiring the rwlock.
>
> In practice I don't think it matters, but it seems inconsistent. If
> locks_held is supposed to track tickets (not just coroutines currently
> holding a lock), then rdlock() and wrlock() should increment before
> yielding.

locks_held (unlike owners) is not part of the lock, it's part of the 
Coroutine and only used for debugging (asserting that terminating 
coroutines are not holding any lock).

Paolo