From patchwork Wed Jul 22 11:43:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 498509 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id C16A91402C6 for ; Wed, 22 Jul 2015 21:48:24 +1000 (AEST) Received: from localhost ([::1]:35893 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHsVb-0006jF-26 for incoming@patchwork.ozlabs.org; Wed, 22 Jul 2015 07:48:23 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHsRP-0007PR-Rf for qemu-devel@nongnu.org; Wed, 22 Jul 2015 07:44:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHsRO-0005um-F0 for qemu-devel@nongnu.org; Wed, 22 Jul 2015 07:44:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41774) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHsRO-0005tJ-5O for qemu-devel@nongnu.org; Wed, 22 Jul 2015 07:44:02 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id BD780BAEF4; Wed, 22 Jul 2015 11:44:01 +0000 (UTC) Received: from localhost (ovpn-112-69.ams2.redhat.com [10.36.112.69]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6MBi01F019626; Wed, 22 Jul 2015 07:44:01 -0400 From: Stefan Hajnoczi To: Date: Wed, 22 Jul 2015 12:43:45 +0100 Message-Id: <1437565425-29861-8-git-send-email-stefanha@redhat.com> In-Reply-To: <1437565425-29861-1-git-send-email-stefanha@redhat.com> References: <1437565425-29861-1-git-send-email-stefanha@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Kevin Wolf , Peter Maydell , Stefan Hajnoczi , Paolo Bonzini Subject: [Qemu-devel] [PULL v2 for-2.4 v2 7/7] AioContext: optimize clearing the EventNotifier X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org From: Paolo Bonzini It is pretty rare for aio_notify to actually set the EventNotifier. It can happen with worker threads such as thread-pool.c's, but otherwise it should never be set thanks to the ctx->notify_me optimization. The previous patch, unfortunately, added an unconditional call to event_notifier_test_and_clear; now add a userspace fast path that avoids the call. Note that it is not possible to do the same with event_notifier_set; it would break, as proved (again) by the included formal model. This patch survived over 3000 reboots on aarch64 KVM. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Tested-by: Richard W.M. Jones Message-id: 1437487673-23740-7-git-send-email-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- aio-posix.c | 2 +- aio-win32.c | 2 +- async.c | 10 ++- docs/aio_notify_accept.promela | 152 +++++++++++++++++++++++++++++++++++++++++ include/block/aio.h | 32 ++++++++- 5 files changed, 194 insertions(+), 4 deletions(-) create mode 100644 docs/aio_notify_accept.promela diff --git a/aio-posix.c b/aio-posix.c index 5c8b266..d477033 100644 --- a/aio-posix.c +++ b/aio-posix.c @@ -276,7 +276,7 @@ bool aio_poll(AioContext *ctx, bool blocking) aio_context_acquire(ctx); } - event_notifier_test_and_clear(&ctx->notifier); + aio_notify_accept(ctx); /* if we have any readable fds, dispatch event */ if (ret > 0) { diff --git a/aio-win32.c b/aio-win32.c index 7afc999..50a6867 100644 --- a/aio-win32.c +++ b/aio-win32.c @@ -338,7 +338,7 @@ bool aio_poll(AioContext *ctx, bool blocking) } if (first) { - event_notifier_test_and_clear(&ctx->notifier); + aio_notify_accept(ctx); progress |= aio_bh_poll(ctx); first = false; } diff --git a/async.c b/async.c index d625e8a..9a98a74 100644 --- a/async.c +++ b/async.c @@ -203,7 +203,7 @@ aio_ctx_check(GSource *source) QEMUBH *bh; atomic_and(&ctx->notify_me, ~1); - event_notifier_test_and_clear(&ctx->notifier); + aio_notify_accept(ctx); for (bh = ctx->first_bh; bh; bh = bh->next) { if (!bh->deleted && bh->scheduled) { @@ -267,6 +267,14 @@ void aio_notify(AioContext *ctx) smp_mb(); if (ctx->notify_me) { event_notifier_set(&ctx->notifier); + atomic_mb_set(&ctx->notified, true); + } +} + +void aio_notify_accept(AioContext *ctx) +{ + if (atomic_xchg(&ctx->notified, false)) { + event_notifier_test_and_clear(&ctx->notifier); } } diff --git a/docs/aio_notify_accept.promela b/docs/aio_notify_accept.promela new file mode 100644 index 0000000..9cef2c9 --- /dev/null +++ b/docs/aio_notify_accept.promela @@ -0,0 +1,152 @@ +/* + * This model describes the interaction between ctx->notified + * and ctx->notifier. + * + * Author: Paolo Bonzini + * + * This file is in the public domain. If you really want a license, + * the WTFPL will do. + * + * To verify the buggy version: + * spin -a -DBUG1 docs/aio_notify_bug.promela + * gcc -O2 pan.c + * ./a.out -a -f + * (or -DBUG2) + * + * To verify the fixed version: + * spin -a docs/aio_notify_bug.promela + * gcc -O2 pan.c + * ./a.out -a -f + * + * Add -DCHECK_REQ to test an alternative invariant and the + * "notify_me" optimization. + */ + +int notify_me; +bool notified; +bool event; +bool req; +bool notifier_done; + +#ifdef CHECK_REQ +#define USE_NOTIFY_ME 1 +#else +#define USE_NOTIFY_ME 0 +#endif + +#ifdef BUG +#error Please define BUG1 or BUG2 instead. +#endif + +active proctype notifier() +{ + do + :: true -> { + req = 1; + if + :: !USE_NOTIFY_ME || notify_me -> +#if defined BUG1 + /* CHECK_REQ does not detect this bug! */ + notified = 1; + event = 1; +#elif defined BUG2 + if + :: !notified -> event = 1; + :: else -> skip; + fi; + notified = 1; +#else + event = 1; + notified = 1; +#endif + :: else -> skip; + fi + } + :: true -> break; + od; + notifier_done = 1; +} + +#define AIO_POLL \ + notify_me++; \ + if \ + :: !req -> { \ + if \ + :: event -> skip; \ + fi; \ + } \ + :: else -> skip; \ + fi; \ + notify_me--; \ + \ + atomic { old = notified; notified = 0; } \ + if \ + :: old -> event = 0; \ + :: else -> skip; \ + fi; \ + \ + req = 0; + +active proctype waiter() +{ + bool old; + + do + :: true -> AIO_POLL; + od; +} + +/* Same as waiter(), but disappears after a while. */ +active proctype temporary_waiter() +{ + bool old; + + do + :: true -> AIO_POLL; + :: true -> break; + od; +} + +#ifdef CHECK_REQ +never { + do + :: req -> goto accept_if_req_not_eventually_false; + :: true -> skip; + od; + +accept_if_req_not_eventually_false: + if + :: req -> goto accept_if_req_not_eventually_false; + fi; + assert(0); +} + +#else +/* There must be infinitely many transitions of event as long + * as the notifier does not exit. + * + * If event stayed always true, the waiters would be busy looping. + * If event stayed always false, the waiters would be sleeping + * forever. + */ +never { + do + :: !event -> goto accept_if_event_not_eventually_true; + :: event -> goto accept_if_event_not_eventually_false; + :: true -> skip; + od; + +accept_if_event_not_eventually_true: + if + :: !event && notifier_done -> do :: true -> skip; od; + :: !event && !notifier_done -> goto accept_if_event_not_eventually_true; + fi; + assert(0); + +accept_if_event_not_eventually_false: + if + :: event -> goto accept_if_event_not_eventually_false; + fi; + assert(0); +} +#endif diff --git a/include/block/aio.h b/include/block/aio.h index be91e3f..9dd32e0 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -99,7 +99,19 @@ struct AioContext { */ int walking_bh; - /* Used for aio_notify. */ + /* Used by aio_notify. + * + * "notified" is used to avoid expensive event_notifier_test_and_clear + * calls. When it is clear, the EventNotifier is clear, or one thread + * is going to clear "notified" before processing more events. False + * positives are possible, i.e. "notified" could be set even though the + * EventNotifier is clear. + * + * Note that event_notifier_set *cannot* be optimized the same way. For + * more information on the problem that would result, see "#ifdef BUG2" + * in the docs/aio_notify_accept.promela formal model. + */ + bool notified; EventNotifier notifier; /* Thread pool for performing work and receiving completion callbacks */ @@ -174,6 +186,24 @@ QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque); void aio_notify(AioContext *ctx); /** + * aio_notify_accept: Acknowledge receiving an aio_notify. + * + * aio_notify() uses an EventNotifier in order to wake up a sleeping + * aio_poll() or g_main_context_iteration(). Calls to aio_notify() are + * usually rare, but the AioContext has to clear the EventNotifier on + * every aio_poll() or g_main_context_iteration() in order to avoid + * busy waiting. This event_notifier_test_and_clear() cannot be done + * using the usual aio_context_set_event_notifier(), because it must + * be done before processing all events (file descriptors, bottom halves, + * timers). + * + * aio_notify_accept() is an optimized event_notifier_test_and_clear() + * that is specific to an AioContext's notifier; it is used internally + * to clear the EventNotifier only if aio_notify() had been called. + */ +void aio_notify_accept(AioContext *ctx); + +/** * aio_bh_poll: Poll bottom halves for an AioContext. * * These are internal functions used by the QEMU main loop.