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.
> void qemu_co_rwlock_upgrade(CoRwlock *lock)
> {
> - Coroutine *self = qemu_coroutine_self();
> -
> qemu_co_mutex_lock(&lock->mutex);
> - assert(lock->reader > 0);
> - lock->reader--;
> - lock->pending_writer++;
> - while (lock->reader) {
> - qemu_co_queue_wait(&lock->queue, &lock->mutex);
> - }
> - lock->pending_writer--;
> + assert(lock->owners > 0);
> + /* For fairness, wait if a writer is in line. */
> + if (lock->owners == 1 && QSIMPLEQ_EMPTY(&lock->tickets)) {
> + lock->owners = -1;
> + qemu_co_mutex_unlock(&lock->mutex);
> + } else {
> + CoRwTicket my_ticket = { false, qemu_coroutine_self() };
>
> - /* The rest of the write-side critical section is run with
> - * the mutex taken, similar to qemu_co_rwlock_wrlock. Do
> - * not account for the lock twice in self->locks_held.
> - */
> - 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.
> + }
> }
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.
signature.asc
Description: PGP signature
