1 | The following changes since commit e4ae62b802cec437f877f2cadc4ef059cc0eca76: | 1 | The following changes since commit 0ab4537f08e09b13788db67efd760592fb7db769: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2018-03-09 17:28:16 +0000) | 3 | Merge remote-tracking branch 'remotes/stefanberger/tags/pull-tpm-2018-03-07-1' into staging (2018-03-08 12:56:39 +0000) |
4 | 4 | ||
5 | are available in the Git repository at: | 5 | are available in the Git repository at: |
6 | 6 | ||
7 | git://github.com/stefanha/qemu.git tags/block-pull-request | 7 | git://github.com/stefanha/qemu.git tags/block-pull-request |
8 | 8 | ||
9 | for you to fetch changes up to 7376eda7c2e0451e819e81bd05fabc56a9deb946: | 9 | for you to fetch changes up to 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268: |
10 | 10 | ||
11 | block: make BDRV_POLL_WHILE() re-entrancy safe (2018-03-12 11:07:37 +0000) | 11 | vl: introduce vm_shutdown() (2018-03-08 17:38:51 +0000) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | 14 | ||
15 | ---------------------------------------------------------------- | 15 | ---------------------------------------------------------------- |
16 | 16 | ||
17 | Stefan Hajnoczi (1): | 17 | Deepa Srinivasan (1): |
18 | block: make BDRV_POLL_WHILE() re-entrancy safe | 18 | block: Fix qemu crash when using scsi-block |
19 | 19 | ||
20 | include/block/aio-wait.h | 61 ++++++++++++++++++++++++------------------------ | 20 | Fam Zheng (1): |
21 | util/aio-wait.c | 2 +- | 21 | README: Fix typo 'git-publish' |
22 | 2 files changed, 31 insertions(+), 32 deletions(-) | 22 | |
23 | Sergio Lopez (1): | ||
24 | virtio-blk: dataplane: Don't batch notifications if EVENT_IDX is | ||
25 | present | ||
26 | |||
27 | Stefan Hajnoczi (4): | ||
28 | block: add aio_wait_bh_oneshot() | ||
29 | virtio-blk: fix race between .ioeventfd_stop() and vq handler | ||
30 | virtio-scsi: fix race between .ioeventfd_stop() and vq handler | ||
31 | vl: introduce vm_shutdown() | ||
32 | |||
33 | include/block/aio-wait.h | 13 +++++++++++ | ||
34 | include/sysemu/iothread.h | 1 - | ||
35 | include/sysemu/sysemu.h | 1 + | ||
36 | block/block-backend.c | 51 ++++++++++++++++++++--------------------- | ||
37 | cpus.c | 16 ++++++++++--- | ||
38 | hw/block/dataplane/virtio-blk.c | 39 +++++++++++++++++++++++-------- | ||
39 | hw/scsi/virtio-scsi-dataplane.c | 9 ++++---- | ||
40 | iothread.c | 31 ------------------------- | ||
41 | util/aio-wait.c | 31 +++++++++++++++++++++++++ | ||
42 | vl.c | 13 +++-------- | ||
43 | README | 2 +- | ||
44 | 11 files changed, 122 insertions(+), 85 deletions(-) | ||
23 | 45 | ||
24 | -- | 46 | -- |
25 | 2.14.3 | 47 | 2.14.3 |
26 | 48 | ||
27 | 49 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Deepa Srinivasan <deepa.srinivasan@oracle.com> | ||
1 | 2 | ||
3 | Starting qemu with the following arguments causes qemu to segfault: | ||
4 | ... -device lsi,id=lsi0 -drive file=iscsi:<...>,format=raw,if=none,node-name= | ||
5 | iscsi1 -device scsi-block,bus=lsi0.0,id=<...>,drive=iscsi1 | ||
6 | |||
7 | This patch fixes blk_aio_ioctl() so it does not pass stack addresses to | ||
8 | blk_aio_ioctl_entry() which may be invoked after blk_aio_ioctl() returns. More | ||
9 | details about the bug follow. | ||
10 | |||
11 | blk_aio_ioctl() invokes blk_aio_prwv() with blk_aio_ioctl_entry as the | ||
12 | coroutine parameter. blk_aio_prwv() ultimately calls aio_co_enter(). | ||
13 | |||
14 | When blk_aio_ioctl() is executed from within a coroutine context (e.g. | ||
15 | iscsi_bh_cb()), aio_co_enter() adds the coroutine (blk_aio_ioctl_entry) to | ||
16 | the current coroutine's wakeup queue. blk_aio_ioctl() then returns. | ||
17 | |||
18 | When blk_aio_ioctl_entry() executes later, it accesses an invalid pointer: | ||
19 | .... | ||
20 | BlkRwCo *rwco = &acb->rwco; | ||
21 | |||
22 | rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, | ||
23 | rwco->qiov->iov[0].iov_base); <--- qiov is | ||
24 | invalid here | ||
25 | ... | ||
26 | |||
27 | In the case when blk_aio_ioctl() is called from a non-coroutine context, | ||
28 | blk_aio_ioctl_entry() executes immediately. But if bdrv_co_ioctl() calls | ||
29 | qemu_coroutine_yield(), blk_aio_ioctl() will return. When the coroutine | ||
30 | execution is complete, control returns to blk_aio_ioctl_entry() after the call | ||
31 | to blk_co_ioctl(). There is no invalid reference after this point, but the | ||
32 | function is still holding on to invalid pointers. | ||
33 | |||
34 | The fix is to change blk_aio_prwv() to accept a void pointer for the IO buffer | ||
35 | rather than a QEMUIOVector. blk_aio_prwv() passes this through in BlkRwCo and the | ||
36 | coroutine function casts it to QEMUIOVector or uses the void pointer directly. | ||
37 | |||
38 | Signed-off-by: Deepa Srinivasan <deepa.srinivasan@oracle.com> | ||
39 | Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> | ||
40 | Reviewed-by: Mark Kanda <mark.kanda@oracle.com> | ||
41 | Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> | ||
42 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
43 | --- | ||
44 | block/block-backend.c | 51 +++++++++++++++++++++++++-------------------------- | ||
45 | 1 file changed, 25 insertions(+), 26 deletions(-) | ||
46 | |||
47 | diff --git a/block/block-backend.c b/block/block-backend.c | ||
48 | index XXXXXXX..XXXXXXX 100644 | ||
49 | --- a/block/block-backend.c | ||
50 | +++ b/block/block-backend.c | ||
51 | @@ -XXX,XX +XXX,XX @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, | ||
52 | typedef struct BlkRwCo { | ||
53 | BlockBackend *blk; | ||
54 | int64_t offset; | ||
55 | - QEMUIOVector *qiov; | ||
56 | + void *iobuf; | ||
57 | int ret; | ||
58 | BdrvRequestFlags flags; | ||
59 | } BlkRwCo; | ||
60 | @@ -XXX,XX +XXX,XX @@ typedef struct BlkRwCo { | ||
61 | static void blk_read_entry(void *opaque) | ||
62 | { | ||
63 | BlkRwCo *rwco = opaque; | ||
64 | + QEMUIOVector *qiov = rwco->iobuf; | ||
65 | |||
66 | - rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, rwco->qiov->size, | ||
67 | - rwco->qiov, rwco->flags); | ||
68 | + rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, qiov->size, | ||
69 | + qiov, rwco->flags); | ||
70 | } | ||
71 | |||
72 | static void blk_write_entry(void *opaque) | ||
73 | { | ||
74 | BlkRwCo *rwco = opaque; | ||
75 | + QEMUIOVector *qiov = rwco->iobuf; | ||
76 | |||
77 | - rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, rwco->qiov->size, | ||
78 | - rwco->qiov, rwco->flags); | ||
79 | + rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, qiov->size, | ||
80 | + qiov, rwco->flags); | ||
81 | } | ||
82 | |||
83 | static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf, | ||
84 | @@ -XXX,XX +XXX,XX @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf, | ||
85 | rwco = (BlkRwCo) { | ||
86 | .blk = blk, | ||
87 | .offset = offset, | ||
88 | - .qiov = &qiov, | ||
89 | + .iobuf = &qiov, | ||
90 | .flags = flags, | ||
91 | .ret = NOT_DONE, | ||
92 | }; | ||
93 | @@ -XXX,XX +XXX,XX @@ static void blk_aio_complete_bh(void *opaque) | ||
94 | } | ||
95 | |||
96 | static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes, | ||
97 | - QEMUIOVector *qiov, CoroutineEntry co_entry, | ||
98 | + void *iobuf, CoroutineEntry co_entry, | ||
99 | BdrvRequestFlags flags, | ||
100 | BlockCompletionFunc *cb, void *opaque) | ||
101 | { | ||
102 | @@ -XXX,XX +XXX,XX @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes, | ||
103 | acb->rwco = (BlkRwCo) { | ||
104 | .blk = blk, | ||
105 | .offset = offset, | ||
106 | - .qiov = qiov, | ||
107 | + .iobuf = iobuf, | ||
108 | .flags = flags, | ||
109 | .ret = NOT_DONE, | ||
110 | }; | ||
111 | @@ -XXX,XX +XXX,XX @@ static void blk_aio_read_entry(void *opaque) | ||
112 | { | ||
113 | BlkAioEmAIOCB *acb = opaque; | ||
114 | BlkRwCo *rwco = &acb->rwco; | ||
115 | + QEMUIOVector *qiov = rwco->iobuf; | ||
116 | |||
117 | - assert(rwco->qiov->size == acb->bytes); | ||
118 | + assert(qiov->size == acb->bytes); | ||
119 | rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, acb->bytes, | ||
120 | - rwco->qiov, rwco->flags); | ||
121 | + qiov, rwco->flags); | ||
122 | blk_aio_complete(acb); | ||
123 | } | ||
124 | |||
125 | @@ -XXX,XX +XXX,XX @@ static void blk_aio_write_entry(void *opaque) | ||
126 | { | ||
127 | BlkAioEmAIOCB *acb = opaque; | ||
128 | BlkRwCo *rwco = &acb->rwco; | ||
129 | + QEMUIOVector *qiov = rwco->iobuf; | ||
130 | |||
131 | - assert(!rwco->qiov || rwco->qiov->size == acb->bytes); | ||
132 | + assert(!qiov || qiov->size == acb->bytes); | ||
133 | rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, acb->bytes, | ||
134 | - rwco->qiov, rwco->flags); | ||
135 | + qiov, rwco->flags); | ||
136 | blk_aio_complete(acb); | ||
137 | } | ||
138 | |||
139 | @@ -XXX,XX +XXX,XX @@ int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf) | ||
140 | static void blk_ioctl_entry(void *opaque) | ||
141 | { | ||
142 | BlkRwCo *rwco = opaque; | ||
143 | + QEMUIOVector *qiov = rwco->iobuf; | ||
144 | + | ||
145 | rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, | ||
146 | - rwco->qiov->iov[0].iov_base); | ||
147 | + qiov->iov[0].iov_base); | ||
148 | } | ||
149 | |||
150 | int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf) | ||
151 | @@ -XXX,XX +XXX,XX @@ static void blk_aio_ioctl_entry(void *opaque) | ||
152 | BlkAioEmAIOCB *acb = opaque; | ||
153 | BlkRwCo *rwco = &acb->rwco; | ||
154 | |||
155 | - rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, | ||
156 | - rwco->qiov->iov[0].iov_base); | ||
157 | + rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, rwco->iobuf); | ||
158 | + | ||
159 | blk_aio_complete(acb); | ||
160 | } | ||
161 | |||
162 | BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf, | ||
163 | BlockCompletionFunc *cb, void *opaque) | ||
164 | { | ||
165 | - QEMUIOVector qiov; | ||
166 | - struct iovec iov; | ||
167 | - | ||
168 | - iov = (struct iovec) { | ||
169 | - .iov_base = buf, | ||
170 | - .iov_len = 0, | ||
171 | - }; | ||
172 | - qemu_iovec_init_external(&qiov, &iov, 1); | ||
173 | - | ||
174 | - return blk_aio_prwv(blk, req, 0, &qiov, blk_aio_ioctl_entry, 0, cb, opaque); | ||
175 | + return blk_aio_prwv(blk, req, 0, buf, blk_aio_ioctl_entry, 0, cb, opaque); | ||
176 | } | ||
177 | |||
178 | int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes) | ||
179 | @@ -XXX,XX +XXX,XX @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc, | ||
180 | static void blk_pdiscard_entry(void *opaque) | ||
181 | { | ||
182 | BlkRwCo *rwco = opaque; | ||
183 | - rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, rwco->qiov->size); | ||
184 | + QEMUIOVector *qiov = rwco->iobuf; | ||
185 | + | ||
186 | + rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, qiov->size); | ||
187 | } | ||
188 | |||
189 | int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes) | ||
190 | -- | ||
191 | 2.14.3 | ||
192 | |||
193 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Fam Zheng <famz@redhat.com> | ||
1 | 2 | ||
3 | Reported-by: Alberto Garcia <berto@igalia.com> | ||
4 | Signed-off-by: Fam Zheng <famz@redhat.com> | ||
5 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
6 | Message-id: 20180306024328.19195-1-famz@redhat.com | ||
7 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
8 | --- | ||
9 | README | 2 +- | ||
10 | 1 file changed, 1 insertion(+), 1 deletion(-) | ||
11 | |||
12 | diff --git a/README b/README | ||
13 | index XXXXXXX..XXXXXXX 100644 | ||
14 | --- a/README | ||
15 | +++ b/README | ||
16 | @@ -XXX,XX +XXX,XX @@ The QEMU website is also maintained under source control. | ||
17 | git clone git://git.qemu.org/qemu-web.git | ||
18 | https://www.qemu.org/2017/02/04/the-new-qemu-website-is-up/ | ||
19 | |||
20 | -A 'git-profile' utility was created to make above process less | ||
21 | +A 'git-publish' utility was created to make above process less | ||
22 | cumbersome, and is highly recommended for making regular contributions, | ||
23 | or even just for sending consecutive patch series revisions. It also | ||
24 | requires a working 'git send-email' setup, and by default doesn't | ||
25 | -- | ||
26 | 2.14.3 | ||
27 | |||
28 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Sergio Lopez <slp@redhat.com> | ||
1 | 2 | ||
3 | Commit 5b2ffbe4d99843fd8305c573a100047a8c962327 ("virtio-blk: dataplane: | ||
4 | notify guest as a batch") deferred guest notification to a BH in order | ||
5 | batch notifications, with purpose of avoiding flooding the guest with | ||
6 | interruptions. | ||
7 | |||
8 | This optimization came with a cost. The average latency perceived in the | ||
9 | guest is increased by a few microseconds, but also when multiple IO | ||
10 | operations finish at the same time, the guest won't be notified until | ||
11 | all completions from each operation has been run. On the contrary, | ||
12 | virtio-scsi issues the notification at the end of each completion. | ||
13 | |||
14 | On the other hand, nowadays we have the EVENT_IDX feature that allows a | ||
15 | better coordination between QEMU and the Guest OS to avoid sending | ||
16 | unnecessary interruptions. | ||
17 | |||
18 | With this change, virtio-blk/dataplane only batches notifications if the | ||
19 | EVENT_IDX feature is not present. | ||
20 | |||
21 | Some numbers obtained with fio (ioengine=sync, iodepth=1, direct=1): | ||
22 | - Test specs: | ||
23 | * fio-3.4 (ioengine=sync, iodepth=1, direct=1) | ||
24 | * qemu master | ||
25 | * virtio-blk with a dedicated iothread (default poll-max-ns) | ||
26 | * backend: null_blk nr_devices=1 irqmode=2 completion_nsec=280000 | ||
27 | * 8 vCPUs pinned to isolated physical cores | ||
28 | * Emulator and iothread also pinned to separate isolated cores | ||
29 | * variance between runs < 1% | ||
30 | |||
31 | - Not patched | ||
32 | * numjobs=1: lat_avg=327.32 irqs=29998 | ||
33 | * numjobs=4: lat_avg=337.89 irqs=29073 | ||
34 | * numjobs=8: lat_avg=342.98 irqs=28643 | ||
35 | |||
36 | - Patched: | ||
37 | * numjobs=1: lat_avg=323.92 irqs=30262 | ||
38 | * numjobs=4: lat_avg=332.65 irqs=29520 | ||
39 | * numjobs=8: lat_avg=335.54 irqs=29323 | ||
40 | |||
41 | Signed-off-by: Sergio Lopez <slp@redhat.com> | ||
42 | Message-id: 20180307114459.26636-1-slp@redhat.com | ||
43 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
44 | --- | ||
45 | hw/block/dataplane/virtio-blk.c | 15 +++++++++++++-- | ||
46 | 1 file changed, 13 insertions(+), 2 deletions(-) | ||
47 | |||
48 | diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c | ||
49 | index XXXXXXX..XXXXXXX 100644 | ||
50 | --- a/hw/block/dataplane/virtio-blk.c | ||
51 | +++ b/hw/block/dataplane/virtio-blk.c | ||
52 | @@ -XXX,XX +XXX,XX @@ struct VirtIOBlockDataPlane { | ||
53 | VirtIODevice *vdev; | ||
54 | QEMUBH *bh; /* bh for guest notification */ | ||
55 | unsigned long *batch_notify_vqs; | ||
56 | + bool batch_notifications; | ||
57 | |||
58 | /* Note that these EventNotifiers are assigned by value. This is | ||
59 | * fine as long as you do not call event_notifier_cleanup on them | ||
60 | @@ -XXX,XX +XXX,XX @@ struct VirtIOBlockDataPlane { | ||
61 | /* Raise an interrupt to signal guest, if necessary */ | ||
62 | void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq) | ||
63 | { | ||
64 | - set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs); | ||
65 | - qemu_bh_schedule(s->bh); | ||
66 | + if (s->batch_notifications) { | ||
67 | + set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs); | ||
68 | + qemu_bh_schedule(s->bh); | ||
69 | + } else { | ||
70 | + virtio_notify_irqfd(s->vdev, vq); | ||
71 | + } | ||
72 | } | ||
73 | |||
74 | static void notify_guest_bh(void *opaque) | ||
75 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
76 | |||
77 | s->starting = true; | ||
78 | |||
79 | + if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { | ||
80 | + s->batch_notifications = true; | ||
81 | + } else { | ||
82 | + s->batch_notifications = false; | ||
83 | + } | ||
84 | + | ||
85 | /* Set up guest notifier (irq) */ | ||
86 | r = k->set_guest_notifiers(qbus->parent, nvqs, true); | ||
87 | if (r != 0) { | ||
88 | -- | ||
89 | 2.14.3 | ||
90 | |||
91 | diff view generated by jsdifflib |
1 | Nested BDRV_POLL_WHILE() calls can occur. Currently | 1 | Sometimes it's necessary for the main loop thread to run a BH in an |
---|---|---|---|
2 | assert(!wait_->wakeup) fails in AIO_WAIT_WHILE() when this happens. | 2 | IOThread and wait for its completion. This primitive is useful during |
3 | startup/shutdown to synchronize and avoid race conditions. | ||
3 | 4 | ||
4 | This patch converts the bool wait_->need_kick flag to an unsigned | ||
5 | wait_->num_waiters counter. | ||
6 | |||
7 | Nesting works correctly because outer AIO_WAIT_WHILE() callers evaluate | ||
8 | the condition again after the inner caller completes (invoking the inner | ||
9 | caller counts as aio_poll() progress). | ||
10 | |||
11 | Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com> | ||
12 | Reviewed-by: Eric Blake <eblake@redhat.com> | ||
13 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 5 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
14 | Message-id: 20180307124619.6218-1-stefanha@redhat.com | 6 | Reviewed-by: Fam Zheng <famz@redhat.com> |
15 | Cc: Paolo Bonzini <pbonzini@redhat.com> | 7 | Acked-by: Paolo Bonzini <pbonzini@redhat.com> |
8 | Message-id: 20180307144205.20619-2-stefanha@redhat.com | ||
16 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 9 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
17 | --- | 10 | --- |
18 | include/block/aio-wait.h | 61 ++++++++++++++++++++++++------------------------ | 11 | include/block/aio-wait.h | 13 +++++++++++++ |
19 | util/aio-wait.c | 2 +- | 12 | util/aio-wait.c | 31 +++++++++++++++++++++++++++++++ |
20 | 2 files changed, 31 insertions(+), 32 deletions(-) | 13 | 2 files changed, 44 insertions(+) |
21 | 14 | ||
22 | diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h | 15 | diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h |
23 | index XXXXXXX..XXXXXXX 100644 | 16 | index XXXXXXX..XXXXXXX 100644 |
24 | --- a/include/block/aio-wait.h | 17 | --- a/include/block/aio-wait.h |
25 | +++ b/include/block/aio-wait.h | 18 | +++ b/include/block/aio-wait.h |
26 | @@ -XXX,XX +XXX,XX @@ | 19 | @@ -XXX,XX +XXX,XX @@ typedef struct { |
27 | * } | ||
28 | */ | 20 | */ |
29 | typedef struct { | 21 | void aio_wait_kick(AioWait *wait); |
30 | - /* Is the main loop waiting for a kick? Accessed with atomic ops. */ | 22 | |
31 | - bool need_kick; | 23 | +/** |
32 | + /* Number of waiting AIO_WAIT_WHILE() callers. Accessed with atomic ops. */ | 24 | + * aio_wait_bh_oneshot: |
33 | + unsigned num_waiters; | 25 | + * @ctx: the aio context |
34 | } AioWait; | 26 | + * @cb: the BH callback function |
35 | 27 | + * @opaque: user data for the BH callback function | |
36 | /** | 28 | + * |
37 | @@ -XXX,XX +XXX,XX @@ typedef struct { | 29 | + * Run a BH in @ctx and wait for it to complete. |
38 | * wait on conditions between two IOThreads since that could lead to deadlock, | 30 | + * |
39 | * go via the main loop instead. | 31 | + * Must be called from the main loop thread with @ctx acquired exactly once. |
40 | */ | 32 | + * Note that main loop event processing may occur. |
41 | -#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \ | 33 | + */ |
42 | - bool waited_ = false; \ | 34 | +void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque); |
43 | - bool busy_ = true; \ | 35 | + |
44 | - AioWait *wait_ = (wait); \ | 36 | #endif /* QEMU_AIO_WAIT */ |
45 | - AioContext *ctx_ = (ctx); \ | ||
46 | - if (in_aio_context_home_thread(ctx_)) { \ | ||
47 | - while ((cond) || busy_) { \ | ||
48 | - busy_ = aio_poll(ctx_, (cond)); \ | ||
49 | - waited_ |= !!(cond) | busy_; \ | ||
50 | - } \ | ||
51 | - } else { \ | ||
52 | - assert(qemu_get_current_aio_context() == \ | ||
53 | - qemu_get_aio_context()); \ | ||
54 | - assert(!wait_->need_kick); \ | ||
55 | - /* Set wait_->need_kick before evaluating cond. */ \ | ||
56 | - atomic_mb_set(&wait_->need_kick, true); \ | ||
57 | - while (busy_) { \ | ||
58 | - if ((cond)) { \ | ||
59 | - waited_ = busy_ = true; \ | ||
60 | - aio_context_release(ctx_); \ | ||
61 | - aio_poll(qemu_get_aio_context(), true); \ | ||
62 | - aio_context_acquire(ctx_); \ | ||
63 | - } else { \ | ||
64 | - busy_ = aio_poll(ctx_, false); \ | ||
65 | - waited_ |= busy_; \ | ||
66 | - } \ | ||
67 | - } \ | ||
68 | - atomic_set(&wait_->need_kick, false); \ | ||
69 | - } \ | ||
70 | +#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \ | ||
71 | + bool waited_ = false; \ | ||
72 | + bool busy_ = true; \ | ||
73 | + AioWait *wait_ = (wait); \ | ||
74 | + AioContext *ctx_ = (ctx); \ | ||
75 | + if (in_aio_context_home_thread(ctx_)) { \ | ||
76 | + while ((cond) || busy_) { \ | ||
77 | + busy_ = aio_poll(ctx_, (cond)); \ | ||
78 | + waited_ |= !!(cond) | busy_; \ | ||
79 | + } \ | ||
80 | + } else { \ | ||
81 | + assert(qemu_get_current_aio_context() == \ | ||
82 | + qemu_get_aio_context()); \ | ||
83 | + /* Increment wait_->num_waiters before evaluating cond. */ \ | ||
84 | + atomic_inc(&wait_->num_waiters); \ | ||
85 | + while (busy_) { \ | ||
86 | + if ((cond)) { \ | ||
87 | + waited_ = busy_ = true; \ | ||
88 | + aio_context_release(ctx_); \ | ||
89 | + aio_poll(qemu_get_aio_context(), true); \ | ||
90 | + aio_context_acquire(ctx_); \ | ||
91 | + } else { \ | ||
92 | + busy_ = aio_poll(ctx_, false); \ | ||
93 | + waited_ |= busy_; \ | ||
94 | + } \ | ||
95 | + } \ | ||
96 | + atomic_dec(&wait_->num_waiters); \ | ||
97 | + } \ | ||
98 | waited_; }) | ||
99 | |||
100 | /** | ||
101 | diff --git a/util/aio-wait.c b/util/aio-wait.c | 37 | diff --git a/util/aio-wait.c b/util/aio-wait.c |
102 | index XXXXXXX..XXXXXXX 100644 | 38 | index XXXXXXX..XXXXXXX 100644 |
103 | --- a/util/aio-wait.c | 39 | --- a/util/aio-wait.c |
104 | +++ b/util/aio-wait.c | 40 | +++ b/util/aio-wait.c |
105 | @@ -XXX,XX +XXX,XX @@ static void dummy_bh_cb(void *opaque) | 41 | @@ -XXX,XX +XXX,XX @@ void aio_wait_kick(AioWait *wait) |
106 | void aio_wait_kick(AioWait *wait) | ||
107 | { | ||
108 | /* The barrier (or an atomic op) is in the caller. */ | ||
109 | - if (atomic_read(&wait->need_kick)) { | ||
110 | + if (atomic_read(&wait->num_waiters)) { | ||
111 | aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL); | 42 | aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL); |
112 | } | 43 | } |
113 | } | 44 | } |
45 | + | ||
46 | +typedef struct { | ||
47 | + AioWait wait; | ||
48 | + bool done; | ||
49 | + QEMUBHFunc *cb; | ||
50 | + void *opaque; | ||
51 | +} AioWaitBHData; | ||
52 | + | ||
53 | +/* Context: BH in IOThread */ | ||
54 | +static void aio_wait_bh(void *opaque) | ||
55 | +{ | ||
56 | + AioWaitBHData *data = opaque; | ||
57 | + | ||
58 | + data->cb(data->opaque); | ||
59 | + | ||
60 | + data->done = true; | ||
61 | + aio_wait_kick(&data->wait); | ||
62 | +} | ||
63 | + | ||
64 | +void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque) | ||
65 | +{ | ||
66 | + AioWaitBHData data = { | ||
67 | + .cb = cb, | ||
68 | + .opaque = opaque, | ||
69 | + }; | ||
70 | + | ||
71 | + assert(qemu_get_current_aio_context() == qemu_get_aio_context()); | ||
72 | + | ||
73 | + aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data); | ||
74 | + AIO_WAIT_WHILE(&data.wait, ctx, !data.done); | ||
75 | +} | ||
114 | -- | 76 | -- |
115 | 2.14.3 | 77 | 2.14.3 |
116 | 78 | ||
117 | 79 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | If the main loop thread invokes .ioeventfd_stop() just as the vq handler | ||
2 | function begins in the IOThread then the handler may lose the race for | ||
3 | the AioContext lock. By the time the vq handler is able to acquire the | ||
4 | AioContext lock the ioeventfd has already been removed and the handler | ||
5 | isn't supposed to run anymore! | ||
1 | 6 | ||
7 | Use the new aio_wait_bh_oneshot() function to perform ioeventfd removal | ||
8 | from within the IOThread. This way no races with the vq handler are | ||
9 | possible. | ||
10 | |||
11 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
12 | Reviewed-by: Fam Zheng <famz@redhat.com> | ||
13 | Acked-by: Paolo Bonzini <pbonzini@redhat.com> | ||
14 | Message-id: 20180307144205.20619-3-stefanha@redhat.com | ||
15 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
16 | --- | ||
17 | hw/block/dataplane/virtio-blk.c | 24 +++++++++++++++++------- | ||
18 | 1 file changed, 17 insertions(+), 7 deletions(-) | ||
19 | |||
20 | diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c | ||
21 | index XXXXXXX..XXXXXXX 100644 | ||
22 | --- a/hw/block/dataplane/virtio-blk.c | ||
23 | +++ b/hw/block/dataplane/virtio-blk.c | ||
24 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
25 | return -ENOSYS; | ||
26 | } | ||
27 | |||
28 | +/* Stop notifications for new requests from guest. | ||
29 | + * | ||
30 | + * Context: BH in IOThread | ||
31 | + */ | ||
32 | +static void virtio_blk_data_plane_stop_bh(void *opaque) | ||
33 | +{ | ||
34 | + VirtIOBlockDataPlane *s = opaque; | ||
35 | + unsigned i; | ||
36 | + | ||
37 | + for (i = 0; i < s->conf->num_queues; i++) { | ||
38 | + VirtQueue *vq = virtio_get_queue(s->vdev, i); | ||
39 | + | ||
40 | + virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, NULL); | ||
41 | + } | ||
42 | +} | ||
43 | + | ||
44 | /* Context: QEMU global mutex held */ | ||
45 | void virtio_blk_data_plane_stop(VirtIODevice *vdev) | ||
46 | { | ||
47 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) | ||
48 | trace_virtio_blk_data_plane_stop(s); | ||
49 | |||
50 | aio_context_acquire(s->ctx); | ||
51 | - | ||
52 | - /* Stop notifications for new requests from guest */ | ||
53 | - for (i = 0; i < nvqs; i++) { | ||
54 | - VirtQueue *vq = virtio_get_queue(s->vdev, i); | ||
55 | - | ||
56 | - virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, NULL); | ||
57 | - } | ||
58 | + aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s); | ||
59 | |||
60 | /* Drain and switch bs back to the QEMU main loop */ | ||
61 | blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); | ||
62 | -- | ||
63 | 2.14.3 | ||
64 | |||
65 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | If the main loop thread invokes .ioeventfd_stop() just as the vq handler | ||
2 | function begins in the IOThread then the handler may lose the race for | ||
3 | the AioContext lock. By the time the vq handler is able to acquire the | ||
4 | AioContext lock the ioeventfd has already been removed and the handler | ||
5 | isn't supposed to run anymore! | ||
1 | 6 | ||
7 | Use the new aio_wait_bh_oneshot() function to perform ioeventfd removal | ||
8 | from within the IOThread. This way no races with the vq handler are | ||
9 | possible. | ||
10 | |||
11 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
12 | Reviewed-by: Fam Zheng <famz@redhat.com> | ||
13 | Acked-by: Paolo Bonzini <pbonzini@redhat.com> | ||
14 | Message-id: 20180307144205.20619-4-stefanha@redhat.com | ||
15 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
16 | --- | ||
17 | hw/scsi/virtio-scsi-dataplane.c | 9 +++++---- | ||
18 | 1 file changed, 5 insertions(+), 4 deletions(-) | ||
19 | |||
20 | diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c | ||
21 | index XXXXXXX..XXXXXXX 100644 | ||
22 | --- a/hw/scsi/virtio-scsi-dataplane.c | ||
23 | +++ b/hw/scsi/virtio-scsi-dataplane.c | ||
24 | @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, | ||
25 | return 0; | ||
26 | } | ||
27 | |||
28 | -/* assumes s->ctx held */ | ||
29 | -static void virtio_scsi_clear_aio(VirtIOSCSI *s) | ||
30 | +/* Context: BH in IOThread */ | ||
31 | +static void virtio_scsi_dataplane_stop_bh(void *opaque) | ||
32 | { | ||
33 | + VirtIOSCSI *s = opaque; | ||
34 | VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); | ||
35 | int i; | ||
36 | |||
37 | @@ -XXX,XX +XXX,XX @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev) | ||
38 | return 0; | ||
39 | |||
40 | fail_vrings: | ||
41 | - virtio_scsi_clear_aio(s); | ||
42 | + aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s); | ||
43 | aio_context_release(s->ctx); | ||
44 | for (i = 0; i < vs->conf.num_queues + 2; i++) { | ||
45 | virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); | ||
46 | @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev) | ||
47 | s->dataplane_stopping = true; | ||
48 | |||
49 | aio_context_acquire(s->ctx); | ||
50 | - virtio_scsi_clear_aio(s); | ||
51 | + aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s); | ||
52 | aio_context_release(s->ctx); | ||
53 | |||
54 | blk_drain_all(); /* ensure there are no in-flight requests */ | ||
55 | -- | ||
56 | 2.14.3 | ||
57 | |||
58 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | Commit 00d09fdbbae5f7864ce754913efc84c12fdf9f1a ("vl: pause vcpus before | ||
2 | stopping iothreads") and commit dce8921b2baaf95974af8176406881872067adfa | ||
3 | ("iothread: Stop threads before main() quits") tried to work around the | ||
4 | fact that emulation was still active during termination by stopping | ||
5 | iothreads. They suffer from race conditions: | ||
6 | 1. virtio_scsi_handle_cmd_vq() racing with iothread_stop_all() hits the | ||
7 | virtio_scsi_ctx_check() assertion failure because the BDS AioContext | ||
8 | has been modified by iothread_stop_all(). | ||
9 | 2. Guest vq kick racing with main loop termination leaves a readable | ||
10 | ioeventfd that is handled by the next aio_poll() when external | ||
11 | clients are enabled again, resulting in unwanted emulation activity. | ||
1 | 12 | ||
13 | This patch obsoletes those commits by fully disabling emulation activity | ||
14 | when vcpus are stopped. | ||
15 | |||
16 | Use the new vm_shutdown() function instead of pause_all_vcpus() so that | ||
17 | vm change state handlers are invoked too. Virtio devices will now stop | ||
18 | their ioeventfds, preventing further emulation activity after vm_stop(). | ||
19 | |||
20 | Note that vm_stop(RUN_STATE_SHUTDOWN) cannot be used because it emits a | ||
21 | QMP STOP event that may affect existing clients. | ||
22 | |||
23 | It is no longer necessary to call replay_disable_events() directly since | ||
24 | vm_shutdown() does so already. | ||
25 | |||
26 | Drop iothread_stop_all() since it is no longer used. | ||
27 | |||
28 | Cc: Fam Zheng <famz@redhat.com> | ||
29 | Cc: Kevin Wolf <kwolf@redhat.com> | ||
30 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
31 | Reviewed-by: Fam Zheng <famz@redhat.com> | ||
32 | Acked-by: Paolo Bonzini <pbonzini@redhat.com> | ||
33 | Message-id: 20180307144205.20619-5-stefanha@redhat.com | ||
34 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
35 | --- | ||
36 | include/sysemu/iothread.h | 1 - | ||
37 | include/sysemu/sysemu.h | 1 + | ||
38 | cpus.c | 16 +++++++++++++--- | ||
39 | iothread.c | 31 ------------------------------- | ||
40 | vl.c | 13 +++---------- | ||
41 | 5 files changed, 17 insertions(+), 45 deletions(-) | ||
42 | |||
43 | diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h | ||
44 | index XXXXXXX..XXXXXXX 100644 | ||
45 | --- a/include/sysemu/iothread.h | ||
46 | +++ b/include/sysemu/iothread.h | ||
47 | @@ -XXX,XX +XXX,XX @@ typedef struct { | ||
48 | char *iothread_get_id(IOThread *iothread); | ||
49 | IOThread *iothread_by_id(const char *id); | ||
50 | AioContext *iothread_get_aio_context(IOThread *iothread); | ||
51 | -void iothread_stop_all(void); | ||
52 | GMainContext *iothread_get_g_main_context(IOThread *iothread); | ||
53 | |||
54 | /* | ||
55 | diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h | ||
56 | index XXXXXXX..XXXXXXX 100644 | ||
57 | --- a/include/sysemu/sysemu.h | ||
58 | +++ b/include/sysemu/sysemu.h | ||
59 | @@ -XXX,XX +XXX,XX @@ void vm_start(void); | ||
60 | int vm_prepare_start(void); | ||
61 | int vm_stop(RunState state); | ||
62 | int vm_stop_force_state(RunState state); | ||
63 | +int vm_shutdown(void); | ||
64 | |||
65 | typedef enum WakeupReason { | ||
66 | /* Always keep QEMU_WAKEUP_REASON_NONE = 0 */ | ||
67 | diff --git a/cpus.c b/cpus.c | ||
68 | index XXXXXXX..XXXXXXX 100644 | ||
69 | --- a/cpus.c | ||
70 | +++ b/cpus.c | ||
71 | @@ -XXX,XX +XXX,XX @@ void cpu_synchronize_all_pre_loadvm(void) | ||
72 | } | ||
73 | } | ||
74 | |||
75 | -static int do_vm_stop(RunState state) | ||
76 | +static int do_vm_stop(RunState state, bool send_stop) | ||
77 | { | ||
78 | int ret = 0; | ||
79 | |||
80 | @@ -XXX,XX +XXX,XX @@ static int do_vm_stop(RunState state) | ||
81 | pause_all_vcpus(); | ||
82 | runstate_set(state); | ||
83 | vm_state_notify(0, state); | ||
84 | - qapi_event_send_stop(&error_abort); | ||
85 | + if (send_stop) { | ||
86 | + qapi_event_send_stop(&error_abort); | ||
87 | + } | ||
88 | } | ||
89 | |||
90 | bdrv_drain_all(); | ||
91 | @@ -XXX,XX +XXX,XX @@ static int do_vm_stop(RunState state) | ||
92 | return ret; | ||
93 | } | ||
94 | |||
95 | +/* Special vm_stop() variant for terminating the process. Historically clients | ||
96 | + * did not expect a QMP STOP event and so we need to retain compatibility. | ||
97 | + */ | ||
98 | +int vm_shutdown(void) | ||
99 | +{ | ||
100 | + return do_vm_stop(RUN_STATE_SHUTDOWN, false); | ||
101 | +} | ||
102 | + | ||
103 | static bool cpu_can_run(CPUState *cpu) | ||
104 | { | ||
105 | if (cpu->stop) { | ||
106 | @@ -XXX,XX +XXX,XX @@ int vm_stop(RunState state) | ||
107 | return 0; | ||
108 | } | ||
109 | |||
110 | - return do_vm_stop(state); | ||
111 | + return do_vm_stop(state, true); | ||
112 | } | ||
113 | |||
114 | /** | ||
115 | diff --git a/iothread.c b/iothread.c | ||
116 | index XXXXXXX..XXXXXXX 100644 | ||
117 | --- a/iothread.c | ||
118 | +++ b/iothread.c | ||
119 | @@ -XXX,XX +XXX,XX @@ void iothread_stop(IOThread *iothread) | ||
120 | qemu_thread_join(&iothread->thread); | ||
121 | } | ||
122 | |||
123 | -static int iothread_stop_iter(Object *object, void *opaque) | ||
124 | -{ | ||
125 | - IOThread *iothread; | ||
126 | - | ||
127 | - iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD); | ||
128 | - if (!iothread) { | ||
129 | - return 0; | ||
130 | - } | ||
131 | - iothread_stop(iothread); | ||
132 | - return 0; | ||
133 | -} | ||
134 | - | ||
135 | static void iothread_instance_init(Object *obj) | ||
136 | { | ||
137 | IOThread *iothread = IOTHREAD(obj); | ||
138 | @@ -XXX,XX +XXX,XX @@ IOThreadInfoList *qmp_query_iothreads(Error **errp) | ||
139 | return head; | ||
140 | } | ||
141 | |||
142 | -void iothread_stop_all(void) | ||
143 | -{ | ||
144 | - Object *container = object_get_objects_root(); | ||
145 | - BlockDriverState *bs; | ||
146 | - BdrvNextIterator it; | ||
147 | - | ||
148 | - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { | ||
149 | - AioContext *ctx = bdrv_get_aio_context(bs); | ||
150 | - if (ctx == qemu_get_aio_context()) { | ||
151 | - continue; | ||
152 | - } | ||
153 | - aio_context_acquire(ctx); | ||
154 | - bdrv_set_aio_context(bs, qemu_get_aio_context()); | ||
155 | - aio_context_release(ctx); | ||
156 | - } | ||
157 | - | ||
158 | - object_child_foreach(container, iothread_stop_iter, NULL); | ||
159 | -} | ||
160 | - | ||
161 | static gpointer iothread_g_main_context_init(gpointer opaque) | ||
162 | { | ||
163 | AioContext *ctx; | ||
164 | diff --git a/vl.c b/vl.c | ||
165 | index XXXXXXX..XXXXXXX 100644 | ||
166 | --- a/vl.c | ||
167 | +++ b/vl.c | ||
168 | @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) | ||
169 | os_setup_post(); | ||
170 | |||
171 | main_loop(); | ||
172 | - replay_disable_events(); | ||
173 | |||
174 | - /* The ordering of the following is delicate. Stop vcpus to prevent new | ||
175 | - * I/O requests being queued by the guest. Then stop IOThreads (this | ||
176 | - * includes a drain operation and completes all request processing). At | ||
177 | - * this point emulated devices are still associated with their IOThreads | ||
178 | - * (if any) but no longer have any work to do. Only then can we close | ||
179 | - * block devices safely because we know there is no more I/O coming. | ||
180 | - */ | ||
181 | - pause_all_vcpus(); | ||
182 | - iothread_stop_all(); | ||
183 | + /* No more vcpu or device emulation activity beyond this point */ | ||
184 | + vm_shutdown(); | ||
185 | + | ||
186 | bdrv_close_all(); | ||
187 | |||
188 | res_free(); | ||
189 | -- | ||
190 | 2.14.3 | ||
191 | |||
192 | diff view generated by jsdifflib |