2009-10-22 19:54:37 +04:00
|
|
|
/*
|
2017-02-13 16:52:18 +03:00
|
|
|
* Data plane event loop
|
2009-10-22 19:54:37 +04:00
|
|
|
*
|
|
|
|
* Copyright (c) 2003-2008 Fabrice Bellard
|
2017-02-13 16:52:18 +03:00
|
|
|
* Copyright (c) 2009-2017 QEMU contributors
|
2009-10-22 19:54:37 +04:00
|
|
|
*
|
|
|
|
* Permission is hereby granted, free of charge, to any person obtaining a copy
|
|
|
|
* of this software and associated documentation files (the "Software"), to deal
|
|
|
|
* in the Software without restriction, including without limitation the rights
|
|
|
|
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
|
|
|
|
* copies of the Software, and to permit persons to whom the Software is
|
|
|
|
* furnished to do so, subject to the following conditions:
|
|
|
|
*
|
|
|
|
* The above copyright notice and this permission notice shall be included in
|
|
|
|
* all copies or substantial portions of the Software.
|
|
|
|
*
|
|
|
|
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
|
|
|
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
|
|
|
|
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
|
|
|
|
* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
|
|
|
|
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
|
|
|
|
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
|
|
|
|
* THE SOFTWARE.
|
|
|
|
*/
|
|
|
|
|
2016-01-29 20:50:05 +03:00
|
|
|
#include "qemu/osdep.h"
|
include/qemu/osdep.h: Don't include qapi/error.h
Commit 57cb38b included qapi/error.h into qemu/osdep.h to get the
Error typedef. Since then, we've moved to include qemu/osdep.h
everywhere. Its file comment explains: "To avoid getting into
possible circular include dependencies, this file should not include
any other QEMU headers, with the exceptions of config-host.h,
compiler.h, os-posix.h and os-win32.h, all of which are doing a
similar job to this file and are under similar constraints."
qapi/error.h doesn't do a similar job, and it doesn't adhere to
similar constraints: it includes qapi-types.h. That's in excess of
100KiB of crap most .c files don't actually need.
Add the typedef to qemu/typedefs.h, and include that instead of
qapi/error.h. Include qapi/error.h in .c files that need it and don't
get it now. Include qapi-types.h in qom/object.h for uint16List.
Update scripts/clean-includes accordingly. Update it further to match
reality: replace config.h by config-target.h, add sysemu/os-posix.h,
sysemu/os-win32.h. Update the list of includes in the qemu/osdep.h
comment quoted above similarly.
This reduces the number of objects depending on qapi/error.h from "all
of them" to less than a third. Unfortunately, the number depending on
qapi-types.h shrinks only a little. More work is needed for that one.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[Fix compilation without the spice devel packages. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2016-03-14 11:01:28 +03:00
|
|
|
#include "qapi/error.h"
|
2012-12-17 21:19:44 +04:00
|
|
|
#include "block/aio.h"
|
2013-03-07 16:41:47 +04:00
|
|
|
#include "block/thread-pool.h"
|
2012-12-17 21:20:00 +04:00
|
|
|
#include "qemu/main-loop.h"
|
2014-07-07 17:18:04 +04:00
|
|
|
#include "qemu/atomic.h"
|
2020-02-21 12:39:51 +03:00
|
|
|
#include "qemu/rcu_queue.h"
|
2016-07-04 19:33:20 +03:00
|
|
|
#include "block/raw-aio.h"
|
2017-02-13 16:52:19 +03:00
|
|
|
#include "qemu/coroutine_int.h"
|
|
|
|
#include "trace.h"
|
2009-10-22 19:54:38 +04:00
|
|
|
|
2009-10-22 19:54:37 +04:00
|
|
|
/***********************************************************/
|
|
|
|
/* bottom halves (can be seen as timers which expire ASAP) */
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
/* QEMUBH::flags values */
|
|
|
|
enum {
|
|
|
|
/* Already enqueued and waiting for aio_bh_poll() */
|
|
|
|
BH_PENDING = (1 << 0),
|
|
|
|
|
|
|
|
/* Invoke the callback */
|
|
|
|
BH_SCHEDULED = (1 << 1),
|
|
|
|
|
|
|
|
/* Delete without invoking callback */
|
|
|
|
BH_DELETED = (1 << 2),
|
|
|
|
|
|
|
|
/* Delete after invoking callback */
|
|
|
|
BH_ONESHOT = (1 << 3),
|
|
|
|
|
|
|
|
/* Schedule periodically when the event loop is idle */
|
|
|
|
BH_IDLE = (1 << 4),
|
|
|
|
};
|
|
|
|
|
2009-10-22 19:54:37 +04:00
|
|
|
struct QEMUBH {
|
2012-09-24 20:44:14 +04:00
|
|
|
AioContext *ctx;
|
2009-10-22 19:54:37 +04:00
|
|
|
QEMUBHFunc *cb;
|
|
|
|
void *opaque;
|
2020-02-21 12:39:51 +03:00
|
|
|
QSLIST_ENTRY(QEMUBH) next;
|
|
|
|
unsigned flags;
|
2009-10-22 19:54:37 +04:00
|
|
|
};
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
/* Called concurrently from any thread */
|
|
|
|
static void aio_bh_enqueue(QEMUBH *bh, unsigned new_flags)
|
|
|
|
{
|
|
|
|
AioContext *ctx = bh->ctx;
|
|
|
|
unsigned old_flags;
|
|
|
|
|
|
|
|
/*
|
|
|
|
* The memory barrier implicit in atomic_fetch_or makes sure that:
|
|
|
|
* 1. idle & any writes needed by the callback are done before the
|
|
|
|
* locations are read in the aio_bh_poll.
|
|
|
|
* 2. ctx is loaded before the callback has a chance to execute and bh
|
|
|
|
* could be freed.
|
|
|
|
*/
|
|
|
|
old_flags = atomic_fetch_or(&bh->flags, BH_PENDING | new_flags);
|
|
|
|
if (!(old_flags & BH_PENDING)) {
|
|
|
|
QSLIST_INSERT_HEAD_ATOMIC(&ctx->bh_list, bh, next);
|
|
|
|
}
|
|
|
|
|
|
|
|
aio_notify(ctx);
|
|
|
|
}
|
|
|
|
|
|
|
|
/* Only called from aio_bh_poll() and aio_ctx_finalize() */
|
|
|
|
static QEMUBH *aio_bh_dequeue(BHList *head, unsigned *flags)
|
|
|
|
{
|
|
|
|
QEMUBH *bh = QSLIST_FIRST_RCU(head);
|
|
|
|
|
|
|
|
if (!bh) {
|
|
|
|
return NULL;
|
|
|
|
}
|
|
|
|
|
|
|
|
QSLIST_REMOVE_HEAD(head, next);
|
|
|
|
|
|
|
|
/*
|
|
|
|
* The atomic_and is paired with aio_bh_enqueue(). The implicit memory
|
|
|
|
* barrier ensures that the callback sees all writes done by the scheduling
|
|
|
|
* thread. It also ensures that the scheduling thread sees the cleared
|
|
|
|
* flag before bh->cb has run, and thus will call aio_notify again if
|
|
|
|
* necessary.
|
|
|
|
*/
|
|
|
|
*flags = atomic_fetch_and(&bh->flags,
|
|
|
|
~(BH_PENDING | BH_SCHEDULED | BH_IDLE));
|
|
|
|
return bh;
|
|
|
|
}
|
|
|
|
|
2016-10-03 19:14:15 +03:00
|
|
|
void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
|
|
|
|
{
|
|
|
|
QEMUBH *bh;
|
|
|
|
bh = g_new(QEMUBH, 1);
|
|
|
|
*bh = (QEMUBH){
|
|
|
|
.ctx = ctx,
|
|
|
|
.cb = cb,
|
|
|
|
.opaque = opaque,
|
|
|
|
};
|
2020-02-21 12:39:51 +03:00
|
|
|
aio_bh_enqueue(bh, BH_SCHEDULED | BH_ONESHOT);
|
2016-10-03 19:14:15 +03:00
|
|
|
}
|
|
|
|
|
2012-10-30 02:45:23 +04:00
|
|
|
QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
|
2009-10-22 19:54:37 +04:00
|
|
|
{
|
|
|
|
QEMUBH *bh;
|
2014-12-17 18:10:00 +03:00
|
|
|
bh = g_new(QEMUBH, 1);
|
|
|
|
*bh = (QEMUBH){
|
|
|
|
.ctx = ctx,
|
|
|
|
.cb = cb,
|
|
|
|
.opaque = opaque,
|
|
|
|
};
|
2009-10-22 19:54:37 +04:00
|
|
|
return bh;
|
|
|
|
}
|
|
|
|
|
2015-09-17 19:24:50 +03:00
|
|
|
void aio_bh_call(QEMUBH *bh)
|
|
|
|
{
|
|
|
|
bh->cb(bh->opaque);
|
|
|
|
}
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
/* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
|
2012-10-30 02:45:23 +04:00
|
|
|
int aio_bh_poll(AioContext *ctx)
|
2009-10-22 19:54:37 +04:00
|
|
|
{
|
2020-02-21 12:39:51 +03:00
|
|
|
BHListSlice slice;
|
|
|
|
BHListSlice *s;
|
|
|
|
int ret = 0;
|
|
|
|
|
|
|
|
QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
|
|
|
|
QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
|
|
|
|
|
|
|
|
while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
|
|
|
|
QEMUBH *bh;
|
|
|
|
unsigned flags;
|
|
|
|
|
|
|
|
bh = aio_bh_dequeue(&s->bh_list, &flags);
|
|
|
|
if (!bh) {
|
|
|
|
QSIMPLEQ_REMOVE_HEAD(&ctx->bh_slice_list, next);
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ((flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
|
2016-10-27 13:49:06 +03:00
|
|
|
/* Idle BHs don't count as progress */
|
2020-02-21 12:39:51 +03:00
|
|
|
if (!(flags & BH_IDLE)) {
|
2009-10-22 19:54:37 +04:00
|
|
|
ret = 1;
|
2015-07-28 19:34:09 +03:00
|
|
|
}
|
2015-09-17 19:24:50 +03:00
|
|
|
aio_bh_call(bh);
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
2020-02-21 12:39:51 +03:00
|
|
|
if (flags & (BH_DELETED | BH_ONESHOT)) {
|
|
|
|
g_free(bh);
|
2017-01-12 21:08:00 +03:00
|
|
|
}
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
|
|
|
|
return ret;
|
|
|
|
}
|
|
|
|
|
|
|
|
void qemu_bh_schedule_idle(QEMUBH *bh)
|
|
|
|
{
|
2020-02-21 12:39:51 +03:00
|
|
|
aio_bh_enqueue(bh, BH_SCHEDULED | BH_IDLE);
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
|
|
|
|
void qemu_bh_schedule(QEMUBH *bh)
|
|
|
|
{
|
2020-02-21 12:39:51 +03:00
|
|
|
aio_bh_enqueue(bh, BH_SCHEDULED);
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
|
2013-07-16 08:28:58 +04:00
|
|
|
/* This func is async.
|
|
|
|
*/
|
2009-10-22 19:54:37 +04:00
|
|
|
void qemu_bh_cancel(QEMUBH *bh)
|
|
|
|
{
|
2020-02-21 12:39:51 +03:00
|
|
|
atomic_and(&bh->flags, ~BH_SCHEDULED);
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
|
2013-07-16 08:28:58 +04:00
|
|
|
/* This func is async.The bottom half will do the delete action at the finial
|
|
|
|
* end.
|
|
|
|
*/
|
2009-10-22 19:54:37 +04:00
|
|
|
void qemu_bh_delete(QEMUBH *bh)
|
|
|
|
{
|
2020-02-21 12:39:51 +03:00
|
|
|
aio_bh_enqueue(bh, BH_DELETED);
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
static int64_t aio_compute_bh_timeout(BHList *head, int timeout)
|
2009-10-22 19:54:37 +04:00
|
|
|
{
|
|
|
|
QEMUBH *bh;
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
QSLIST_FOREACH_RCU(bh, head, next) {
|
|
|
|
if ((bh->flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
|
|
|
|
if (bh->flags & BH_IDLE) {
|
2009-10-22 19:54:37 +04:00
|
|
|
/* idle bottom halves will be polled at least
|
|
|
|
* every 10ms */
|
2014-07-09 13:53:01 +04:00
|
|
|
timeout = 10000000;
|
2009-10-22 19:54:37 +04:00
|
|
|
} else {
|
|
|
|
/* non-idle bottom halves will be executed
|
|
|
|
* immediately */
|
2014-07-09 13:53:01 +04:00
|
|
|
return 0;
|
2009-10-22 19:54:37 +04:00
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
2012-09-24 16:57:41 +04:00
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
return timeout;
|
|
|
|
}
|
|
|
|
|
|
|
|
int64_t
|
|
|
|
aio_compute_timeout(AioContext *ctx)
|
|
|
|
{
|
|
|
|
BHListSlice *s;
|
|
|
|
int64_t deadline;
|
|
|
|
int timeout = -1;
|
|
|
|
|
|
|
|
timeout = aio_compute_bh_timeout(&ctx->bh_list, timeout);
|
|
|
|
if (timeout == 0) {
|
|
|
|
return 0;
|
|
|
|
}
|
|
|
|
|
|
|
|
QSIMPLEQ_FOREACH(s, &ctx->bh_slice_list, next) {
|
|
|
|
timeout = aio_compute_bh_timeout(&s->bh_list, timeout);
|
|
|
|
if (timeout == 0) {
|
|
|
|
return 0;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-07-09 13:53:01 +04:00
|
|
|
deadline = timerlistgroup_deadline_ns(&ctx->tlg);
|
2013-08-21 19:02:51 +04:00
|
|
|
if (deadline == 0) {
|
2014-07-09 13:53:01 +04:00
|
|
|
return 0;
|
2013-08-21 19:02:51 +04:00
|
|
|
} else {
|
2014-07-09 13:53:01 +04:00
|
|
|
return qemu_soonest_timeout(timeout, deadline);
|
2013-08-21 19:02:51 +04:00
|
|
|
}
|
2014-07-09 13:53:01 +04:00
|
|
|
}
|
2013-08-21 19:02:51 +04:00
|
|
|
|
2014-07-09 13:53:01 +04:00
|
|
|
static gboolean
|
|
|
|
aio_ctx_prepare(GSource *source, gint *timeout)
|
|
|
|
{
|
|
|
|
AioContext *ctx = (AioContext *) source;
|
|
|
|
|
async: use explicit memory barriers
When using C11 atomics, non-seqcst reads and writes do not participate
in the total order of seqcst operations. In util/async.c and util/aio-posix.c,
in particular, the pattern that we use
write ctx->notify_me write bh->scheduled
read bh->scheduled read ctx->notify_me
if !bh->scheduled, sleep if ctx->notify_me, notify
needs to use seqcst operations for both the write and the read. In
general this is something that we do not want, because there can be
many sources that are polled in addition to bottom halves. The
alternative is to place a seqcst memory barrier between the write
and the read. This also comes with a disadvantage, in that the
memory barrier is implicit on strongly-ordered architectures and
it wastes a few dozen clock cycles.
Fortunately, ctx->notify_me is never written concurrently by two
threads, so we can assert that and relax the writes to ctx->notify_me.
The resulting solution works and performs well on both aarch64 and x86.
Note that the atomic_set/atomic_read combination is not an atomic
read-modify-write, and therefore it is even weaker than C11 ATOMIC_RELAXED;
on x86, ATOMIC_RELAXED compiles to a locked operation.
Analyzed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Ying Fang <fangying1@huawei.com>
Message-Id: <20200407140746.8041-6-pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2020-04-07 17:07:46 +03:00
|
|
|
atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) | 1);
|
|
|
|
|
|
|
|
/*
|
|
|
|
* Write ctx->notify_me before computing the timeout
|
|
|
|
* (reading bottom half flags, etc.). Pairs with
|
|
|
|
* smp_mb in aio_notify().
|
|
|
|
*/
|
|
|
|
smp_mb();
|
AioContext: fix broken ctx->dispatching optimization
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>
2015-07-21 17:07:51 +03:00
|
|
|
|
2014-07-09 13:53:01 +04:00
|
|
|
/* We assume there is no timeout already supplied */
|
|
|
|
*timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx));
|
2014-07-09 13:53:08 +04:00
|
|
|
|
|
|
|
if (aio_prepare(ctx)) {
|
|
|
|
*timeout = 0;
|
|
|
|
}
|
|
|
|
|
2014-07-09 13:53:01 +04:00
|
|
|
return *timeout == 0;
|
2012-09-24 16:57:41 +04:00
|
|
|
}
|
|
|
|
|
|
|
|
static gboolean
|
|
|
|
aio_ctx_check(GSource *source)
|
|
|
|
{
|
|
|
|
AioContext *ctx = (AioContext *) source;
|
|
|
|
QEMUBH *bh;
|
2020-02-21 12:39:51 +03:00
|
|
|
BHListSlice *s;
|
2012-09-24 16:57:41 +04:00
|
|
|
|
async: use explicit memory barriers
When using C11 atomics, non-seqcst reads and writes do not participate
in the total order of seqcst operations. In util/async.c and util/aio-posix.c,
in particular, the pattern that we use
write ctx->notify_me write bh->scheduled
read bh->scheduled read ctx->notify_me
if !bh->scheduled, sleep if ctx->notify_me, notify
needs to use seqcst operations for both the write and the read. In
general this is something that we do not want, because there can be
many sources that are polled in addition to bottom halves. The
alternative is to place a seqcst memory barrier between the write
and the read. This also comes with a disadvantage, in that the
memory barrier is implicit on strongly-ordered architectures and
it wastes a few dozen clock cycles.
Fortunately, ctx->notify_me is never written concurrently by two
threads, so we can assert that and relax the writes to ctx->notify_me.
The resulting solution works and performs well on both aarch64 and x86.
Note that the atomic_set/atomic_read combination is not an atomic
read-modify-write, and therefore it is even weaker than C11 ATOMIC_RELAXED;
on x86, ATOMIC_RELAXED compiles to a locked operation.
Analyzed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Ying Fang <fangying1@huawei.com>
Message-Id: <20200407140746.8041-6-pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2020-04-07 17:07:46 +03:00
|
|
|
/* Finish computing the timeout before clearing the flag. */
|
|
|
|
atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) & ~1);
|
2015-07-21 17:07:53 +03:00
|
|
|
aio_notify_accept(ctx);
|
2015-07-21 17:07:52 +03:00
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
QSLIST_FOREACH_RCU(bh, &ctx->bh_list, next) {
|
|
|
|
if ((bh->flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
|
2012-09-24 16:57:41 +04:00
|
|
|
return true;
|
2016-07-14 16:10:43 +03:00
|
|
|
}
|
2012-09-24 16:57:41 +04:00
|
|
|
}
|
2020-02-21 12:39:51 +03:00
|
|
|
|
|
|
|
QSIMPLEQ_FOREACH(s, &ctx->bh_slice_list, next) {
|
|
|
|
QSLIST_FOREACH_RCU(bh, &s->bh_list, next) {
|
|
|
|
if ((bh->flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
2013-08-21 19:02:51 +04:00
|
|
|
return aio_pending(ctx) || (timerlistgroup_deadline_ns(&ctx->tlg) == 0);
|
2012-09-24 16:57:41 +04:00
|
|
|
}
|
|
|
|
|
|
|
|
static gboolean
|
|
|
|
aio_ctx_dispatch(GSource *source,
|
|
|
|
GSourceFunc callback,
|
|
|
|
gpointer user_data)
|
|
|
|
{
|
|
|
|
AioContext *ctx = (AioContext *) source;
|
|
|
|
|
|
|
|
assert(callback == NULL);
|
2017-02-13 16:52:33 +03:00
|
|
|
aio_dispatch(ctx);
|
2012-09-24 16:57:41 +04:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
2012-09-24 20:44:14 +04:00
|
|
|
static void
|
|
|
|
aio_ctx_finalize(GSource *source)
|
|
|
|
{
|
|
|
|
AioContext *ctx = (AioContext *) source;
|
2020-02-21 12:39:51 +03:00
|
|
|
QEMUBH *bh;
|
|
|
|
unsigned flags;
|
2012-09-24 20:44:14 +04:00
|
|
|
|
2013-03-07 16:41:47 +04:00
|
|
|
thread_pool_free(ctx->thread_pool);
|
2015-07-28 19:34:08 +03:00
|
|
|
|
2016-07-04 19:33:20 +03:00
|
|
|
#ifdef CONFIG_LINUX_AIO
|
|
|
|
if (ctx->linux_aio) {
|
|
|
|
laio_detach_aio_context(ctx->linux_aio, ctx);
|
|
|
|
laio_cleanup(ctx->linux_aio);
|
|
|
|
ctx->linux_aio = NULL;
|
|
|
|
}
|
|
|
|
#endif
|
|
|
|
|
2020-01-20 17:18:49 +03:00
|
|
|
#ifdef CONFIG_LINUX_IO_URING
|
|
|
|
if (ctx->linux_io_uring) {
|
|
|
|
luring_detach_aio_context(ctx->linux_io_uring, ctx);
|
|
|
|
luring_cleanup(ctx->linux_io_uring);
|
|
|
|
ctx->linux_io_uring = NULL;
|
|
|
|
}
|
|
|
|
#endif
|
|
|
|
|
2017-02-13 16:52:19 +03:00
|
|
|
assert(QSLIST_EMPTY(&ctx->scheduled_coroutines));
|
|
|
|
qemu_bh_delete(ctx->co_schedule_bh);
|
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
/* There must be no aio_bh_poll() calls going on */
|
|
|
|
assert(QSIMPLEQ_EMPTY(&ctx->bh_slice_list));
|
2015-07-28 19:34:08 +03:00
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
while ((bh = aio_bh_dequeue(&ctx->bh_list, &flags))) {
|
2015-07-28 19:34:08 +03:00
|
|
|
/* qemu_bh_delete() must have been called on BHs in this AioContext */
|
2020-02-21 12:39:51 +03:00
|
|
|
assert(flags & BH_DELETED);
|
2015-07-28 19:34:08 +03:00
|
|
|
|
2020-02-21 12:39:51 +03:00
|
|
|
g_free(bh);
|
2015-07-28 19:34:08 +03:00
|
|
|
}
|
|
|
|
|
2016-12-01 22:26:41 +03:00
|
|
|
aio_set_event_notifier(ctx, &ctx->notifier, false, NULL, NULL);
|
2012-09-24 20:44:14 +04:00
|
|
|
event_notifier_cleanup(&ctx->notifier);
|
2016-10-27 13:49:08 +03:00
|
|
|
qemu_rec_mutex_destroy(&ctx->lock);
|
2017-01-12 21:07:53 +03:00
|
|
|
qemu_lockcnt_destroy(&ctx->list_lock);
|
2013-08-21 19:02:49 +04:00
|
|
|
timerlistgroup_deinit(&ctx->tlg);
|
2018-05-17 03:42:43 +03:00
|
|
|
aio_context_destroy(ctx);
|
2012-09-24 20:44:14 +04:00
|
|
|
}
|
|
|
|
|
2012-09-24 16:57:41 +04:00
|
|
|
static GSourceFuncs aio_source_funcs = {
|
|
|
|
aio_ctx_prepare,
|
|
|
|
aio_ctx_check,
|
|
|
|
aio_ctx_dispatch,
|
2012-09-24 20:44:14 +04:00
|
|
|
aio_ctx_finalize
|
2012-09-24 16:57:41 +04:00
|
|
|
};
|
|
|
|
|
|
|
|
GSource *aio_get_g_source(AioContext *ctx)
|
|
|
|
{
|
|
|
|
g_source_ref(&ctx->source);
|
|
|
|
return &ctx->source;
|
|
|
|
}
|
2012-09-13 14:28:51 +04:00
|
|
|
|
2013-03-07 16:41:47 +04:00
|
|
|
ThreadPool *aio_get_thread_pool(AioContext *ctx)
|
|
|
|
{
|
|
|
|
if (!ctx->thread_pool) {
|
|
|
|
ctx->thread_pool = thread_pool_new(ctx);
|
|
|
|
}
|
|
|
|
return ctx->thread_pool;
|
|
|
|
}
|
|
|
|
|
2016-07-04 19:33:20 +03:00
|
|
|
#ifdef CONFIG_LINUX_AIO
|
linux-aio: properly bubble up errors from initialization
laio_init() can fail for a couple of reasons, which will lead to a NULL
pointer dereference in laio_attach_aio_context().
To solve this, add a aio_setup_linux_aio() function which is called
early in raw_open_common. If this fails, propagate the error up. The
signature of aio_get_linux_aio() was not modified, because it seems
preferable to return the actual errno from the possible failing
initialization calls.
Additionally, when the AioContext changes, we need to associate a
LinuxAioState with the new AioContext. Use the bdrv_attach_aio_context
callback and call the new aio_setup_linux_aio(), which will allocate a
new AioContext if needed, and return errors on failures. If it fails for
any reason, fallback to threaded AIO with an error message, as the
device is already in-use by the guest.
Add an assert that aio_get_linux_aio() cannot return NULL.
Signed-off-by: Nishanth Aravamudan <naravamudan@digitalocean.com>
Message-id: 20180622193700.6523-1-naravamudan@digitalocean.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2018-06-22 22:37:00 +03:00
|
|
|
LinuxAioState *aio_setup_linux_aio(AioContext *ctx, Error **errp)
|
2016-07-04 19:33:20 +03:00
|
|
|
{
|
|
|
|
if (!ctx->linux_aio) {
|
linux-aio: properly bubble up errors from initialization
laio_init() can fail for a couple of reasons, which will lead to a NULL
pointer dereference in laio_attach_aio_context().
To solve this, add a aio_setup_linux_aio() function which is called
early in raw_open_common. If this fails, propagate the error up. The
signature of aio_get_linux_aio() was not modified, because it seems
preferable to return the actual errno from the possible failing
initialization calls.
Additionally, when the AioContext changes, we need to associate a
LinuxAioState with the new AioContext. Use the bdrv_attach_aio_context
callback and call the new aio_setup_linux_aio(), which will allocate a
new AioContext if needed, and return errors on failures. If it fails for
any reason, fallback to threaded AIO with an error message, as the
device is already in-use by the guest.
Add an assert that aio_get_linux_aio() cannot return NULL.
Signed-off-by: Nishanth Aravamudan <naravamudan@digitalocean.com>
Message-id: 20180622193700.6523-1-naravamudan@digitalocean.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2018-06-22 22:37:00 +03:00
|
|
|
ctx->linux_aio = laio_init(errp);
|
|
|
|
if (ctx->linux_aio) {
|
|
|
|
laio_attach_aio_context(ctx->linux_aio, ctx);
|
|
|
|
}
|
2016-07-04 19:33:20 +03:00
|
|
|
}
|
|
|
|
return ctx->linux_aio;
|
|
|
|
}
|
linux-aio: properly bubble up errors from initialization
laio_init() can fail for a couple of reasons, which will lead to a NULL
pointer dereference in laio_attach_aio_context().
To solve this, add a aio_setup_linux_aio() function which is called
early in raw_open_common. If this fails, propagate the error up. The
signature of aio_get_linux_aio() was not modified, because it seems
preferable to return the actual errno from the possible failing
initialization calls.
Additionally, when the AioContext changes, we need to associate a
LinuxAioState with the new AioContext. Use the bdrv_attach_aio_context
callback and call the new aio_setup_linux_aio(), which will allocate a
new AioContext if needed, and return errors on failures. If it fails for
any reason, fallback to threaded AIO with an error message, as the
device is already in-use by the guest.
Add an assert that aio_get_linux_aio() cannot return NULL.
Signed-off-by: Nishanth Aravamudan <naravamudan@digitalocean.com>
Message-id: 20180622193700.6523-1-naravamudan@digitalocean.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2018-06-22 22:37:00 +03:00
|
|
|
|
|
|
|
LinuxAioState *aio_get_linux_aio(AioContext *ctx)
|
|
|
|
{
|
|
|
|
assert(ctx->linux_aio);
|
|
|
|
return ctx->linux_aio;
|
|
|
|
}
|
2016-07-04 19:33:20 +03:00
|
|
|
#endif
|
|
|
|
|
2020-01-20 17:18:49 +03:00
|
|
|
#ifdef CONFIG_LINUX_IO_URING
|
|
|
|
LuringState *aio_setup_linux_io_uring(AioContext *ctx, Error **errp)
|
|
|
|
{
|
|
|
|
if (ctx->linux_io_uring) {
|
|
|
|
return ctx->linux_io_uring;
|
|
|
|
}
|
|
|
|
|
|
|
|
ctx->linux_io_uring = luring_init(errp);
|
|
|
|
if (!ctx->linux_io_uring) {
|
|
|
|
return NULL;
|
|
|
|
}
|
|
|
|
|
|
|
|
luring_attach_aio_context(ctx->linux_io_uring, ctx);
|
|
|
|
return ctx->linux_io_uring;
|
|
|
|
}
|
|
|
|
|
|
|
|
LuringState *aio_get_linux_io_uring(AioContext *ctx)
|
|
|
|
{
|
|
|
|
assert(ctx->linux_io_uring);
|
|
|
|
return ctx->linux_io_uring;
|
|
|
|
}
|
|
|
|
#endif
|
|
|
|
|
2012-09-24 20:44:14 +04:00
|
|
|
void aio_notify(AioContext *ctx)
|
|
|
|
{
|
AioContext: fix broken ctx->dispatching optimization
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>
2015-07-21 17:07:51 +03:00
|
|
|
/* Write e.g. bh->scheduled before reading ctx->notify_me. Pairs
|
async: use explicit memory barriers
When using C11 atomics, non-seqcst reads and writes do not participate
in the total order of seqcst operations. In util/async.c and util/aio-posix.c,
in particular, the pattern that we use
write ctx->notify_me write bh->scheduled
read bh->scheduled read ctx->notify_me
if !bh->scheduled, sleep if ctx->notify_me, notify
needs to use seqcst operations for both the write and the read. In
general this is something that we do not want, because there can be
many sources that are polled in addition to bottom halves. The
alternative is to place a seqcst memory barrier between the write
and the read. This also comes with a disadvantage, in that the
memory barrier is implicit on strongly-ordered architectures and
it wastes a few dozen clock cycles.
Fortunately, ctx->notify_me is never written concurrently by two
threads, so we can assert that and relax the writes to ctx->notify_me.
The resulting solution works and performs well on both aarch64 and x86.
Note that the atomic_set/atomic_read combination is not an atomic
read-modify-write, and therefore it is even weaker than C11 ATOMIC_RELAXED;
on x86, ATOMIC_RELAXED compiles to a locked operation.
Analyzed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Ying Fang <fangying1@huawei.com>
Message-Id: <20200407140746.8041-6-pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2020-04-07 17:07:46 +03:00
|
|
|
* with smp_mb in aio_ctx_prepare or aio_poll.
|
AioContext: fix broken ctx->dispatching optimization
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>
2015-07-21 17:07:51 +03:00
|
|
|
*/
|
2014-07-07 17:18:04 +04:00
|
|
|
smp_mb();
|
async: use explicit memory barriers
When using C11 atomics, non-seqcst reads and writes do not participate
in the total order of seqcst operations. In util/async.c and util/aio-posix.c,
in particular, the pattern that we use
write ctx->notify_me write bh->scheduled
read bh->scheduled read ctx->notify_me
if !bh->scheduled, sleep if ctx->notify_me, notify
needs to use seqcst operations for both the write and the read. In
general this is something that we do not want, because there can be
many sources that are polled in addition to bottom halves. The
alternative is to place a seqcst memory barrier between the write
and the read. This also comes with a disadvantage, in that the
memory barrier is implicit on strongly-ordered architectures and
it wastes a few dozen clock cycles.
Fortunately, ctx->notify_me is never written concurrently by two
threads, so we can assert that and relax the writes to ctx->notify_me.
The resulting solution works and performs well on both aarch64 and x86.
Note that the atomic_set/atomic_read combination is not an atomic
read-modify-write, and therefore it is even weaker than C11 ATOMIC_RELAXED;
on x86, ATOMIC_RELAXED compiles to a locked operation.
Analyzed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Ying Fang <fangying1@huawei.com>
Message-Id: <20200407140746.8041-6-pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2020-04-07 17:07:46 +03:00
|
|
|
if (atomic_read(&ctx->notify_me)) {
|
2014-07-07 17:18:04 +04:00
|
|
|
event_notifier_set(&ctx->notifier);
|
2015-07-21 17:07:53 +03:00
|
|
|
atomic_mb_set(&ctx->notified, true);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
void aio_notify_accept(AioContext *ctx)
|
|
|
|
{
|
2019-10-01 16:26:09 +03:00
|
|
|
if (atomic_xchg(&ctx->notified, false)
|
|
|
|
#ifdef WIN32
|
|
|
|
|| true
|
|
|
|
#endif
|
|
|
|
) {
|
2015-07-21 17:07:53 +03:00
|
|
|
event_notifier_test_and_clear(&ctx->notifier);
|
2014-07-07 17:18:04 +04:00
|
|
|
}
|
2012-09-24 20:44:14 +04:00
|
|
|
}
|
|
|
|
|
2017-03-03 13:50:29 +03:00
|
|
|
static void aio_timerlist_notify(void *opaque, QEMUClockType type)
|
2013-08-21 19:02:50 +04:00
|
|
|
{
|
|
|
|
aio_notify(opaque);
|
|
|
|
}
|
|
|
|
|
2015-07-21 17:07:52 +03:00
|
|
|
static void event_notifier_dummy_cb(EventNotifier *e)
|
|
|
|
{
|
|
|
|
}
|
|
|
|
|
2016-12-01 22:26:42 +03:00
|
|
|
/* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
|
|
|
|
static bool event_notifier_poll(void *opaque)
|
|
|
|
{
|
|
|
|
EventNotifier *e = opaque;
|
|
|
|
AioContext *ctx = container_of(e, AioContext, notifier);
|
|
|
|
|
|
|
|
return atomic_read(&ctx->notified);
|
|
|
|
}
|
|
|
|
|
2017-02-13 16:52:19 +03:00
|
|
|
static void co_schedule_bh_cb(void *opaque)
|
|
|
|
{
|
|
|
|
AioContext *ctx = opaque;
|
|
|
|
QSLIST_HEAD(, Coroutine) straight, reversed;
|
|
|
|
|
|
|
|
QSLIST_MOVE_ATOMIC(&reversed, &ctx->scheduled_coroutines);
|
|
|
|
QSLIST_INIT(&straight);
|
|
|
|
|
|
|
|
while (!QSLIST_EMPTY(&reversed)) {
|
|
|
|
Coroutine *co = QSLIST_FIRST(&reversed);
|
|
|
|
QSLIST_REMOVE_HEAD(&reversed, co_scheduled_next);
|
|
|
|
QSLIST_INSERT_HEAD(&straight, co, co_scheduled_next);
|
|
|
|
}
|
|
|
|
|
|
|
|
while (!QSLIST_EMPTY(&straight)) {
|
|
|
|
Coroutine *co = QSLIST_FIRST(&straight);
|
|
|
|
QSLIST_REMOVE_HEAD(&straight, co_scheduled_next);
|
|
|
|
trace_aio_co_schedule_bh_cb(ctx, co);
|
2017-02-13 16:52:31 +03:00
|
|
|
aio_context_acquire(ctx);
|
2017-11-18 06:27:09 +03:00
|
|
|
|
|
|
|
/* Protected by write barrier in qemu_aio_coroutine_enter */
|
|
|
|
atomic_set(&co->scheduled, NULL);
|
util/async: use qemu_aio_coroutine_enter in co_schedule_bh_cb
AIO Coroutines shouldn't by managed by an AioContext different than the
one assigned when they are created. aio_co_enter avoids entering a
coroutine from a different AioContext, calling aio_co_schedule instead.
Scheduled coroutines are then entered by co_schedule_bh_cb using
qemu_coroutine_enter, which just calls qemu_aio_coroutine_enter with the
current AioContext obtained with qemu_get_current_aio_context.
Eventually, co->ctx will be set to the AioContext passed as an argument
to qemu_aio_coroutine_enter.
This means that, if an IO Thread's AioConext is being processed by the
Main Thread (due to aio_poll being called with a BDS AioContext, as it
happens in AIO_WAIT_WHILE among other places), the AioContext from some
coroutines may be wrongly replaced with the one from the Main Thread.
This is the root cause behind some crashes, mainly triggered by the
drain code at block/io.c. The most common are these abort and failed
assertion:
util/async.c:aio_co_schedule
456 if (scheduled) {
457 fprintf(stderr,
458 "%s: Co-routine was already scheduled in '%s'\n",
459 __func__, scheduled);
460 abort();
461 }
util/qemu-coroutine-lock.c:
286 assert(mutex->holder == self);
But it's also known to cause random errors at different locations, and
even SIGSEGV with broken coroutine backtraces.
By using qemu_aio_coroutine_enter directly in co_schedule_bh_cb, we can
pass the correct AioContext as an argument, making sure co->ctx is not
wrongly altered.
Signed-off-by: Sergio Lopez <slp@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-09-05 12:33:51 +03:00
|
|
|
qemu_aio_coroutine_enter(ctx, co);
|
2017-02-13 16:52:31 +03:00
|
|
|
aio_context_release(ctx);
|
2017-02-13 16:52:19 +03:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-09-18 15:30:49 +04:00
|
|
|
AioContext *aio_context_new(Error **errp)
|
2012-10-30 02:45:23 +04:00
|
|
|
{
|
2014-09-18 15:30:49 +04:00
|
|
|
int ret;
|
2012-09-24 20:44:14 +04:00
|
|
|
AioContext *ctx;
|
2015-10-30 07:06:28 +03:00
|
|
|
|
2012-09-24 20:44:14 +04:00
|
|
|
ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
|
2020-02-21 12:39:51 +03:00
|
|
|
QSLIST_INIT(&ctx->bh_list);
|
|
|
|
QSIMPLEQ_INIT(&ctx->bh_slice_list);
|
2016-07-15 13:28:44 +03:00
|
|
|
aio_context_setup(ctx);
|
|
|
|
|
2014-09-18 15:30:49 +04:00
|
|
|
ret = event_notifier_init(&ctx->notifier, false);
|
|
|
|
if (ret < 0) {
|
|
|
|
error_setg_errno(errp, -ret, "Failed to initialize event notifier");
|
2015-10-30 07:06:28 +03:00
|
|
|
goto fail;
|
2014-09-18 15:30:49 +04:00
|
|
|
}
|
2014-12-17 18:09:58 +03:00
|
|
|
g_source_set_can_recurse(&ctx->source, true);
|
2017-01-12 21:07:53 +03:00
|
|
|
qemu_lockcnt_init(&ctx->list_lock);
|
2017-02-13 16:52:19 +03:00
|
|
|
|
|
|
|
ctx->co_schedule_bh = aio_bh_new(ctx, co_schedule_bh_cb, ctx);
|
|
|
|
QSLIST_INIT(&ctx->scheduled_coroutines);
|
|
|
|
|
2014-09-18 15:30:49 +04:00
|
|
|
aio_set_event_notifier(ctx, &ctx->notifier,
|
2015-10-23 06:08:05 +03:00
|
|
|
false,
|
2016-12-01 22:26:41 +03:00
|
|
|
event_notifier_dummy_cb,
|
2016-12-01 22:26:42 +03:00
|
|
|
event_notifier_poll);
|
2016-07-04 19:33:20 +03:00
|
|
|
#ifdef CONFIG_LINUX_AIO
|
|
|
|
ctx->linux_aio = NULL;
|
|
|
|
#endif
|
2020-01-20 17:18:49 +03:00
|
|
|
|
|
|
|
#ifdef CONFIG_LINUX_IO_URING
|
|
|
|
ctx->linux_io_uring = NULL;
|
|
|
|
#endif
|
|
|
|
|
2013-03-07 16:41:47 +04:00
|
|
|
ctx->thread_pool = NULL;
|
2016-10-27 13:49:08 +03:00
|
|
|
qemu_rec_mutex_init(&ctx->lock);
|
2013-08-21 19:02:50 +04:00
|
|
|
timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx);
|
2012-09-24 20:44:14 +04:00
|
|
|
|
2016-12-01 22:26:51 +03:00
|
|
|
ctx->poll_ns = 0;
|
2016-12-01 22:26:42 +03:00
|
|
|
ctx->poll_max_ns = 0;
|
2016-12-01 22:26:51 +03:00
|
|
|
ctx->poll_grow = 0;
|
|
|
|
ctx->poll_shrink = 0;
|
2016-12-01 22:26:42 +03:00
|
|
|
|
2012-09-24 20:44:14 +04:00
|
|
|
return ctx;
|
2015-10-30 07:06:28 +03:00
|
|
|
fail:
|
|
|
|
g_source_destroy(&ctx->source);
|
|
|
|
return NULL;
|
2012-09-24 16:57:41 +04:00
|
|
|
}
|
|
|
|
|
2017-02-13 16:52:19 +03:00
|
|
|
void aio_co_schedule(AioContext *ctx, Coroutine *co)
|
|
|
|
{
|
|
|
|
trace_aio_co_schedule(ctx, co);
|
2017-11-18 06:27:09 +03:00
|
|
|
const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL,
|
|
|
|
__func__);
|
|
|
|
|
|
|
|
if (scheduled) {
|
|
|
|
fprintf(stderr,
|
|
|
|
"%s: Co-routine was already scheduled in '%s'\n",
|
|
|
|
__func__, scheduled);
|
|
|
|
abort();
|
|
|
|
}
|
|
|
|
|
2019-07-23 22:06:23 +03:00
|
|
|
/* The coroutine might run and release the last ctx reference before we
|
|
|
|
* invoke qemu_bh_schedule(). Take a reference to keep ctx alive until
|
|
|
|
* we're done.
|
|
|
|
*/
|
|
|
|
aio_context_ref(ctx);
|
|
|
|
|
2017-02-13 16:52:19 +03:00
|
|
|
QSLIST_INSERT_HEAD_ATOMIC(&ctx->scheduled_coroutines,
|
|
|
|
co, co_scheduled_next);
|
|
|
|
qemu_bh_schedule(ctx->co_schedule_bh);
|
2019-07-23 22:06:23 +03:00
|
|
|
|
|
|
|
aio_context_unref(ctx);
|
2017-02-13 16:52:19 +03:00
|
|
|
}
|
|
|
|
|
|
|
|
void aio_co_wake(struct Coroutine *co)
|
|
|
|
{
|
|
|
|
AioContext *ctx;
|
|
|
|
|
|
|
|
/* Read coroutine before co->ctx. Matches smp_wmb in
|
|
|
|
* qemu_coroutine_enter.
|
|
|
|
*/
|
|
|
|
smp_read_barrier_depends();
|
|
|
|
ctx = atomic_read(&co->ctx);
|
|
|
|
|
2017-04-10 15:07:35 +03:00
|
|
|
aio_co_enter(ctx, co);
|
|
|
|
}
|
|
|
|
|
|
|
|
void aio_co_enter(AioContext *ctx, struct Coroutine *co)
|
|
|
|
{
|
2017-02-13 16:52:19 +03:00
|
|
|
if (ctx != qemu_get_current_aio_context()) {
|
|
|
|
aio_co_schedule(ctx, co);
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (qemu_in_coroutine()) {
|
|
|
|
Coroutine *self = qemu_coroutine_self();
|
|
|
|
assert(self != co);
|
|
|
|
QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, co, co_queue_next);
|
|
|
|
} else {
|
|
|
|
aio_context_acquire(ctx);
|
2017-04-10 15:07:35 +03:00
|
|
|
qemu_aio_coroutine_enter(ctx, co);
|
2017-02-13 16:52:19 +03:00
|
|
|
aio_context_release(ctx);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-09-24 16:57:41 +04:00
|
|
|
void aio_context_ref(AioContext *ctx)
|
|
|
|
{
|
|
|
|
g_source_ref(&ctx->source);
|
|
|
|
}
|
|
|
|
|
|
|
|
void aio_context_unref(AioContext *ctx)
|
|
|
|
{
|
|
|
|
g_source_unref(&ctx->source);
|
2012-10-30 02:45:23 +04:00
|
|
|
}
|
2014-03-03 14:30:04 +04:00
|
|
|
|
|
|
|
void aio_context_acquire(AioContext *ctx)
|
|
|
|
{
|
2016-10-27 13:49:08 +03:00
|
|
|
qemu_rec_mutex_lock(&ctx->lock);
|
2014-03-03 14:30:04 +04:00
|
|
|
}
|
|
|
|
|
|
|
|
void aio_context_release(AioContext *ctx)
|
|
|
|
{
|
2016-10-27 13:49:08 +03:00
|
|
|
qemu_rec_mutex_unlock(&ctx->lock);
|
2014-03-03 14:30:04 +04:00
|
|
|
}
|