On Fri, Oct 11, 2019 at 08:30:02AM +0000, Jan Glauber wrote:
> On Fri, Oct 11, 2019 at 10:18:18AM +0200, Paolo Bonzini wrote:
> > On 11/10/19 08:05, Jan Glauber wrote:
> > > On Wed, Oct 09, 2019 at 11:15:04AM +0200, Paolo Bonzini wrote:
> > >>> ...but if I bump notify_me size to uint64_t the issue goes away.
> > >>
> > >> Ouch. :) Is this with or without my patch(es)?
> >
> > You didn't answer this question.
>
> Oh, sorry... I did but the mail probably didn't make it out.
> I have both of your changes applied (as I think they make sense).
>
> > >> Also, what if you just add a dummy uint32_t after notify_me?
> > >
> > > With the dummy the testcase also runs fine for 500 iterations.
> >
> > You might be lucky and causing list_lock to be in another cache line.
> > What if you add __attribute__((aligned(16)) to notify_me (and keep the
> > dummy)?
>
> Good point. I'll try to force both into the same cacheline.
On the Hi1620, this still hangs in the first iteration:
diff --git a/include/block/aio.h b/include/block/aio.h
index 6b0d52f732..00e56a5412 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -82,7 +82,7 @@ struct AioContext {
* Instead, the aio_poll calls include both the prepare and the
* dispatch phase, hence a simple counter is enough for them.
*/
- uint32_t notify_me;
+ __attribute__((aligned(16))) uint64_t notify_me;
/* 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/async.c b/util/async.c
index ca83e32c7f..024c4c567d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -242,7 +242,7 @@ aio_ctx_check(GSource *source)
aio_notify_accept(ctx);
for (bh = ctx->first_bh; bh; bh = bh->next) {
- if (bh->scheduled) {
+ if (atomic_mb_read(&bh->scheduled)) {
return true;
}
}
@@ -342,12 +342,12 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx)
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.
+ /* Using atomic_mb_read ensures that e.g. bh->scheduled is written before
+ * ctx->notify_me is read. Pairs with atomic_or in aio_ctx_prepare or
+ * atomic_add in aio_poll.
*/
- smp_mb();
- if (ctx->notify_me) {
- event_notifier_set(&ctx->notifier);
+ if (atomic_mb_read(&ctx->notify_me)) {
+ event_notifier_set(&ctx->notifier); atomic_mb_set(&ctx->notified, true);
}
}
On Fri, Oct 11, 2019 at 08:30:02AM +0000, Jan Glauber wrote: _((aligned( 16)) to notify_me (and keep the
> On Fri, Oct 11, 2019 at 10:18:18AM +0200, Paolo Bonzini wrote:
> > On 11/10/19 08:05, Jan Glauber wrote:
> > > On Wed, Oct 09, 2019 at 11:15:04AM +0200, Paolo Bonzini wrote:
> > >>> ...but if I bump notify_me size to uint64_t the issue goes away.
> > >>
> > >> Ouch. :) Is this with or without my patch(es)?
> >
> > You didn't answer this question.
>
> Oh, sorry... I did but the mail probably didn't make it out.
> I have both of your changes applied (as I think they make sense).
>
> > >> Also, what if you just add a dummy uint32_t after notify_me?
> > >
> > > With the dummy the testcase also runs fine for 500 iterations.
> >
> > You might be lucky and causing list_lock to be in another cache line.
> > What if you add __attribute_
> > dummy)?
>
> Good point. I'll try to force both into the same cacheline.
On the Hi1620, this still hangs in the first iteration:
diff --git a/include/ block/aio. h b/include/ block/aio. h .00e56a5412 100644 block/aio. h block/aio. h _((aligned( 16))) uint64_t notify_me;
index 6b0d52f732.
--- a/include/
+++ b/include/
@@ -82,7 +82,7 @@ struct AioContext {
* Instead, the aio_poll calls include both the prepare and the
* dispatch phase, hence a simple counter is enough for them.
*/
- uint32_t notify_me;
+ __attribute_
/* A lock to protect between QEMUBH and AioHandler adders and deleter, .024c4c567d 100644 check(GSource *source) notify_ accept( ctx);
* and to ensure that no callbacks are removed while we're walking and
diff --git a/util/async.c b/util/async.c
index ca83e32c7f.
--- a/util/async.c
+++ b/util/async.c
@@ -242,7 +242,7 @@ aio_ctx_
aio_
for (bh = ctx->first_bh; bh; bh = bh->next) { mb_read( &bh->scheduled) ) { linux_aio( AioContext *ctx)
- if (bh->scheduled) {
+ if (atomic_
return true;
}
}
@@ -342,12 +342,12 @@ LinuxAioState *aio_get_
void aio_notify( AioContext *ctx) set(&ctx- >notifier) ; mb_read( &ctx->notify_ me)) { set(&ctx- >notifier) ;
atomic_ mb_set( &ctx->notified, true);
{
- /* Write e.g. bh->scheduled before reading ctx->notify_me. Pairs
- * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
+ /* Using atomic_mb_read ensures that e.g. bh->scheduled is written before
+ * ctx->notify_me is read. Pairs with atomic_or in aio_ctx_prepare or
+ * atomic_add in aio_poll.
*/
- smp_mb();
- if (ctx->notify_me) {
- event_notifier_
+ if (atomic_
+ event_notifier_
}
}