diff mbox

coroutine: always use pooling

Message ID 1348577046-12643-1-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Sept. 25, 2012, 12:44 p.m. UTC 
It makes sense to use it for other implementations than ucontext, too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 coroutine-ucontext.c | 43 +------------------------------------------
 qemu-coroutine.c     | 38 ++++++++++++++++++++++++++++++++++++--
 2 file modificati, 37 inserzioni(+), 44 rimozioni(-)

Comments

Peter Maydell Sept. 26, 2012, 2:34 p.m. UTC | #1 
On 25 September 2012 13:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It makes sense to use it for other implementations than ucontext, too.
>  Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
>  {
> -    Coroutine *co = qemu_coroutine_new();
> +    Coroutine *co;
> +
> +    co = QSLIST_FIRST(&pool);
> +    if (co) {
> +        QSLIST_REMOVE_HEAD(&pool, pool_next);
> +        pool_size--;
> +    } else {
> +        co = qemu_coroutine_new();
> +    }
>      co->entry = entry;
>      return co;
>  }

Since this is obviously going to blow up badly if it's called
from multiple threads, is there some kind of assert we can add
so that we fail obviously if somebody makes that coding error?
[the difficulty is probably when your backend is the gthread
one, at least I assume creating a coroutine inside a coroutine
is allowed.]

-- PMM
Paolo Bonzini Sept. 26, 2012, 3:18 p.m. UTC | #2 
Il 26/09/2012 16:34, Peter Maydell ha scritto:
>> It makes sense to use it for other implementations than ucontext, too.
>> >  Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
>> >  {
>> > -    Coroutine *co = qemu_coroutine_new();
>> > +    Coroutine *co;
>> > +
>> > +    co = QSLIST_FIRST(&pool);
>> > +    if (co) {
>> > +        QSLIST_REMOVE_HEAD(&pool, pool_next);
>> > +        pool_size--;
>> > +    } else {
>> > +        co = qemu_coroutine_new();
>> > +    }
>> >      co->entry = entry;
>> >      return co;
>> >  }
> Since this is obviously going to blow up badly if it's called
> from multiple threads, is there some kind of assert we can add
> so that we fail obviously if somebody makes that coding error?
> [the difficulty is probably when your backend is the gthread
> one, at least I assume creating a coroutine inside a coroutine
> is allowed.]

Yes, it is; however, creating a coroutine outside the big QEMU lock is
not allowed.  Since one coroutine only runs at a time the code is safe,
just like the other global variables that are used in
qemu-coroutine-lock.c for example.

Paolo
diff mbox

Patch

diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c
index 784081a..744f4f6 100644
--- a/coroutine-ucontext.c
+++ b/coroutine-ucontext.c
@@ -34,15 +34,6 @@ 
 #include <valgrind/valgrind.h>
 #endif
 
-enum {
-    /* Maximum free pool size prevents holding too many freed coroutines */
-    POOL_MAX_SIZE = 64,
-};
-
-/** Free list to speed up creation */
-static QSLIST_HEAD(, Coroutine) pool = QSLIST_HEAD_INITIALIZER(pool);
-static unsigned int pool_size;
-
 typedef struct {
     Coroutine base;
     void *stack;
@@ -96,17 +87,6 @@  static void qemu_coroutine_thread_cleanup(void *opaque)
     g_free(s);
 }
 
-static void __attribute__((destructor)) coroutine_cleanup(void)
-{
-    Coroutine *co;
-    Coroutine *tmp;
-
-    QSLIST_FOREACH_SAFE(co, &pool, pool_next, tmp) {
-        g_free(DO_UPCAST(CoroutineUContext, base, co)->stack);
-        g_free(co);
-    }
-}
-
 static void __attribute__((constructor)) coroutine_init(void)
 {
     int ret;
@@ -140,7 +120,7 @@  static void coroutine_trampoline(int i0, int i1)
     }
 }
 
-static Coroutine *coroutine_new(void)
+Coroutine *qemu_coroutine_new(void)
 {
     const size_t stack_size = 1 << 20;
     CoroutineUContext *co;
@@ -185,20 +165,6 @@  static Coroutine *coroutine_new(void)
     return &co->base;
 }
 
-Coroutine *qemu_coroutine_new(void)
-{
-    Coroutine *co;
-
-    co = QSLIST_FIRST(&pool);
-    if (co) {
-        QSLIST_REMOVE_HEAD(&pool, pool_next);
-        pool_size--;
-    } else {
-        co = coroutine_new();
-    }
-    return co;
-}
-
 #ifdef CONFIG_VALGRIND_H
 #ifdef CONFIG_PRAGMA_DISABLE_UNUSED_BUT_SET
 /* Work around an unused variable in the valgrind.h macro... */
@@ -217,13 +183,6 @@  void qemu_coroutine_delete(Coroutine *co_)
 {
     CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
 
-    if (pool_size < POOL_MAX_SIZE) {
-        QSLIST_INSERT_HEAD(&pool, &co->base, pool_next);
-        co->base.caller = NULL;
-        pool_size++;
-        return;
-    }
-
 #ifdef CONFIG_VALGRIND_H
     valgrind_stack_deregister(co);
 #endif
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 600be26..5a86837 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -17,9 +17,26 @@ 
 #include "qemu-coroutine.h"
 #include "qemu-coroutine-int.h"
 
+enum {
+    /* Maximum free pool size prevents holding too many freed coroutines */
+    POOL_MAX_SIZE = 64,
+};
+
+/** Free list to speed up creation */
+static QSLIST_HEAD(, Coroutine) pool = QSLIST_HEAD_INITIALIZER(pool);
+static unsigned int pool_size;
+
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
-    Coroutine *co = qemu_coroutine_new();
+    Coroutine *co;
+
+    co = QSLIST_FIRST(&pool);
+    if (co) {
+        QSLIST_REMOVE_HEAD(&pool, pool_next);
+        pool_size--;
+    } else {
+        co = qemu_coroutine_new();
+    }
     co->entry = entry;
     return co;
 }
@@ -35,13 +52,30 @@  static void coroutine_swap(Coroutine *from, Coroutine *to)
         return;
     case COROUTINE_TERMINATE:
         trace_qemu_coroutine_terminate(to);
-        qemu_coroutine_delete(to);
+
+        if (pool_size < POOL_MAX_SIZE) {
+            QSLIST_INSERT_HEAD(&pool, to, pool_next);
+            to->caller = NULL;
+            pool_size++;
+        } else {
+            qemu_coroutine_delete(to);
+        }
         return;
     default:
         abort();
     }
 }
 
+static void __attribute__((destructor)) coroutine_cleanup(void)
+{
+    Coroutine *co;
+    Coroutine *tmp;
+
+    QSLIST_FOREACH_SAFE(co, &pool, pool_next, tmp) {
+        qemu_coroutine_delete(co);
+    }
+}
+
 void qemu_coroutine_enter(Coroutine *co, void *opaque)
 {
     Coroutine *self = qemu_coroutine_self();