diff mbox

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

Message ID 1467104499-27517-5-git-send-email-pl@kamp.de (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Lieven June 28, 2016, 9:01 a.m. UTC
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(-)

Comments

Paolo Bonzini June 28, 2016, 10:41 a.m. UTC | #1
On 28/06/2016 11:01, Peter Lieven wrote:
> 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.

The cost is 2^16 bytes * 2^6 coroutines, that is 4 MB.  I don't think
this is worth an extra knob that no one will use...

Paolo
Peter Lieven June 28, 2016, 10:47 a.m. UTC | #2
Am 28.06.2016 um 12:41 schrieb Paolo Bonzini:
>
> On 28/06/2016 11:01, Peter Lieven wrote:
>> 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.
> The cost is 2^16 bytes * 2^6 coroutines, that is 4 MB.  I don't think
> this is worth an extra knob that no one will use...

You are right, I was having this patch prior to reducing the stack size.
In fact its 2 * 2^6 coroutines. But maybe its worth of thinking making
the release_pool the same max size as the alloc_pool? Otherwise when
the release_pool is stolen it could make the alloc_pool also that big.

Peter
diff mbox

Patch

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);
 }