[Qemu-devel] [PATCH v2 0/2] coroutine: dynamically scale pool size

[Qemu-devel] [PATCH v2 0/2] coroutine: dynamically scale pool size

[Stefan Hajnoczi]

v2:
 * Assert that callers never reduce pool below default size [eblake]

The coroutine pool reuses exited coroutines to make qemu_coroutine_create()
cheap.  The size of the pool is capped to prevent it from hogging memory after
a period of high coroutine activity.  Previously the max size was hardcoded to
64 but this doesn't scale with guest size.

A guest with lots of disks can do more parallel I/O and therefore requires a
larger coroutine pool size.  This series tries to solve the problem by scaling
pool size according to the number of drives.

Ming has confirmed that this patch series, together with his block plug/unplug
series, solves the dataplane performance regression in QEMU 2.1.

Stefan Hajnoczi (2):
  coroutine: make pool size dynamic
  block: bump coroutine pool size for drives

 block.c                   |  4 ++++
 include/block/coroutine.h | 11 +++++++++++
 qemu-coroutine.c          | 26 +++++++++++++++++++++++---
 3 files changed, 38 insertions(+), 3 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] coroutine: make pool size dynamic

[Stefan Hajnoczi]

Allow coroutine users to adjust the pool size.  For example, if the
guest has multiple emulated disk drives we should keep around more
coroutines.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/coroutine.h | 11 +++++++++++
 qemu-coroutine.c          | 26 +++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index a1797ae..07eeb3c 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -223,4 +223,15 @@ void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
  * Note that this function clobbers the handlers for the file descriptor.
  */
 void coroutine_fn yield_until_fd_readable(int fd);
+
+/**
+ * Add or subtract from the coroutine pool size
+ *
+ * The coroutine implementation keeps a pool of coroutines to be reused by
+ * qemu_coroutine_create().  This makes coroutine creation cheap.  Heavy
+ * coroutine users should call this to reserve pool space.  Call it again with
+ * a negative number to release pool space.
+ */
+void qemu_coroutine_adjust_pool_size(int n);
+
 #endif /* QEMU_COROUTINE_H */
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 4708521..bd574aa 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -19,14 +19,14 @@
 #include "block/coroutine_int.h"
 
 enum {
-    /* Maximum free pool size prevents holding too many freed coroutines */
-    POOL_MAX_SIZE = 64,
+    POOL_DEFAULT_SIZE = 64,
 };
 
 /** Free list to speed up creation */
 static QemuMutex pool_lock;
 static QSLIST_HEAD(, Coroutine) pool = QSLIST_HEAD_INITIALIZER(pool);
 static unsigned int pool_size;
+static unsigned int pool_max_size = POOL_DEFAULT_SIZE;
 
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
@@ -55,7 +55,7 @@ static void coroutine_delete(Coroutine *co)
 {
     if (CONFIG_COROUTINE_POOL) {
         qemu_mutex_lock(&pool_lock);
-        if (pool_size < POOL_MAX_SIZE) {
+        if (pool_size < pool_max_size) {
             QSLIST_INSERT_HEAD(&pool, co, pool_next);
             co->caller = NULL;
             pool_size++;
@@ -137,3 +137,23 @@ void coroutine_fn qemu_coroutine_yield(void)
     self->caller = NULL;
     coroutine_swap(self, to);
 }
+
+void qemu_coroutine_adjust_pool_size(int n)
+{
+    qemu_mutex_lock(&pool_lock);
+
+    pool_max_size += n;
+
+    /* Callers should never take away more than they added */
+    assert(pool_max_size >= POOL_DEFAULT_SIZE);
+
+    /* Trim oversized pool down to new max */
+    while (pool_size > pool_max_size) {
+        Coroutine *co = QSLIST_FIRST(&pool);
+        QSLIST_REMOVE_HEAD(&pool, pool_next);
+        pool_size--;
+        qemu_coroutine_delete(co);
+    }
+
+    qemu_mutex_unlock(&pool_lock);
+}
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Stefan Hajnoczi]

When a BlockDriverState is associated with a storage controller
DeviceState we expect guest I/O.  Use this opportunity to bump the
coroutine pool size by 64.

This patch ensures that the coroutine pool size scales with the number
of drives attached to the guest.  It should increase coroutine pool
usage (which makes qemu_coroutine_create() fast) without hogging too
much memory when fewer drives are attached.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index f80e2b2..c8379ca 100644
--- a/block.c
+++ b/block.c
@@ -2093,6 +2093,9 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
     }
     bs->dev = dev;
     bdrv_iostatus_reset(bs);
+
+    /* We're expecting I/O from the device so bump up coroutine pool size */
+    qemu_coroutine_adjust_pool_size(64);
     return 0;
 }
 
@@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
     bs->dev_ops = NULL;
     bs->dev_opaque = NULL;
     bs->guest_block_size = 512;
+    qemu_coroutine_adjust_pool_size(-64);
 }
 
 /* TODO change to return DeviceState * when all users are qdevified */
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> When a BlockDriverState is associated with a storage controller
> DeviceState we expect guest I/O.  Use this opportunity to bump the
> coroutine pool size by 64.
>
> This patch ensures that the coroutine pool size scales with the number
> of drives attached to the guest.  It should increase coroutine pool
> usage (which makes qemu_coroutine_create() fast) without hogging too
> much memory when fewer drives are attached.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/block.c b/block.c
> index f80e2b2..c8379ca 100644
> --- a/block.c
> +++ b/block.c
> @@ -2093,6 +2093,9 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
>      }
>      bs->dev = dev;
>      bdrv_iostatus_reset(bs);
> +
> +    /* We're expecting I/O from the device so bump up coroutine pool size */
> +    qemu_coroutine_adjust_pool_size(64);
>      return 0;
>  }
>  
> @@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
>      bs->dev_ops = NULL;
>      bs->dev_opaque = NULL;
>      bs->guest_block_size = 512;
> +    qemu_coroutine_adjust_pool_size(-64);
>  }
>  
>  /* TODO change to return DeviceState * when all users are qdevified */

This enlarges the pool regardless of how the device model uses the block
layer.  Isn't this a bit crude?

Have you considered adapting the number of coroutines to actual demand?
Within reasonable limits, of course.

Re: [Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Fri, Jul 04, 2014 at 12:03:27PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> > @@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
> >      bs->dev_ops = NULL;
> >      bs->dev_opaque = NULL;
> >      bs->guest_block_size = 512;
> > +    qemu_coroutine_adjust_pool_size(-64);
> >  }
> >  
> >  /* TODO change to return DeviceState * when all users are qdevified */
> 
> This enlarges the pool regardless of how the device model uses the block
> layer.  Isn't this a bit crude?
> 
> Have you considered adapting the number of coroutines to actual demand?
> Within reasonable limits, of course.

I picked the simplest algorithm because I couldn't think of one which is
clearly better.  We cannot predict future coroutine usage so any
algorithm will have pathological cases.

In this case we might as well stick to the simplest implementation.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Jul 04, 2014 at 12:03:27PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> > @@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
>> >      bs->dev_ops = NULL;
>> >      bs->dev_opaque = NULL;
>> >      bs->guest_block_size = 512;
>> > +    qemu_coroutine_adjust_pool_size(-64);
>> >  }
>> >  
>> >  /* TODO change to return DeviceState * when all users are qdevified */
>> 
>> This enlarges the pool regardless of how the device model uses the block
>> layer.  Isn't this a bit crude?
>> 
>> Have you considered adapting the number of coroutines to actual demand?
>> Within reasonable limits, of course.
>
> I picked the simplest algorithm because I couldn't think of one which is
> clearly better.  We cannot predict future coroutine usage so any
> algorithm will have pathological cases.

Dynamically adapting to actual usage arguably involves less predicting
than 64 * #block backends.  Grow the pool some when we're running out of
coroutines, shrink it some when it's been underutilized for some time.

> In this case we might as well stick to the simplest implementation.

Keeping it simple is always a weighty argument.

Re: [Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Lluís Vilanova]

Stefan Hajnoczi writes:

> When a BlockDriverState is associated with a storage controller
> DeviceState we expect guest I/O.  Use this opportunity to bump the
> coroutine pool size by 64.

> This patch ensures that the coroutine pool size scales with the number
> of drives attached to the guest.  It should increase coroutine pool
> usage (which makes qemu_coroutine_create() fast) without hogging too
> much memory when fewer drives are attached.

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block.c | 4 ++++
>  1 file changed, 4 insertions(+)

> diff --git a/block.c b/block.c
> index f80e2b2..c8379ca 100644
> --- a/block.c
> +++ b/block.c
> @@ -2093,6 +2093,9 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
>      }
bs-> dev = dev;
>      bdrv_iostatus_reset(bs);
> +
> +    /* We're expecting I/O from the device so bump up coroutine pool size */
> +    qemu_coroutine_adjust_pool_size(64);
>      return 0;
>  }
 
> @@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
bs-> dev_ops = NULL;
bs-> dev_opaque = NULL;
bs-> guest_block_size = 512;
> +    qemu_coroutine_adjust_pool_size(-64);
>  }
 
>  /* TODO change to return DeviceState * when all users are qdevified */

Small nitpick. Wouldn't it be better to refactor that constant to a define/enum
(like in POOL_DEFAULT_SIZE)?


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v2 2/2] block: bump coroutine pool size for drives

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Fri, Jul 04, 2014 at 12:36:13PM +0200, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> > @@ -2093,6 +2093,9 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
> >      }
> bs-> dev = dev;
> >      bdrv_iostatus_reset(bs);
> > +
> > +    /* We're expecting I/O from the device so bump up coroutine pool size */
> > +    qemu_coroutine_adjust_pool_size(64);
> >      return 0;
> >  }
>  
> > @@ -2112,6 +2115,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
> bs-> dev_ops = NULL;
> bs-> dev_opaque = NULL;
> bs-> guest_block_size = 512;
> > +    qemu_coroutine_adjust_pool_size(-64);
> >  }
>  
> >  /* TODO change to return DeviceState * when all users are qdevified */
> 
> Small nitpick. Wouldn't it be better to refactor that constant to a define/enum
> (like in POOL_DEFAULT_SIZE)?

You are right.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]