Re: [Qemu-devel] [PATCH v8 0/5] Coroutines for better asynchronous programming

[Kevin Wolf]

Am 27.07.2011 13:39, schrieb Aneesh Kumar K.V:
> Can you review the patch that add CoRWlock ?
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/105402
> Message-id:address@hidden
> 
> commit 8c787d8b81aca1f4f7be45adb67b9e1a6dde7f1f
> Author: Aneesh Kumar K.V <address@hidden>
> Date:   Tue May 24 22:14:04 2011 +0530
> 
>     coroutine: Add CoRwlock support
>     
>     Signed-off-by: Aneesh Kumar K.V <address@hidden>

Nice! I think I'll need this, too.

> diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
> index 5071fb8..5ecaa94 100644
> --- a/qemu-coroutine-lock.c
> +++ b/qemu-coroutine-lock.c
> @@ -115,3 +115,47 @@ void qemu_co_mutex_unlock(CoMutex *mutex)
>  
>      trace_qemu_co_mutex_unlock_return(mutex, self);
>  }
> +
> +void qemu_co_rwlock_init(CoRwlock *lock)
> +{
> +    memset(lock, 0, sizeof(*lock));
> +    qemu_co_queue_init(&lock->queue);
> +}
> +
> +void qemu_co_rwlock_rdlock(CoRwlock *lock)
> +{
> +    while (lock->writer) {
> +        qemu_co_queue_wait(&lock->queue);
> +    }
> +    lock->reader++;
> +}
> +
> +void qemu_co_rwlock_unlock(CoRwlock *lock)
> +{
> +    assert(qemu_in_coroutine());
> +    if (lock->writer) {
> +        lock->writer--;;

Please don't do arithmetics on bools, just say lock->write = false;

Also there's a double semicolon.

> +        assert(lock->writer == 0);
> +        while (!qemu_co_queue_empty(&lock->queue)) {
> +            /*
> +             * Wakeup every body. This will include some
> +             * writers too.
> +             */
> +            qemu_co_queue_next(&lock->queue);
> +        }
> +    } else {
> +        lock->reader--;
> +        assert(lock->reader >= 0);
> +        /* Wakeup only one waiting writer */
> +        qemu_co_queue_next(&lock->queue);

This is only useful if lock->reader == 0.

> +    }
> +}
> +
> +void qemu_co_rwlock_wrlock(CoRwlock *lock)
> +{
> +    while (lock->writer || lock->reader) {
> +        qemu_co_queue_wait(&lock->queue);
> +    }
> +    lock->writer++;
> +    assert(lock->writer == 1);
> +}

I wonder if we should have a mechanism that stops new readers from
taking the lock while a writer is waiting in order to avoid starvation.

Anyway, the locking itself looks correct.

Kevin