1 | The following changes since commit ca61fa4b803e5d0abaf6f1ceb690f23bb78a4def: | 1 | The following changes since commit 887cba855bb6ff4775256f7968409281350b568c: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/quic/tags/pull-hex-20211006' into staging (2021-10-06 12:11:14 -0700) | 3 | configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100) |
4 | 4 | ||
5 | are available in the Git repository at: | 5 | are available in the Git repository at: |
6 | 6 | ||
7 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request | 7 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request |
8 | 8 | ||
9 | for you to fetch changes up to 1cc7eada97914f090125e588497986f6f7900514: | 9 | for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6: |
10 | 10 | ||
11 | iothread: use IOThreadParamInfo in iothread_[set|get]_param() (2021-10-07 15:29:50 +0100) | 11 | virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Pull request | 14 | Pull request |
15 | 15 | ||
16 | ---------------------------------------------------------------- | 16 | ---------------------------------------------------------------- |
17 | 17 | ||
18 | Stefano Garzarella (2): | 18 | Stefan Hajnoczi (1): |
19 | iothread: rename PollParamInfo to IOThreadParamInfo | 19 | virtio-blk: fix host notifier issues during dataplane start/stop |
20 | iothread: use IOThreadParamInfo in iothread_[set|get]_param() | ||
21 | 20 | ||
22 | iothread.c | 28 +++++++++++++++------------- | 21 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
23 | 1 file changed, 15 insertions(+), 13 deletions(-) | 22 | 1 file changed, 38 insertions(+), 29 deletions(-) |
24 | 23 | ||
25 | -- | 24 | -- |
26 | 2.31.1 | 25 | 2.40.1 |
27 | |||
28 | |||
29 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | From: Stefano Garzarella <sgarzare@redhat.com> | ||
2 | 1 | ||
3 | Commit 1793ad0247 ("iothread: add aio-max-batch parameter") added | ||
4 | a new parameter (aio-max-batch) to IOThread and used PollParamInfo | ||
5 | structure to handle it. | ||
6 | |||
7 | Since it is not a parameter of the polling mechanism, we rename the | ||
8 | structure to a more generic IOThreadParamInfo. | ||
9 | |||
10 | Suggested-by: Kevin Wolf <kwolf@redhat.com> | ||
11 | Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> | ||
12 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
13 | Message-id: 20210727145936.147032-2-sgarzare@redhat.com | ||
14 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
15 | --- | ||
16 | iothread.c | 14 +++++++------- | ||
17 | 1 file changed, 7 insertions(+), 7 deletions(-) | ||
18 | |||
19 | diff --git a/iothread.c b/iothread.c | ||
20 | index XXXXXXX..XXXXXXX 100644 | ||
21 | --- a/iothread.c | ||
22 | +++ b/iothread.c | ||
23 | @@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp) | ||
24 | typedef struct { | ||
25 | const char *name; | ||
26 | ptrdiff_t offset; /* field's byte offset in IOThread struct */ | ||
27 | -} PollParamInfo; | ||
28 | +} IOThreadParamInfo; | ||
29 | |||
30 | -static PollParamInfo poll_max_ns_info = { | ||
31 | +static IOThreadParamInfo poll_max_ns_info = { | ||
32 | "poll-max-ns", offsetof(IOThread, poll_max_ns), | ||
33 | }; | ||
34 | -static PollParamInfo poll_grow_info = { | ||
35 | +static IOThreadParamInfo poll_grow_info = { | ||
36 | "poll-grow", offsetof(IOThread, poll_grow), | ||
37 | }; | ||
38 | -static PollParamInfo poll_shrink_info = { | ||
39 | +static IOThreadParamInfo poll_shrink_info = { | ||
40 | "poll-shrink", offsetof(IOThread, poll_shrink), | ||
41 | }; | ||
42 | -static PollParamInfo aio_max_batch_info = { | ||
43 | +static IOThreadParamInfo aio_max_batch_info = { | ||
44 | "aio-max-batch", offsetof(IOThread, aio_max_batch), | ||
45 | }; | ||
46 | |||
47 | @@ -XXX,XX +XXX,XX @@ static void iothread_get_param(Object *obj, Visitor *v, | ||
48 | const char *name, void *opaque, Error **errp) | ||
49 | { | ||
50 | IOThread *iothread = IOTHREAD(obj); | ||
51 | - PollParamInfo *info = opaque; | ||
52 | + IOThreadParamInfo *info = opaque; | ||
53 | int64_t *field = (void *)iothread + info->offset; | ||
54 | |||
55 | visit_type_int64(v, name, field, errp); | ||
56 | @@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v, | ||
57 | const char *name, void *opaque, Error **errp) | ||
58 | { | ||
59 | IOThread *iothread = IOTHREAD(obj); | ||
60 | - PollParamInfo *info = opaque; | ||
61 | + IOThreadParamInfo *info = opaque; | ||
62 | int64_t *field = (void *)iothread + info->offset; | ||
63 | int64_t value; | ||
64 | |||
65 | -- | ||
66 | 2.31.1 | ||
67 | |||
68 | diff view generated by jsdifflib |
1 | From: Stefano Garzarella <sgarzare@redhat.com> | 1 | The main loop thread can consume 100% CPU when using --device |
---|---|---|---|
2 | virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but | ||
3 | reading virtqueue host notifiers fails with EAGAIN. The file descriptors | ||
4 | are stale and remain registered with the AioContext because of bugs in | ||
5 | the virtio-blk dataplane start/stop code. | ||
2 | 6 | ||
3 | Commit 0445409d74 ("iothread: generalize | 7 | The problem is that the dataplane start/stop code involves drain |
4 | iothread_set_param/iothread_get_param") moved common code to set and | 8 | operations, which call virtio_blk_drained_begin() and |
5 | get IOThread parameters in two new functions. | 9 | virtio_blk_drained_end() at points where the host notifier is not |
10 | operational: | ||
11 | - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after | ||
12 | vblk->dataplane_started has been set to true but the host notifier has | ||
13 | not been attached yet. | ||
14 | - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context() | ||
15 | drain after the host notifier has already been detached but with | ||
16 | vblk->dataplane_started still set to true. | ||
6 | 17 | ||
7 | These functions are called inside callbacks, so we don't need to use an | 18 | I would like to simplify ->ioeventfd_start/stop() to avoid interactions |
8 | opaque pointer. Let's replace `void *opaque` parameter with | 19 | with drain entirely, but couldn't find a way to do that. Instead, this |
9 | `IOThreadParamInfo *info`. | 20 | patch accepts the fragile nature of the code and reorders it so that |
21 | vblk->dataplane_started is false during drain operations. This way the | ||
22 | virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't | ||
23 | touch the host notifier. The result is that | ||
24 | virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have | ||
25 | complete control over the host notifier and stale file descriptors are | ||
26 | no longer left in the AioContext. | ||
10 | 27 | ||
11 | Suggested-by: Kevin Wolf <kwolf@redhat.com> | 28 | This patch fixes the 100% CPU consumption in the main loop thread and |
12 | Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> | 29 | correctly moves host notifier processing to the IOThread. |
13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | 30 | |
14 | Message-id: 20210727145936.147032-3-sgarzare@redhat.com | 31 | Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()") |
32 | Reported-by: Lukáš Doktor <ldoktor@redhat.com> | ||
33 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
34 | Tested-by: Lukas Doktor <ldoktor@redhat.com> | ||
35 | Message-id: 20230704151527.193586-1-stefanha@redhat.com | ||
15 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 36 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
16 | --- | 37 | --- |
17 | iothread.c | 18 ++++++++++-------- | 38 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
18 | 1 file changed, 10 insertions(+), 8 deletions(-) | 39 | 1 file changed, 38 insertions(+), 29 deletions(-) |
19 | 40 | ||
20 | diff --git a/iothread.c b/iothread.c | 41 | diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c |
21 | index XXXXXXX..XXXXXXX 100644 | 42 | index XXXXXXX..XXXXXXX 100644 |
22 | --- a/iothread.c | 43 | --- a/hw/block/dataplane/virtio-blk.c |
23 | +++ b/iothread.c | 44 | +++ b/hw/block/dataplane/virtio-blk.c |
24 | @@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo aio_max_batch_info = { | 45 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) |
25 | }; | 46 | |
26 | 47 | memory_region_transaction_commit(); | |
27 | static void iothread_get_param(Object *obj, Visitor *v, | 48 | |
28 | - const char *name, void *opaque, Error **errp) | 49 | - /* |
29 | + const char *name, IOThreadParamInfo *info, Error **errp) | 50 | - * These fields are visible to the IOThread so we rely on implicit barriers |
30 | { | 51 | - * in aio_context_acquire() on the write side and aio_notify_accept() on |
31 | IOThread *iothread = IOTHREAD(obj); | 52 | - * the read side. |
32 | - IOThreadParamInfo *info = opaque; | 53 | - */ |
33 | int64_t *field = (void *)iothread + info->offset; | 54 | - s->starting = false; |
34 | 55 | - vblk->dataplane_started = true; | |
35 | visit_type_int64(v, name, field, errp); | 56 | trace_virtio_blk_data_plane_start(s); |
57 | |||
58 | old_context = blk_get_aio_context(s->conf->conf.blk); | ||
59 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
60 | event_notifier_set(virtio_queue_get_host_notifier(vq)); | ||
61 | } | ||
62 | |||
63 | + /* | ||
64 | + * These fields must be visible to the IOThread when it processes the | ||
65 | + * virtqueue, otherwise it will think dataplane has not started yet. | ||
66 | + * | ||
67 | + * Make sure ->dataplane_started is false when blk_set_aio_context() is | ||
68 | + * called above so that draining does not cause the host notifier to be | ||
69 | + * detached/attached prematurely. | ||
70 | + */ | ||
71 | + s->starting = false; | ||
72 | + vblk->dataplane_started = true; | ||
73 | + smp_wmb(); /* paired with aio_notify_accept() on the read side */ | ||
74 | + | ||
75 | /* Get this show started by hooking up our callbacks */ | ||
76 | if (!blk_in_drain(s->conf->conf.blk)) { | ||
77 | aio_context_acquire(s->ctx); | ||
78 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
79 | fail_guest_notifiers: | ||
80 | vblk->dataplane_disabled = true; | ||
81 | s->starting = false; | ||
82 | - vblk->dataplane_started = true; | ||
83 | return -ENOSYS; | ||
36 | } | 84 | } |
37 | 85 | ||
38 | static bool iothread_set_param(Object *obj, Visitor *v, | 86 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) |
39 | - const char *name, void *opaque, Error **errp) | 87 | aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s); |
40 | + const char *name, IOThreadParamInfo *info, Error **errp) | 88 | } |
41 | { | 89 | |
42 | IOThread *iothread = IOTHREAD(obj); | 90 | + /* |
43 | - IOThreadParamInfo *info = opaque; | 91 | + * Batch all the host notifiers in a single transaction to avoid |
44 | int64_t *field = (void *)iothread + info->offset; | 92 | + * quadratic time complexity in address_space_update_ioeventfds(). |
45 | int64_t value; | 93 | + */ |
46 | 94 | + memory_region_transaction_begin(); | |
47 | @@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v, | 95 | + |
48 | static void iothread_get_poll_param(Object *obj, Visitor *v, | 96 | + for (i = 0; i < nvqs; i++) { |
49 | const char *name, void *opaque, Error **errp) | 97 | + virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); |
50 | { | 98 | + } |
51 | + IOThreadParamInfo *info = opaque; | 99 | + |
52 | 100 | + /* | |
53 | - iothread_get_param(obj, v, name, opaque, errp); | 101 | + * The transaction expects the ioeventfds to be open when it |
54 | + iothread_get_param(obj, v, name, info, errp); | 102 | + * commits. Do it now, before the cleanup loop. |
103 | + */ | ||
104 | + memory_region_transaction_commit(); | ||
105 | + | ||
106 | + for (i = 0; i < nvqs; i++) { | ||
107 | + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
108 | + } | ||
109 | + | ||
110 | + /* | ||
111 | + * Set ->dataplane_started to false before draining so that host notifiers | ||
112 | + * are not detached/attached anymore. | ||
113 | + */ | ||
114 | + vblk->dataplane_started = false; | ||
115 | + | ||
116 | aio_context_acquire(s->ctx); | ||
117 | |||
118 | /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */ | ||
119 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) | ||
120 | |||
121 | aio_context_release(s->ctx); | ||
122 | |||
123 | - /* | ||
124 | - * Batch all the host notifiers in a single transaction to avoid | ||
125 | - * quadratic time complexity in address_space_update_ioeventfds(). | ||
126 | - */ | ||
127 | - memory_region_transaction_begin(); | ||
128 | - | ||
129 | - for (i = 0; i < nvqs; i++) { | ||
130 | - virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); | ||
131 | - } | ||
132 | - | ||
133 | - /* | ||
134 | - * The transaction expects the ioeventfds to be open when it | ||
135 | - * commits. Do it now, before the cleanup loop. | ||
136 | - */ | ||
137 | - memory_region_transaction_commit(); | ||
138 | - | ||
139 | - for (i = 0; i < nvqs; i++) { | ||
140 | - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
141 | - } | ||
142 | - | ||
143 | qemu_bh_cancel(s->bh); | ||
144 | notify_guest_bh(s); /* final chance to notify guest */ | ||
145 | |||
146 | /* Clean up guest notifier (irq) */ | ||
147 | k->set_guest_notifiers(qbus->parent, nvqs, false); | ||
148 | |||
149 | - vblk->dataplane_started = false; | ||
150 | s->stopping = false; | ||
55 | } | 151 | } |
56 | |||
57 | static void iothread_set_poll_param(Object *obj, Visitor *v, | ||
58 | const char *name, void *opaque, Error **errp) | ||
59 | { | ||
60 | IOThread *iothread = IOTHREAD(obj); | ||
61 | + IOThreadParamInfo *info = opaque; | ||
62 | |||
63 | - if (!iothread_set_param(obj, v, name, opaque, errp)) { | ||
64 | + if (!iothread_set_param(obj, v, name, info, errp)) { | ||
65 | return; | ||
66 | } | ||
67 | |||
68 | @@ -XXX,XX +XXX,XX @@ static void iothread_set_poll_param(Object *obj, Visitor *v, | ||
69 | static void iothread_get_aio_param(Object *obj, Visitor *v, | ||
70 | const char *name, void *opaque, Error **errp) | ||
71 | { | ||
72 | + IOThreadParamInfo *info = opaque; | ||
73 | |||
74 | - iothread_get_param(obj, v, name, opaque, errp); | ||
75 | + iothread_get_param(obj, v, name, info, errp); | ||
76 | } | ||
77 | |||
78 | static void iothread_set_aio_param(Object *obj, Visitor *v, | ||
79 | const char *name, void *opaque, Error **errp) | ||
80 | { | ||
81 | IOThread *iothread = IOTHREAD(obj); | ||
82 | + IOThreadParamInfo *info = opaque; | ||
83 | |||
84 | - if (!iothread_set_param(obj, v, name, opaque, errp)) { | ||
85 | + if (!iothread_set_param(obj, v, name, info, errp)) { | ||
86 | return; | ||
87 | } | ||
88 | |||
89 | -- | 152 | -- |
90 | 2.31.1 | 153 | 2.40.1 |
91 | 154 | ||
92 | 155 | diff view generated by jsdifflib |