Re: [Qemu-devel] [RFC PATCH v2 09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread

[Paolo Bonzini <pbonzini@redhat.com>]

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 <famz@redhat.com>
> ---
>  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 0000000..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 <famz@redhat.com>
> + *
> + * 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 <hw/scsi/scsi.h>
> +#include <block/scsi.h>
> +#include <hw/virtio/virtio-bus.h>
> +#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 *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_CONFIG_S_DRIVER_OK)) {
> +        return;
> +    }
> +
> +    if (s->events_dropped) {
> +        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
> +    }
> +}
> +
> +static void virtio_scsi_iothread_handle_cmd(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_cmd_req(s, req);
> +    }

Perhaps add bdrv_io_plug/bdrv_io_unplug?

> +}
> +
> +/* Context: QEMU global mutex held */

> +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.

> +    /* 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 {
> @@ -239,4 +251,11 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req);
>  void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
>                              uint32_t event, uint32_t reason);
>  
> +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread);
> +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s);
> +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s);
> +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
> +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
> +                                         VirtIOSCSIVring *vring);
> +
>  #endif /* _QEMU_VIRTIO_SCSI_H */
>