Re: [Qemu-devel] [PATCH v3 6/6] dataplane: replace internal thread with IOThread

[Paolo Bonzini <pbonzini@redhat.com>]

Il 20/02/2014 13:50, Stefan Hajnoczi ha scritto:
> Today virtio-blk dataplane uses a 1:1 device-per-thread model.  Now that
> IOThreads have been introduced we can generalize this to N:M devices per
> threads.
>
> This patch drops thread code from dataplane in favor of running inside
> an IOThread AioContext.
>
> As a bonus we solve the case where a guest keeps submitting I/O requests
> while dataplane is trying to stop.  Previously the dataplane thread
> would continue to process requests until the request gave it a break.
> Now we can shut down in bounded time thanks to
> aio_context_acquire/release.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 96 +++++++++++++++++++++++------------------
>  include/hw/virtio/virtio-blk.h  |  8 +++-
>  2 files changed, 60 insertions(+), 44 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 2237edb..a5afc21 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -23,6 +23,7 @@
>  #include "virtio-blk.h"
>  #include "block/aio.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "monitor/monitor.h" /* for object_add() */
>
>  enum {
>      SEG_MAX = 126,                  /* maximum number of I/O segments */
> @@ -44,8 +45,6 @@ struct VirtIOBlockDataPlane {
>      bool started;
>      bool starting;
>      bool stopping;
> -    QEMUBH *start_bh;
> -    QemuThread thread;
>
>      VirtIOBlkConf *blk;
>      int fd;                         /* image file descriptor */
> @@ -59,12 +58,14 @@ struct VirtIOBlockDataPlane {
>       * (because you don't own the file descriptor or handle; you just
>       * use it).
>       */
> +    IOThread *iothread;
> +    bool internal_iothread;
>      AioContext *ctx;
>      EventNotifier io_notifier;      /* Linux AIO completion */
>      EventNotifier host_notifier;    /* doorbell */
>
>      IOQueue ioqueue;                /* Linux AIO queue (should really be per
> -                                       dataplane thread) */
> +                                       IOThread) */
>      VirtIOBlockRequest requests[REQ_MAX]; /* pool of requests, managed by the
>                                               queue */
>
> @@ -342,26 +343,7 @@ static void handle_io(EventNotifier *e)
>      }
>  }
>
> -static void *data_plane_thread(void *opaque)
> -{
> -    VirtIOBlockDataPlane *s = opaque;
> -
> -    while (!s->stopping || s->num_reqs > 0) {
> -        aio_poll(s->ctx, true);
> -    }
> -    return NULL;
> -}
> -
> -static void start_data_plane_bh(void *opaque)
> -{
> -    VirtIOBlockDataPlane *s = opaque;
> -
> -    qemu_bh_delete(s->start_bh);
> -    s->start_bh = NULL;
> -    qemu_thread_create(&s->thread, data_plane_thread,
> -                       s, QEMU_THREAD_JOINABLE);
> -}
> -
> +/* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>                                    VirtIOBlockDataPlane **dataplane,
>                                    Error **errp)
> @@ -408,12 +390,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>      s->fd = fd;
>      s->blk = blk;
>
> +    if (blk->iothread) {
> +        s->internal_iothread = false;
> +        s->iothread = blk->iothread;
> +        object_ref(OBJECT(s->iothread));
> +    } else {
> +        /* Create per-device IOThread if none specified */
> +        Error *local_err = NULL;
> +
> +        s->internal_iothread = true;
> +        object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err);
> +        if (error_is_set(&local_err)) {
> +            error_propagate(errp, local_err);
> +            g_free(s);
> +            return;
> +        }
> +        s->iothread = iothread_find(vdev->name);
> +        assert(s->iothread);
> +    }
> +    s->ctx = iothread_get_aio_context(s->iothread);
> +
>      /* Prevent block operations that conflict with data plane thread */
>      bdrv_set_in_use(blk->conf.bs, 1);
>
>      *dataplane = s;
>  }
>
> +/* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>  {
>      if (!s) {
> @@ -422,9 +425,14 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>
>      virtio_blk_data_plane_stop(s);
>      bdrv_set_in_use(s->blk->conf.bs, 0);
> +    object_unref(OBJECT(s->iothread));
> +    if (s->internal_iothread) {
> +        object_unparent(OBJECT(s->iothread));
> +    }
>      g_free(s);
>  }
>
> +/* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
> @@ -448,8 +456,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>          return;
>      }
>
> -    s->ctx = aio_context_new();
> -
>      /* Set up guest notifier (irq) */
>      if (k->set_guest_notifiers(qbus->parent, 1, true) != 0) {
>          fprintf(stderr, "virtio-blk failed to set guest notifier, "
> @@ -464,7 +470,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>          exit(1);
>      }
>      s->host_notifier = *virtio_queue_get_host_notifier(vq);
> -    aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
>
>      /* Set up ioqueue */
>      ioq_init(&s->ioqueue, s->fd, REQ_MAX);
> @@ -472,7 +477,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>          ioq_put_iocb(&s->ioqueue, &s->requests[i].iocb);
>      }
>      s->io_notifier = *ioq_get_notifier(&s->ioqueue);
> -    aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io);
>
>      s->starting = false;
>      s->started = true;
> @@ -481,11 +485,14 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      /* Kick right away to begin processing requests already in vring */
>      event_notifier_set(virtio_queue_get_host_notifier(vq));
>
> -    /* Spawn thread in BH so it inherits iothread cpusets */
> -    s->start_bh = qemu_bh_new(start_data_plane_bh, s);
> -    qemu_bh_schedule(s->start_bh);
> +    /* Get this show started by hooking up our callbacks */
> +    aio_context_acquire(s->ctx);
> +    aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
> +    aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io);
> +    aio_context_release(s->ctx);
>  }
>
> +/* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>  {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
> @@ -496,27 +503,32 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      s->stopping = true;
>      trace_virtio_blk_data_plane_stop(s);
>
> -    /* Stop thread or cancel pending thread creation BH */
> -    if (s->start_bh) {
> -        qemu_bh_delete(s->start_bh);
> -        s->start_bh = NULL;
> -    } else {
> -        aio_notify(s->ctx);
> -        qemu_thread_join(&s->thread);
> +    aio_context_acquire(s->ctx);
> +
> +    /* Stop notifications for new requests from guest */
> +    aio_set_event_notifier(s->ctx, &s->host_notifier, NULL);
> +
> +    /* Complete pending requests */
> +    while (s->num_reqs > 0) {
> +        aio_poll(s->ctx, true);
>      }
>
> +    /* Stop ioq callbacks (there are no pending requests left) */
>      aio_set_event_notifier(s->ctx, &s->io_notifier, NULL);
> -    ioq_cleanup(&s->ioqueue);
>
> -    aio_set_event_notifier(s->ctx, &s->host_notifier, NULL);
> -    k->set_host_notifier(qbus->parent, 0, false);
> +    aio_context_release(s->ctx);
>
> -    aio_context_unref(s->ctx);
> +    /* Sync vring state back to virtqueue so that non-dataplane request
> +     * processing can continue when we disable the host notifier below.
> +     */
> +    vring_teardown(&s->vring, s->vdev, 0);
> +
> +    ioq_cleanup(&s->ioqueue);
> +    k->set_host_notifier(qbus->parent, 0, false);
>
>      /* Clean up guest notifier (irq) */
>      k->set_guest_notifiers(qbus->parent, 1, false);
>
> -    vring_teardown(&s->vring, s->vdev, 0);
>      s->started = false;
>      s->stopping = false;
>  }
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 41885da..12193fd 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -16,6 +16,7 @@
>
>  #include "hw/virtio/virtio.h"
>  #include "hw/block/block.h"
> +#include "sysemu/iothread.h"
>
>  #define TYPE_VIRTIO_BLK "virtio-blk-device"
>  #define VIRTIO_BLK(obj) \
> @@ -106,6 +107,7 @@ struct virtio_scsi_inhdr
>  struct VirtIOBlkConf
>  {
>      BlockConf conf;
> +    IOThread *iothread;
>      char *serial;
>      uint32_t scsi;
>      uint32_t config_wce;
> @@ -140,13 +142,15 @@ typedef struct VirtIOBlock {
>          DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
>          DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
>          DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
> -        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true)
> +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),                \
> +        DEFINE_PROP_IOTHREAD("iothread", _state, _field.iothread)
>  #else
>  #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
>          DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
>          DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
>          DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
> -        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
> +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
> +        DEFINE_PROP_IOTHREAD("iothread", _state, _field.iothread)

Please make this x-iothread.  I think not-so-long term we should make 
this a link<>, and I'd rather not have people rely on any ABI from 
-device virtio-blk-pci,?

Paolo