Patchwork [v8,0/5] Coroutines for better asynchronous programming

login
register
mail settings
Submitter Aneesh Kumar K.V
Date July 27, 2011, 11:39 a.m.
Message ID <87tya83qc7.fsf@skywalker.in.ibm.com>
Download mbox | patch
Permalink /patch/107040/
State New
Headers show

Comments

Aneesh Kumar K.V - July 27, 2011, 11:39 a.m.
On Wed, 27 Jul 2011 12:03:57 +0200, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.07.2011 11:45, schrieb Aneesh Kumar K.V:
> > On Tue, 26 Jul 2011 10:21:12 +0100, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> >> QEMU is event-driven and suffers when blocking operations are performed because
> >> VM execution may be stopped until the operation completes.  Therefore many
> >> operations that could block are performed asynchronously and a callback is
> >> invoked when the operation has completed.  This allows QEMU to continue
> >> executing while the operation is pending.
> >>
> >> The downside to callbacks is that they split up code into many smaller
> >> functions, each of which is a single step in a state machine that quickly
> >> becomes complex and hard to understand.  Callback functions also result in lots
> >> of noise as variables are packed and unpacked into temporary structs that pass
> >> state to the callback function.
> >>
> >> This patch series introduces coroutines as a solution for writing asynchronous
> >> code while still having a nice sequential control flow.  The semantics are
> >> explained in the second patch.  The fourth patch adds automated tests.
> >>
> >> A nice feature of coroutines is that it is relatively easy to take synchronous
> >> code and lift it into a coroutine to make it asynchronous.  Work has been done
> >> to move qcow2 request processing into coroutines and thereby make it
> >> asynchronous (today qcow2 will perform synchronous metadata accesses).  This
> >> qcow2 work is still ongoing and not quite ready for mainline yet.
> >>
> >> v8:
> >>  * Bisectability: introduce gthread implementation before ucontext/fibers
> > 
> > Can we also get CoMutex and CoQueue patches also merged. I use them in
> > the VirtFS series. 
> > 
> > http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/coroutine-devel
> > http://repo.or.cz/w/qemu/v9fs.git/commit/318ef0b9b01cd296f8c30d8288139b9bed859892
> 
> I introduce these in my block coroutine patches. I posted a RFC last
> week and the first "real" patch series yesterday, so I hope they get
> review and can be merged into master soon.
> 

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.kumar@linux.vnet.ibm.com

commit 8c787d8b81aca1f4f7be45adb67b9e1a6dde7f1f
Author: Aneesh Kumar K.V <aneesh.kumar@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.kumar@linux.vnet.ibm.com>
Kevin Wolf - July 27, 2011, 12:14 p.m.
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.kumar@linux.vnet.ibm.com
> 
> commit 8c787d8b81aca1f4f7be45adb67b9e1a6dde7f1f
> Author: Aneesh Kumar K.V <aneesh.kumar@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.kumar@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

Patch

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--;;
+        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);
+    }
+}
+
+void qemu_co_rwlock_wrlock(CoRwlock *lock)
+{
+    while (lock->writer || lock->reader) {
+        qemu_co_queue_wait(&lock->queue);
+    }
+    lock->writer++;
+    assert(lock->writer == 1);
+}
diff --git a/qemu-coroutine.h b/qemu-coroutine.h
index 71cfa5a..a9735fb 100644
--- a/qemu-coroutine.h
+++ b/qemu-coroutine.h
@@ -17,6 +17,7 @@ 
 #include <stdbool.h>
 
 #include "qemu-queue.h"
+#include "qemu-thread.h"
 
 /**
  * Coroutines are a mechanism for stack switching and can be used for
@@ -114,4 +115,15 @@  void qemu_co_mutex_init(CoMutex *mutex);
 void qemu_co_mutex_lock(CoMutex *mutex);
 void qemu_co_mutex_unlock(CoMutex *mutex);
 
+typedef struct CoRwlock {
+    bool writer;
+    int reader;
+    CoQueue queue;
+} CoRwlock;
+
+void qemu_co_rwlock_init(CoRwlock *lock);
+void qemu_co_rwlock_rdlock(CoRwlock *lock);
+void qemu_co_rwlock_wrlock(CoRwlock *lock);
+void qemu_co_rwlock_unlock(CoRwlock *lock);
+
 #endif /* QEMU_COROUTINE_H */