From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQEGg-0000B0-Io for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:02:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQEGX-0000hg-IS for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:02:42 -0400 Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:59750) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQEGX-0000hQ-3h for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:02:33 -0400 Received: from /spool/local by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 19 Mar 2014 11:02:30 -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 B7EB92190069 for ; Wed, 19 Mar 2014 11:02:24 +0000 (GMT) Received: from d06av11.portsmouth.uk.ibm.com (d06av11.portsmouth.uk.ibm.com [9.149.37.252]) by b06cxnps3074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2JB2HuL20119700 for ; Wed, 19 Mar 2014 11:02:17 GMT Received: from d06av11.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av11.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2JB2S7U010987 for ; Wed, 19 Mar 2014 05:02:29 -0600 Message-ID: <53297939.1080306@de.ibm.com> Date: Wed, 19 Mar 2014 12:02:17 +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> <53271A5B.9080906@de.ibm.com> In-Reply-To: <53271A5B.9080906@de.ibm.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 Hmm, now I have trouble getting the whole thing started (Dont know how I was able to start the guest from below). The problem seems to be that qdev->name is always "virtio-blk". So this code in virtio_blk_data_plane_create will always add a child called "virtio-blk", which obviously doesnt work so great with more that one disk. } 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); On 17/03/14 16:52, Christian Borntraeger wrote: > 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); >> >