[Qemu-devel] [PATCH 04/15] coroutine: add a knob to disable the shared release pool

[Peter Lieven <pl@kamp.de>]

the current coroutine freelist implementation has 2 kinds of pools.
One shared release pool between all threads and additionally one
allocation pool per thread. The release pool is especially necessary
if the coroutine is created in a different thread than it is released.
This is e.g. the case if an IDE interface is used.

But in times of virtio and dataplane the the release pool adds costs
which are not entirely necessary. At first if virtio is used the release
pool tends to fill up to 100% because all coroutines are first handed
back to the release pool. On coroutine create a thread can steal this
release pool and make it its local allocation pool, but during mixed
I/O pattern at the end the release pool is full of useless coroutines
and the alloc_pool has also filled to maximum size.

So this patch introduces a knob to disable the release pool to avoid
this behaviour. If this switch is used it should be made sure that
all fast block devices use virtio and each virtio device
has its own thread (dataplane).

An IDE cdrom might still be used, but coroutine creation will be slow,
but a CDROM is considred slow anyway.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 configure             | 15 ++++++++--
 util/qemu-coroutine.c | 79 ++++++++++++++++++++++++++++-----------------------
 2 files changed, 56 insertions(+), 38 deletions(-)

diff --git a/configure b/configure
index 82bcc25..fb29034 100755
--- a/configure
+++ b/configure
@@ -296,6 +296,7 @@ libiscsi=""
 libnfs=""
 coroutine=""
 coroutine_pool=""
+coroutine_release_pool="yes"
 coroutine_stack_size_debug="no"
 seccomp=""
 glusterfs=""
@@ -1001,10 +1002,14 @@ for opt do
   ;;
   --with-coroutine=*) coroutine="$optarg"
   ;;
-  --disable-coroutine-pool) coroutine_pool="no"
+  --disable-coroutine-pool)
+      coroutine_pool="no"
+      coroutine_release_pool="no"
   ;;
   --enable-coroutine-pool) coroutine_pool="yes"
   ;;
+  --disable-coroutine-release-pool) coroutine_release_pool="no"
+  ;;
   --enable-coroutine-stack-size-debug) coroutine_stack_size_debug="yes"
   ;;
   --disable-docs) docs="no"
@@ -1364,6 +1369,7 @@ disabled with --disable-FEATURE, default is enabled if available:
                   (for reading bzip2-compressed dmg images)
   seccomp         seccomp support
   coroutine-pool  coroutine freelist (better performance)
+  coroutine-release-pool  coroutine freelist is shared between threads
   coroutine-stack-size-debug
                   report coroutine max stack usage (only for debugging)
   glusterfs       GlusterFS backend
@@ -4310,6 +4316,7 @@ if test "$coroutine_stack_size_debug" = "yes"; then
   if test "$coroutine_pool" = "yes"; then
     echo "WARN: disabling coroutine pool for stack size debugging"
     coroutine_pool=no
+    coroutine_release_pool=no
   fi
 fi
 
@@ -4880,6 +4887,7 @@ echo "QGA MSI support   $guest_agent_msi"
 echo "seccomp support   $seccomp"
 echo "coroutine backend $coroutine"
 echo "coroutine pool    $coroutine_pool"
+echo "coroutine release pool    $coroutine_release_pool"
 echo "coroutine stack size debug $coroutine_stack_size_debug"
 echo "GlusterFS support $glusterfs"
 echo "Archipelago support $archipelago"
@@ -5347,12 +5355,13 @@ fi
 echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
 if test "$coroutine_pool" = "yes" ; then
   echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
-else
-  echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
 fi
 if test "$coroutine_stack_size_debug" = "yes" ; then
   echo "CONFIG_COROUTINE_STACK_SIZE_DEBUG=y" >> $config_host_mak
 fi
+if test "$coroutine_release_pool" = "yes"; then
+  echo "CONFIG_COROUTINE_RELEASE_POOL=y" >> $config_host_mak
+fi
 
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index 5816702..7dda0ca 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -20,13 +20,12 @@
 #include "qemu/coroutine.h"
 #include "qemu/coroutine_int.h"
 
+#ifdef CONFIG_COROUTINE_POOL
+/* per thread free list to speed up creation */
 enum {
     POOL_BATCH_SIZE = 64,
 };
 
-/** Free list to speed up creation */
-static QSLIST_HEAD(, Coroutine) release_pool = QSLIST_HEAD_INITIALIZER(pool);
-static unsigned int release_pool_size;
 static __thread QSLIST_HEAD(, Coroutine) alloc_pool = QSLIST_HEAD_INITIALIZER(pool);
 static __thread unsigned int alloc_pool_size;
 static __thread Notifier coroutine_pool_cleanup_notifier;
@@ -41,35 +40,43 @@ static void coroutine_pool_cleanup(Notifier *n, void *value)
         qemu_coroutine_delete(co);
     }
 }
+#endif
+#ifdef CONFIG_COROUTINE_RELEASE_POOL
+/* add an additional shared release pool */
+static QSLIST_HEAD(, Coroutine) release_pool = QSLIST_HEAD_INITIALIZER(pool);
+static unsigned int release_pool_size;
+#endif
 
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
     Coroutine *co = NULL;
 
-    if (CONFIG_COROUTINE_POOL) {
-        co = QSLIST_FIRST(&alloc_pool);
-        if (!co) {
-            if (release_pool_size > POOL_BATCH_SIZE) {
-                /* Slow path; a good place to register the destructor, too.  */
-                if (!coroutine_pool_cleanup_notifier.notify) {
-                    coroutine_pool_cleanup_notifier.notify = coroutine_pool_cleanup;
-                    qemu_thread_atexit_add(&coroutine_pool_cleanup_notifier);
-                }
-
-                /* This is not exact; there could be a little skew between
-                 * release_pool_size and the actual size of release_pool.  But
-                 * it is just a heuristic, it does not need to be perfect.
-                 */
-                alloc_pool_size = atomic_xchg(&release_pool_size, 0);
-                QSLIST_MOVE_ATOMIC(&alloc_pool, &release_pool);
-                co = QSLIST_FIRST(&alloc_pool);
-            }
+#ifdef CONFIG_COROUTINE_POOL
+    co = QSLIST_FIRST(&alloc_pool);
+    if (!co) {
+        /* Slow path; a good place to register the destructor, too.  */
+        if (!coroutine_pool_cleanup_notifier.notify) {
+            coroutine_pool_cleanup_notifier.notify = coroutine_pool_cleanup;
+            qemu_thread_atexit_add(&coroutine_pool_cleanup_notifier);
         }
-        if (co) {
-            QSLIST_REMOVE_HEAD(&alloc_pool, pool_next);
-            alloc_pool_size--;
+#ifdef CONFIG_COROUTINE_RELEASE_POOL
+        if (release_pool_size > POOL_BATCH_SIZE) {
+
+            /* This is not exact; there could be a little skew between
+             * release_pool_size and the actual size of release_pool.  But
+             * it is just a heuristic, it does not need to be perfect.
+             */
+            alloc_pool_size = atomic_xchg(&release_pool_size, 0);
+            QSLIST_MOVE_ATOMIC(&alloc_pool, &release_pool);
+            co = QSLIST_FIRST(&alloc_pool);
         }
+#endif
+    }
+    if (co) {
+        QSLIST_REMOVE_HEAD(&alloc_pool, pool_next);
+        alloc_pool_size--;
     }
+#endif
 
     if (!co) {
         co = qemu_coroutine_new();
@@ -84,18 +91,20 @@ static void coroutine_delete(Coroutine *co)
 {
     co->caller = NULL;
 
-    if (CONFIG_COROUTINE_POOL) {
-        if (release_pool_size < POOL_BATCH_SIZE * 2) {
-            QSLIST_INSERT_HEAD_ATOMIC(&release_pool, co, pool_next);
-            atomic_inc(&release_pool_size);
-            return;
-        }
-        if (alloc_pool_size < POOL_BATCH_SIZE) {
-            QSLIST_INSERT_HEAD(&alloc_pool, co, pool_next);
-            alloc_pool_size++;
-            return;
-        }
+#ifdef CONFIG_COROUTINE_RELEASE_POOL
+    if (release_pool_size < POOL_BATCH_SIZE * 2) {
+        QSLIST_INSERT_HEAD_ATOMIC(&release_pool, co, pool_next);
+        atomic_inc(&release_pool_size);
+        return;
+    }
+#endif
+#ifdef CONFIG_COROUTINE_POOL
+    if (alloc_pool_size < POOL_BATCH_SIZE) {
+        QSLIST_INSERT_HEAD(&alloc_pool, co, pool_next);
+        alloc_pool_size++;
+        return;
     }
+#endif
 
     qemu_coroutine_delete(co);
 }
-- 
1.9.1