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

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

[Stefan Hajnoczi]

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: Please let me know if this eliminates the rt_sigprocmask system calls you
are seeing.  It should solve part of the performance regression you have seen
in qemu.git/master virtio-blk dataplane.

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          | 25 +++++++++++++++++++------
 3 files changed, 34 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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          | 25 +++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 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..d94234a 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -18,15 +18,11 @@
 #include "block/coroutine.h"
 #include "block/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 QemuMutex pool_lock;
 static QSLIST_HEAD(, Coroutine) pool = QSLIST_HEAD_INITIALIZER(pool);
 static unsigned int pool_size;
+static unsigned int pool_max_size = 64;
 
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
@@ -55,7 +51,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 +133,20 @@ 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;
+
+    /* 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 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 1/2] coroutine: make pool size dynamic

[Eric Blake]

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

On 07/03/2014 07:52 AM, Stefan Hajnoczi wrote:
> 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          | 25 +++++++++++++++++++------
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 

> +
> +void qemu_coroutine_adjust_pool_size(int n)
> +{
> +    qemu_mutex_lock(&pool_lock);
> +
> +    pool_max_size += n;

Where do you check that this doesn't fall below a bare-minimum
acceptable level? Is it worth assert(pool_max_size >= 64)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

On 07/03/2014 07:52 AM, Stefan Hajnoczi wrote:
> 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(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Stefan Hajnoczi]

On Thu, Jul 3, 2014 at 3:59 PM, Eric Blake <eblake@redhat.com> wrote:
> On 07/03/2014 07:52 AM, Stefan Hajnoczi wrote:
>> 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          | 25 +++++++++++++++++++------
>>  2 files changed, 30 insertions(+), 6 deletions(-)
>>
>
>> +
>> +void qemu_coroutine_adjust_pool_size(int n)
>> +{
>> +    qemu_mutex_lock(&pool_lock);
>> +
>> +    pool_max_size += n;
>
> Where do you check that this doesn't fall below a bare-minimum
> acceptable level? Is it worth assert(pool_max_size >= 64)?

I don't.  I think you are right; it's worth checking.

Stefan

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

[Ming Lei]

On Thu, Jul 3, 2014 at 9:51 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 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: Please let me know if this eliminates the rt_sigprocmask system calls you
> are seeing.  It should solve part of the performance regression you have seen
> in qemu.git/master virtio-blk dataplane.

With both the two coroutine patches and the block plug&unplug patches,
performance of qemu.git/master virtio-blk dataplane can recover to level of
QEMU 2.0.

>
> 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          | 25 +++++++++++++++++++------
>  3 files changed, 34 insertions(+), 6 deletions(-)
>
> --
> 1.9.3
>
>

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

[Paolo Bonzini]

Il 04/07/2014 07:10, Ming Lei ha scritto:
> With both the two coroutine patches and the block plug&unplug patches,
> performance of qemu.git/master virtio-blk dataplane can recover to level of
> QEMU 2.0.

So the excessive writes (to eventfd and irqfd) are not a problem? 
That'd be a relief. :)

Paolo

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

[Ming Lei]

On Fri, Jul 4, 2014 at 2:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/07/2014 07:10, Ming Lei ha scritto:
>
>> With both the two coroutine patches and the block plug&unplug patches,
>> performance of qemu.git/master virtio-blk dataplane can recover to level
>> of
>> QEMU 2.0.
>
>
> So the excessive writes (to eventfd and irqfd) are not a problem? That'd be
> a relief. :)

I mean it is in a level, but your aio_notify() patch still can improve virtioblk
dataplane performance some, in my test, with 5~10K IOPS improvement,
which should belong to same level, because my test is a bit coarse, :-)

IMO not only virtioblk can benefit from your patch, others can benefit too.

Thanks,
--
Ming Lei

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

[Paolo Bonzini]

Il 04/07/2014 09:02, Ming Lei ha scritto:
>> > So the excessive writes (to eventfd and irqfd) are not a problem? That'd be
>> > a relief. :)
> I mean it is in a level, but your aio_notify() patch still can improve virtioblk
> dataplane performance some, in my test, with 5~10K IOPS improvement,
> which should belong to same level, because my test is a bit coarse, :-)
>
> IMO not only virtioblk can benefit from your patch, others can benefit too.

Definitely, but it's not the kind of patch I enjoy including 3 weeks 
from release.  I have already broken enough eggs during the 2.1 
development period. :)

Paolo

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

[Stefan Hajnoczi]

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

On Fri, Jul 04, 2014 at 01:10:39PM +0800, Ming Lei wrote:
> On Thu, Jul 3, 2014 at 9:51 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 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: Please let me know if this eliminates the rt_sigprocmask system calls you
> > are seeing.  It should solve part of the performance regression you have seen
> > in qemu.git/master virtio-blk dataplane.
> 
> With both the two coroutine patches and the block plug&unplug patches,
> performance of qemu.git/master virtio-blk dataplane can recover to level of
> QEMU 2.0.

/me does the happy dance

Thanks for your great efforts in investigating and fixing the
regression!

Stefan

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