* [Qemu-devel] [PATCH for-2.1] AioContext: do not rely on aio_poll(ctx, true) result to end a loop
@ 2014-07-09 8:49 Paolo Bonzini
2014-07-09 12:56 ` Kevin Wolf
0 siblings, 1 reply; 2+ messages in thread
From: Paolo Bonzini @ 2014-07-09 8:49 UTC (permalink / raw
To: qemu-devel; +Cc: kwolf, borntraeger, stefanha
Currently, whenever aio_poll(ctx, true) has completed all pending
work it returns true *and* the next call to aio_poll(ctx, true)
will not block.
This invariant has its roots in qemu_aio_flush()'s implementation
as "while (qemu_aio_wait()) {}". However, qemu_aio_flush() does
not exist anymore and bdrv_drain_all() is implemented differently;
and this invariant is complicated to maintain and subtly different
from the return value of GMainLoop's g_main_context_iteration.
All calls to aio_poll(ctx, true) except one are guarded by a
while() loop checking for a request to be incomplete, or a
BlockDriverState to be idle. The one remaining call (in
iothread.c) uses this to delay the aio_context_release/acquire
pair until the AioContext is quiescent, however:
- we can do the same just by using non-blocking aio_poll,
similar to how vl.c invokes main_loop_wait
- it is buggy, because it does not ensure that the AioContext
is released between an aio_notify and the next time the
iothread goes to sleep. This leads to hangs when stopping
the dataplane thread.
In the end, these semantics are a bad match for the current
users of AioContext. So modify that one exception in iothread.c,
which also fixes the hangs, as well as the testcase so that
it use the same idiom as the actual QEMU code.
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/block/aio.h | 6 +++---
iothread.c | 5 ++++-
tests/test-aio.c | 25 +++++++++++++------------
3 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index 433e7ff..c23de3c 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -214,9 +214,9 @@ bool aio_pending(AioContext *ctx);
/* Progress in completing AIO work to occur. This can issue new pending
* aio as a result of executing I/O completion or bh callbacks.
*
- * If there is no pending AIO operation or completion (bottom half),
- * return false. If there are pending AIO operations of bottom halves,
- * return true.
+ * Return whether any progress was made by executing AIO or bottom half
+ * handlers. If @blocking == true, this should always be true except
+ * if someone called aio_notify.
*
* If there are no pending bottom halves, but there are pending AIO
* operations, it may not be possible to make any progress without
diff --git a/iothread.c b/iothread.c
index 1fbf9f1..d9403cf 100644
--- a/iothread.c
+++ b/iothread.c
@@ -30,6 +30,7 @@ typedef ObjectClass IOThreadClass;
static void *iothread_run(void *opaque)
{
IOThread *iothread = opaque;
+ bool blocking;
qemu_mutex_lock(&iothread->init_done_lock);
iothread->thread_id = qemu_get_thread_id();
@@ -38,8 +39,10 @@ static void *iothread_run(void *opaque)
while (!iothread->stopping) {
aio_context_acquire(iothread->ctx);
- while (!iothread->stopping && aio_poll(iothread->ctx, true)) {
+ blocking = true;
+ while (!iothread->stopping && aio_poll(iothread->ctx, blocking)) {
/* Progress was made, keep going */
+ blocking = false;
}
aio_context_release(iothread->ctx);
}
diff --git a/tests/test-aio.c b/tests/test-aio.c
index 264dab9..4c40a49 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -24,14 +24,6 @@ typedef struct {
bool auto_set;
} EventNotifierTestData;
-/* Wait until there are no more BHs or AIO requests */
-static void wait_for_aio(void)
-{
- while (aio_poll(ctx, true)) {
- /* Do nothing */
- }
-}
-
/* Wait until event notifier becomes inactive */
static void wait_until_inactive(EventNotifierTestData *data)
{
@@ -204,7 +196,9 @@ static void test_bh_schedule10(void)
g_assert(aio_poll(ctx, true));
g_assert_cmpint(data.n, ==, 2);
- wait_for_aio();
+ while (data.n < 10) {
+ aio_poll(ctx, true);
+ }
g_assert_cmpint(data.n, ==, 10);
g_assert(!aio_poll(ctx, false));
@@ -252,7 +246,9 @@ static void test_bh_delete_from_cb(void)
qemu_bh_schedule(data1.bh);
g_assert_cmpint(data1.n, ==, 0);
- wait_for_aio();
+ while (data1.n < data1.max) {
+ aio_poll(ctx, true);
+ }
g_assert_cmpint(data1.n, ==, data1.max);
g_assert(data1.bh == NULL);
@@ -287,7 +283,12 @@ static void test_bh_delete_from_cb_many(void)
g_assert_cmpint(data4.n, ==, 1);
g_assert(data1.bh == NULL);
- wait_for_aio();
+ while (data1.n < data1.max ||
+ data2.n < data2.max ||
+ data3.n < data3.max ||
+ data4.n < data4.max) {
+ aio_poll(ctx, true);
+ }
g_assert_cmpint(data1.n, ==, data1.max);
g_assert_cmpint(data2.n, ==, data2.max);
g_assert_cmpint(data3.n, ==, data3.max);
@@ -306,7 +307,7 @@ static void test_bh_flush(void)
qemu_bh_schedule(data.bh);
g_assert_cmpint(data.n, ==, 0);
- wait_for_aio();
+ g_assert(aio_poll(ctx, true));
g_assert_cmpint(data.n, ==, 1);
g_assert(!aio_poll(ctx, false));
--
1.8.3.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.1] AioContext: do not rely on aio_poll(ctx, true) result to end a loop
2014-07-09 8:49 [Qemu-devel] [PATCH for-2.1] AioContext: do not rely on aio_poll(ctx, true) result to end a loop Paolo Bonzini
@ 2014-07-09 12:56 ` Kevin Wolf
0 siblings, 0 replies; 2+ messages in thread
From: Kevin Wolf @ 2014-07-09 12:56 UTC (permalink / raw
To: Paolo Bonzini; +Cc: borntraeger, qemu-devel, stefanha
Am 09.07.2014 um 10:49 hat Paolo Bonzini geschrieben:
> Currently, whenever aio_poll(ctx, true) has completed all pending
> work it returns true *and* the next call to aio_poll(ctx, true)
> will not block.
>
> This invariant has its roots in qemu_aio_flush()'s implementation
> as "while (qemu_aio_wait()) {}". However, qemu_aio_flush() does
> not exist anymore and bdrv_drain_all() is implemented differently;
> and this invariant is complicated to maintain and subtly different
> from the return value of GMainLoop's g_main_context_iteration.
>
> All calls to aio_poll(ctx, true) except one are guarded by a
> while() loop checking for a request to be incomplete, or a
> BlockDriverState to be idle. The one remaining call (in
> iothread.c) uses this to delay the aio_context_release/acquire
> pair until the AioContext is quiescent, however:
>
> - we can do the same just by using non-blocking aio_poll,
> similar to how vl.c invokes main_loop_wait
>
> - it is buggy, because it does not ensure that the AioContext
> is released between an aio_notify and the next time the
> iothread goes to sleep. This leads to hangs when stopping
> the dataplane thread.
>
> In the end, these semantics are a bad match for the current
> users of AioContext. So modify that one exception in iothread.c,
> which also fixes the hangs, as well as the testcase so that
> it use the same idiom as the actual QEMU code.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-07-09 12:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-09 8:49 [Qemu-devel] [PATCH for-2.1] AioContext: do not rely on aio_poll(ctx, true) result to end a loop Paolo Bonzini
2014-07-09 12:56 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.