1 | The following changes since commit ca61fa4b803e5d0abaf6f1ceb690f23bb78a4def: | 1 | The following changes since commit b0fbe46ad82982b289a44ee2495b59b0bad8a842: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/quic/tags/pull-hex-20211006' into staging (2021-10-06 12:11:14 -0700) | 3 | Update version for v2.11.0-rc0 release (2017-11-07 16:05:28 +0000) |
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 | git://github.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 ef6dada8b44e1e7c4bec5c1115903af9af415b50: |
10 | 10 | ||
11 | iothread: use IOThreadParamInfo in iothread_[set|get]_param() (2021-10-07 15:29:50 +0100) | 11 | util/async: use atomic_mb_set in qemu_bh_cancel (2017-11-08 19:09:15 +0000) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Pull request | 14 | Pull request |
15 | 15 | ||
16 | v2: | ||
17 | * v1 emails 2/3 and 3/3 weren't sent due to an email failure | ||
18 | * Included Sergio's updated wording in the commit description | ||
19 | |||
16 | ---------------------------------------------------------------- | 20 | ---------------------------------------------------------------- |
17 | 21 | ||
18 | Stefano Garzarella (2): | 22 | Sergio Lopez (1): |
19 | iothread: rename PollParamInfo to IOThreadParamInfo | 23 | util/async: use atomic_mb_set in qemu_bh_cancel |
20 | iothread: use IOThreadParamInfo in iothread_[set|get]_param() | ||
21 | 24 | ||
22 | iothread.c | 28 +++++++++++++++------------- | 25 | Stefan Hajnoczi (1): |
23 | 1 file changed, 15 insertions(+), 13 deletions(-) | 26 | tests-aio-multithread: fix /aio/multi/schedule race condition |
27 | |||
28 | tests/test-aio-multithread.c | 5 ++--- | ||
29 | util/async.c | 2 +- | ||
30 | 2 files changed, 3 insertions(+), 4 deletions(-) | ||
24 | 31 | ||
25 | -- | 32 | -- |
26 | 2.31.1 | 33 | 2.13.6 |
27 | 34 | ||
28 | 35 | ||
29 | diff view generated by jsdifflib |
1 | From: Stefano Garzarella <sgarzare@redhat.com> | 1 | test_multi_co_schedule_entry() set to_schedule[id] in the final loop |
---|---|---|---|
2 | iteration before terminating the coroutine. There is a race condition | ||
3 | where the main thread attempts to enter the terminating or terminated | ||
4 | coroutine when signalling coroutines to stop: | ||
2 | 5 | ||
3 | Commit 1793ad0247 ("iothread: add aio-max-batch parameter") added | 6 | atomic_mb_set(&now_stopping, true); |
4 | a new parameter (aio-max-batch) to IOThread and used PollParamInfo | 7 | for (i = 0; i < NUM_CONTEXTS; i++) { |
5 | structure to handle it. | 8 | ctx_run(i, finish_cb, NULL); <--- enters dead coroutine! |
9 | to_schedule[i] = NULL; | ||
10 | } | ||
6 | 11 | ||
7 | Since it is not a parameter of the polling mechanism, we rename the | 12 | Make sure only to set to_schedule[id] if this coroutine really needs to |
8 | structure to a more generic IOThreadParamInfo. | 13 | be scheduled! |
9 | 14 | ||
10 | Suggested-by: Kevin Wolf <kwolf@redhat.com> | 15 | Reported-by: "R.Nageswara Sastry" <nasastry@in.ibm.com> |
11 | Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> | 16 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
12 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | 17 | Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> |
13 | Message-id: 20210727145936.147032-2-sgarzare@redhat.com | 18 | Message-id: 20171106190233.1175-1-stefanha@redhat.com |
14 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 19 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
15 | --- | 20 | --- |
16 | iothread.c | 14 +++++++------- | 21 | tests/test-aio-multithread.c | 5 ++--- |
17 | 1 file changed, 7 insertions(+), 7 deletions(-) | 22 | 1 file changed, 2 insertions(+), 3 deletions(-) |
18 | 23 | ||
19 | diff --git a/iothread.c b/iothread.c | 24 | diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c |
20 | index XXXXXXX..XXXXXXX 100644 | 25 | index XXXXXXX..XXXXXXX 100644 |
21 | --- a/iothread.c | 26 | --- a/tests/test-aio-multithread.c |
22 | +++ b/iothread.c | 27 | +++ b/tests/test-aio-multithread.c |
23 | @@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp) | 28 | @@ -XXX,XX +XXX,XX @@ static void finish_cb(void *opaque) |
24 | typedef struct { | 29 | static coroutine_fn void test_multi_co_schedule_entry(void *opaque) |
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 | { | 30 | { |
50 | IOThread *iothread = IOTHREAD(obj); | 31 | g_assert(to_schedule[id] == NULL); |
51 | - PollParamInfo *info = opaque; | 32 | - atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
52 | + IOThreadParamInfo *info = opaque; | 33 | |
53 | int64_t *field = (void *)iothread + info->offset; | 34 | while (!atomic_mb_read(&now_stopping)) { |
54 | 35 | int n; | |
55 | visit_type_int64(v, name, field, errp); | 36 | |
56 | @@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v, | 37 | n = g_test_rand_int_range(0, NUM_CONTEXTS); |
57 | const char *name, void *opaque, Error **errp) | 38 | schedule_next(n); |
58 | { | 39 | + |
59 | IOThread *iothread = IOTHREAD(obj); | 40 | + atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
60 | - PollParamInfo *info = opaque; | 41 | qemu_coroutine_yield(); |
61 | + IOThreadParamInfo *info = opaque; | 42 | - |
62 | int64_t *field = (void *)iothread + info->offset; | 43 | g_assert(to_schedule[id] == NULL); |
63 | int64_t value; | 44 | - atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
45 | } | ||
46 | } | ||
64 | 47 | ||
65 | -- | 48 | -- |
66 | 2.31.1 | 49 | 2.13.6 |
67 | 50 | ||
68 | 51 | diff view generated by jsdifflib |
1 | From: Stefano Garzarella <sgarzare@redhat.com> | 1 | From: Sergio Lopez <slp@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | Commit 0445409d74 ("iothread: generalize | 3 | Commit b7a745d added a qemu_bh_cancel call to the completion function |
4 | iothread_set_param/iothread_get_param") moved common code to set and | 4 | as an optimization to prevent it from unnecessarily rescheduling itself. |
5 | get IOThread parameters in two new functions. | ||
6 | 5 | ||
7 | These functions are called inside callbacks, so we don't need to use an | 6 | This completion function is scheduled from worker_thread, after setting |
8 | opaque pointer. Let's replace `void *opaque` parameter with | 7 | the state of a ThreadPoolElement to THREAD_DONE. |
9 | `IOThreadParamInfo *info`. | ||
10 | 8 | ||
11 | Suggested-by: Kevin Wolf <kwolf@redhat.com> | 9 | This was considered to be safe, as the completion function restarts the |
12 | Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> | 10 | loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW |
13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | 11 | memory barrier, the read of req->state may actually happen _before_ the |
14 | Message-id: 20210727145936.147032-3-sgarzare@redhat.com | 12 | call, seeing it still as THREAD_QUEUED, and ending the completion |
13 | function without having processed a pending TPE linked at pool->head: | ||
14 | |||
15 | worker thread | I/O thread | ||
16 | ------------------------------------------------------------------------ | ||
17 | | speculatively read req->state | ||
18 | req->state = THREAD_DONE; | | ||
19 | qemu_bh_schedule(p->completion_bh) | | ||
20 | bh->scheduled = 1; | | ||
21 | | qemu_bh_cancel(p->completion_bh) | ||
22 | | bh->scheduled = 0; | ||
23 | | if (req->state == THREAD_DONE) | ||
24 | | // sees THREAD_QUEUED | ||
25 | |||
26 | The source of the misunderstanding was that qemu_bh_cancel is now being | ||
27 | used by the _consumer_ rather than the producer, and therefore now needs | ||
28 | to have acquire semantics just like e.g. aio_bh_poll. | ||
29 | |||
30 | In some situations, if there are no other independent requests in the | ||
31 | same aio context that could eventually trigger the scheduling of the | ||
32 | completion function, the omitted TPE and all operations pending on it | ||
33 | will get stuck forever. | ||
34 | |||
35 | [Added Sergio's updated wording about the HW memory barrier. | ||
36 | --Stefan] | ||
37 | |||
38 | Signed-off-by: Sergio Lopez <slp@redhat.com> | ||
39 | Message-id: 20171108063447.2842-1-slp@redhat.com | ||
15 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 40 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
16 | --- | 41 | --- |
17 | iothread.c | 18 ++++++++++-------- | 42 | util/async.c | 2 +- |
18 | 1 file changed, 10 insertions(+), 8 deletions(-) | 43 | 1 file changed, 1 insertion(+), 1 deletion(-) |
19 | 44 | ||
20 | diff --git a/iothread.c b/iothread.c | 45 | diff --git a/util/async.c b/util/async.c |
21 | index XXXXXXX..XXXXXXX 100644 | 46 | index XXXXXXX..XXXXXXX 100644 |
22 | --- a/iothread.c | 47 | --- a/util/async.c |
23 | +++ b/iothread.c | 48 | +++ b/util/async.c |
24 | @@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo aio_max_batch_info = { | 49 | @@ -XXX,XX +XXX,XX @@ void qemu_bh_schedule(QEMUBH *bh) |
25 | }; | 50 | */ |
26 | 51 | void qemu_bh_cancel(QEMUBH *bh) | |
27 | static void iothread_get_param(Object *obj, Visitor *v, | ||
28 | - const char *name, void *opaque, Error **errp) | ||
29 | + const char *name, IOThreadParamInfo *info, Error **errp) | ||
30 | { | 52 | { |
31 | IOThread *iothread = IOTHREAD(obj); | 53 | - bh->scheduled = 0; |
32 | - IOThreadParamInfo *info = opaque; | 54 | + atomic_mb_set(&bh->scheduled, 0); |
33 | int64_t *field = (void *)iothread + info->offset; | ||
34 | |||
35 | visit_type_int64(v, name, field, errp); | ||
36 | } | 55 | } |
37 | 56 | ||
38 | static bool iothread_set_param(Object *obj, Visitor *v, | 57 | /* This func is async.The bottom half will do the delete action at the finial |
39 | - const char *name, void *opaque, Error **errp) | ||
40 | + const char *name, IOThreadParamInfo *info, Error **errp) | ||
41 | { | ||
42 | IOThread *iothread = IOTHREAD(obj); | ||
43 | - IOThreadParamInfo *info = opaque; | ||
44 | int64_t *field = (void *)iothread + info->offset; | ||
45 | int64_t value; | ||
46 | |||
47 | @@ -XXX,XX +XXX,XX @@ static bool iothread_set_param(Object *obj, Visitor *v, | ||
48 | static void iothread_get_poll_param(Object *obj, Visitor *v, | ||
49 | const char *name, void *opaque, Error **errp) | ||
50 | { | ||
51 | + IOThreadParamInfo *info = opaque; | ||
52 | |||
53 | - iothread_get_param(obj, v, name, opaque, errp); | ||
54 | + iothread_get_param(obj, v, name, info, errp); | ||
55 | } | ||
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 | -- | 58 | -- |
90 | 2.31.1 | 59 | 2.13.6 |
91 | 60 | ||
92 | 61 | diff view generated by jsdifflib |