Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
On Mon, 09/22 10:09, Paolo Bonzini wrote: > Il 22/09/2014 07:56, Fam Zheng ha scritto: > >> > > >> > Please add these to VirtIOSCSI rather than VirtIOSCSICommon. Same for > >> > the new functions you declare below. > > What's the rationale, please? Asking because especially the VirtIOSCSIVring > > fields are the dataplane counterparts of VirtQueue fields, so putting in > > VirtIOSCSI seems unnatural for me. > > Because everything you put in VirtIOSCSICommon will be shared between > virtio-scsi and vhost-scsi, and vhost-scsi need not use neither vring > nor dataplane. > I see, I'll move it into VirtIOSCSI then! Fam
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
Il 22/09/2014 07:56, Fam Zheng ha scritto: >> > >> > Please add these to VirtIOSCSI rather than VirtIOSCSICommon. Same for >> > the new functions you declare below. > What's the rationale, please? Asking because especially the VirtIOSCSIVring > fields are the dataplane counterparts of VirtQueue fields, so putting in > VirtIOSCSI seems unnatural for me. Because everything you put in VirtIOSCSICommon will be shared between virtio-scsi and vhost-scsi, and vhost-scsi need not use neither vring nor dataplane. Paolo
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
On Fri, 09/19 11:29, Paolo Bonzini wrote: > Il 06/08/2014 07:35, Fam Zheng ha scritto: > > +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s) > > +{ > > +int i; > > +int rc; > > +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); > > +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > > + > > +if (s->dataplane_started || > > +s->dataplane_starting || > > +s->ctx != iothread_get_aio_context(s->conf.iothread)) { > > +return; > > +} > > + > > +s->dataplane_starting = true; > > Please do a bdrv_drain_all() here. Then you can set the contexts for > all children here too (for the hotplug case: in virtio_scsi_hotplug). > > Then you do not have to do it in virtio_scsi_do_tmf and > virtio_scsi_handle_cmd_req. Do we want multiple iothreads in the future? If yes, the aio context will need to be checked/set for each command, that's why I put it in virtio_scsi_do_tmf and virtio_scsi_handle_cmd_req. Fam > > > +/* Set up guest notifier (irq) */ > > +rc = k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, > > true); > > +if (rc != 0) { > > +fprintf(stderr, "virtio-scsi: Failed to set guest notifiers, " > > +"ensure -enable-kvm is set\n"); > > +exit(1); > > +} > > + > > +aio_context_acquire(s->ctx); > > +s->ctrl_vring = virtio_scsi_vring_init(s, s->ctrl_vq, > > + > > virtio_scsi_iothread_handle_ctrl, > > + 0); > > +s->event_vring = virtio_scsi_vring_init(s, s->event_vq, > > + > > virtio_scsi_iothread_handle_event, > > +1); > > +s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * > > s->conf.num_queues); > > +for (i = 0; i < s->conf.num_queues; i++) { > > +s->cmd_vrings[i] = > > +virtio_scsi_vring_init(s, s->cmd_vqs[i], > > + virtio_scsi_iothread_handle_cmd, > > + i + 2); > > +} > > + > > +aio_context_release(s->ctx); > > +s->dataplane_starting = false; > > +s->dataplane_started = true; > > +} > > + > > +/* Context: QEMU global mutex held */ > > +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s) > > +{ > > +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); > > +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > > +VirtIODevice *vdev = VIRTIO_DEVICE(s); > > +int i; > > + > > +if (!s->dataplane_started || s->dataplane_stopping) { > > +return; > > +} > > +s->dataplane_stopping = true; > > +assert(s->ctx == iothread_get_aio_context(s->conf.iothread)); > > + > > +aio_context_acquire(s->ctx); > > + > > +aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL); > > +aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL); > > +for (i = 0; i < s->conf.num_queues; i++) { > > +aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, > > NULL); > > +} > > + > > +bdrv_drain_all(); /* ensure there are no in-flight requests */ > > + > > +aio_context_release(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->ctrl_vring->vring, vdev, 0); > > +vring_teardown(&s->event_vring->vring, vdev, 1); > > +for (i = 0; i < s->conf.num_queues; i++) { > > +vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i); > > +} > > + > > +for (i = 0; i < s->conf.num_queues + 2; i++) { > > +k->set_host_notifier(qbus->parent, i, false); > > +} > > + > > +/* Clean up guest notifier (irq) */ > > +k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, false); > > +s->dataplane_stopping = false; > > +s->dataplane_started = false; > > +} > > diff --git a/include/hw/virtio/virtio-scsi.h > > b/include/hw/virtio/virtio-scsi.h > > index 6f92c29..b9f2197 100644 > > --- a/include/hw/virtio/virtio-scsi.h > > +++ b/include/hw/virtio/virtio-scsi.h > > @@ -174,6 +174,18 @@ typedef struct VirtIOSCSICommon { > > VirtQueue *ctrl_vq; > > VirtQueue *event_vq; > > VirtQueue **cmd_vqs; > > + > > +/* Fields for dataplane below */ > > +AioContext *ctx; /* one iothread per virtio-scsi-pci for now */ > > + > > +/* Vring is used instead of vq in dataplane code, because of the > > underlying > > + * memory layer thread safety */ > > +VirtIOSCSIVring *ctrl_vring; > > +VirtIOSCSIVring *event_vring; > > +VirtIOSCSIVring **cmd_vrings; > > +bool dataplane_started; > > +bool dataplane_starting; > > +bool dataplane_stopping; > > Please add these to VirtIOSCSI rather than VirtIOSCSICommon. Same for > the new functions you declare below. > > Paolo > > > } VirtIOSCSICommon; > > > > typedef struct { >
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
On Fri, 09/19 11:29, Paolo Bonzini wrote: > Il 06/08/2014 07:35, Fam Zheng ha scritto: > > diff --git a/include/hw/virtio/virtio-scsi.h > > b/include/hw/virtio/virtio-scsi.h > > index 6f92c29..b9f2197 100644 > > --- a/include/hw/virtio/virtio-scsi.h > > +++ b/include/hw/virtio/virtio-scsi.h > > @@ -174,6 +174,18 @@ typedef struct VirtIOSCSICommon { > > VirtQueue *ctrl_vq; > > VirtQueue *event_vq; > > VirtQueue **cmd_vqs; > > + > > +/* Fields for dataplane below */ > > +AioContext *ctx; /* one iothread per virtio-scsi-pci for now */ > > + > > +/* Vring is used instead of vq in dataplane code, because of the > > underlying > > + * memory layer thread safety */ > > +VirtIOSCSIVring *ctrl_vring; > > +VirtIOSCSIVring *event_vring; > > +VirtIOSCSIVring **cmd_vrings; > > +bool dataplane_started; > > +bool dataplane_starting; > > +bool dataplane_stopping; > > Please add these to VirtIOSCSI rather than VirtIOSCSICommon. Same for > the new functions you declare below. What's the rationale, please? Asking because especially the VirtIOSCSIVring fields are the dataplane counterparts of VirtQueue fields, so putting in VirtIOSCSI seems unnatural for me. Fam
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
Il 06/08/2014 07:35, Fam Zheng ha scritto: > This implements the core part of dataplane feature of virtio-scsi. > > A few fields are added in VirtIOSCSICommon to maintain the dataplane > status. These fields are managed by a new source file: > virtio-scsi-dataplane.c. > > Most code in this file will run on an iothread, unless otherwise > commented as in a global mutex context, such as those functions to > start, stop and setting the iothread property. > > Upon start, we set up guest/host event notifiers, in a same way as > virtio-blk does. The handlers then pop request from vring and call into > virtio-scsi.c functions to process it. So we need to make sure make all > those called functions work with iothread, too. > > Signed-off-by: Fam Zheng > --- > hw/scsi/Makefile.objs | 2 +- > hw/scsi/virtio-scsi-dataplane.c | 219 > > include/hw/virtio/virtio-scsi.h | 19 > 3 files changed, 239 insertions(+), 1 deletion(-) > create mode 100644 hw/scsi/virtio-scsi-dataplane.c > > diff --git a/hw/scsi/Makefile.objs b/hw/scsi/Makefile.objs > index 121ddc5..40c79d3 100644 > --- a/hw/scsi/Makefile.objs > +++ b/hw/scsi/Makefile.objs > @@ -8,6 +8,6 @@ common-obj-$(CONFIG_ESP_PCI) += esp-pci.o > obj-$(CONFIG_PSERIES) += spapr_vscsi.o > > ifeq ($(CONFIG_VIRTIO),y) > -obj-y += virtio-scsi.o > +obj-y += virtio-scsi.o virtio-scsi-dataplane.o > obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o > endif > diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c > new file mode 100644 > index 000..d077b67 > --- /dev/null > +++ b/hw/scsi/virtio-scsi-dataplane.c > @@ -0,0 +1,219 @@ > +/* > + * Virtio SCSI dataplane > + * > + * Copyright Red Hat, Inc. 2014 > + * > + * Authors: > + * Fam Zheng > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#include "hw/virtio/virtio-scsi.h" > +#include "qemu/error-report.h" > +#include > +#include > +#include > +#include "hw/virtio/virtio-access.h" > +#include "stdio.h" > + > +/* Context: QEMU global mutex held */ > +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread) > +{ > +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); > +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > + > +s->ctx = iothread_get_aio_context(s->conf.iothread); assert that it's NULL? > + > +/* Don't try if transport does not support notifiers. */ > +if (!k->set_guest_notifiers || !k->set_host_notifier) { > +fprintf(stderr, "virtio-scsi: Failed to set iothread " > + "(transport does not support notifiers)"); > +exit(1); > +} > +} > + > +static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSICommon *s, > + VirtQueue *vq, > + EventNotifierHandler *handler, > + int n) > +{ > +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); > +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > +VirtIOSCSIVring *r = g_slice_new(VirtIOSCSIVring); > + > +/* Set up virtqueue notify */ > +if (k->set_host_notifier(qbus->parent, n, true) != 0) { > +fprintf(stderr, "virtio-scsi: Failed to set host notifier\n"); > +exit(1); > +} > +r->host_notifier = *virtio_queue_get_host_notifier(vq); > +r->guest_notifier = *virtio_queue_get_guest_notifier(vq); > +aio_set_event_notifier(s->ctx, &r->host_notifier, handler); > + > +r->parent = s; > + > +if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) { > +fprintf(stderr, "virtio-scsi: VRing setup failed\n"); > +exit(1); > +} > +return r; > +} > + > +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s, > + VirtIOSCSIVring *vring) > +{ > +VirtIOSCSIReq *req = virtio_scsi_init_req(s, NULL); > +int r; > + > +req->vring = vring; > +r = vring_pop((VirtIODevice *)s, &vring->vring, &req->elem); > +if (r < 0) { > +virtio_scsi_free_req(req); > +req = NULL; > +} > +return req; > +} > + > +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req) > +{ > +vring_push(&req->vring->vring, &req->elem, > + req->qsgl.size + req->resp_iov.size); > +event_notifier_set(&req->vring->guest_notifier); > +} > + > +static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier) > +{ > +VirtIOSCSIVring *vring = container_of(notifier, > + VirtIOSCSIVring, host_notifier); > +VirtIOSCSI *s = VIRTIO_SCSI(vring->parent); > +VirtIOSCSIReq *req; > + > +event_notifier_test_and_clear(notifier); > +while ((req = virtio_scsi_pop_req_vring(s, vring))) { > +virtio_scsi_handle_ctrl_req(s, req); > +} > +} > + > +static void virtio_scsi_iothread_handle_event(EventNotifier *n
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
On Wed, 08/06 10:45, Paolo Bonzini wrote: > Il 06/08/2014 07:35, Fam Zheng ha scritto: > > ifeq ($(CONFIG_VIRTIO),y) > > -obj-y += virtio-scsi.o > > +obj-y += virtio-scsi.o virtio-scsi-dataplane.o > > obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o > > endif > > I first thought that this must be conditional on > CONFIG_VIRTIO_BLK_DATA_PLANE. However, CONFIG_VIRTIO_BLK_DATA_PLANE is > itself obsolete: > > ## > # adjust virtio-blk-data-plane based on linux-aio > > if test "$virtio_blk_data_plane" = "yes" -a \ > "$linux_aio" != "yes" ; then > error_exit "virtio-blk-data-plane requires Linux AIO, please try > --enable-linux-aio" > elif test -z "$virtio_blk_data_plane" ; then > virtio_blk_data_plane=$linux_aio > fi > > and there's no requirement to have Linux AIO anymore. Can you prepare a > patch to drop CONFIG_VIRTIO_BLK_DATA_PLANE, and replace patch 1 with it? > > We can leave --disable-virtio-blk-data-plane and > --enable-virtio-blk-data-plane > in for a couple of releases. > Yes. Sounds a good idea. Fam
Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
Il 06/08/2014 07:35, Fam Zheng ha scritto: > ifeq ($(CONFIG_VIRTIO),y) > -obj-y += virtio-scsi.o > +obj-y += virtio-scsi.o virtio-scsi-dataplane.o > obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o > endif I first thought that this must be conditional on CONFIG_VIRTIO_BLK_DATA_PLANE. However, CONFIG_VIRTIO_BLK_DATA_PLANE is itself obsolete: ## # adjust virtio-blk-data-plane based on linux-aio if test "$virtio_blk_data_plane" = "yes" -a \ "$linux_aio" != "yes" ; then error_exit "virtio-blk-data-plane requires Linux AIO, please try --enable-linux-aio" elif test -z "$virtio_blk_data_plane" ; then virtio_blk_data_plane=$linux_aio fi and there's no requirement to have Linux AIO anymore. Can you prepare a patch to drop CONFIG_VIRTIO_BLK_DATA_PLANE, and replace patch 1 with it? We can leave --disable-virtio-blk-data-plane and --enable-virtio-blk-data-plane in for a couple of releases. Paolo
[Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread
This implements the core part of dataplane feature of virtio-scsi. A few fields are added in VirtIOSCSICommon to maintain the dataplane status. These fields are managed by a new source file: virtio-scsi-dataplane.c. Most code in this file will run on an iothread, unless otherwise commented as in a global mutex context, such as those functions to start, stop and setting the iothread property. Upon start, we set up guest/host event notifiers, in a same way as virtio-blk does. The handlers then pop request from vring and call into virtio-scsi.c functions to process it. So we need to make sure make all those called functions work with iothread, too. Signed-off-by: Fam Zheng --- hw/scsi/Makefile.objs | 2 +- hw/scsi/virtio-scsi-dataplane.c | 219 include/hw/virtio/virtio-scsi.h | 19 3 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 hw/scsi/virtio-scsi-dataplane.c diff --git a/hw/scsi/Makefile.objs b/hw/scsi/Makefile.objs index 121ddc5..40c79d3 100644 --- a/hw/scsi/Makefile.objs +++ b/hw/scsi/Makefile.objs @@ -8,6 +8,6 @@ common-obj-$(CONFIG_ESP_PCI) += esp-pci.o obj-$(CONFIG_PSERIES) += spapr_vscsi.o ifeq ($(CONFIG_VIRTIO),y) -obj-y += virtio-scsi.o +obj-y += virtio-scsi.o virtio-scsi-dataplane.o obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o endif diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c new file mode 100644 index 000..d077b67 --- /dev/null +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -0,0 +1,219 @@ +/* + * Virtio SCSI dataplane + * + * Copyright Red Hat, Inc. 2014 + * + * Authors: + * Fam Zheng + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "hw/virtio/virtio-scsi.h" +#include "qemu/error-report.h" +#include +#include +#include +#include "hw/virtio/virtio-access.h" +#include "stdio.h" + +/* Context: QEMU global mutex held */ +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread) +{ +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); + +s->ctx = iothread_get_aio_context(s->conf.iothread); + +/* Don't try if transport does not support notifiers. */ +if (!k->set_guest_notifiers || !k->set_host_notifier) { +fprintf(stderr, "virtio-scsi: Failed to set iothread " + "(transport does not support notifiers)"); +exit(1); +} +} + +static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSICommon *s, + VirtQueue *vq, + EventNotifierHandler *handler, + int n) +{ +BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s))); +VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); +VirtIOSCSIVring *r = g_slice_new(VirtIOSCSIVring); + +/* Set up virtqueue notify */ +if (k->set_host_notifier(qbus->parent, n, true) != 0) { +fprintf(stderr, "virtio-scsi: Failed to set host notifier\n"); +exit(1); +} +r->host_notifier = *virtio_queue_get_host_notifier(vq); +r->guest_notifier = *virtio_queue_get_guest_notifier(vq); +aio_set_event_notifier(s->ctx, &r->host_notifier, handler); + +r->parent = s; + +if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) { +fprintf(stderr, "virtio-scsi: VRing setup failed\n"); +exit(1); +} +return r; +} + +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s, + VirtIOSCSIVring *vring) +{ +VirtIOSCSIReq *req = virtio_scsi_init_req(s, NULL); +int r; + +req->vring = vring; +r = vring_pop((VirtIODevice *)s, &vring->vring, &req->elem); +if (r < 0) { +virtio_scsi_free_req(req); +req = NULL; +} +return req; +} + +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req) +{ +vring_push(&req->vring->vring, &req->elem, + req->qsgl.size + req->resp_iov.size); +event_notifier_set(&req->vring->guest_notifier); +} + +static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier) +{ +VirtIOSCSIVring *vring = container_of(notifier, + VirtIOSCSIVring, host_notifier); +VirtIOSCSI *s = VIRTIO_SCSI(vring->parent); +VirtIOSCSIReq *req; + +event_notifier_test_and_clear(notifier); +while ((req = virtio_scsi_pop_req_vring(s, vring))) { +virtio_scsi_handle_ctrl_req(s, req); +} +} + +static void virtio_scsi_iothread_handle_event(EventNotifier *notifier) +{ +VirtIOSCSIVring *vring = container_of(notifier, + VirtIOSCSIVring, host_notifier); +VirtIOSCSICommon *vs = vring->parent; +VirtIOSCSI *s = VIRTIO_SCSI(vs); +VirtIODevice *vdev = VIRTIO_DEVICE(s); + +event_notifier_test_and_clear(notifier); + +if (!(vdev->status & VIRTIO_CON