From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5YsQ-0006dv-3Z for qemu-devel@nongnu.org; Fri, 11 Jul 2014 07:20:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X5YsJ-0008Fu-FC for qemu-devel@nongnu.org; Fri, 11 Jul 2014 07:20:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5YsJ-0008FE-6b for qemu-devel@nongnu.org; Fri, 11 Jul 2014 07:20:23 -0400 From: Stefan Hajnoczi Date: Fri, 11 Jul 2014 13:20:12 +0200 Message-Id: <1405077612-7806-3-git-send-email-stefanha@redhat.com> In-Reply-To: <1405077612-7806-1-git-send-email-stefanha@redhat.com> References: <1405077612-7806-1-git-send-email-stefanha@redhat.com> Subject: [Qemu-devel] [PATCH for-2.1? 2/2] thread-pool: avoid deadlock in nested aio_poll() calls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Paolo Bonzini , Christian Borntraeger 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 --- 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