[Qemu-devel] [PATCH for-2.1? 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Qemu-devel] [PATCH for-2.1? 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Stefan Hajnoczi]

These patches convert thread-pool.c from EventNotifier to QEMUBH.  They then
solve the deadlock when nested aio_poll() calls are made.

Please speak out whether you want this in QEMU 2.1 or not.  I'm not aware of
the nested aio_poll() deadlock ever having been reported, so maybe we can defer
to QEMU 2.2.

Stefan Hajnoczi (2):
  thread-pool: avoid per-thread-pool EventNotifier
  thread-pool: avoid deadlock in nested aio_poll() calls

 thread-pool.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH for-2.1? 1/2] thread-pool: avoid per-thread-pool EventNotifier

[Stefan Hajnoczi]

EventNotifier is implemented using an eventfd or pipe.  It therefore
consumes file descriptors, which can be limited by rlimits and should
therefore be used sparingly.

Switch from EventNotifier to QEMUBH in thread-pool.c.  Originally
EventNotifier was used because qemu_bh_schedule() was not thread-safe
yet.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 thread-pool.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/thread-pool.c b/thread-pool.c
index dfb699d..4cfd078 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -21,7 +21,6 @@
 #include "block/coroutine.h"
 #include "trace.h"
 #include "block/block_int.h"
-#include "qemu/event_notifier.h"
 #include "block/thread-pool.h"
 #include "qemu/main-loop.h"
 
@@ -57,8 +56,8 @@ struct ThreadPoolElement {
 };
 
 struct ThreadPool {
-    EventNotifier notifier;
     AioContext *ctx;
+    QEMUBH *completion_bh;
     QemuMutex lock;
     QemuCond check_cancel;
     QemuCond worker_stopped;
@@ -119,7 +118,7 @@ static void *worker_thread(void *opaque)
             qemu_cond_broadcast(&pool->check_cancel);
         }
 
-        event_notifier_set(&pool->notifier);
+        qemu_bh_schedule(pool->completion_bh);
     }
 
     pool->cur_threads--;
@@ -168,12 +167,11 @@ static void spawn_thread(ThreadPool *pool)
     }
 }
 
-static void event_notifier_ready(EventNotifier *notifier)
+static void thread_pool_completion_bh(void *opaque)
 {
-    ThreadPool *pool = container_of(notifier, ThreadPool, notifier);
+    ThreadPool *pool = opaque;
     ThreadPoolElement *elem, *next;
 
-    event_notifier_test_and_clear(notifier);
 restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
         if (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
@@ -215,7 +213,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
         qemu_sem_timedwait(&pool->sem, 0) == 0) {
         QTAILQ_REMOVE(&pool->request_list, elem, reqs);
         elem->state = THREAD_CANCELED;
-        event_notifier_set(&pool->notifier);
+        qemu_bh_schedule(pool->completion_bh);
     } else {
         pool->pending_cancellations++;
         while (elem->state != THREAD_CANCELED && elem->state != THREAD_DONE) {
@@ -224,7 +222,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
         pool->pending_cancellations--;
     }
     qemu_mutex_unlock(&pool->lock);
-    event_notifier_ready(&pool->notifier);
+    thread_pool_completion_bh(pool);
 }
 
 static const AIOCBInfo thread_pool_aiocb_info = {
@@ -293,8 +291,8 @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
     }
 
     memset(pool, 0, sizeof(*pool));
-    event_notifier_init(&pool->notifier, false);
     pool->ctx = ctx;
+    pool->completion_bh = aio_bh_new(ctx, thread_pool_completion_bh, pool);
     qemu_mutex_init(&pool->lock);
     qemu_cond_init(&pool->check_cancel);
     qemu_cond_init(&pool->worker_stopped);
@@ -304,8 +302,6 @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
 
     QLIST_INIT(&pool->head);
     QTAILQ_INIT(&pool->request_list);
-
-    aio_set_event_notifier(ctx, &pool->notifier, event_notifier_ready);
 }
 
 ThreadPool *thread_pool_new(AioContext *ctx)
@@ -339,11 +335,10 @@ void thread_pool_free(ThreadPool *pool)
 
     qemu_mutex_unlock(&pool->lock);
 
-    aio_set_event_notifier(pool->ctx, &pool->notifier, NULL);
+    qemu_bh_delete(pool->completion_bh);
     qemu_sem_destroy(&pool->sem);
     qemu_cond_destroy(&pool->check_cancel);
     qemu_cond_destroy(&pool->worker_stopped);
     qemu_mutex_destroy(&pool->lock);
-    event_notifier_cleanup(&pool->notifier);
     g_free(pool);
 }
-- 
1.9.3

[Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

The thread pool has a race condition if two elements complete before
thread_pool_completion_bh() runs:

  If element A's callback waits for element B using aio_poll() it will
  deadlock since pool->completion_bh is not marked scheduled when the
  nested aio_poll() runs.

Fix this by marking the BH scheduled while thread_pool_completion_bh()
is executing.  This way any nested aio_poll() loops will enter
thread_pool_completion_bh() and complete the remaining elements.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 thread-pool.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/thread-pool.c b/thread-pool.c
index 4cfd078..0ede168 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -65,6 +65,9 @@ struct ThreadPool {
     int max_threads;
     QEMUBH *new_thread_bh;
 
+    /* Atomic counter to detect completions while completion handler runs */
+    uint32_t completion_token;
+
     /* The following variables are only accessed from one AioContext. */
     QLIST_HEAD(, ThreadPoolElement) head;
 
@@ -118,6 +121,7 @@ static void *worker_thread(void *opaque)
             qemu_cond_broadcast(&pool->check_cancel);
         }
 
+        atomic_inc(&pool->completion_token);
         qemu_bh_schedule(pool->completion_bh);
     }
 
@@ -167,9 +171,8 @@ static void spawn_thread(ThreadPool *pool)
     }
 }
 
-static void thread_pool_completion_bh(void *opaque)
+static void thread_pool_complete_elements(ThreadPool *pool)
 {
-    ThreadPool *pool = opaque;
     ThreadPoolElement *elem, *next;
 
 restart:
@@ -196,6 +199,26 @@ restart:
     }
 }
 
+static void thread_pool_completion_bh(void *opaque)
+{
+    ThreadPool *pool = opaque;
+    uint32_t token;
+
+    do {
+        token = atomic_mb_read(&pool->completion_token);
+
+        /* Stay scheduled in case elem->common.cb() makes a nested aio_poll()
+         * call.  This avoids deadlock if element A's callback waits for
+         * element B and both completed at the same time.
+         */
+        qemu_bh_schedule(pool->completion_bh);
+
+        thread_pool_complete_elements(pool);
+
+        qemu_bh_cancel(pool->completion_bh);
+    } while (token != pool->completion_token);
+}
+
 static void thread_pool_cancel(BlockDriverAIOCB *acb)
 {
     ThreadPoolElement *elem = (ThreadPoolElement *)acb;
-- 
1.9.3

Re: [Qemu-devel] [PATCH for-2.1? 0/2] thread-pool: avoid fd usage and fix nested aio_poll() deadlock

[Christian Borntraeger]

On 11/07/14 13:20, Stefan Hajnoczi wrote:
> These patches convert thread-pool.c from EventNotifier to QEMUBH.  They then
> solve the deadlock when nested aio_poll() calls are made.
> 
> Please speak out whether you want this in QEMU 2.1 or not.  I'm not aware of
> the nested aio_poll() deadlock ever having been reported, so maybe we can defer
> to QEMU 2.2.
> 
> Stefan Hajnoczi (2):
>   thread-pool: avoid per-thread-pool EventNotifier
>   thread-pool: avoid deadlock in nested aio_poll() calls
> 
>  thread-pool.c | 44 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 

Seems to work ok and saves an fd. I think its still better scheduled for 2.2

Christian

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Paolo Bonzini]

Il 11/07/2014 13:20, Stefan Hajnoczi ha scritto:
> The thread pool has a race condition if two elements complete before
> thread_pool_completion_bh() runs:
>
>   If element A's callback waits for element B using aio_poll() it will
>   deadlock since pool->completion_bh is not marked scheduled when the
>   nested aio_poll() runs.
>
> Fix this by marking the BH scheduled while thread_pool_completion_bh()
> is executing.  This way any nested aio_poll() loops will enter
> thread_pool_completion_bh() and complete the remaining elements.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  thread-pool.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/thread-pool.c b/thread-pool.c
> index 4cfd078..0ede168 100644
> --- a/thread-pool.c
> +++ b/thread-pool.c
> @@ -65,6 +65,9 @@ struct ThreadPool {
>      int max_threads;
>      QEMUBH *new_thread_bh;
>
> +    /* Atomic counter to detect completions while completion handler runs */
> +    uint32_t completion_token;
> +
>      /* The following variables are only accessed from one AioContext. */
>      QLIST_HEAD(, ThreadPoolElement) head;
>
> @@ -118,6 +121,7 @@ static void *worker_thread(void *opaque)
>              qemu_cond_broadcast(&pool->check_cancel);
>          }
>
> +        atomic_inc(&pool->completion_token);
>          qemu_bh_schedule(pool->completion_bh);
>      }
>
> @@ -167,9 +171,8 @@ static void spawn_thread(ThreadPool *pool)
>      }
>  }
>
> -static void thread_pool_completion_bh(void *opaque)
> +static void thread_pool_complete_elements(ThreadPool *pool)
>  {
> -    ThreadPool *pool = opaque;
>      ThreadPoolElement *elem, *next;
>
>  restart:
> @@ -196,6 +199,26 @@ restart:
>      }
>  }
>
> +static void thread_pool_completion_bh(void *opaque)
> +{
> +    ThreadPool *pool = opaque;
> +    uint32_t token;
> +
> +    do {
> +        token = atomic_mb_read(&pool->completion_token);
> +
> +        /* Stay scheduled in case elem->common.cb() makes a nested aio_poll()
> +         * call.  This avoids deadlock if element A's callback waits for
> +         * element B and both completed at the same time.
> +         */
> +        qemu_bh_schedule(pool->completion_bh);
> +
> +        thread_pool_complete_elements(pool);
> +
> +        qemu_bh_cancel(pool->completion_bh);
> +    } while (token != pool->completion_token);
> +}
> +
>  static void thread_pool_cancel(BlockDriverAIOCB *acb)
>  {
>      ThreadPoolElement *elem = (ThreadPoolElement *)acb;
>

I am not sure I understand this patch.

The simplest way to fix deadlock is to change this in 
thread_pool_completion_bh:

             elem->common.cb(elem->common.opaque, elem->ret);
             qemu_aio_release(elem);
             goto restart;

to

             /* In case elem->common.cb() makes a nested aio_poll() call,
              * next may become invalid as well.  Instead of just
              * restarting the QLIST_FOREACH_SAFE, go through the BH
              * once more, which also avoids deadlock if element A's
              * callback waits for element B and both completed at the
              * same time.
              */
             qemu_bh_schedule(pool->completion_bh);
             elem->common.cb(elem->common.opaque, elem->ret);
             qemu_aio_release(elem);

There is no change in logic, it's just that the goto is switched to a BH 
representing a continuation.  I am then not sure why 
pool->completion_token is necessary?

Perhaps it is just an optimization to avoid going multiple times around 
aio_poll()?

Paolo

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Paolo Bonzini]

Il 14/07/2014 10:36, Paolo Bonzini ha scritto:
>
>
> to
>
>             /* In case elem->common.cb() makes a nested aio_poll() call,
>              * next may become invalid as well.  Instead of just
>              * restarting the QLIST_FOREACH_SAFE, go through the BH
>              * once more, which also avoids deadlock if element A's
>              * callback waits for element B and both completed at the
>              * same time.
>              */
>             qemu_bh_schedule(pool->completion_bh);
>             elem->common.cb(elem->common.opaque, elem->ret);
>             qemu_aio_release(elem);

This is of course missing here:

		break;

Paolo

>
> There is no change in logic, it's just that the goto is switched to a BH
> representing a continuation.  I am then not sure why
> pool->completion_token is necessary?

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4201 bytes --]

On Mon, Jul 14, 2014 at 10:36:21AM +0200, Paolo Bonzini wrote:
> Il 11/07/2014 13:20, Stefan Hajnoczi ha scritto:
> >The thread pool has a race condition if two elements complete before
> >thread_pool_completion_bh() runs:
> >
> >  If element A's callback waits for element B using aio_poll() it will
> >  deadlock since pool->completion_bh is not marked scheduled when the
> >  nested aio_poll() runs.
> >
> >Fix this by marking the BH scheduled while thread_pool_completion_bh()
> >is executing.  This way any nested aio_poll() loops will enter
> >thread_pool_completion_bh() and complete the remaining elements.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >---
> > thread-pool.c | 27 +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> >diff --git a/thread-pool.c b/thread-pool.c
> >index 4cfd078..0ede168 100644
> >--- a/thread-pool.c
> >+++ b/thread-pool.c
> >@@ -65,6 +65,9 @@ struct ThreadPool {
> >     int max_threads;
> >     QEMUBH *new_thread_bh;
> >
> >+    /* Atomic counter to detect completions while completion handler runs */
> >+    uint32_t completion_token;
> >+
> >     /* The following variables are only accessed from one AioContext. */
> >     QLIST_HEAD(, ThreadPoolElement) head;
> >
> >@@ -118,6 +121,7 @@ static void *worker_thread(void *opaque)
> >             qemu_cond_broadcast(&pool->check_cancel);
> >         }
> >
> >+        atomic_inc(&pool->completion_token);
> >         qemu_bh_schedule(pool->completion_bh);
> >     }
> >
> >@@ -167,9 +171,8 @@ static void spawn_thread(ThreadPool *pool)
> >     }
> > }
> >
> >-static void thread_pool_completion_bh(void *opaque)
> >+static void thread_pool_complete_elements(ThreadPool *pool)
> > {
> >-    ThreadPool *pool = opaque;
> >     ThreadPoolElement *elem, *next;
> >
> > restart:
> >@@ -196,6 +199,26 @@ restart:
> >     }
> > }
> >
> >+static void thread_pool_completion_bh(void *opaque)
> >+{
> >+    ThreadPool *pool = opaque;
> >+    uint32_t token;
> >+
> >+    do {
> >+        token = atomic_mb_read(&pool->completion_token);
> >+
> >+        /* Stay scheduled in case elem->common.cb() makes a nested aio_poll()
> >+         * call.  This avoids deadlock if element A's callback waits for
> >+         * element B and both completed at the same time.
> >+         */
> >+        qemu_bh_schedule(pool->completion_bh);
> >+
> >+        thread_pool_complete_elements(pool);
> >+
> >+        qemu_bh_cancel(pool->completion_bh);
> >+    } while (token != pool->completion_token);
> >+}
> >+
> > static void thread_pool_cancel(BlockDriverAIOCB *acb)
> > {
> >     ThreadPoolElement *elem = (ThreadPoolElement *)acb;
> >
> 
> I am not sure I understand this patch.
> 
> The simplest way to fix deadlock is to change this in
> thread_pool_completion_bh:
> 
>             elem->common.cb(elem->common.opaque, elem->ret);
>             qemu_aio_release(elem);
>             goto restart;
> 
> to
> 
>             /* In case elem->common.cb() makes a nested aio_poll() call,
>              * next may become invalid as well.  Instead of just
>              * restarting the QLIST_FOREACH_SAFE, go through the BH
>              * once more, which also avoids deadlock if element A's
>              * callback waits for element B and both completed at the
>              * same time.
>              */
>             qemu_bh_schedule(pool->completion_bh);
>             elem->common.cb(elem->common.opaque, elem->ret);
>             qemu_aio_release(elem);
> 
> There is no change in logic, it's just that the goto is switched to a BH
> representing a continuation.  I am then not sure why pool->completion_token
> is necessary?
> 
> Perhaps it is just an optimization to avoid going multiple times around
> aio_poll()?

Yes, it's just an optimization.  I wanted to cancel pool->completion_bh
to avoid needlessly entering the BH.  But if we call cancel then we must
watch for worker threads that complete a request while we're invoking
completions - hence the completion_token.

Your approach is simpler and I like that.  I'll send a v2.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

On Mon, Jul 14, 2014 at 12:49:38PM +0200, Paolo Bonzini wrote:
> Il 14/07/2014 10:36, Paolo Bonzini ha scritto:
> >
> >
> >to
> >
> >            /* In case elem->common.cb() makes a nested aio_poll() call,
> >             * next may become invalid as well.  Instead of just
> >             * restarting the QLIST_FOREACH_SAFE, go through the BH
> >             * once more, which also avoids deadlock if element A's
> >             * callback waits for element B and both completed at the
> >             * same time.
> >             */
> >            qemu_bh_schedule(pool->completion_bh);
> >            elem->common.cb(elem->common.opaque, elem->ret);
> >            qemu_aio_release(elem);
> 
> This is of course missing here:
> 
> 		break;

Let's keep goto restart so we don't use the BH for each completion
callback.  We just need the BH scheduled once to protect against the
deadlock.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Paolo Bonzini]

Il 15/07/2014 16:37, Stefan Hajnoczi ha scritto:
>> > This is of course missing here:
>> >
>> > 		break;
> Let's keep goto restart so we don't use the BH for each completion
> callback.  We just need the BH scheduled once to protect against the
> deadlock.

Ah, I missed this remark.

Then you could add qemu_bh_cancel at the end of the BH handler.  I do 
find the code a little harder to follow with the "goto" though, as it's 
not clear who will take care of the rest of the list...

Paolo

Re: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Tue, Jul 15, 2014 at 05:21:21PM +0200, Paolo Bonzini wrote:
> Il 15/07/2014 16:37, Stefan Hajnoczi ha scritto:
> >>> This is of course missing here:
> >>>
> >>> 		break;
> >Let's keep goto restart so we don't use the BH for each completion
> >callback.  We just need the BH scheduled once to protect against the
> >deadlock.
> 
> Ah, I missed this remark.
> 
> Then you could add qemu_bh_cancel at the end of the BH handler.

No, because a worker thread could be completing a work item just as we
leave the BH handler.  Then the BH would be cancelled and the work item
completion would never get executed (until the next work item
completes).

This is why I needed the atomic completion_token.  That way I could
check whether more requests completed while we were in the BH handler.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]