Re: [PATCH 1/1] util: adjust coroutine pool size to virtio block queue

[Stefan Hajnoczi]

On Tue, Jan 11, 2022 at 06:19:50PM +0900, Hiroki Narukawa wrote:
> Coroutine pool size was 64 from long ago, and the basis was organized in the 
> commit message in c740ad92.
> 
> At that time, virtio-blk queue-size and num-queue were not configuable, and 
> equivalent values were 128 and 1.
> 
> Coroutine pool size 64 was fine then.
> 
> Later queue-size and num-queue got configuable, and default values were 
> increased.
> 
> Coroutine pool with size 64 exhausts frequently with random disk IO in new 
> size, and slows down.
> 
> This commit adjusts coroutine pool size adaptively with new values.
> 
> This commit adds 64 by default, but now coroutine is not only for block 
> devices,
> 
> and is not too much burdon comparing with new default.
> 
> pool size of 128 * vCPUs.
> 
> Signed-off-by: Hiroki Narukawa <hnarukaw@yahoo-corp.jp>
> ---
>  hw/block/virtio-blk.c    |  3 +++
>  include/qemu/coroutine.h |  5 +++++
>  util/qemu-coroutine.c    | 15 +++++++++++----
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f139cd7cc9..726dbe14de 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -32,6 +32,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "migration/qemu-file-types.h"
>  #include "hw/virtio/virtio-access.h"
> +#include "qemu/coroutine.h"
>  
>  /* Config size before the discard support (hide associated config fields) */
>  #define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
> @@ -1222,6 +1223,8 @@ static void virtio_blk_device_realize(DeviceState *dev, 
> Error **errp)
>      for (i = 0; i < conf->num_queues; i++) {
>          virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
>      }
> +    qemu_coroutine_increase_pool_batch_size(conf->num_queues * 
> conf->queue_size
> +                                            / 2);

Why "/ 2"?

>      virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
>      if (err != NULL) {
>          error_propagate(errp, err);

Please handle hot unplug (->unrealize()) so the coroutine pool shrinks
down again when virtio-blk devices are removed.

My main concern is memory footprint. A burst of I/O can create many
coroutines and they might never be used again. But I think we can deal
with that using a timer in a separate future patch (if necessary).

Thanks,
Stefan

signature.asc
Description: PGP signature