From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPZqm-0007bI-Vx for qemu-devel@nongnu.org; Mon, 17 Mar 2014 11:53:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPZqe-0004Zy-0a for qemu-devel@nongnu.org; Mon, 17 Mar 2014 11:53:16 -0400 Received: from e06smtp18.uk.ibm.com ([195.75.94.114]:59760) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPZqd-0004SI-JL for qemu-devel@nongnu.org; Mon, 17 Mar 2014 11:53:07 -0400 Received: from /spool/local by e06smtp18.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 17 Mar 2014 15:53:04 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 0C88C219005F for ; Mon, 17 Mar 2014 15:52:56 +0000 (GMT) Received: from d06av01.portsmouth.uk.ibm.com (d06av01.portsmouth.uk.ibm.com [9.149.37.212]) by b06cxnps3074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2HFqmnG31785076 for ; Mon, 17 Mar 2014 15:52:48 GMT Received: from d06av01.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2HFqxZZ023057 for ; Mon, 17 Mar 2014 09:53:00 -0600 Message-ID: <53271A5B.9080906@de.ibm.com> Date: Mon, 17 Mar 2014 16:52:59 +0100 From: Christian Borntraeger MIME-Version: 1.0 References: <1394719868-24312-1-git-send-email-stefanha@redhat.com> <1394719868-24312-14-git-send-email-stefanha@redhat.com> In-Reply-To: <1394719868-24312-14-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL v2 for-2.0 13/24] dataplane: replace internal thread with IOThread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Peter Maydell , qemu-devel@nongnu.org, Dominik Dingel , Jens Freimann , Anthony Liguori , Cornelia Huck This causes the following bug during managedsave of s390 with a guest that has multiple disks with dataplane. (gdb) bt #0 object_deinit (type=0x0, obj=0x807e8750) at /home/cborntra/REPOS/qemu/qom/object.c:410 #1 object_finalize (data=0x807e8750) at /home/cborntra/REPOS/qemu/qom/object.c:424 #2 object_unref (obj=0x807e8750) at /home/cborntra/REPOS/qemu/qom/object.c:726 #3 0x000000008011da60 in object_property_del (obj=0x807e8d80, name=0x807e8d60 "virtio-blk", errp=) at /home/cborntra/REPOS/qemu/qom/object.c:783 #4 0x000000008011dc0e in object_property_del_child (errp=0x0, child=0x807e8750, obj=0x807e8d80) at /home/cborntra/REPOS/qemu/qom/object.c:386 #5 object_unparent (obj=0x807e8750) at /home/cborntra/REPOS/qemu/qom/object.c:403 #6 0x0000000080183898 in virtio_blk_data_plane_destroy (s=0x8088a750) at /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:430 #7 0x00000000801845a0 in virtio_blk_migration_state_changed (notifier=0x807ebf20, data=0x80314088 ) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:666 #8 0x00000000802161f8 in notifier_list_notify (list=list@entry=0x80726310 , data=data@entry=0x80314088 ) at /home/cborntra/REPOS/qemu/util/notify.c:39 #9 0x00000000800ccecc in migrate_fd_connect (s=s@entry=0x80314088 ) at /home/cborntra/REPOS/qemu/migration.c:696 #10 0x00000000800cae9c in fd_start_outgoing_migration (s=s@entry=0x80314088 , fdname=, errp=errp@entry=0x3fffff298c0) at /home/cborntra/REPOS/qemu/migration-fd.c:42 #11 0x00000000800cc986 in qmp_migrate (uri=0x808f6190 "fd:migrate", has_blk=, blk=, has_inc=, inc=inc@entry=false, has_detach=true, detach=true, errp= 0x3fffff29998) at /home/cborntra/REPOS/qemu/migration.c:450 #12 0x00000000801166f6 in qmp_marshal_input_migrate (mon=, qdict=, ret=) at qmp-marshal.c:3285 #13 0x00000000801b252a in qmp_call_cmd (cmd=, params=0x808f6280, mon=0x808150d0) at /home/cborntra/REPOS/qemu/monitor.c:4760 #14 handle_qmp_command (parser=, tokens=) at /home/cborntra/REPOS/qemu/monitor.c:4826 #15 0x0000000080202f66 in json_message_process_token (lexer=0x80815050, token=0x808f3e20, type=, x=, y=9) at /home/cborntra/REPOS/qemu/qobject/json-streamer.c:87 #16 0x000000008021b36a in json_lexer_feed_char (lexer=lexer@entry=0x80815050, ch=, flush=flush@entry=false) at /home/cborntra/REPOS/qemu/qobject/json-lexer.c:303 #17 0x000000008021b4ec in json_lexer_feed (lexer=0x80815050, buffer=, size=) at /home/cborntra/REPOS/qemu/qobject/json-lexer.c:356 #18 0x00000000802031a2 in json_message_parser_feed (parser=, buffer=, size=) at /home/cborntra/REPOS/qemu/qobject/json-streamer.c:110 #19 0x00000000801b015a in monitor_control_read (opaque=, buf=, size=) at /home/cborntra/REPOS/qemu/monitor.c:4847 #20 0x00000000801023e2 in qemu_chr_be_write (len=1, buf=0x3fffff29e18 "}[A", s=0x807cdf40) at /home/cborntra/REPOS/qemu/qemu-char.c:165 #21 tcp_chr_read (chan=, cond=, opaque=0x807cdf40) at /home/cborntra/REPOS/qemu/qemu-char.c:2487 #22 0x000003fffd54005a in g_main_context_dispatch () from /lib64/libglib-2.0.so.0 #23 0x00000000800ca9b2 in glib_pollfds_poll () at /home/cborntra/REPOS/qemu/main-loop.c:190 #24 os_host_main_loop_wait (timeout=) at /home/cborntra/REPOS/qemu/main-loop.c:235 #25 main_loop_wait (nonblocking=nonblocking@entry=0) at /home/cborntra/REPOS/qemu/main-loop.c:484 #26 0x0000000080014f2a in main_loop () at /home/cborntra/REPOS/qemu/vl.c:2053 #27 main (argc=, argv=, envp=) at /home/cborntra/REPOS/qemu/vl.c:4524 Reverting commit 48ff269272f18d2b8fa53cb08365df417588f585 seems to fix this particualar bug. Any ideas? Christian On 13/03/14 15:10, Stefan Hajnoczi wrote: > Today virtio-blk dataplane uses a 1:1 device-per-thread model. Now that > IOThreads have been introduced we can generalize this to N:M devices per > threads. > > This patch drops thread code from dataplane in favor of running inside > an IOThread AioContext. > > As a bonus we solve the case where a guest keeps submitting I/O requests > while dataplane is trying to stop. Previously the dataplane thread > would continue to process requests until the request gave it a break. > Now we can shut down in bounded time thanks to > aio_context_acquire/release. > > Signed-off-by: Stefan Hajnoczi > --- > hw/block/dataplane/virtio-blk.c | 96 +++++++++++++++++++++++------------------ > include/hw/virtio/virtio-blk.h | 8 +++- > 2 files changed, 60 insertions(+), 44 deletions(-) > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index d1c7ad4..a5afc21 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -23,6 +23,7 @@ > #include "virtio-blk.h" > #include "block/aio.h" > #include "hw/virtio/virtio-bus.h" > +#include "monitor/monitor.h" /* for object_add() */ > > enum { > SEG_MAX = 126, /* maximum number of I/O segments */ > @@ -44,8 +45,6 @@ struct VirtIOBlockDataPlane { > bool started; > bool starting; > bool stopping; > - QEMUBH *start_bh; > - QemuThread thread; > > VirtIOBlkConf *blk; > int fd; /* image file descriptor */ > @@ -59,12 +58,14 @@ struct VirtIOBlockDataPlane { > * (because you don't own the file descriptor or handle; you just > * use it). > */ > + IOThread *iothread; > + bool internal_iothread; > AioContext *ctx; > EventNotifier io_notifier; /* Linux AIO completion */ > EventNotifier host_notifier; /* doorbell */ > > IOQueue ioqueue; /* Linux AIO queue (should really be per > - dataplane thread) */ > + IOThread) */ > VirtIOBlockRequest requests[REQ_MAX]; /* pool of requests, managed by the > queue */ > > @@ -342,26 +343,7 @@ static void handle_io(EventNotifier *e) > } > } > > -static void *data_plane_thread(void *opaque) > -{ > - VirtIOBlockDataPlane *s = opaque; > - > - while (!s->stopping || s->num_reqs > 0) { > - aio_poll(s->ctx, true); > - } > - 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", data_plane_thread, > - s, QEMU_THREAD_JOINABLE); > -} > - > +/* Context: QEMU global mutex held */ > void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, > VirtIOBlockDataPlane **dataplane, > Error **errp) > @@ -408,12 +390,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, > s->fd = fd; > s->blk = blk; > > + if (blk->iothread) { > + s->internal_iothread = false; > + s->iothread = blk->iothread; > + object_ref(OBJECT(s->iothread)); > + } else { > + /* Create per-device IOThread if none specified */ > + Error *local_err = NULL; > + > + s->internal_iothread = true; > + object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err); > + if (error_is_set(&local_err)) { > + error_propagate(errp, local_err); > + g_free(s); > + return; > + } > + s->iothread = iothread_find(vdev->name); > + assert(s->iothread); > + } > + s->ctx = iothread_get_aio_context(s->iothread); > + > /* Prevent block operations that conflict with data plane thread */ > bdrv_set_in_use(blk->conf.bs, 1); > > *dataplane = s; > } > > +/* Context: QEMU global mutex held */ > void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) > { > if (!s) { > @@ -422,9 +425,14 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) > > virtio_blk_data_plane_stop(s); > bdrv_set_in_use(s->blk->conf.bs, 0); > + object_unref(OBJECT(s->iothread)); > + if (s->internal_iothread) { > + object_unparent(OBJECT(s->iothread)); > + } > g_free(s); > } > > +/* Context: QEMU global mutex held */ > void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > { > BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev))); > @@ -448,8 +456,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > return; > } > > - s->ctx = aio_context_new(); > - > /* Set up guest notifier (irq) */ > if (k->set_guest_notifiers(qbus->parent, 1, true) != 0) { > fprintf(stderr, "virtio-blk failed to set guest notifier, " > @@ -464,7 +470,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > exit(1); > } > s->host_notifier = *virtio_queue_get_host_notifier(vq); > - aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify); > > /* Set up ioqueue */ > ioq_init(&s->ioqueue, s->fd, REQ_MAX); > @@ -472,7 +477,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > ioq_put_iocb(&s->ioqueue, &s->requests[i].iocb); > } > s->io_notifier = *ioq_get_notifier(&s->ioqueue); > - aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io); > > s->starting = false; > s->started = true; > @@ -481,11 +485,14 @@ 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); > + /* Get this show started by hooking up our callbacks */ > + aio_context_acquire(s->ctx); > + aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify); > + aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io); > + aio_context_release(s->ctx); > } > > +/* Context: QEMU global mutex held */ > void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) > { > BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev))); > @@ -496,27 +503,32 @@ 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_context_acquire(s->ctx); > + > + /* Stop notifications for new requests from guest */ > + aio_set_event_notifier(s->ctx, &s->host_notifier, NULL); > + > + /* Complete pending requests */ > + while (s->num_reqs > 0) { > + aio_poll(s->ctx, true); > } > > + /* Stop ioq callbacks (there are no pending requests left) */ > aio_set_event_notifier(s->ctx, &s->io_notifier, NULL); > - ioq_cleanup(&s->ioqueue); > > - aio_set_event_notifier(s->ctx, &s->host_notifier, NULL); > - k->set_host_notifier(qbus->parent, 0, false); > + aio_context_release(s->ctx); > > - aio_context_unref(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->vring, s->vdev, 0); > + > + ioq_cleanup(&s->ioqueue); > + k->set_host_notifier(qbus->parent, 0, false); > > /* Clean up guest notifier (irq) */ > k->set_guest_notifiers(qbus->parent, 1, false); > > - vring_teardown(&s->vring, s->vdev, 0); > s->started = false; > s->stopping = false; > } > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index 41885da..e4c41ff 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -16,6 +16,7 @@ > > #include "hw/virtio/virtio.h" > #include "hw/block/block.h" > +#include "sysemu/iothread.h" > > #define TYPE_VIRTIO_BLK "virtio-blk-device" > #define VIRTIO_BLK(obj) \ > @@ -106,6 +107,7 @@ struct virtio_scsi_inhdr > struct VirtIOBlkConf > { > BlockConf conf; > + IOThread *iothread; > char *serial; > uint32_t scsi; > uint32_t config_wce; > @@ -140,13 +142,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_IOTHREAD("x-iothread", _state, _field.iothread) > #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_IOTHREAD("x-iothread", _state, _field.iothread) > #endif /* __linux__ */ > > void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk); >