From: Fam Zheng <famz@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: tubo@linux.vnet.ibm.com, mst@redhat.com, qemu-devel@nongnu.org,
borntraeger@de.ibm.com, stefanha@redhat.com,
cornelia.huck@de.ibm.com
Subject: Re: [Qemu-devel] [PATCH 7/9] virtio-scsi: use aio handler for data plane
Date: Tue, 5 Apr 2016 17:06:23 +0800 [thread overview]
Message-ID: <20160405090623.GE1150@ad.usersys.redhat.com> (raw)
In-Reply-To: <1459516794-23629-8-git-send-email-pbonzini@redhat.com>
On Fri, 04/01 15:19, Paolo Bonzini wrote:
> In addition to handling IO in vcpu thread and in io thread, dataplane
> introduces yet another mode: handling it by aio.
>
> This reuses the same handler as previous modes, which triggers races as
> these were not designed to be reentrant.
>
> Use a separate handler just for aio, and disable regular handlers when
> dataplane is active.
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/virtio-scsi-dataplane.c | 43 ++++++++++++++++++++++++---
> hw/scsi/virtio-scsi.c | 65 ++++++++++++++++++++++++++++-------------
> include/hw/virtio/virtio-scsi.h | 6 ++--
> 3 files changed, 86 insertions(+), 28 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 21d5bfd..a497a2c 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -38,7 +38,35 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread)
> }
> }
>
> -static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
> +static void virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
> + VirtQueue *vq)
> +{
> + VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> +
> + assert(s->ctx && s->dataplane_started);
> + virtio_scsi_handle_cmd_vq(s, vq);
> +}
> +
> +static void virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
> + VirtQueue *vq)
> +{
> + VirtIOSCSI *s = VIRTIO_SCSI(vdev);
> +
> + assert(s->ctx && s->dataplane_started);
> + virtio_scsi_handle_ctrl_vq(s, vq);
> +}
> +
> +static void virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
> + VirtQueue *vq)
> +{
> + VirtIOSCSI *s = VIRTIO_SCSI(vdev);
> +
> + assert(s->ctx && s->dataplane_started);
> + virtio_scsi_handle_event_vq(s, vq);
> +}
> +
> +static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
> + void (*fn)(VirtIODevice *vdev, VirtQueue *vq))
Are tabs used by mistake? Three more occasions below...
> {
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> @@ -54,6 +82,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
> }
>
> virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true);
> + virtio_set_queue_aio(vq, fn);
> return 0;
> }
>
>
> @@ -104,16 +136,19 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
> }
>
> aio_context_acquire(s->ctx);
> - rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0);
> + rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0,
> + virtio_scsi_data_plane_handle_ctrl);
Tabs.
> if (rc) {
> goto fail_vrings;
> }
> - rc = virtio_scsi_vring_init(s, vs->event_vq, 1);
> + rc = virtio_scsi_vring_init(s, vs->event_vq, 1,
> + virtio_scsi_data_plane_handle_event);
Tabs.
> if (rc) {
> goto fail_vrings;
> }
> for (i = 0; i < vs->conf.num_queues; i++) {
> - rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2);
> + rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2,
> + virtio_scsi_data_plane_handle_cmd);
Tabs.
> if (rc) {
> goto fail_vrings;
> }
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 38f1e2c..30415c6 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -374,7 +374,7 @@ fail:
> return ret;
> }
>
> -void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
> {
> VirtIODevice *vdev = (VirtIODevice *)s;
> uint32_t type;
> @@ -412,20 +412,28 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
> }
> }
>
> -static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
> {
> - VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> VirtIOSCSIReq *req;
>
> - if (s->ctx && !s->dataplane_started) {
> - virtio_scsi_dataplane_start(s);
> - return;
> - }
> while ((req = virtio_scsi_pop_req(s, vq))) {
> virtio_scsi_handle_ctrl_req(s, req);
> }
> }
>
> +static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> +
> + if (s->ctx) {
> + virtio_scsi_dataplane_start(s);
> + if (!s->dataplane_fenced) {
> + return;
> + }
Could be more readable if virtio_scsi_dataplane_start has a return value
indicating success.
> + }
> + virtio_scsi_handle_ctrl_vq(s, vq);
> +}
> +
> static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
> {
> /* Sense data is not in req->resp and is copied separately
> @@ -508,7 +516,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> virtio_scsi_complete_cmd_req(req);
> }
>
> -bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +static bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
Isn't this line too long now?
> {
> VirtIOSCSICommon *vs = &s->parent_obj;
> SCSIDevice *d;
> @@ -550,7 +558,7 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> return true;
> }
>
> -void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
> {
> SCSIRequest *sreq = req->sreq;
> if (scsi_req_enqueue(sreq)) {
> @@ -560,17 +568,11 @@ void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
> scsi_req_unref(sreq);
> }
>
> -static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
> +void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
> {
> - /* use non-QOM casts in the data path */
> - VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> VirtIOSCSIReq *req, *next;
> QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
>
> - if (s->ctx && !s->dataplane_started) {
> - virtio_scsi_dataplane_start(s);
> - return;
> - }
> while ((req = virtio_scsi_pop_req(s, vq))) {
> if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
> QTAILQ_INSERT_TAIL(&reqs, req, next);
> @@ -582,6 +584,20 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
> }
> }
>
> +static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + /* use non-QOM casts in the data path */
> + VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> +
> + if (s->ctx) {
> + virtio_scsi_dataplane_start(s);
> + if (!s->dataplane_fenced) {
> + return;
> + }
> + }
> + virtio_scsi_handle_cmd_vq(s, vq);
> +}
> +
> static void virtio_scsi_get_config(VirtIODevice *vdev,
> uint8_t *config)
> {
> @@ -725,17 +741,24 @@ out:
> }
> }
>
> +void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq)
> +{
> + if (s->events_dropped) {
> + virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
> + }
> +}
> +
> static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
> {
> VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>
> - if (s->ctx && !s->dataplane_started) {
> + if (s->ctx) {
> virtio_scsi_dataplane_start(s);
> - return;
> - }
> - if (s->events_dropped) {
> - virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
> + if (!s->dataplane_fenced) {
> + return;
> + }
> }
> + virtio_scsi_handle_event_vq(s, vq);
> }
>
> static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index eef4e95..ba2f5ce 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -139,9 +139,9 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
> HandleOutput cmd);
>
> void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp);
> -void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req);
> -bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req);
> -void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req);
> +void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq);
> +void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq);
> +void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq);
> void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req);
> void virtio_scsi_free_req(VirtIOSCSIReq *req);
> void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> --
> 1.8.3.1
>
>
next prev parent reply other threads:[~2016-04-05 9:06 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-01 13:19 [Qemu-devel] [PATCH v2 0/9] virtio: aio handler API Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 1/9] virtio-dataplane: pass assign=true to virtio_queue_aio_set_host_notifier_handler Paolo Bonzini
2016-04-01 14:02 ` Cornelia Huck
2016-04-05 10:38 ` Michael S. Tsirkin
2016-04-05 10:42 ` Paolo Bonzini
2016-04-05 10:59 ` Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 2/9] virtio: make virtio_queue_notify_vq static Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 3/9] virtio-blk: fix disabled mode Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 4/9] virtio-scsi: " Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 5/9] virtio: add aio handler Paolo Bonzini
2016-04-01 14:04 ` Cornelia Huck
2016-04-01 13:19 ` [Qemu-devel] [PATCH 6/9] virtio-blk: use aio handler for data plane Paolo Bonzini
2016-04-01 14:05 ` Cornelia Huck
2016-04-01 13:19 ` [Qemu-devel] [PATCH 7/9] virtio-scsi: " Paolo Bonzini
2016-04-05 9:06 ` Fam Zheng [this message]
2016-04-01 13:19 ` [Qemu-devel] [PATCH 8/9] virtio: merge virtio_queue_aio_set_host_notifier_handler with virtio_queue_set_aio Paolo Bonzini
2016-04-03 9:06 ` Michael S. Tsirkin
2016-04-03 17:13 ` Paolo Bonzini
2016-04-01 13:19 ` [Qemu-devel] [PATCH 9/9] virtio: remove starting/stopping checks Paolo Bonzini
2016-04-01 14:14 ` Christian Borntraeger
2016-04-01 14:30 ` Cornelia Huck
2016-04-03 10:30 ` Michael S. Tsirkin
2016-04-01 14:02 ` [Qemu-devel] [PATCH v2 0/9] virtio: aio handler API Christian Borntraeger
2016-04-01 15:16 ` Christian Borntraeger
2016-04-03 9:00 ` Michael S. Tsirkin
2016-04-01 14:06 ` Cornelia Huck
2016-04-03 9:29 ` Michael S. Tsirkin
2016-04-05 9:13 ` Fam Zheng
2016-04-05 10:15 ` Christian Borntraeger
-- strict thread matches above, loose matches on Subject: below --
2016-03-30 12:47 [Qemu-devel] [PATCH resend " Paolo Bonzini
2016-03-30 12:48 ` [Qemu-devel] [PATCH 7/9] virtio-scsi: use aio handler for data plane Paolo Bonzini
2016-03-30 15:31 ` Cornelia Huck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160405090623.GE1150@ad.usersys.redhat.com \
--to=famz@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cornelia.huck@de.ibm.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=tubo@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.