* [Qemu-devel] [PULL v2 for-2.4 v2 1/7] mirror: Speed up bitmap initial scanning
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 2/7] qemu-timer: initialize "timers_done_ev" to set Stefan Hajnoczi
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Fam Zheng, Stefan Hajnoczi
From: Fam Zheng <famz@redhat.com>
Limiting to sectors_per_chunk for each bdrv_is_allocated_above is slow,
because the underlying protocol driver would issue much more queries
than necessary. We should coalesce the query.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: <1436413678-7114-4-git-send-email-famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/mirror.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 323f747..fc4d8f5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -388,7 +388,7 @@ static void coroutine_fn mirror_run(void *opaque)
MirrorBlockJob *s = opaque;
MirrorExitData *data;
BlockDriverState *bs = s->common.bs;
- int64_t sector_num, end, sectors_per_chunk, length;
+ int64_t sector_num, end, length;
uint64_t last_pause_ns;
BlockDriverInfo bdi;
char backing_filename[2]; /* we only need 2 characters because we are only
@@ -442,7 +442,6 @@ static void coroutine_fn mirror_run(void *opaque)
goto immediate_exit;
}
- sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
mirror_free_init(s);
last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
@@ -450,7 +449,9 @@ static void coroutine_fn mirror_run(void *opaque)
/* First part, loop on the sectors and initialize the dirty bitmap. */
BlockDriverState *base = s->base;
for (sector_num = 0; sector_num < end; ) {
- int64_t next = (sector_num | (sectors_per_chunk - 1)) + 1;
+ /* Just to make sure we are not exceeding int limit. */
+ int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
+ end - sector_num);
int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
if (now - last_pause_ns > SLICE_TIME) {
@@ -462,8 +463,7 @@ static void coroutine_fn mirror_run(void *opaque)
goto immediate_exit;
}
- ret = bdrv_is_allocated_above(bs, base,
- sector_num, next - sector_num, &n);
+ ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
if (ret < 0) {
goto immediate_exit;
@@ -472,10 +472,8 @@ static void coroutine_fn mirror_run(void *opaque)
assert(n > 0);
if (ret == 1) {
bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
- sector_num = next;
- } else {
- sector_num += n;
}
+ sector_num += n;
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PULL v2 for-2.4 v2 2/7] qemu-timer: initialize "timers_done_ev" to set
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 1/7] mirror: Speed up bitmap initial scanning Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 3/7] tests: remove irrelevant assertions from test-aio Stefan Hajnoczi
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
The normal value for the event is to be set. If we do not do
this, pause_all_vcpus (through qemu_clock_enable) hangs unless
timerlist_run_timers has been run at least once for the timerlist.
This can happen with the following patches, that make aio_notify do
nothing most of the time.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-2-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qemu-timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-timer.c b/qemu-timer.c
index aa6757e..2463fe6 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -99,7 +99,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
QEMUClock *clock = qemu_clock_ptr(type);
timer_list = g_malloc0(sizeof(QEMUTimerList));
- qemu_event_init(&timer_list->timers_done_ev, false);
+ qemu_event_init(&timer_list->timers_done_ev, true);
timer_list->clock = clock;
timer_list->notify_cb = cb;
timer_list->notify_opaque = opaque;
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PULL v2 for-2.4 v2 3/7] tests: remove irrelevant assertions from test-aio
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 1/7] mirror: Speed up bitmap initial scanning Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 2/7] qemu-timer: initialize "timers_done_ev" to set Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 4/7] aio-win32: reorganize polling loop Stefan Hajnoczi
` (4 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
In these tests, the purpose of the initial calls to aio_poll and
g_main_context_iteration is simply to put the AioContext in a
known state; the return value of the function does not really
matter. The next patch will change those return values; change
the assertions to a while loop which expresses the intention
better.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-3-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/test-aio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/test-aio.c b/tests/test-aio.c
index a7cb5c9..e7bbb83 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -331,7 +331,7 @@ static void test_wait_event_notifier(void)
EventNotifierTestData data = { .n = 0, .active = 1 };
event_notifier_init(&data.e, false);
aio_set_event_notifier(ctx, &data.e, event_ready_cb);
- g_assert(!aio_poll(ctx, false));
+ while (aio_poll(ctx, false));
g_assert_cmpint(data.n, ==, 0);
g_assert_cmpint(data.active, ==, 1);
@@ -356,7 +356,7 @@ static void test_flush_event_notifier(void)
EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true };
event_notifier_init(&data.e, false);
aio_set_event_notifier(ctx, &data.e, event_ready_cb);
- g_assert(!aio_poll(ctx, false));
+ while (aio_poll(ctx, false));
g_assert_cmpint(data.n, ==, 0);
g_assert_cmpint(data.active, ==, 10);
@@ -669,7 +669,7 @@ static void test_source_wait_event_notifier(void)
EventNotifierTestData data = { .n = 0, .active = 1 };
event_notifier_init(&data.e, false);
aio_set_event_notifier(ctx, &data.e, event_ready_cb);
- g_assert(g_main_context_iteration(NULL, false));
+ while (g_main_context_iteration(NULL, false));
g_assert_cmpint(data.n, ==, 0);
g_assert_cmpint(data.active, ==, 1);
@@ -694,7 +694,7 @@ static void test_source_flush_event_notifier(void)
EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true };
event_notifier_init(&data.e, false);
aio_set_event_notifier(ctx, &data.e, event_ready_cb);
- g_assert(g_main_context_iteration(NULL, false));
+ while (g_main_context_iteration(NULL, false));
g_assert_cmpint(data.n, ==, 0);
g_assert_cmpint(data.active, ==, 10);
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PULL v2 for-2.4 v2 4/7] aio-win32: reorganize polling loop
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
` (2 preceding siblings ...)
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 3/7] tests: remove irrelevant assertions from test-aio Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization Stefan Hajnoczi
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
Preparatory bugfixes and tweaks to the loop before the next patch:
- disable dispatch optimization during aio_prepare. This fixes a bug.
- do not modify "blocking" until after the first WaitForMultipleObjects
call. This is needed in the next patch.
- change the loop to do...while. This makes it obvious that the loop
is always entered at least once. In the next patch this is important
because the first iteration undoes the ctx->notify_me increment that
happened before entering the loop.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-4-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
aio-win32.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/aio-win32.c b/aio-win32.c
index 233d8f5..9268b5c 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -284,11 +284,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
int timeout;
aio_context_acquire(ctx);
- have_select_revents = aio_prepare(ctx);
- if (have_select_revents) {
- blocking = false;
- }
-
was_dispatching = ctx->dispatching;
progress = false;
@@ -304,6 +299,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
*/
aio_set_dispatching(ctx, !blocking);
+ have_select_revents = aio_prepare(ctx);
+
ctx->walking_handlers++;
/* fill fd sets */
@@ -317,12 +314,18 @@ bool aio_poll(AioContext *ctx, bool blocking)
ctx->walking_handlers--;
first = true;
- /* wait until next event */
- while (count > 0) {
+ /* ctx->notifier is always registered. */
+ assert(count > 0);
+
+ /* Multiple iterations, all of them non-blocking except the first,
+ * may be necessary to process all pending events. After the first
+ * WaitForMultipleObjects call ctx->notify_me will be decremented.
+ */
+ do {
HANDLE event;
int ret;
- timeout = blocking
+ timeout = blocking && !have_select_revents
? qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)) : 0;
if (timeout) {
aio_context_release(ctx);
@@ -351,7 +354,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
blocking = false;
progress |= aio_dispatch_handlers(ctx, event);
- }
+ } while (count > 0);
progress |= timerlistgroup_run_timers(&ctx->tlg);
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
` (3 preceding siblings ...)
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 4/7] aio-win32: reorganize polling loop Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-23 14:14 ` Cornelia Huck
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 6/7] AioContext: fix broken placement of event_notifier_test_and_clear Stefan Hajnoczi
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
This patch rewrites the ctx->dispatching optimization, which was the cause
of some mysterious hangs that could be reproduced on aarch64 KVM only.
The hangs were indirectly caused by aio_poll() and in particular by
flash memory updates's call to blk_write(), which invokes aio_poll().
Fun stuff: they had an extremely short race window, so much that
adding all kind of tracing to either the kernel or QEMU made it
go away (a single printf made it half as reproducible).
On the plus side, the failure mode (a hang until the next keypress)
made it very easy to examine the state of the process with a debugger.
And there was a very nice reproducer from Laszlo, which failed pretty
often (more than half of the time) on any version of QEMU with a non-debug
kernel; it also failed fast, while still in the firmware. So, it could
have been worse.
For some unknown reason they happened only with virtio-scsi, but
that's not important. It's more interesting that they disappeared with
io=native, making thread-pool.c a likely suspect for where the bug arose.
thread-pool.c is also one of the few places which use bottom halves
across threads, by the way.
I hope that no other similar bugs exist, but just in case :) I am
going to describe how the successful debugging went... Since the
likely culprit was the ctx->dispatching optimization, which mostly
affects bottom halves, the first observation was that there are two
qemu_bh_schedule() invocations in the thread pool: the one in the aio
worker and the one in thread_pool_completion_bh. The latter always
causes the optimization to trigger, the former may or may not. In
order to restrict the possibilities, I introduced new functions
qemu_bh_schedule_slow() and qemu_bh_schedule_fast():
/* qemu_bh_schedule_slow: */
ctx = bh->ctx;
bh->idle = 0;
if (atomic_xchg(&bh->scheduled, 1) == 0) {
event_notifier_set(&ctx->notifier);
}
/* qemu_bh_schedule_fast: */
ctx = bh->ctx;
bh->idle = 0;
assert(ctx->dispatching);
atomic_xchg(&bh->scheduled, 1);
Notice how the atomic_xchg is still in qemu_bh_schedule_slow(). This
was already debated a few months ago, so I assumed it to be correct.
In retrospect this was a very good idea, as you'll see later.
Changing thread_pool_completion_bh() to qemu_bh_schedule_fast() didn't
trigger the assertion (as expected). Changing the worker's invocation
to qemu_bh_schedule_slow() didn't hide the bug (another assumption
which luckily held). This already limited heavily the amount of
interaction between the threads, hinting that the problematic events
must have triggered around thread_pool_completion_bh().
As mentioned early, invoking a debugger to examine the state of a
hung process was pretty easy; the iothread was always waiting on a
poll(..., -1) system call. Infinite timeouts are much rarer on x86,
and this could be the reason why the bug was never observed there.
With the buggy sequence more or less resolved to an interaction between
thread_pool_completion_bh() and poll(..., -1), my "tracing" strategy was
to just add a few qemu_clock_get_ns(QEMU_CLOCK_REALTIME) calls, hoping
that the ordering of aio_ctx_prepare(), aio_ctx_dispatch, poll() and
qemu_bh_schedule_fast() would provide some hint. The output was:
(gdb) p last_prepare
$3 = 103885451
(gdb) p last_dispatch
$4 = 103876492
(gdb) p last_poll
$5 = 115909333
(gdb) p last_schedule
$6 = 115925212
Notice how the last call to qemu_poll_ns() came after aio_ctx_dispatch().
This makes little sense unless there is an aio_poll() call involved,
and indeed with a slightly different instrumentation you can see that
there is one:
(gdb) p last_prepare
$3 = 107569679
(gdb) p last_dispatch
$4 = 107561600
(gdb) p last_aio_poll
$5 = 110671400
(gdb) p last_schedule
$6 = 110698917
So the scenario becomes clearer:
iothread VCPU thread
--------------------------------------------------------------------------
aio_ctx_prepare
aio_ctx_check
qemu_poll_ns(timeout=-1)
aio_poll
aio_dispatch
thread_pool_completion_bh
qemu_bh_schedule()
At this point bh->scheduled = 1 and the iothread has not been woken up.
The solution must be close, but this alone should not be a problem,
because the bottom half is only rescheduled to account for rare situations
(see commit 3c80ca1, thread-pool: avoid deadlock in nested aio_poll()
calls, 2014-07-15).
Introducing a third thread---a thread pool worker thread, which
also does qemu_bh_schedule()---does bring out the problematic case.
The third thread must be awakened *after* the callback is complete and
thread_pool_completion_bh has redone the whole loop, explaining the
short race window. And then this is what happens:
thread pool worker
--------------------------------------------------------------------------
<I/O completes>
qemu_bh_schedule()
Tada, bh->scheduled is already 1, so qemu_bh_schedule() does nothing
and the iothread is never woken up. This is where the bh->scheduled
optimization comes into play---it is correct, but removing it would
have masked the bug.
So, what is the bug?
Well, the question asked by the ctx->dispatching optimization ("is any
active aio_poll dispatching?") was wrong. The right question to ask
instead is "is any active aio_poll *not* dispatching", i.e. in the prepare
or poll phases? In that case, the aio_poll is sleeping or might go to
sleep anytime soon, and the EventNotifier must be invoked to wake
it up.
In any other case (including if there is *no* active aio_poll at all!)
we can just wait for the next prepare phase to pick up the event (e.g. a
bottom half); the prepare phase will avoid the blocking and service the
bottom half.
Expressing the invariant with a logic formula, the broken one looked like:
!(exists(thread): in_dispatching(thread)) => !optimize
or equivalently:
!(exists(thread):
in_aio_poll(thread) && in_dispatching(thread)) => !optimize
In the correct one, the negation is in a slightly different place:
(exists(thread):
in_aio_poll(thread) && !in_dispatching(thread)) => !optimize
or equivalently:
(exists(thread): in_prepare_or_poll(thread)) => !optimize
Even if the difference boils down to moving an exclamation mark :)
the implementation is quite different. However, I think the new
one is simpler to understand.
In the old implementation, the "exists" was implemented with a boolean
value. This didn't really support well the case of multiple concurrent
event loops, but I thought that this was okay: aio_poll holds the
AioContext lock so there cannot be concurrent aio_poll invocations, and
I was just considering nested event loops. However, aio_poll _could_
indeed be concurrent with the GSource. This is why I came up with the
wrong invariant.
In the new implementation, "exists" is computed simply by counting how many
threads are in the prepare or poll phases. There are some interesting
points to consider, but the gist of the idea remains:
1) AioContext can be used through GSource as well; as mentioned in the
patch, bit 0 of the counter is reserved for the GSource.
2) the counter need not be updated for a non-blocking aio_poll, because
it won't sleep forever anyway. This is just a matter of checking
the "blocking" variable. This requires some changes to the win32
implementation, but is otherwise not too complicated.
3) as mentioned above, the new implementation will not call aio_notify
when there is *no* active aio_poll at all. The tests have to be
adjusted for this change. The calls to aio_notify in async.c are fine;
they only want to kick aio_poll out of a blocking wait, but need not
do anything if aio_poll is not running.
4) nested aio_poll: these just work with the new implementation; when
a nested event loop is invoked, the outer event loop is never in the
prepare or poll phases. The outer event loop thus has already decremented
the counter.
Reported-by: Richard W. M. Jones <rjones@redhat.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-5-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
aio-posix.c | 18 +++++-------
aio-win32.c | 20 ++++++-------
async.c | 21 +++++---------
docs/aio_notify.promela | 77 +++++++++++++++++++++----------------------------
include/block/aio.h | 29 +++++++++++++++----
tests/test-aio.c | 18 ------------
6 files changed, 81 insertions(+), 102 deletions(-)
diff --git a/aio-posix.c b/aio-posix.c
index 4abec38..249889f 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -233,26 +233,23 @@ static void add_pollfd(AioHandler *node)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
- bool was_dispatching;
int i, ret;
bool progress;
int64_t timeout;
aio_context_acquire(ctx);
- was_dispatching = ctx->dispatching;
progress = false;
/* aio_notify can avoid the expensive event_notifier_set if
* everything (file descriptors, bottom halves, timers) will
* be re-evaluated before the next blocking poll(). This is
* already true when aio_poll is called with blocking == false;
- * if blocking == true, it is only true after poll() returns.
- *
- * If we're in a nested event loop, ctx->dispatching might be true.
- * In that case we can restore it just before returning, but we
- * have to clear it now.
+ * if blocking == true, it is only true after poll() returns,
+ * so disable the optimization now.
*/
- aio_set_dispatching(ctx, !blocking);
+ if (blocking) {
+ atomic_add(&ctx->notify_me, 2);
+ }
ctx->walking_handlers++;
@@ -272,6 +269,9 @@ bool aio_poll(AioContext *ctx, bool blocking)
aio_context_release(ctx);
}
ret = qemu_poll_ns((GPollFD *)pollfds, npfd, timeout);
+ if (blocking) {
+ atomic_sub(&ctx->notify_me, 2);
+ }
if (timeout) {
aio_context_acquire(ctx);
}
@@ -287,12 +287,10 @@ bool aio_poll(AioContext *ctx, bool blocking)
ctx->walking_handlers--;
/* Run dispatch even if there were no readable fds to run timers */
- aio_set_dispatching(ctx, true);
if (aio_dispatch(ctx)) {
progress = true;
}
- aio_set_dispatching(ctx, was_dispatching);
aio_context_release(ctx);
return progress;
diff --git a/aio-win32.c b/aio-win32.c
index 9268b5c..ea655b0 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -279,25 +279,23 @@ bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
- bool was_dispatching, progress, have_select_revents, first;
+ bool progress, have_select_revents, first;
int count;
int timeout;
aio_context_acquire(ctx);
- was_dispatching = ctx->dispatching;
progress = false;
/* aio_notify can avoid the expensive event_notifier_set if
* everything (file descriptors, bottom halves, timers) will
* be re-evaluated before the next blocking poll(). This is
* already true when aio_poll is called with blocking == false;
- * if blocking == true, it is only true after poll() returns.
- *
- * If we're in a nested event loop, ctx->dispatching might be true.
- * In that case we can restore it just before returning, but we
- * have to clear it now.
+ * if blocking == true, it is only true after poll() returns,
+ * so disable the optimization now.
*/
- aio_set_dispatching(ctx, !blocking);
+ if (blocking) {
+ atomic_add(&ctx->notify_me, 2);
+ }
have_select_revents = aio_prepare(ctx);
@@ -331,10 +329,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
aio_context_release(ctx);
}
ret = WaitForMultipleObjects(count, events, FALSE, timeout);
+ if (blocking) {
+ assert(first);
+ atomic_sub(&ctx->notify_me, 2);
+ }
if (timeout) {
aio_context_acquire(ctx);
}
- aio_set_dispatching(ctx, true);
if (first && aio_bh_poll(ctx)) {
progress = true;
@@ -358,7 +359,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
progress |= timerlistgroup_run_timers(&ctx->tlg);
- aio_set_dispatching(ctx, was_dispatching);
aio_context_release(ctx);
return progress;
}
diff --git a/async.c b/async.c
index 77d080d..a232192 100644
--- a/async.c
+++ b/async.c
@@ -184,6 +184,8 @@ aio_ctx_prepare(GSource *source, gint *timeout)
{
AioContext *ctx = (AioContext *) source;
+ atomic_or(&ctx->notify_me, 1);
+
/* We assume there is no timeout already supplied */
*timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx));
@@ -200,6 +202,7 @@ aio_ctx_check(GSource *source)
AioContext *ctx = (AioContext *) source;
QEMUBH *bh;
+ atomic_and(&ctx->notify_me, ~1);
for (bh = ctx->first_bh; bh; bh = bh->next) {
if (!bh->deleted && bh->scheduled) {
return true;
@@ -254,23 +257,13 @@ ThreadPool *aio_get_thread_pool(AioContext *ctx)
return ctx->thread_pool;
}
-void aio_set_dispatching(AioContext *ctx, bool dispatching)
-{
- ctx->dispatching = dispatching;
- if (!dispatching) {
- /* Write ctx->dispatching before reading e.g. bh->scheduled.
- * Optimization: this is only needed when we're entering the "unsafe"
- * phase where other threads must call event_notifier_set.
- */
- smp_mb();
- }
-}
-
void aio_notify(AioContext *ctx)
{
- /* Write e.g. bh->scheduled before reading ctx->dispatching. */
+ /* Write e.g. bh->scheduled before reading ctx->notify_me. Pairs
+ * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
+ */
smp_mb();
- if (!ctx->dispatching) {
+ if (ctx->notify_me) {
event_notifier_set(&ctx->notifier);
}
}
diff --git a/docs/aio_notify.promela b/docs/aio_notify.promela
index ad3f6f0..fccc7ee 100644
--- a/docs/aio_notify.promela
+++ b/docs/aio_notify.promela
@@ -1,5 +1,5 @@
/*
- * This model describes the interaction between aio_set_dispatching()
+ * This model describes the interaction between ctx->notify_me
* and aio_notify().
*
* Author: Paolo Bonzini <pbonzini@redhat.com>
@@ -14,57 +14,53 @@
* spin -a docs/aio_notify.promela
* gcc -O2 pan.c
* ./a.out -a
+ *
+ * To verify it (with a bug planted in the model):
+ * spin -a -DBUG docs/aio_notify.promela
+ * gcc -O2 pan.c
+ * ./a.out -a
*/
#define MAX 4
#define LAST (1 << (MAX - 1))
#define FINAL ((LAST << 1) - 1)
-bool dispatching;
+bool notify_me;
bool event;
-int req, done;
+int req;
+int done;
active proctype waiter()
{
- int fetch, blocking;
+ int fetch;
- do
- :: done != FINAL -> {
- // Computing "blocking" is separate from execution of the
- // "bottom half"
- blocking = (req == 0);
+ do
+ :: true -> {
+ notify_me++;
+
+ if
+#ifndef BUG
+ :: (req > 0) -> skip;
+#endif
+ :: else ->
+ // Wait for a nudge from the other side
+ do
+ :: event == 1 -> { event = 0; break; }
+ od;
+ fi;
+
+ notify_me--;
- // This is our "bottom half"
atomic { fetch = req; req = 0; }
done = done | fetch;
-
- // Wait for a nudge from the other side
- do
- :: event == 1 -> { event = 0; break; }
- :: !blocking -> break;
- od;
-
- dispatching = 1;
-
- // If you are simulating this model, you may want to add
- // something like this here:
- //
- // int foo; foo++; foo++; foo++;
- //
- // This only wastes some time and makes it more likely
- // that the notifier process hits the "fast path".
-
- dispatching = 0;
}
- :: else -> break;
od
}
active proctype notifier()
{
int next = 1;
- int sets = 0;
do
:: next <= LAST -> {
@@ -74,8 +70,8 @@ active proctype notifier()
// aio_notify
if
- :: dispatching == 0 -> sets++; event = 1;
- :: else -> skip;
+ :: notify_me == 1 -> event = 1;
+ :: else -> printf("Skipped event_notifier_set\n"); skip;
fi;
// Test both synchronous and asynchronous delivery
@@ -86,19 +82,12 @@ active proctype notifier()
:: 1 -> skip;
fi;
}
- :: else -> break;
od;
- printf("Skipped %d event_notifier_set\n", MAX - sets);
}
-#define p (done == FINAL)
-
-never {
- do
- :: 1 // after an arbitrarily long prefix
- :: p -> break // p becomes true
- od;
- do
- :: !p -> accept: break // it then must remains true forever after
- od
+never { /* [] done < FINAL */
+accept_init:
+ do
+ :: done < FINAL -> skip;
+ od;
}
diff --git a/include/block/aio.h b/include/block/aio.h
index b46103e..be91e3f 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -63,10 +63,30 @@ struct AioContext {
*/
int walking_handlers;
- /* Used to avoid unnecessary event_notifier_set calls in aio_notify.
- * Writes protected by lock or BQL, reads are lockless.
+ /* Used to avoid unnecessary event_notifier_set calls in aio_notify;
+ * accessed with atomic primitives. If this field is 0, everything
+ * (file descriptors, bottom halves, timers) will be re-evaluated
+ * before the next blocking poll(), thus the event_notifier_set call
+ * can be skipped. If it is non-zero, you may need to wake up a
+ * concurrent aio_poll or the glib main event loop, making
+ * event_notifier_set necessary.
+ *
+ * Bit 0 is reserved for GSource usage of the AioContext, and is 1
+ * between a call to aio_ctx_check and the next call to aio_ctx_dispatch.
+ * Bits 1-31 simply count the number of active calls to aio_poll
+ * that are in the prepare or poll phase.
+ *
+ * The GSource and aio_poll must use a different mechanism because
+ * there is no certainty that a call to GSource's prepare callback
+ * (via g_main_context_prepare) is indeed followed by check and
+ * dispatch. It's not clear whether this would be a bug, but let's
+ * play safe and allow it---it will just cause extra calls to
+ * event_notifier_set until the next call to dispatch.
+ *
+ * Instead, the aio_poll calls include both the prepare and the
+ * dispatch phase, hence a simple counter is enough for them.
*/
- bool dispatching;
+ uint32_t notify_me;
/* lock to protect between bh's adders and deleter */
QemuMutex bh_lock;
@@ -89,9 +109,6 @@ struct AioContext {
QEMUTimerListGroup tlg;
};
-/* Used internally to synchronize aio_poll against qemu_bh_schedule. */
-void aio_set_dispatching(AioContext *ctx, bool dispatching);
-
/**
* aio_context_new: Allocate a new AioContext.
*
diff --git a/tests/test-aio.c b/tests/test-aio.c
index e7bbb83..217e337 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -97,14 +97,6 @@ static void event_ready_cb(EventNotifier *e)
/* Tests using aio_*. */
-static void test_notify(void)
-{
- g_assert(!aio_poll(ctx, false));
- aio_notify(ctx);
- g_assert(!aio_poll(ctx, true));
- g_assert(!aio_poll(ctx, false));
-}
-
typedef struct {
QemuMutex start_lock;
bool thread_acquired;
@@ -494,14 +486,6 @@ static void test_timer_schedule(void)
* works well, and that's what I am using.
*/
-static void test_source_notify(void)
-{
- while (g_main_context_iteration(NULL, false));
- aio_notify(ctx);
- g_assert(g_main_context_iteration(NULL, true));
- g_assert(!g_main_context_iteration(NULL, false));
-}
-
static void test_source_flush(void)
{
g_assert(!g_main_context_iteration(NULL, false));
@@ -830,7 +814,6 @@ int main(int argc, char **argv)
while (g_main_context_iteration(NULL, false));
g_test_init(&argc, &argv, NULL);
- g_test_add_func("/aio/notify", test_notify);
g_test_add_func("/aio/acquire", test_acquire);
g_test_add_func("/aio/bh/schedule", test_bh_schedule);
g_test_add_func("/aio/bh/schedule10", test_bh_schedule10);
@@ -845,7 +828,6 @@ int main(int argc, char **argv)
g_test_add_func("/aio/event/flush", test_flush_event_notifier);
g_test_add_func("/aio/timer/schedule", test_timer_schedule);
- g_test_add_func("/aio-gsource/notify", test_source_notify);
g_test_add_func("/aio-gsource/flush", test_source_flush);
g_test_add_func("/aio-gsource/bh/schedule", test_source_bh_schedule);
g_test_add_func("/aio-gsource/bh/schedule10", test_source_bh_schedule10);
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization Stefan Hajnoczi
@ 2015-07-23 14:14 ` Cornelia Huck
2015-07-23 17:20 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2015-07-23 14:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi,
Christian Borntraeger
On Wed, 22 Jul 2015 12:43:43 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> This patch rewrites the ctx->dispatching optimization, which was the cause
> of some mysterious hangs that could be reproduced on aarch64 KVM only.
> The hangs were indirectly caused by aio_poll() and in particular by
> flash memory updates's call to blk_write(), which invokes aio_poll().
> Fun stuff: they had an extremely short race window, so much that
> adding all kind of tracing to either the kernel or QEMU made it
> go away (a single printf made it half as reproducible).
Sadly, this commit seems to *introduce* hangs on my s390x setup - i.e.
the guest does not react at all.
Thread 1 seems to be stuck in dataplane setup:
(gdb) bt
#0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
from /lib64/libpthread.so.0
#1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
mutex=mutex@entry=0x9717d920)
at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
#2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
at /data/git/yyy/qemu/util/rfifolock.c:59
#3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
at /data/git/yyy/qemu/async.c:331
#4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
#5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
#6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
#7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
at /data/git/yyy/qemu/main-loop.c:494
#8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
#9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
at /data/git/yyy/qemu/vl.c:4653
I've stripped down the setup to the following commandline:
/data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp 4,sockets=4,cores=1,threads=1 -nographic -drive file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native -device virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
Instant hang for me :(
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 14:14 ` Cornelia Huck
@ 2015-07-23 17:20 ` Paolo Bonzini
2015-07-23 18:19 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-23 17:20 UTC (permalink / raw)
To: Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi,
Christian Borntraeger
On 23/07/2015 16:14, Cornelia Huck wrote:
> (gdb) bt
> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
> from /lib64/libpthread.so.0
> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
> mutex=mutex@entry=0x9717d920)
> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
> at /data/git/yyy/qemu/util/rfifolock.c:59
> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
> at /data/git/yyy/qemu/async.c:331
> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
> at /data/git/yyy/qemu/main-loop.c:494
> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
> at /data/git/yyy/qemu/vl.c:4653
>
> I've stripped down the setup to the following commandline:
>
> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
> 4,sockets=4,cores=1,threads=1 -nographic -drive
> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
> -device
> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
What's the backtrace like for the other threads? This is almost
definitely a latent bug somewhere else.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 17:20 ` Paolo Bonzini
@ 2015-07-23 18:19 ` Paolo Bonzini
2015-07-23 19:19 ` Christian Borntraeger
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-23 18:19 UTC (permalink / raw)
To: Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi,
Christian Borntraeger
On 23/07/2015 19:20, Paolo Bonzini wrote:
>
>
> On 23/07/2015 16:14, Cornelia Huck wrote:
>> (gdb) bt
>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
>> from /lib64/libpthread.so.0
>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
>> mutex=mutex@entry=0x9717d920)
>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
>> at /data/git/yyy/qemu/util/rfifolock.c:59
>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
>> at /data/git/yyy/qemu/async.c:331
>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
>> at /data/git/yyy/qemu/main-loop.c:494
>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>> at /data/git/yyy/qemu/vl.c:4653
>>
>> I've stripped down the setup to the following commandline:
>>
>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
>> 4,sockets=4,cores=1,threads=1 -nographic -drive
>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
>> -device
>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
>
> What's the backtrace like for the other threads? This is almost
> definitely a latent bug somewhere else.
BTW, I can reproduce this---I'm asking because I cannot even attach gdb
to the hung process.
The simplest workaround is to reintroduce commit a0710f7995 (iothread:
release iothread around aio_poll, 2015-02-20), though it also comes with
some risk. It avoids the bug because it limits the contention on the
RFifoLock.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 18:19 ` Paolo Bonzini
@ 2015-07-23 19:19 ` Christian Borntraeger
2015-07-23 19:25 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2015-07-23 19:19 UTC (permalink / raw)
To: Paolo Bonzini, Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi
Am 23.07.2015 um 20:19 schrieb Paolo Bonzini:
>
>
> On 23/07/2015 19:20, Paolo Bonzini wrote:
>>
>>
>> On 23/07/2015 16:14, Cornelia Huck wrote:
>>> (gdb) bt
>>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
>>> from /lib64/libpthread.so.0
>>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
>>> mutex=mutex@entry=0x9717d920)
>>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
>>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
>>> at /data/git/yyy/qemu/util/rfifolock.c:59
>>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
>>> at /data/git/yyy/qemu/async.c:331
>>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
>>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
>>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
>>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
>>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
>>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
>>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
>>> at /data/git/yyy/qemu/main-loop.c:494
>>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
>>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>>> at /data/git/yyy/qemu/vl.c:4653
>>>
>>> I've stripped down the setup to the following commandline:
>>>
>>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
>>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
>>> 4,sockets=4,cores=1,threads=1 -nographic -drive
>>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
>>> -device
>>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
>>
>> What's the backtrace like for the other threads? This is almost
>> definitely a latent bug somewhere else.
>
> BTW, I can reproduce this---I'm asking because I cannot even attach gdb
> to the hung process.
>
> The simplest workaround is to reintroduce commit a0710f7995 (iothread:
> release iothread around aio_poll, 2015-02-20), though it also comes with
> some risk. It avoids the bug because it limits the contention on the
> RFifoLock.
>
> Paolo
>
I can reproduce this with the following backtrace (with --enable-debug info added qemu is the tag v2.4.0-rc2)
Thread 4 (Thread 0x3fffb4ce910 (LWP 57750)):
#0 0x000003fffc185e8e in syscall () from /lib64/libc.so.6
#1 0x00000000803578ee in futex_wait (ev=0x8098486c <rcu_call_ready_event>, val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:301
#2 0x0000000080357ae8 in qemu_event_wait (ev=0x8098486c <rcu_call_ready_event>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
#3 0x000000008036e202 in call_rcu_thread (opaque=0x0) at /home/cborntra/REPOS/qemu/util/rcu.c:233
#4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
#5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
Thread 3 (Thread 0x3fffacce910 (LWP 57751)):
#0 0x000003fffc17f5d6 in ppoll () from /lib64/libc.so.6
#1 0x0000000080294c2e in qemu_poll_ns (fds=0x3fff40008c0, nfds=1, timeout=-1) at /home/cborntra/REPOS/qemu/qemu-timer.c:310
#2 0x0000000080296788 in aio_poll (ctx=0x809c2830, blocking=true) at /home/cborntra/REPOS/qemu/aio-posix.c:271
#3 0x0000000080137f58 in iothread_run (opaque=0x809c2450) at /home/cborntra/REPOS/qemu/iothread.c:42
#4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
#5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
Thread 2 (Thread 0x3fff9411910 (LWP 57754)):
#0 0x000003fffc23e662 in __lll_lock_wait () from /lib64/libpthread.so.0
#1 0x000003fffc2394a4 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2 0x000000008035706e in qemu_mutex_lock (mutex=0x80559288 <qemu_global_mutex>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:73
#3 0x000000008005ae46 in qemu_mutex_lock_iothread () at /home/cborntra/REPOS/qemu/cpus.c:1164
#4 0x000000008012f44e in kvm_arch_handle_exit (cs=0x82141930, run=0x3fffd31a000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2010
#5 0x00000000800782f8 in kvm_cpu_exec (cpu=0x82141930) at /home/cborntra/REPOS/qemu/kvm-all.c:1901
#6 0x000000008005a73c in qemu_kvm_cpu_thread_fn (arg=0x82141930) at /home/cborntra/REPOS/qemu/cpus.c:977
#7 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
#8 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
Thread 1 (Thread 0x3fffb4d0bd0 (LWP 57736)):
#0 0x000003fffc23b57c in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
#1 0x00000000803572fa in qemu_cond_wait (cond=0x809c28c0, mutex=0x809c2890) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:132
#2 0x000000008036dc3e in rfifolock_lock (r=0x809c2890) at /home/cborntra/REPOS/qemu/util/rfifolock.c:59
#3 0x0000000080281162 in aio_context_acquire (ctx=0x809c2830) at /home/cborntra/REPOS/qemu/async.c:331
#4 0x00000000800a2f08 in virtio_blk_data_plane_start (s=0x80a0d6f0) at /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:285
#5 0x00000000800a0bfe in virtio_blk_handle_output (vdev=0x809b5e18, vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:599
#6 0x00000000800d065c in virtio_queue_notify_vq (vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:921
#7 0x00000000800d29b4 in virtio_queue_host_notifier_read (n=0x80a70988) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1480
#8 0x0000000080293b76 in qemu_iohandler_poll (pollfds=0x809b3600, ret=1) at /home/cborntra/REPOS/qemu/iohandler.c:126
#9 0x0000000080293714 in main_loop_wait (nonblocking=0) at /home/cborntra/REPOS/qemu/main-loop.c:494
#10 0x000000008014dc1c in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1902
#11 0x000000008015627c in main (argc=44, argv=0x3ffffcd66e8, envp=0x3ffffcd6850) at /home/cborntra/REPOS/qemu/vl.c:4653
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 19:19 ` Christian Borntraeger
@ 2015-07-23 19:25 ` Paolo Bonzini
2015-07-23 19:29 ` Christian Borntraeger
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-23 19:25 UTC (permalink / raw)
To: Christian Borntraeger, Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi
On 23/07/2015 21:19, Christian Borntraeger wrote:
> Am 23.07.2015 um 20:19 schrieb Paolo Bonzini:
>>
>>
>> On 23/07/2015 19:20, Paolo Bonzini wrote:
>>>
>>>
>>> On 23/07/2015 16:14, Cornelia Huck wrote:
>>>> (gdb) bt
>>>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
>>>> from /lib64/libpthread.so.0
>>>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
>>>> mutex=mutex@entry=0x9717d920)
>>>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
>>>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
>>>> at /data/git/yyy/qemu/util/rfifolock.c:59
>>>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
>>>> at /data/git/yyy/qemu/async.c:331
>>>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
>>>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
>>>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
>>>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
>>>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
>>>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
>>>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
>>>> at /data/git/yyy/qemu/main-loop.c:494
>>>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
>>>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>>>> at /data/git/yyy/qemu/vl.c:4653
>>>>
>>>> I've stripped down the setup to the following commandline:
>>>>
>>>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
>>>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
>>>> 4,sockets=4,cores=1,threads=1 -nographic -drive
>>>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
>>>> -device
>>>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
>>>
>>> What's the backtrace like for the other threads? This is almost
>>> definitely a latent bug somewhere else.
>>
>> BTW, I can reproduce this---I'm asking because I cannot even attach gdb
>> to the hung process.
>>
>> The simplest workaround is to reintroduce commit a0710f7995 (iothread:
>> release iothread around aio_poll, 2015-02-20), though it also comes with
>> some risk. It avoids the bug because it limits the contention on the
>> RFifoLock.
>
> I can reproduce this with the following backtrace (with --enable-debug info added qemu is the tag v2.4.0-rc2)
Can you check that cherry-picking a0710f7995 works for you?
Paolo
> Thread 4 (Thread 0x3fffb4ce910 (LWP 57750)):
> #0 0x000003fffc185e8e in syscall () from /lib64/libc.so.6
> #1 0x00000000803578ee in futex_wait (ev=0x8098486c <rcu_call_ready_event>, val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:301
> #2 0x0000000080357ae8 in qemu_event_wait (ev=0x8098486c <rcu_call_ready_event>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
> #3 0x000000008036e202 in call_rcu_thread (opaque=0x0) at /home/cborntra/REPOS/qemu/util/rcu.c:233
> #4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
> #5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>
> Thread 3 (Thread 0x3fffacce910 (LWP 57751)):
> #0 0x000003fffc17f5d6 in ppoll () from /lib64/libc.so.6
> #1 0x0000000080294c2e in qemu_poll_ns (fds=0x3fff40008c0, nfds=1, timeout=-1) at /home/cborntra/REPOS/qemu/qemu-timer.c:310
> #2 0x0000000080296788 in aio_poll (ctx=0x809c2830, blocking=true) at /home/cborntra/REPOS/qemu/aio-posix.c:271
> #3 0x0000000080137f58 in iothread_run (opaque=0x809c2450) at /home/cborntra/REPOS/qemu/iothread.c:42
> #4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
> #5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>
> Thread 2 (Thread 0x3fff9411910 (LWP 57754)):
> #0 0x000003fffc23e662 in __lll_lock_wait () from /lib64/libpthread.so.0
> #1 0x000003fffc2394a4 in pthread_mutex_lock () from /lib64/libpthread.so.0
> #2 0x000000008035706e in qemu_mutex_lock (mutex=0x80559288 <qemu_global_mutex>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:73
> #3 0x000000008005ae46 in qemu_mutex_lock_iothread () at /home/cborntra/REPOS/qemu/cpus.c:1164
> #4 0x000000008012f44e in kvm_arch_handle_exit (cs=0x82141930, run=0x3fffd31a000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2010
> #5 0x00000000800782f8 in kvm_cpu_exec (cpu=0x82141930) at /home/cborntra/REPOS/qemu/kvm-all.c:1901
> #6 0x000000008005a73c in qemu_kvm_cpu_thread_fn (arg=0x82141930) at /home/cborntra/REPOS/qemu/cpus.c:977
> #7 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
> #8 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>
> Thread 1 (Thread 0x3fffb4d0bd0 (LWP 57736)):
> #0 0x000003fffc23b57c in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
> #1 0x00000000803572fa in qemu_cond_wait (cond=0x809c28c0, mutex=0x809c2890) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:132
> #2 0x000000008036dc3e in rfifolock_lock (r=0x809c2890) at /home/cborntra/REPOS/qemu/util/rfifolock.c:59
> #3 0x0000000080281162 in aio_context_acquire (ctx=0x809c2830) at /home/cborntra/REPOS/qemu/async.c:331
> #4 0x00000000800a2f08 in virtio_blk_data_plane_start (s=0x80a0d6f0) at /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:285
> #5 0x00000000800a0bfe in virtio_blk_handle_output (vdev=0x809b5e18, vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:599
> #6 0x00000000800d065c in virtio_queue_notify_vq (vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:921
> #7 0x00000000800d29b4 in virtio_queue_host_notifier_read (n=0x80a70988) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1480
> #8 0x0000000080293b76 in qemu_iohandler_poll (pollfds=0x809b3600, ret=1) at /home/cborntra/REPOS/qemu/iohandler.c:126
> #9 0x0000000080293714 in main_loop_wait (nonblocking=0) at /home/cborntra/REPOS/qemu/main-loop.c:494
> #10 0x000000008014dc1c in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1902
> #11 0x000000008015627c in main (argc=44, argv=0x3ffffcd66e8, envp=0x3ffffcd6850) at /home/cborntra/REPOS/qemu/vl.c:4653
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 19:25 ` Paolo Bonzini
@ 2015-07-23 19:29 ` Christian Borntraeger
2015-07-24 6:46 ` Cornelia Huck
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2015-07-23 19:29 UTC (permalink / raw)
To: Paolo Bonzini, Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Stefan Hajnoczi
Am 23.07.2015 um 21:25 schrieb Paolo Bonzini:
>
>
> On 23/07/2015 21:19, Christian Borntraeger wrote:
>> Am 23.07.2015 um 20:19 schrieb Paolo Bonzini:
>>>
>>>
>>> On 23/07/2015 19:20, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 23/07/2015 16:14, Cornelia Huck wrote:
>>>>> (gdb) bt
>>>>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
>>>>> from /lib64/libpthread.so.0
>>>>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
>>>>> mutex=mutex@entry=0x9717d920)
>>>>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
>>>>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
>>>>> at /data/git/yyy/qemu/util/rfifolock.c:59
>>>>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
>>>>> at /data/git/yyy/qemu/async.c:331
>>>>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
>>>>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
>>>>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
>>>>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
>>>>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
>>>>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
>>>>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
>>>>> at /data/git/yyy/qemu/main-loop.c:494
>>>>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
>>>>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>>>>> at /data/git/yyy/qemu/vl.c:4653
>>>>>
>>>>> I've stripped down the setup to the following commandline:
>>>>>
>>>>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
>>>>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
>>>>> 4,sockets=4,cores=1,threads=1 -nographic -drive
>>>>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
>>>>> -device
>>>>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
>>>>
>>>> What's the backtrace like for the other threads? This is almost
>>>> definitely a latent bug somewhere else.
>>>
>>> BTW, I can reproduce this---I'm asking because I cannot even attach gdb
>>> to the hung process.
>>>
>>> The simplest workaround is to reintroduce commit a0710f7995 (iothread:
>>> release iothread around aio_poll, 2015-02-20), though it also comes with
>>> some risk. It avoids the bug because it limits the contention on the
>>> RFifoLock.
>>
>> I can reproduce this with the following backtrace (with --enable-debug info added qemu is the tag v2.4.0-rc2)
>
> Can you check that cherry-picking a0710f7995 works for you?
>
Yes, this seems to help for me. Conny, can you verify?
> Paolo
>
>> Thread 4 (Thread 0x3fffb4ce910 (LWP 57750)):
>> #0 0x000003fffc185e8e in syscall () from /lib64/libc.so.6
>> #1 0x00000000803578ee in futex_wait (ev=0x8098486c <rcu_call_ready_event>, val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:301
>> #2 0x0000000080357ae8 in qemu_event_wait (ev=0x8098486c <rcu_call_ready_event>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
>> #3 0x000000008036e202 in call_rcu_thread (opaque=0x0) at /home/cborntra/REPOS/qemu/util/rcu.c:233
>> #4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
>> #5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>>
>> Thread 3 (Thread 0x3fffacce910 (LWP 57751)):
>> #0 0x000003fffc17f5d6 in ppoll () from /lib64/libc.so.6
>> #1 0x0000000080294c2e in qemu_poll_ns (fds=0x3fff40008c0, nfds=1, timeout=-1) at /home/cborntra/REPOS/qemu/qemu-timer.c:310
>> #2 0x0000000080296788 in aio_poll (ctx=0x809c2830, blocking=true) at /home/cborntra/REPOS/qemu/aio-posix.c:271
>> #3 0x0000000080137f58 in iothread_run (opaque=0x809c2450) at /home/cborntra/REPOS/qemu/iothread.c:42
>> #4 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
>> #5 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>>
>> Thread 2 (Thread 0x3fff9411910 (LWP 57754)):
>> #0 0x000003fffc23e662 in __lll_lock_wait () from /lib64/libpthread.so.0
>> #1 0x000003fffc2394a4 in pthread_mutex_lock () from /lib64/libpthread.so.0
>> #2 0x000000008035706e in qemu_mutex_lock (mutex=0x80559288 <qemu_global_mutex>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:73
>> #3 0x000000008005ae46 in qemu_mutex_lock_iothread () at /home/cborntra/REPOS/qemu/cpus.c:1164
>> #4 0x000000008012f44e in kvm_arch_handle_exit (cs=0x82141930, run=0x3fffd31a000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2010
>> #5 0x00000000800782f8 in kvm_cpu_exec (cpu=0x82141930) at /home/cborntra/REPOS/qemu/kvm-all.c:1901
>> #6 0x000000008005a73c in qemu_kvm_cpu_thread_fn (arg=0x82141930) at /home/cborntra/REPOS/qemu/cpus.c:977
>> #7 0x000003fffc2374e6 in start_thread () from /lib64/libpthread.so.0
>> #8 0x000003fffc18b0fa in thread_start () from /lib64/libc.so.6
>>
>> Thread 1 (Thread 0x3fffb4d0bd0 (LWP 57736)):
>> #0 0x000003fffc23b57c in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
>> #1 0x00000000803572fa in qemu_cond_wait (cond=0x809c28c0, mutex=0x809c2890) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:132
>> #2 0x000000008036dc3e in rfifolock_lock (r=0x809c2890) at /home/cborntra/REPOS/qemu/util/rfifolock.c:59
>> #3 0x0000000080281162 in aio_context_acquire (ctx=0x809c2830) at /home/cborntra/REPOS/qemu/async.c:331
>> #4 0x00000000800a2f08 in virtio_blk_data_plane_start (s=0x80a0d6f0) at /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:285
>> #5 0x00000000800a0bfe in virtio_blk_handle_output (vdev=0x809b5e18, vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:599
>> #6 0x00000000800d065c in virtio_queue_notify_vq (vq=0x80a70940) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:921
>> #7 0x00000000800d29b4 in virtio_queue_host_notifier_read (n=0x80a70988) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1480
>> #8 0x0000000080293b76 in qemu_iohandler_poll (pollfds=0x809b3600, ret=1) at /home/cborntra/REPOS/qemu/iohandler.c:126
>> #9 0x0000000080293714 in main_loop_wait (nonblocking=0) at /home/cborntra/REPOS/qemu/main-loop.c:494
>> #10 0x000000008014dc1c in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1902
>> #11 0x000000008015627c in main (argc=44, argv=0x3ffffcd66e8, envp=0x3ffffcd6850) at /home/cborntra/REPOS/qemu/vl.c:4653
>>
>>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-23 19:29 ` Christian Borntraeger
@ 2015-07-24 6:46 ` Cornelia Huck
2015-07-27 15:41 ` Stefan Hajnoczi
0 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2015-07-24 6:46 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi,
Peter Maydell
On Thu, 23 Jul 2015 21:29:05 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Am 23.07.2015 um 21:25 schrieb Paolo Bonzini:
> >
> >
> > On 23/07/2015 21:19, Christian Borntraeger wrote:
> >> Am 23.07.2015 um 20:19 schrieb Paolo Bonzini:
> >>>
> >>>
> >>> On 23/07/2015 19:20, Paolo Bonzini wrote:
> >>>>
> >>>>
> >>>> On 23/07/2015 16:14, Cornelia Huck wrote:
> >>>>> (gdb) bt
> >>>>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
> >>>>> from /lib64/libpthread.so.0
> >>>>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
> >>>>> mutex=mutex@entry=0x9717d920)
> >>>>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
> >>>>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
> >>>>> at /data/git/yyy/qemu/util/rfifolock.c:59
> >>>>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
> >>>>> at /data/git/yyy/qemu/async.c:331
> >>>>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
> >>>>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
> >>>>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
> >>>>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
> >>>>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
> >>>>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
> >>>>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
> >>>>> at /data/git/yyy/qemu/main-loop.c:494
> >>>>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
> >>>>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
> >>>>> at /data/git/yyy/qemu/vl.c:4653
> >>>>>
> >>>>> I've stripped down the setup to the following commandline:
> >>>>>
> >>>>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
> >>>>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
> >>>>> 4,sockets=4,cores=1,threads=1 -nographic -drive
> >>>>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
> >>>>> -device
> >>>>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
> >>>>
> >>>> What's the backtrace like for the other threads? This is almost
> >>>> definitely a latent bug somewhere else.
> >>>
> >>> BTW, I can reproduce this---I'm asking because I cannot even attach gdb
> >>> to the hung process.
> >>>
> >>> The simplest workaround is to reintroduce commit a0710f7995 (iothread:
> >>> release iothread around aio_poll, 2015-02-20), though it also comes with
> >>> some risk. It avoids the bug because it limits the contention on the
> >>> RFifoLock.
> >>
> >> I can reproduce this with the following backtrace (with --enable-debug info added qemu is the tag v2.4.0-rc2)
> >
> > Can you check that cherry-picking a0710f7995 works for you?
> >
>
> Yes, this seems to help for me. Conny, can you verify?
I can't reproduce the hang with this on top, so seems to help.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization
2015-07-24 6:46 ` Cornelia Huck
@ 2015-07-27 15:41 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-27 15:41 UTC (permalink / raw)
To: Cornelia Huck
Cc: Kevin Wolf, Peter Maydell, qemu-devel, Christian Borntraeger,
Stefan Hajnoczi, Paolo Bonzini
On Fri, Jul 24, 2015 at 7:46 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Thu, 23 Jul 2015 21:29:05 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> Am 23.07.2015 um 21:25 schrieb Paolo Bonzini:
>> >
>> >
>> > On 23/07/2015 21:19, Christian Borntraeger wrote:
>> >> Am 23.07.2015 um 20:19 schrieb Paolo Bonzini:
>> >>>
>> >>>
>> >>> On 23/07/2015 19:20, Paolo Bonzini wrote:
>> >>>>
>> >>>>
>> >>>> On 23/07/2015 16:14, Cornelia Huck wrote:
>> >>>>> (gdb) bt
>> >>>>> #0 0x000003fffc5871b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
>> >>>>> from /lib64/libpthread.so.0
>> >>>>> #1 0x000000008024cfca in qemu_cond_wait (cond=cond@entry=0x9717d950,
>> >>>>> mutex=mutex@entry=0x9717d920)
>> >>>>> at /data/git/yyy/qemu/util/qemu-thread-posix.c:132
>> >>>>> #2 0x000000008025e83a in rfifolock_lock (r=0x9717d920)
>> >>>>> at /data/git/yyy/qemu/util/rfifolock.c:59
>> >>>>> #3 0x00000000801b78fa in aio_context_acquire (ctx=<optimized out>)
>> >>>>> at /data/git/yyy/qemu/async.c:331
>> >>>>> #4 0x000000008007ceb4 in virtio_blk_data_plane_start (s=0x9717d710)
>> >>>>> at /data/git/yyy/qemu/hw/block/dataplane/virtio-blk.c:285
>> >>>>> #5 0x000000008007c64a in virtio_blk_handle_output (vdev=<optimized out>,
>> >>>>> vq=<optimized out>) at /data/git/yyy/qemu/hw/block/virtio-blk.c:599
>> >>>>> #6 0x00000000801c56dc in qemu_iohandler_poll (pollfds=0x97142800,
>> >>>>> ret=ret@entry=1) at /data/git/yyy/qemu/iohandler.c:126
>> >>>>> #7 0x00000000801c5178 in main_loop_wait (nonblocking=<optimized out>)
>> >>>>> at /data/git/yyy/qemu/main-loop.c:494
>> >>>>> #8 0x0000000080013ee2 in main_loop () at /data/git/yyy/qemu/vl.c:1902
>> >>>>> #9 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>> >>>>> at /data/git/yyy/qemu/vl.c:4653
>> >>>>>
>> >>>>> I've stripped down the setup to the following commandline:
>> >>>>>
>> >>>>> /data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine
>> >>>>> s390-ccw-virtio-2.4,accel=kvm,usb=off -m 1024 -smp
>> >>>>> 4,sockets=4,cores=1,threads=1 -nographic -drive
>> >>>>> file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none,aio=native
>> >>>>> -device
>> >>>>> virtio-blk-ccw,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,x-data-plane=on
>> >>>>
>> >>>> What's the backtrace like for the other threads? This is almost
>> >>>> definitely a latent bug somewhere else.
>> >>>
>> >>> BTW, I can reproduce this---I'm asking because I cannot even attach gdb
>> >>> to the hung process.
>> >>>
>> >>> The simplest workaround is to reintroduce commit a0710f7995 (iothread:
>> >>> release iothread around aio_poll, 2015-02-20), though it also comes with
>> >>> some risk. It avoids the bug because it limits the contention on the
>> >>> RFifoLock.
>> >>
>> >> I can reproduce this with the following backtrace (with --enable-debug info added qemu is the tag v2.4.0-rc2)
>> >
>> > Can you check that cherry-picking a0710f7995 works for you?
>> >
>>
>> Yes, this seems to help for me. Conny, can you verify?
>
> I can't reproduce the hang with this on top, so seems to help.
Thanks Christian and Cornelia. I'm sending a fix.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PULL v2 for-2.4 v2 6/7] AioContext: fix broken placement of event_notifier_test_and_clear
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
` (4 preceding siblings ...)
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 5/7] AioContext: fix broken ctx->dispatching optimization Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 7/7] AioContext: optimize clearing the EventNotifier Stefan Hajnoczi
2015-07-22 14:02 ` [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Peter Maydell
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
event_notifier_test_and_clear must be called before processing events.
Otherwise, an aio_poll could "eat" the notification before the main
I/O thread invokes ppoll(). The main I/O thread then never wakes up.
This is an example of what could happen:
i/o thread vcpu thread worker thread
---------------------------------------------------------------------
lock_iothread
notify_me = 1
...
unlock_iothread
bh->scheduled = 1
event_notifier_set
lock_iothread
notify_me = 3
ppoll
notify_me = 1
aio_dispatch
aio_bh_poll
thread_pool_completion_bh
bh->scheduled = 1
event_notifier_set
node->io_read(node->opaque)
event_notifier_test_and_clear
ppoll
*** hang ***
"Tracing" with qemu_clock_get_ns shows pretty much the same behavior as
in the previous bug, so there are no new tricks here---just stare more
at the code until it is apparent.
One could also use a formal model, of course. The included one shows
this with three processes: notifier corresponds to a QEMU thread pool
worker, temporary_waiter to a VCPU thread that invokes aio_poll(),
waiter to the main I/O thread. I would be happy to say that the
formal model found the bug for me, but actually I wrote it after the
fact.
This patch is a bit of a big hammer. The next one optimizes it,
with help (this time for real rather than a posteriori :)) from
another, similar formal model.
Reported-by: Richard W. M. Jones <rjones@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-6-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
aio-posix.c | 2 +
aio-win32.c | 7 ++-
async.c | 8 ++-
docs/aio_notify_bug.promela | 140 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 153 insertions(+), 4 deletions(-)
create mode 100644 docs/aio_notify_bug.promela
diff --git a/aio-posix.c b/aio-posix.c
index 249889f..5c8b266 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -276,6 +276,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
aio_context_acquire(ctx);
}
+ event_notifier_test_and_clear(&ctx->notifier);
+
/* if we have any readable fds, dispatch event */
if (ret > 0) {
for (i = 0; i < npfd; i++) {
diff --git a/aio-win32.c b/aio-win32.c
index ea655b0..7afc999 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -337,10 +337,11 @@ bool aio_poll(AioContext *ctx, bool blocking)
aio_context_acquire(ctx);
}
- if (first && aio_bh_poll(ctx)) {
- progress = true;
+ if (first) {
+ event_notifier_test_and_clear(&ctx->notifier);
+ progress |= aio_bh_poll(ctx);
+ first = false;
}
- first = false;
/* if we have any signaled events, dispatch event */
event = NULL;
diff --git a/async.c b/async.c
index a232192..d625e8a 100644
--- a/async.c
+++ b/async.c
@@ -203,6 +203,8 @@ aio_ctx_check(GSource *source)
QEMUBH *bh;
atomic_and(&ctx->notify_me, ~1);
+ event_notifier_test_and_clear(&ctx->notifier);
+
for (bh = ctx->first_bh; bh; bh = bh->next) {
if (!bh->deleted && bh->scheduled) {
return true;
@@ -279,6 +281,10 @@ static void aio_rfifolock_cb(void *opaque)
aio_notify(opaque);
}
+static void event_notifier_dummy_cb(EventNotifier *e)
+{
+}
+
AioContext *aio_context_new(Error **errp)
{
int ret;
@@ -293,7 +299,7 @@ AioContext *aio_context_new(Error **errp)
g_source_set_can_recurse(&ctx->source, true);
aio_set_event_notifier(ctx, &ctx->notifier,
(EventNotifierHandler *)
- event_notifier_test_and_clear);
+ event_notifier_dummy_cb);
ctx->thread_pool = NULL;
qemu_mutex_init(&ctx->bh_lock);
rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
diff --git a/docs/aio_notify_bug.promela b/docs/aio_notify_bug.promela
new file mode 100644
index 0000000..b3bfca1
--- /dev/null
+++ b/docs/aio_notify_bug.promela
@@ -0,0 +1,140 @@
+/*
+ * This model describes a bug in aio_notify. If ctx->notifier is
+ * cleared too late, a wakeup could be lost.
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This file is in the public domain. If you really want a license,
+ * the WTFPL will do.
+ *
+ * To verify the buggy version:
+ * spin -a -DBUG docs/aio_notify_bug.promela
+ * gcc -O2 pan.c
+ * ./a.out -a -f
+ *
+ * To verify the fixed version:
+ * spin -a docs/aio_notify_bug.promela
+ * gcc -O2 pan.c
+ * ./a.out -a -f
+ *
+ * Add -DCHECK_REQ to test an alternative invariant and the
+ * "notify_me" optimization.
+ */
+
+int notify_me;
+bool event;
+bool req;
+bool notifier_done;
+
+#ifdef CHECK_REQ
+#define USE_NOTIFY_ME 1
+#else
+#define USE_NOTIFY_ME 0
+#endif
+
+active proctype notifier()
+{
+ do
+ :: true -> {
+ req = 1;
+ if
+ :: !USE_NOTIFY_ME || notify_me -> event = 1;
+ :: else -> skip;
+ fi
+ }
+ :: true -> break;
+ od;
+ notifier_done = 1;
+}
+
+#ifdef BUG
+#define AIO_POLL \
+ notify_me++; \
+ if \
+ :: !req -> { \
+ if \
+ :: event -> skip; \
+ fi; \
+ } \
+ :: else -> skip; \
+ fi; \
+ notify_me--; \
+ \
+ req = 0; \
+ event = 0;
+#else
+#define AIO_POLL \
+ notify_me++; \
+ if \
+ :: !req -> { \
+ if \
+ :: event -> skip; \
+ fi; \
+ } \
+ :: else -> skip; \
+ fi; \
+ notify_me--; \
+ \
+ event = 0; \
+ req = 0;
+#endif
+
+active proctype waiter()
+{
+ do
+ :: true -> AIO_POLL;
+ od;
+}
+
+/* Same as waiter(), but disappears after a while. */
+active proctype temporary_waiter()
+{
+ do
+ :: true -> AIO_POLL;
+ :: true -> break;
+ od;
+}
+
+#ifdef CHECK_REQ
+never {
+ do
+ :: req -> goto accept_if_req_not_eventually_false;
+ :: true -> skip;
+ od;
+
+accept_if_req_not_eventually_false:
+ if
+ :: req -> goto accept_if_req_not_eventually_false;
+ fi;
+ assert(0);
+}
+
+#else
+/* There must be infinitely many transitions of event as long
+ * as the notifier does not exit.
+ *
+ * If event stayed always true, the waiters would be busy looping.
+ * If event stayed always false, the waiters would be sleeping
+ * forever.
+ */
+never {
+ do
+ :: !event -> goto accept_if_event_not_eventually_true;
+ :: event -> goto accept_if_event_not_eventually_false;
+ :: true -> skip;
+ od;
+
+accept_if_event_not_eventually_true:
+ if
+ :: !event && notifier_done -> do :: true -> skip; od;
+ :: !event && !notifier_done -> goto accept_if_event_not_eventually_true;
+ fi;
+ assert(0);
+
+accept_if_event_not_eventually_false:
+ if
+ :: event -> goto accept_if_event_not_eventually_false;
+ fi;
+ assert(0);
+}
+#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PULL v2 for-2.4 v2 7/7] AioContext: optimize clearing the EventNotifier
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
` (5 preceding siblings ...)
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 6/7] AioContext: fix broken placement of event_notifier_test_and_clear Stefan Hajnoczi
@ 2015-07-22 11:43 ` Stefan Hajnoczi
2015-07-22 14:02 ` [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Peter Maydell
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-07-22 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Paolo Bonzini
From: Paolo Bonzini <pbonzini@redhat.com>
It is pretty rare for aio_notify to actually set the EventNotifier. It
can happen with worker threads such as thread-pool.c's, but otherwise it
should never be set thanks to the ctx->notify_me optimization. The
previous patch, unfortunately, added an unconditional call to
event_notifier_test_and_clear; now add a userspace fast path that
avoids the call.
Note that it is not possible to do the same with event_notifier_set;
it would break, as proved (again) by the included formal model.
This patch survived over 3000 reboots on aarch64 KVM.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-7-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
aio-posix.c | 2 +-
aio-win32.c | 2 +-
async.c | 10 ++-
docs/aio_notify_accept.promela | 152 +++++++++++++++++++++++++++++++++++++++++
include/block/aio.h | 32 ++++++++-
5 files changed, 194 insertions(+), 4 deletions(-)
create mode 100644 docs/aio_notify_accept.promela
diff --git a/aio-posix.c b/aio-posix.c
index 5c8b266..d477033 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -276,7 +276,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
aio_context_acquire(ctx);
}
- event_notifier_test_and_clear(&ctx->notifier);
+ aio_notify_accept(ctx);
/* if we have any readable fds, dispatch event */
if (ret > 0) {
diff --git a/aio-win32.c b/aio-win32.c
index 7afc999..50a6867 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -338,7 +338,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
}
if (first) {
- event_notifier_test_and_clear(&ctx->notifier);
+ aio_notify_accept(ctx);
progress |= aio_bh_poll(ctx);
first = false;
}
diff --git a/async.c b/async.c
index d625e8a..9a98a74 100644
--- a/async.c
+++ b/async.c
@@ -203,7 +203,7 @@ aio_ctx_check(GSource *source)
QEMUBH *bh;
atomic_and(&ctx->notify_me, ~1);
- event_notifier_test_and_clear(&ctx->notifier);
+ aio_notify_accept(ctx);
for (bh = ctx->first_bh; bh; bh = bh->next) {
if (!bh->deleted && bh->scheduled) {
@@ -267,6 +267,14 @@ void aio_notify(AioContext *ctx)
smp_mb();
if (ctx->notify_me) {
event_notifier_set(&ctx->notifier);
+ atomic_mb_set(&ctx->notified, true);
+ }
+}
+
+void aio_notify_accept(AioContext *ctx)
+{
+ if (atomic_xchg(&ctx->notified, false)) {
+ event_notifier_test_and_clear(&ctx->notifier);
}
}
diff --git a/docs/aio_notify_accept.promela b/docs/aio_notify_accept.promela
new file mode 100644
index 0000000..9cef2c9
--- /dev/null
+++ b/docs/aio_notify_accept.promela
@@ -0,0 +1,152 @@
+/*
+ * This model describes the interaction between ctx->notified
+ * and ctx->notifier.
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This file is in the public domain. If you really want a license,
+ * the WTFPL will do.
+ *
+ * To verify the buggy version:
+ * spin -a -DBUG1 docs/aio_notify_bug.promela
+ * gcc -O2 pan.c
+ * ./a.out -a -f
+ * (or -DBUG2)
+ *
+ * To verify the fixed version:
+ * spin -a docs/aio_notify_bug.promela
+ * gcc -O2 pan.c
+ * ./a.out -a -f
+ *
+ * Add -DCHECK_REQ to test an alternative invariant and the
+ * "notify_me" optimization.
+ */
+
+int notify_me;
+bool notified;
+bool event;
+bool req;
+bool notifier_done;
+
+#ifdef CHECK_REQ
+#define USE_NOTIFY_ME 1
+#else
+#define USE_NOTIFY_ME 0
+#endif
+
+#ifdef BUG
+#error Please define BUG1 or BUG2 instead.
+#endif
+
+active proctype notifier()
+{
+ do
+ :: true -> {
+ req = 1;
+ if
+ :: !USE_NOTIFY_ME || notify_me ->
+#if defined BUG1
+ /* CHECK_REQ does not detect this bug! */
+ notified = 1;
+ event = 1;
+#elif defined BUG2
+ if
+ :: !notified -> event = 1;
+ :: else -> skip;
+ fi;
+ notified = 1;
+#else
+ event = 1;
+ notified = 1;
+#endif
+ :: else -> skip;
+ fi
+ }
+ :: true -> break;
+ od;
+ notifier_done = 1;
+}
+
+#define AIO_POLL \
+ notify_me++; \
+ if \
+ :: !req -> { \
+ if \
+ :: event -> skip; \
+ fi; \
+ } \
+ :: else -> skip; \
+ fi; \
+ notify_me--; \
+ \
+ atomic { old = notified; notified = 0; } \
+ if \
+ :: old -> event = 0; \
+ :: else -> skip; \
+ fi; \
+ \
+ req = 0;
+
+active proctype waiter()
+{
+ bool old;
+
+ do
+ :: true -> AIO_POLL;
+ od;
+}
+
+/* Same as waiter(), but disappears after a while. */
+active proctype temporary_waiter()
+{
+ bool old;
+
+ do
+ :: true -> AIO_POLL;
+ :: true -> break;
+ od;
+}
+
+#ifdef CHECK_REQ
+never {
+ do
+ :: req -> goto accept_if_req_not_eventually_false;
+ :: true -> skip;
+ od;
+
+accept_if_req_not_eventually_false:
+ if
+ :: req -> goto accept_if_req_not_eventually_false;
+ fi;
+ assert(0);
+}
+
+#else
+/* There must be infinitely many transitions of event as long
+ * as the notifier does not exit.
+ *
+ * If event stayed always true, the waiters would be busy looping.
+ * If event stayed always false, the waiters would be sleeping
+ * forever.
+ */
+never {
+ do
+ :: !event -> goto accept_if_event_not_eventually_true;
+ :: event -> goto accept_if_event_not_eventually_false;
+ :: true -> skip;
+ od;
+
+accept_if_event_not_eventually_true:
+ if
+ :: !event && notifier_done -> do :: true -> skip; od;
+ :: !event && !notifier_done -> goto accept_if_event_not_eventually_true;
+ fi;
+ assert(0);
+
+accept_if_event_not_eventually_false:
+ if
+ :: event -> goto accept_if_event_not_eventually_false;
+ fi;
+ assert(0);
+}
+#endif
diff --git a/include/block/aio.h b/include/block/aio.h
index be91e3f..9dd32e0 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -99,7 +99,19 @@ struct AioContext {
*/
int walking_bh;
- /* Used for aio_notify. */
+ /* Used by aio_notify.
+ *
+ * "notified" is used to avoid expensive event_notifier_test_and_clear
+ * calls. When it is clear, the EventNotifier is clear, or one thread
+ * is going to clear "notified" before processing more events. False
+ * positives are possible, i.e. "notified" could be set even though the
+ * EventNotifier is clear.
+ *
+ * Note that event_notifier_set *cannot* be optimized the same way. For
+ * more information on the problem that would result, see "#ifdef BUG2"
+ * in the docs/aio_notify_accept.promela formal model.
+ */
+ bool notified;
EventNotifier notifier;
/* Thread pool for performing work and receiving completion callbacks */
@@ -174,6 +186,24 @@ QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque);
void aio_notify(AioContext *ctx);
/**
+ * aio_notify_accept: Acknowledge receiving an aio_notify.
+ *
+ * aio_notify() uses an EventNotifier in order to wake up a sleeping
+ * aio_poll() or g_main_context_iteration(). Calls to aio_notify() are
+ * usually rare, but the AioContext has to clear the EventNotifier on
+ * every aio_poll() or g_main_context_iteration() in order to avoid
+ * busy waiting. This event_notifier_test_and_clear() cannot be done
+ * using the usual aio_context_set_event_notifier(), because it must
+ * be done before processing all events (file descriptors, bottom halves,
+ * timers).
+ *
+ * aio_notify_accept() is an optimized event_notifier_test_and_clear()
+ * that is specific to an AioContext's notifier; it is used internally
+ * to clear the EventNotifier only if aio_notify() had been called.
+ */
+void aio_notify_accept(AioContext *ctx);
+
+/**
* aio_bh_poll: Poll bottom halves for an AioContext.
*
* These are internal functions used by the QEMU main loop.
--
2.4.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches
2015-07-22 11:43 [Qemu-devel] [PULL v2 for-2.4 v2 0/7] Block patches Stefan Hajnoczi
` (6 preceding siblings ...)
2015-07-22 11:43 ` [Qemu-devel] [PULL v2 for-2.4 v2 7/7] AioContext: optimize clearing the EventNotifier Stefan Hajnoczi
@ 2015-07-22 14:02 ` Peter Maydell
7 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2015-07-22 14:02 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, QEMU Developers
On 22 July 2015 at 12:43, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit 774ee4772b6838b78741ea52d4bf26b8922244c5:
>
> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20150721' into staging (2015-07-21 12:21:08 +0100)
>
> are available in the git repository at:
>
> git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 05e514b1d4d5bd4209e2c8bbc76ff05c85a235f3:
>
> AioContext: optimize clearing the EventNotifier (2015-07-22 12:41:40 +0100)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread