Re: [Qemu-block] [Qemu-devel] [PATCH 6/6] coroutine-lock: make CoRwlock thread-safe and fair

[Fam Zheng]

On Wed, 02/15 12:20, Paolo Bonzini wrote:
> 
> 
> On 15/02/2017 10:23, Fam Zheng wrote:
> > On Mon, 02/13 19:12, Paolo Bonzini wrote:
> >> This adds a CoMutex around the existing CoQueue.  Because the write-side
> > 
> > s/CoQueue/CoRwlock/
> 
> No, I meant that CoRwlock has a CoQueue, and after this patch it is
> wrapped by a CoMutex too.

OK.

> 
> 
> >> @@ -375,16 +384,20 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
> >>              qemu_co_queue_next(&lock->queue);
> >>          }
> >>      }
> >> -    self->locks_held--;
> >> +    qemu_co_mutex_unlock(&lock->mutex);
> >>  }
> >>  
> >>  void qemu_co_rwlock_wrlock(CoRwlock *lock)
> >>  {
> >> -    Coroutine *self = qemu_coroutine_self();
> >> -
> >> -    while (lock->writer || lock->reader) {
> >> -        qemu_co_queue_wait(&lock->queue, NULL);
> >> +    qemu_co_mutex_lock(&lock->mutex);
> >> +    lock->pending_writer++;
> >> +    while (lock->reader) {
> >> +        qemu_co_queue_wait(&lock->queue, &lock->mutex);
> >>      }
> >> -    lock->writer = true;
> >> -    self->locks_held++;
> >> +    lock->pending_writer--;
> >> +
> >> +    /* The rest of the write-side critical section is run with
> >> +     * the mutex taken, so that lock->reader remains zero.
> >> +     * There is no need to update self->locks_held.
> >> +     */
> > 
> > But is it still better to update self->locks_held anyway for the
> > 'assert(!co->locks_held)' in qemu_coroutine_enter? Or is the same thing 
> > checked
> > elsewhere?
> 
> self->locks_held is already incremented by the qemu_co_mutex_lock call
> at the beginning of qemu_co_rwlock_wrlock.  It is then decremented in
> qemu_co_rwlock_unlock.
> 
> For the read side, rdlock _unlocks_ lock->mutex before returning, so
> self->locks_held must be incremented by rdlock and decremented by unlock.

Makes sense.

Fam