[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in pla
From: |
mdroth |
Subject: |
Re: [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread |
Date: |
Mon, 6 May 2013 14:13:43 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> > ---
> > 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);
> >
>
- [Qemu-devel] [PATCH 3/9] QSource: QEMU event source object, (continued)
- [Qemu-devel] [PATCH 3/9] QSource: QEMU event source object, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 5/9] GlibQContext: a QContext wrapper around GMainContexts, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 4/9] QContext: QEMU event loop context, abstract base class, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 6/9] QContext: add unit tests, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 8/9] main-loop: drive main event loop via QContext, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource, Michael Roth, 2013/05/03
- [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread, Michael Roth, 2013/05/03
- Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops, liu ping fan, 2013/05/05
- Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops, Paolo Bonzini, 2013/05/06
- Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops, mdroth, 2013/05/06