| Message ID | 1864070a-2f84-1d98-341e-f01ddf74ec4b@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Bug,1805256] Re: qemu_futex_wait() lockups in ARM64: 2 possible issues | expand |
On Wed, Sep 11, 2019 at 04:09:25PM -0300, Rafael David Tinoco wrote: > > Zhengui's theory that notify_me doesn't work properly on ARM is more > > promising, but he couldn't provide a clear explanation of why he thought > > notify_me is involved. In particular, I would have expected notify_me to > > be wrong if the qemu_poll_ns call came from aio_ctx_dispatch, for example: > > > > > > glib_pollfds_fill > > g_main_context_prepare > > aio_ctx_prepare > > atomic_or(&ctx->notify_me, 1) > > qemu_poll_ns > > glib_pollfds_poll > > g_main_context_check > > aio_ctx_check > > atomic_and(&ctx->notify_me, ~1) > > g_main_context_dispatch > > aio_ctx_dispatch > > /* do something for event */ > > qemu_poll_ns > > > > Paolo, > > I tried confining execution in a single NUMA domain (cpu & mem) and > still faced the issue, then, I added a mutex "ctx->notify_me_lcktest" > into context to protect "ctx->notify_me", like showed bellow, and it > seems to have either fixed or mitigated it. > > I was able to cause the hung once every 3 or 4 runs. I have already ran > qemu-img convert more than 30 times now and couldn't reproduce it again. > > Next step is to play with the barriers and check why existing ones > aren't enough for ordering access to ctx->notify_me ... or should I > try/do something else in your opinion ? > > This arch/machine (Huawei D06): > > $ lscpu > Architecture: aarch64 > Byte Order: Little Endian > CPU(s): 96 > On-line CPU(s) list: 0-95 > Thread(s) per core: 1 > Core(s) per socket: 48 > Socket(s): 2 > NUMA node(s): 4 > Vendor ID: 0x48 > Model: 0 > Stepping: 0x0 > CPU max MHz: 2000.0000 > CPU min MHz: 200.0000 > BogoMIPS: 200.00 > L1d cache: 64K > L1i cache: 64K > L2 cache: 512K > L3 cache: 32768K > NUMA node0 CPU(s): 0-23 > NUMA node1 CPU(s): 24-47 > NUMA node2 CPU(s): 48-71 > NUMA node3 CPU(s): 72-95 > Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics > cpuid asimdrdm dcpop Note that I'm also seeing this on a ThunderX2 (same calltrace): $ lscpu Architecture: aarch64 Byte Order: Little Endian CPU(s): 224 On-line CPU(s) list: 0-223 Thread(s) per core: 4 Core(s) per socket: 28 Socket(s): 2 NUMA node(s): 2 Vendor ID: Cavium Model: 1 Model name: ThunderX2 99xx Stepping: 0x1 BogoMIPS: 400.00 L1d cache: 32K L1i cache: 32K L2 cache: 256K L3 cache: 32768K NUMA node0 CPU(s): 0-111 NUMA node1 CPU(s): 112-223 Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics cpuid asimdrdm -dann > ---- > > diff --git a/include/block/aio.h b/include/block/aio.h > index 0ca25dfec6..0724086d91 100644 > --- a/include/block/aio.h > +++ b/include/block/aio.h > @@ -84,6 +84,7 @@ struct AioContext { > * dispatch phase, hence a simple counter is enough for them. > */ > uint32_t notify_me; > + QemuMutex notify_me_lcktest; > > /* A lock to protect between QEMUBH and AioHandler adders and deleter, > * and to ensure that no callbacks are removed while we're walking and > diff --git a/util/aio-posix.c b/util/aio-posix.c > index 51c41ed3c9..031d6e2997 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -529,7 +529,9 @@ static bool run_poll_handlers(AioContext *ctx, > int64_t max_ns, int64_t *timeout) > bool progress; > int64_t start_time, elapsed_time; > > + qemu_mutex_lock(&ctx->notify_me_lcktest); > assert(ctx->notify_me); > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > assert(qemu_lockcnt_count(&ctx->list_lock) > 0); > > trace_run_poll_handlers_begin(ctx, max_ns, *timeout); > @@ -601,8 +603,10 @@ bool aio_poll(AioContext *ctx, bool blocking) > * so disable the optimization now. > */ > if (blocking) { > + qemu_mutex_lock(&ctx->notify_me_lcktest); > assert(in_aio_context_home_thread(ctx)); > atomic_add(&ctx->notify_me, 2); > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > } > > qemu_lockcnt_inc(&ctx->list_lock); > @@ -647,8 +651,10 @@ bool aio_poll(AioContext *ctx, bool blocking) > } > > if (blocking) { > + qemu_mutex_lock(&ctx->notify_me_lcktest); > atomic_sub(&ctx->notify_me, 2); > aio_notify_accept(ctx); > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > } > > /* Adjust polling time */ > diff --git a/util/async.c b/util/async.c > index c10642a385..140e1e86f5 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -221,7 +221,9 @@ aio_ctx_prepare(GSource *source, gint *timeout) > { > AioContext *ctx = (AioContext *) source; > > + qemu_mutex_lock(&ctx->notify_me_lcktest); > atomic_or(&ctx->notify_me, 1); > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > > /* We assume there is no timeout already supplied */ > *timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)); > @@ -239,8 +241,10 @@ aio_ctx_check(GSource *source) > AioContext *ctx = (AioContext *) source; > QEMUBH *bh; > > + qemu_mutex_lock(&ctx->notify_me_lcktest); > atomic_and(&ctx->notify_me, ~1); > aio_notify_accept(ctx); > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > > for (bh = ctx->first_bh; bh; bh = bh->next) { > if (bh->scheduled) { > @@ -346,11 +350,13 @@ void aio_notify(AioContext *ctx) > /* 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(); > + //smp_mb(); > + qemu_mutex_lock(&ctx->notify_me_lcktest); > if (ctx->notify_me) { > event_notifier_set(&ctx->notifier); > atomic_mb_set(&ctx->notified, true); > } > + qemu_mutex_unlock(&ctx->notify_me_lcktest); > } > > void aio_notify_accept(AioContext *ctx) > @@ -424,6 +430,8 @@ AioContext *aio_context_new(Error **errp) > ctx->co_schedule_bh = aio_bh_new(ctx, co_schedule_bh_cb, ctx); > QSLIST_INIT(&ctx->scheduled_coroutines); > > + qemu_rec_mutex_init(&ctx->notify_me_lcktest); > + > aio_set_event_notifier(ctx, &ctx->notifier, > false, > (EventNotifierHandler *) >
I've looked into this on ThunderX2. The arm64 code generated for the
atomic_[add|sub] accesses of ctx->notify_me doesn't contain any
memory barriers. It is just plain ldaxr/stlxr.
From my understanding this is not sufficient for SMP sync.
If I read this comment correct:
void aio_notify(AioContext *ctx)
{
/* 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->notify_me) {
it points out that the smp_mb() should be paired. But as
I said the used atomics don't generate any barriers at all.
I've tried to verify me theory with this patch and didn't run into the
issue for ~500 iterations (usually I would trigger the issue ~20 iterations).
--Jan
diff --git a/util/aio-posix.c b/util/aio-posix.c
index d8f0cb4af8dd..d07dcd4e9993 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -591,6 +591,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
*/
if (blocking) {
atomic_add(&ctx->notify_me, 2);
+ smp_mb();
}
qemu_lockcnt_inc(&ctx->list_lock);
@@ -632,6 +633,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
if (blocking) {
atomic_sub(&ctx->notify_me, 2);
+ smp_mb();
}
/* Adjust polling time */
diff --git a/util/async.c b/util/async.c
index 4dd9d95a9e73..92ac209c4615 100644
--- a/util/async.c
+++ b/util/async.c
@@ -222,6 +222,7 @@ aio_ctx_prepare(GSource *source, gint *timeout)
AioContext *ctx = (AioContext *) source;
atomic_or(&ctx->notify_me, 1);
+ smp_mb();
/* We assume there is no timeout already supplied */
*timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx));
@@ -240,6 +241,7 @@ aio_ctx_check(GSource *source)
QEMUBH *bh;
atomic_and(&ctx->notify_me, ~1);
+ smp_mb();
aio_notify_accept(ctx);
for (bh = ctx->first_bh; bh; bh = bh->next) {
On 02/10/19 11:23, Jan Glauber wrote: > I've tried to verify me theory with this patch and didn't run into the > issue for ~500 iterations (usually I would trigger the issue ~20 iterations). Awesome! That would be a compiler bug though, as atomic_add and atomic_sub are defined as sequentially consistent: #define atomic_add(ptr, n) ((void) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST)) #define atomic_sub(ptr, n) ((void) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST)) What compiler are you using and what distro? Can you compile util/aio-posix.c with "-fdump-rtl-all -fdump-tree-all", zip the boatload of debugging files and send them my way? Thanks, Paolo
On Wed, Oct 02, 2019 at 11:45:19AM +0200, Paolo Bonzini wrote: > On 02/10/19 11:23, Jan Glauber wrote: > > I've tried to verify me theory with this patch and didn't run into the > > issue for ~500 iterations (usually I would trigger the issue ~20 iterations). > > Awesome! That would be a compiler bug though, as atomic_add and atomic_sub > are defined as sequentially consistent: > > #define atomic_add(ptr, n) ((void) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST)) > #define atomic_sub(ptr, n) ((void) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST)) Compiler bug sounds kind of unlikely... > What compiler are you using and what distro? Can you compile util/aio-posix.c > with "-fdump-rtl-all -fdump-tree-all", zip the boatload of debugging files and > send them my way? This is on Ubuntu 18.04.3, gcc version 7.4.0 (Ubuntu/Linaro 7.4.0-1ubuntu1~18.04.1) I've uploaded the debug files to: https://bugs.launchpad.net/qemu/+bug/1805256/+attachment/5293619/+files/aio-posix.tar.xz Thanks, Jan > Thanks, > > Paolo
On 02/10/19 13:05, Jan Glauber wrote: > The arm64 code generated for the > atomic_[add|sub] accesses of ctx->notify_me doesn't contain any > memory barriers. It is just plain ldaxr/stlxr. > > From my understanding this is not sufficient for SMP sync. > >>> If I read this comment correct: >>> >>> void aio_notify(AioContext *ctx) >>> { >>> /* 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->notify_me) { >>> >>> it points out that the smp_mb() should be paired. But as >>> I said the used atomics don't generate any barriers at all. >> >> Awesome! That would be a compiler bug though, as atomic_add and atomic_sub >> are defined as sequentially consistent: >> >> #define atomic_add(ptr, n) ((void) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST)) >> #define atomic_sub(ptr, n) ((void) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST)) > > Compiler bug sounds kind of unlikely... Indeed the assembly produced by the compiler matches for example the mappings at https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. A small testcase is as follows: int ctx_notify_me; int bh_scheduled; int x() { int one = 1; int ret; __atomic_store(&bh_scheduled, &one, __ATOMIC_RELEASE); // x1 __atomic_thread_fence(__ATOMIC_SEQ_CST); // x2 __atomic_load(&ctx_notify_me, &ret, __ATOMIC_RELAXED); // x3 return ret; } int y() { int ret; __atomic_fetch_add(&ctx_notify_me, 2, __ATOMIC_SEQ_CST); // y1 __atomic_load(&bh_scheduled, &ret, __ATOMIC_RELAXED); // y2 return ret; } Here y (which is aio_poll) wants to order the write to ctx->notify_me before reads of bh->scheduled. However, the processor can speculate the load of bh->scheduled between the load-acquire and store-release of ctx->notify_me. So you can have something like: thread 0 (y) thread 1 (x) ----------------------------------- ----------------------------- y1: load-acq ctx->notify_me y2: load-rlx bh->scheduled x1: store-rel bh->scheduled <-- 1 x2: memory barrier x3: load-rlx ctx->notify_me y1: store-rel ctx->notify_me <-- 2 Being very puzzled, I tried to put this into cppmem: int main() { atomic_int ctx_notify_me = 0; atomic_int bh_scheduled = 0; {{{ { bh_scheduled.store(1, mo_release); atomic_thread_fence(mo_seq_cst); // must be zero since the bug report shows no notification ctx_notify_me.load(mo_relaxed).readsvalue(0); } ||| { ctx_notify_me.store(2, mo_seq_cst); r2=bh_scheduled.load(mo_relaxed); } }}}; return 0; } and much to my surprise, the tool said r2 *can* be 0. Same if I put a CAS like cas_strong_explicit(ctx_notify_me.readsvalue(0), 0, 2, mo_seq_cst, mo_seq_cst); which resembles the code in the test case a bit more. I then found a discussion about using the C11 memory model in Linux (https://gcc.gnu.org/ml/gcc/2014-02/msg00058.html) which contains the following statement, which is a bit disheartening even though it is about a different test: My first gut feeling was that the assertion should never fire, but that was wrong because (as I seem to usually forget) the seq-cst total order is just a constraint but doesn't itself contribute to synchronizes-with -- but this is different for seq-cst fences. and later in the thread: Use of C11 atomics to implement Linux kernel atomic operations requires knowledge of the underlying architecture and the compiler's implementation, as was noted earlier in this thread. Indeed if I add an atomic_thread_fence I get only one valid execution, where r2 must be 1. This is similar to GCC's bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697, and we can fix it in QEMU by using __sync_fetch_and_add; in fact cppmem also shows one valid execution if the store is replaced with something like GCC's assembly for __sync_fetch_and_add (or Linux's assembly for atomic_add_return): cas_strong_explicit(ctx_notify_me.readsvalue(0), 0, 2, mo_release, mo_release); atomic_thread_fence(mo_seq_cst); So we should: 1) understand why ATOMIC_SEQ_CST is not enough in this case. QEMU code seems to be making the same assumptions as Linux about the memory model, and this is wrong because QEMU uses C11 atomics if available. Fortunately, this kind of synchronization in QEMU is relatively rare and only this particular bit seems affected. If there is a fix which stays within the C11 memory model, and does not pessimize code on x86, we can use it[1] and document the pitfall. 2) if there's no way to fix the bug, qemu/atomic.h needs to switch to __sync_fetch_and_add and friends. And again, in this case the difference between the C11 and Linux/QEMU memory models must be documented. Torvald, Will, help me please... :(( Paolo [1] as would be the case if fetch_add was implemented as fetch_add(RELEASE)+thread_fence(SEQ_CST).
On Wed, 2019-10-02 at 15:20 +0200, Paolo Bonzini wrote: > On 02/10/19 13:05, Jan Glauber wrote: > > The arm64 code generated for the > > atomic_[add|sub] accesses of ctx->notify_me doesn't contain any > > memory barriers. It is just plain ldaxr/stlxr. > > > > From my understanding this is not sufficient for SMP sync. > > > > > > If I read this comment correct: > > > > > > > > void aio_notify(AioContext *ctx) > > > > { > > > > /* 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->notify_me) { > > > > > > > > it points out that the smp_mb() should be paired. But as > > > > I said the used atomics don't generate any barriers at all. > > > > > > Awesome! That would be a compiler bug though, as atomic_add and atomic_sub > > > are defined as sequentially consistent: > > > > > > #define atomic_add(ptr, n) ((void) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST)) > > > #define atomic_sub(ptr, n) ((void) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST)) > > > > Compiler bug sounds kind of unlikely... > > Indeed the assembly produced by the compiler matches for example the > mappings at https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. A > small testcase is as follows: > > int ctx_notify_me; > int bh_scheduled; > > int x() > { > int one = 1; > int ret; > __atomic_store(&bh_scheduled, &one, __ATOMIC_RELEASE); // x1 > __atomic_thread_fence(__ATOMIC_SEQ_CST); // x2 > __atomic_load(&ctx_notify_me, &ret, __ATOMIC_RELAXED); // x3 > return ret; > } > > int y() > { > int ret; > __atomic_fetch_add(&ctx_notify_me, 2, __ATOMIC_SEQ_CST); // y1 > __atomic_load(&bh_scheduled, &ret, __ATOMIC_RELAXED); // y2 > return ret; > } > > Here y (which is aio_poll) wants to order the write to ctx->notify_me > before reads of bh->scheduled. However, the processor can speculate the > load of bh->scheduled between the load-acquire and store-release of > ctx->notify_me. So you can have something like: > > thread 0 (y) thread 1 (x) > ----------------------------------- ----------------------------- > y1: load-acq ctx->notify_me > y2: load-rlx bh->scheduled > x1: store-rel bh->scheduled <-- 1 > x2: memory barrier > x3: load-rlx ctx->notify_me > y1: store-rel ctx->notify_me <-- 2 > > Being very puzzled, I tried to put this into cppmem: > > int main() { > atomic_int ctx_notify_me = 0; > atomic_int bh_scheduled = 0; > {{{ { > bh_scheduled.store(1, mo_release); > atomic_thread_fence(mo_seq_cst); > // must be zero since the bug report shows no notification > ctx_notify_me.load(mo_relaxed).readsvalue(0); > } > ||| { > ctx_notify_me.store(2, mo_seq_cst); > r2=bh_scheduled.load(mo_relaxed); > } > }}}; > return 0; > } > > and much to my surprise, the tool said r2 *can* be 0. Same if I put a > CAS like > > cas_strong_explicit(ctx_notify_me.readsvalue(0), 0, 2, > mo_seq_cst, mo_seq_cst); > > which resembles the code in the test case a bit more. This example looks like Dekker synchronization (if I get the intent right). Two possible implementations of this are either (1) with all memory accesses having seq-cst MO, or (2) with relaxed-MO accesses and seq-cst fences on between the store and load on both ends. It's possible to mix both, but that get's trickier I think. I'd prefer the one with just fences, just because it's easiest, conceptually. > I then found a discussion about using the C11 memory model in Linux > (https://gcc.gnu.org/ml/gcc/2014-02/msg00058.html) which contains the > following statement, which is a bit disheartening even though it is > about a different test: > > My first gut feeling was that the assertion should never fire, but > that was wrong because (as I seem to usually forget) the seq-cst > total order is just a constraint but doesn't itself contribute > to synchronizes-with -- but this is different for seq-cst fences. It works if you use (1) or (2) consistently. cppmem and the Batty et al. tech report should give you the gory details. My comment is just about seq-cst working differently on memory accesses vs. fences (in the way it's specified in the memory model). > and later in the thread: > > Use of C11 atomics to implement Linux kernel atomic operations > requires knowledge of the underlying architecture and the compiler's > implementation, as was noted earlier in this thread. > > Indeed if I add an atomic_thread_fence I get only one valid execution, > where r2 must be 1. This is similar to GCC's bug > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697, and we can fix it in > QEMU by using __sync_fetch_and_add; in fact cppmem also shows one valid > execution if the store is replaced with something like GCC's assembly > for __sync_fetch_and_add (or Linux's assembly for atomic_add_return): > > cas_strong_explicit(ctx_notify_me.readsvalue(0), 0, 2, > mo_release, mo_release); > atomic_thread_fence(mo_seq_cst); > > So we should: > > 1) understand why ATOMIC_SEQ_CST is not enough in this case. QEMU code > seems to be making the same assumptions as Linux about the memory model, > and this is wrong because QEMU uses C11 atomics if available. > Fortunately, this kind of synchronization in QEMU is relatively rare and > only this particular bit seems affected. If there is a fix which stays > within the C11 memory model, and does not pessimize code on x86, we can > use it[1] and document the pitfall. Using the fences between the store/load pairs in Dekker-like synchronization should do that, right? It's also relatively easy to deal with. > 2) if there's no way to fix the bug, qemu/atomic.h needs to switch to > __sync_fetch_and_add and friends. And again, in this case the > difference between the C11 and Linux/QEMU memory models must be documented. I surely not aware of all the constraints here, but I'd be surprised if the C11 memory model isn't good enough for portable synchronization code (with the exception of the consume MO minefield, perhaps).
On 02/10/19 16:58, Torvald Riegel wrote: > This example looks like Dekker synchronization (if I get the intent right). It is the same pattern. However, one of the two synchronized variables is a counter rather than just a flag. > Two possible implementations of this are either (1) with all memory > accesses having seq-cst MO, or (2) with relaxed-MO accesses and seq-cst > fences on between the store and load on both ends. It's possible to mix > both, but that get's trickier I think. I'd prefer the one with just > fences, just because it's easiest, conceptually. Got it. I'd also prefer the one with just fences, because we only really control one side of the synchronization primitive (ctx_notify_me in my litmus test) and I don't like the idea of forcing seq-cst MO on the other side (bh_scheduled). The performance issue that I mentioned is that x86 doesn't have relaxed fetch and add, so you'd have a redundant fence like this: lock xaddl $2, mem1 mfence ... movl mem1, %r8 (Gory QEMU details however allow us to use relaxed load and store here, because there's only one writer). > It works if you use (1) or (2) consistently. cppmem and the Batty et al. > tech report should give you the gory details. > >> 1) understand why ATOMIC_SEQ_CST is not enough in this case. QEMU code >> seems to be making the same assumptions as Linux about the memory model, >> and this is wrong because QEMU uses C11 atomics if available. >> Fortunately, this kind of synchronization in QEMU is relatively rare and >> only this particular bit seems affected. If there is a fix which stays >> within the C11 memory model, and does not pessimize code on x86, we can >> use it[1] and document the pitfall. > > Using the fences between the store/load pairs in Dekker-like > synchronization should do that, right? It's also relatively easy to deal > with. > >> 2) if there's no way to fix the bug, qemu/atomic.h needs to switch to >> __sync_fetch_and_add and friends. And again, in this case the >> difference between the C11 and Linux/QEMU memory models must be documented. > > I surely not aware of all the constraints here, but I'd be surprised if the > C11 memory model isn't good enough for portable synchronization code (with > the exception of the consume MO minefield, perhaps). This helps a lot already; I'll work on a documentation and code patch. Thanks very much. Paolo >> int main() { >> atomic_int ctx_notify_me = 0; >> atomic_int bh_scheduled = 0; >> {{{ { >> bh_scheduled.store(1, mo_release); >> atomic_thread_fence(mo_seq_cst); >> // must be zero since the bug report shows no notification >> ctx_notify_me.load(mo_relaxed).readsvalue(0); >> } >> ||| { >> ctx_notify_me.store(2, mo_seq_cst); >> r2=bh_scheduled.load(mo_relaxed); >> } >> }}}; >> return 0; >> }
diff --git a/include/block/aio.h b/include/block/aio.h index 0ca25dfec6..0724086d91 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -84,6 +84,7 @@ struct AioContext { * dispatch phase, hence a simple counter is enough for them. */ uint32_t notify_me; + QemuMutex notify_me_lcktest; /* A lock to protect between QEMUBH and AioHandler adders and deleter, * and to ensure that no callbacks are removed while we're walking and diff --git a/util/aio-posix.c b/util/aio-posix.c index 51c41ed3c9..031d6e2997 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -529,7 +529,9 @@ static bool run_poll_handlers(AioContext *ctx, int64_t max_ns, int64_t *timeout) bool progress; int64_t start_time, elapsed_time; + qemu_mutex_lock(&ctx->notify_me_lcktest); assert(ctx->notify_me); + qemu_mutex_unlock(&ctx->notify_me_lcktest); assert(qemu_lockcnt_count(&ctx->list_lock) > 0); trace_run_poll_handlers_begin(ctx, max_ns, *timeout); @@ -601,8 +603,10 @@ bool aio_poll(AioContext *ctx, bool blocking) * so disable the optimization now. */ if (blocking) { + qemu_mutex_lock(&ctx->notify_me_lcktest); assert(in_aio_context_home_thread(ctx)); atomic_add(&ctx->notify_me, 2); + qemu_mutex_unlock(&ctx->notify_me_lcktest); } qemu_lockcnt_inc(&ctx->list_lock); @@ -647,8 +651,10 @@ bool aio_poll(AioContext *ctx, bool blocking) } if (blocking) { + qemu_mutex_lock(&ctx->notify_me_lcktest); atomic_sub(&ctx->notify_me, 2); aio_notify_accept(ctx); + qemu_mutex_unlock(&ctx->notify_me_lcktest); } /* Adjust polling time */ diff --git a/util/async.c b/util/async.c index c10642a385..140e1e86f5 100644 --- a/util/async.c +++ b/util/async.c @@ -221,7 +221,9 @@ aio_ctx_prepare(GSource *source, gint *timeout) { AioContext *ctx = (AioContext *) source; + qemu_mutex_lock(&ctx->notify_me_lcktest); atomic_or(&ctx->notify_me, 1); + qemu_mutex_unlock(&ctx->notify_me_lcktest); /* We assume there is no timeout already supplied */ *timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)); @@ -239,8 +241,10 @@ aio_ctx_check(GSource *source) AioContext *ctx = (AioContext *) source; QEMUBH *bh; + qemu_mutex_lock(&ctx->notify_me_lcktest); atomic_and(&ctx->notify_me, ~1); aio_notify_accept(ctx); + qemu_mutex_unlock(&ctx->notify_me_lcktest); for (bh = ctx->first_bh; bh; bh = bh->next) { if (bh->scheduled) { @@ -346,11 +350,13 @@ void aio_notify(AioContext *ctx) /* 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(); + //smp_mb(); + qemu_mutex_lock(&ctx->notify_me_lcktest); if (ctx->notify_me) { event_notifier_set(&ctx->notifier); atomic_mb_set(&ctx->notified, true); } + qemu_mutex_unlock(&ctx->notify_me_lcktest); } void aio_notify_accept(AioContext *ctx) @@ -424,6 +430,8 @@ AioContext *aio_context_new(Error **errp) ctx->co_schedule_bh = aio_bh_new(ctx, co_schedule_bh_cb, ctx); QSLIST_INIT(&ctx->scheduled_coroutines); + qemu_rec_mutex_init(&ctx->notify_me_lcktest); + aio_set_event_notifier(ctx, &ctx->notifier, false, (EventNotifierHandler *)