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:1307382497-3737-2-git-send-email-aneesh.ku...@linux.vnet.ibm.com
>
> commit 8c787d8b81aca1f4f7be45adb67b9e1a6dde7f1f
> Author: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
> Date: Tue May 24 22:14:04 2011 +0530
>
> coroutine: Add CoRwlock support
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
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
