Re: [Qemu-devel] [regression] dataplane: throughout -40% by commit 580b6b2aa2

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/07/2014 17:45, Ming Lei ha scritto:
> The attachment debug patch skips aio_notify() if qemu_bh_schedule
> is running from current aio context, but looks there is still 120K
> writes triggered. (without the patch, 400K can be observed in
> same test)

Nice.  Another observation is that after aio_dispatch we'll always
re-evaluate everything (bottom halves, file descriptors and timeouts),
so we can skip the aio_notify if we're inside aio_dispatch.

So what about this untested patch:

diff --git a/aio-posix.c b/aio-posix.c
index f921d4f..a23d85d 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -124,6 +124,9 @@ static bool aio_dispatch(AioContext *ctx)
     AioHandler *node;
     bool progress = false;
 
+    /* No need to set the event notifier during aio_notify.  */
+    ctx->running++;
+
     /*
      * We have to walk very carefully in case qemu_aio_set_fd_handler is
      * called while we're walking.
@@ -169,6 +171,11 @@ static bool aio_dispatch(AioContext *ctx)
     /* Run our timers */
     progress |= timerlistgroup_run_timers(&ctx->tlg);
 
+    smp_wmb();
+    ctx->iter_count++;
+    smp_wmb();
+    ctx->running--;
+
     return progress;
 }
 
diff --git a/async.c b/async.c
index 5b6fe6b..1f56afa 100644
--- a/async.c
+++ b/async.c
@@ -249,7 +249,19 @@ ThreadPool *aio_get_thread_pool(AioContext *ctx)
 
 void aio_notify(AioContext *ctx)
 {
-    event_notifier_set(&ctx->notifier);
+    uint32_t iter_count;
+    do {
+        iter_count = ctx->iter_count;
+        /* Read ctx->iter_count before ctx->running.  */
+        smb_rmb();
+        if (!ctx->running) {
+            event_notifier_set(&ctx->notifier);
+            return;
+        }
+        /* Read ctx->running before ctx->iter_count.  */
+        smb_rmb();
+        /* ctx might have gone to sleep.  */
+    } while (iter_count != ctx->iter_count);
 }
 
 static void aio_timerlist_notify(void *opaque)
@@ -269,6 +279,7 @@ AioContext *aio_context_new(void)
     ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
     ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
     ctx->thread_pool = NULL;
+    ctx->iter_count = ctx->running = 0;
     qemu_mutex_init(&ctx->bh_lock);
     rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
     event_notifier_init(&ctx->notifier, false);
diff --git a/include/block/aio.h b/include/block/aio.h
index a92511b..9f51c4f 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -51,6 +51,9 @@ struct AioContext {
     /* Protects all fields from multi-threaded access */
     RFifoLock lock;
 
+    /* Used to avoid aio_notify while dispatching event handlers.
+     * Writes protected by lock or BQL, reads are lockless.
+     */
+    uint32_t iter_count, running;
+
     /* The list of registered AIO handlers */
     QLIST_HEAD(, AioHandler) aio_handlers;
 

Please review carefully.

> So is there still other writes not found in the path?

What do perf or gdb say? :)

Paolo