Patchwork [9/9] dataplane: use a QContext event loop in place of custom thread

login
register
mail settings
Submitter Michael Roth
Date May 3, 2013, 4:03 p.m.
Message ID <1367597032-28934-10-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/241340/
State New
Headers show

Comments

Michael Roth - May 3, 2013, 4:03 p.m.
virtio-blk dataplane currently creates/manages it's own thread to
offload work to a separate event loop.

This patch insteads allows us to specify a QContext-based event loop by
adding a "context" property for virtio-blk we can use like so:

  qemu ... \
    -object glib-qcontext,id=ctx0,threaded=yes
    -drive file=file.raw,id=drive0,aio=native,cache=none \
    -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0

virtio-blk dataplane then simply attachs/detaches it's AioContext to the
ctx0 event loop on start/stop.

This also makes available the option to drive a virtio-blk dataplane via
the default main loop:

  qemu ... \
    -drive file=file.raw,id=drive0,aio=native,cache=none \
    -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main

This doesn't do much in and of itself, but helps to demonstrate how we
might model a general mechanism to offload device workloads to separate
threads.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/block/dataplane/virtio-blk.c |   46 ++++++++++++---------------------------
 include/hw/virtio/virtio-blk.h  |    7 ++++--
 2 files changed, 19 insertions(+), 34 deletions(-)
Paolo Bonzini - May 6, 2013, 7:54 a.m.
Il 03/05/2013 18:03, Michael Roth ha scritto:
> virtio-blk dataplane currently creates/manages it's own thread to
> offload work to a separate event loop.
> 
> This patch insteads allows us to specify a QContext-based event loop by
> adding a "context" property for virtio-blk we can use like so:
> 
>   qemu ... \
>     -object glib-qcontext,id=ctx0,threaded=yes
>     -drive file=file.raw,id=drive0,aio=native,cache=none \
>     -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
> 
> virtio-blk dataplane then simply attachs/detaches it's AioContext to the
> ctx0 event loop on start/stop.
> 
> This also makes available the option to drive a virtio-blk dataplane via
> the default main loop:
> 
>   qemu ... \
>     -drive file=file.raw,id=drive0,aio=native,cache=none \
>     -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
> 
> This doesn't do much in and of itself, but helps to demonstrate how we
> might model a general mechanism to offload device workloads to separate
> threads.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/block/dataplane/virtio-blk.c |   46 ++++++++++++---------------------------
>  include/hw/virtio/virtio-blk.h  |    7 ++++--
>  2 files changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 0356665..08ea10f 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -24,6 +24,8 @@
>  #include "virtio-blk.h"
>  #include "block/aio.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "qcontext/qcontext.h"
> +#include "qcontext/glib-qcontext.h"
>  
>  enum {
>      SEG_MAX = 126,                  /* maximum number of I/O segments */
> @@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
>       * use it).
>       */
>      AioContext *ctx;
> +    QContext *qctx;
>      EventNotifier io_notifier;      /* Linux AIO completion */
>      EventNotifier host_notifier;    /* doorbell */
>  
> @@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
>      }
>  }
>  
> -static void *data_plane_thread(void *opaque)
> -{
> -    VirtIOBlockDataPlane *s = opaque;
> -
> -    do {
> -        aio_poll(s->ctx, true);
> -    } while (!s->stopping || s->num_reqs > 0);
> -    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);
> -}
> -
>  bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>                                    VirtIOBlockDataPlane **dataplane)
>  {
> @@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtQueue *vq;
>      int i;
> +    Error *err = NULL;
>  
>      if (s->started) {
>          return;
> @@ -502,9 +486,16 @@ 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);
> +    /* use QEMU main loop/context by default */
> +    if (!s->blk->context) {
> +        s->blk->context = g_strdup("main");
> +    }

Or rather create a device-specific context by default?

Paolo

> +    s->qctx = qcontext_find_by_name(s->blk->context, &err);
> +    if (err) {
> +        fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
> +        exit(1);
> +    }
> +    aio_context_attach(s->ctx, s->qctx);
>  }
>  
>  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> @@ -517,15 +508,6 @@ 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_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
>      ioq_cleanup(&s->ioqueue);
>  
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index fc71853..c5514a4 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -110,6 +110,7 @@ struct VirtIOBlkConf
>      uint32_t scsi;
>      uint32_t config_wce;
>      uint32_t data_plane;
> +    char *context;
>  };
>  
>  struct VirtIOBlockDataPlane;
> @@ -138,13 +139,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_STRING("context", _state, _field.context)
>  #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_STRING("context", _state, _field.context)
>  #endif /* __linux__ */
>  
>  void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>
Michael Roth - May 6, 2013, 7:13 p.m.
On Mon, May 06, 2013 at 09:54:03AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > virtio-blk dataplane currently creates/manages it's own thread to
> > offload work to a separate event loop.
> > 
> > This patch insteads allows us to specify a QContext-based event loop by
> > adding a "context" property for virtio-blk we can use like so:
> > 
> >   qemu ... \
> >     -object glib-qcontext,id=ctx0,threaded=yes
> >     -drive file=file.raw,id=drive0,aio=native,cache=none \
> >     -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
> > 
> > virtio-blk dataplane then simply attachs/detaches it's AioContext to the
> > ctx0 event loop on start/stop.
> > 
> > This also makes available the option to drive a virtio-blk dataplane via
> > the default main loop:
> > 
> >   qemu ... \
> >     -drive file=file.raw,id=drive0,aio=native,cache=none \
> >     -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
> > 
> > This doesn't do much in and of itself, but helps to demonstrate how we
> > might model a general mechanism to offload device workloads to separate
> > threads.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  hw/block/dataplane/virtio-blk.c |   46 ++++++++++++---------------------------
> >  include/hw/virtio/virtio-blk.h  |    7 ++++--
> >  2 files changed, 19 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index 0356665..08ea10f 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -24,6 +24,8 @@
> >  #include "virtio-blk.h"
> >  #include "block/aio.h"
> >  #include "hw/virtio/virtio-bus.h"
> > +#include "qcontext/qcontext.h"
> > +#include "qcontext/glib-qcontext.h"
> >  
> >  enum {
> >      SEG_MAX = 126,                  /* maximum number of I/O segments */
> > @@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
> >       * use it).
> >       */
> >      AioContext *ctx;
> > +    QContext *qctx;
> >      EventNotifier io_notifier;      /* Linux AIO completion */
> >      EventNotifier host_notifier;    /* doorbell */
> >  
> > @@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
> >      }
> >  }
> >  
> > -static void *data_plane_thread(void *opaque)
> > -{
> > -    VirtIOBlockDataPlane *s = opaque;
> > -
> > -    do {
> > -        aio_poll(s->ctx, true);
> > -    } while (!s->stopping || s->num_reqs > 0);
> > -    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);
> > -}
> > -
> >  bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> >                                    VirtIOBlockDataPlane **dataplane)
> >  {
> > @@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> >      VirtQueue *vq;
> >      int i;
> > +    Error *err = NULL;
> >  
> >      if (s->started) {
> >          return;
> > @@ -502,9 +486,16 @@ 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);
> > +    /* use QEMU main loop/context by default */
> > +    if (!s->blk->context) {
> > +        s->blk->context = g_strdup("main");
> > +    }
> 
> Or rather create a device-specific context by default?

Yup, definitely.

I think I did it this way to to give an idea of how a "normal" threaded device
might look (i.e. reworked or written from the start to always be capable of
being driven by a separate event loop)

But x-data-plane=on should imply a new context be used so we don't break
things, and I'll do it that way on the next pass.

> 
> Paolo
> 
> > +    s->qctx = qcontext_find_by_name(s->blk->context, &err);
> > +    if (err) {
> > +        fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
> > +        exit(1);
> > +    }
> > +    aio_context_attach(s->ctx, s->qctx);
> >  }
> >  
> >  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> > @@ -517,15 +508,6 @@ 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_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
> >      ioq_cleanup(&s->ioqueue);
> >  
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index fc71853..c5514a4 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -110,6 +110,7 @@ struct VirtIOBlkConf
> >      uint32_t scsi;
> >      uint32_t config_wce;
> >      uint32_t data_plane;
> > +    char *context;
> >  };
> >  
> >  struct VirtIOBlockDataPlane;
> > @@ -138,13 +139,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_STRING("context", _state, _field.context)
> >  #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_STRING("context", _state, _field.context)
> >  #endif /* __linux__ */
> >  
> >  void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
> > 
>

Patch

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..08ea10f 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,6 +24,8 @@ 
 #include "virtio-blk.h"
 #include "block/aio.h"
 #include "hw/virtio/virtio-bus.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
 
 enum {
     SEG_MAX = 126,                  /* maximum number of I/O segments */
@@ -60,6 +62,7 @@  struct VirtIOBlockDataPlane {
      * use it).
      */
     AioContext *ctx;
+    QContext *qctx;
     EventNotifier io_notifier;      /* Linux AIO completion */
     EventNotifier host_notifier;    /* doorbell */
 
@@ -375,26 +378,6 @@  static void handle_io(EventNotifier *e)
     }
 }
 
-static void *data_plane_thread(void *opaque)
-{
-    VirtIOBlockDataPlane *s = opaque;
-
-    do {
-        aio_poll(s->ctx, true);
-    } while (!s->stopping || s->num_reqs > 0);
-    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);
-}
-
 bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
                                   VirtIOBlockDataPlane **dataplane)
 {
@@ -460,6 +443,7 @@  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtQueue *vq;
     int i;
+    Error *err = NULL;
 
     if (s->started) {
         return;
@@ -502,9 +486,16 @@  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);
+    /* use QEMU main loop/context by default */
+    if (!s->blk->context) {
+        s->blk->context = g_strdup("main");
+    }
+    s->qctx = qcontext_find_by_name(s->blk->context, &err);
+    if (err) {
+        fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
+        exit(1);
+    }
+    aio_context_attach(s->ctx, s->qctx);
 }
 
 void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
@@ -517,15 +508,6 @@  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_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
     ioq_cleanup(&s->ioqueue);
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fc71853..c5514a4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -110,6 +110,7 @@  struct VirtIOBlkConf
     uint32_t scsi;
     uint32_t config_wce;
     uint32_t data_plane;
+    char *context;
 };
 
 struct VirtIOBlockDataPlane;
@@ -138,13 +139,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_STRING("context", _state, _field.context)
 #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_STRING("context", _state, _field.context)
 #endif /* __linux__ */
 
 void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);