From patchwork Wed Mar 16 14:16:55 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 8601021 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 2B00F9F6E1 for ; Wed, 16 Mar 2016 14:21:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 104F3202F2 for ; Wed, 16 Mar 2016 14:21:17 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0E478202AE for ; Wed, 16 Mar 2016 14:21:11 +0000 (UTC) Received: from localhost ([::1]:56625 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agCJy-0001TB-95 for patchwork-qemu-devel@patchwork.kernel.org; Wed, 16 Mar 2016 10:21:10 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42237) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agCGS-0003T3-2N for qemu-devel@nongnu.org; Wed, 16 Mar 2016 10:17:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agCGL-000671-MG for qemu-devel@nongnu.org; Wed, 16 Mar 2016 10:17:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agCGL-00066v-CO for qemu-devel@nongnu.org; Wed, 16 Mar 2016 10:17:25 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 1D9C8C029332 for ; Wed, 16 Mar 2016 14:17:25 +0000 (UTC) Received: from donizetti.redhat.com (ovpn-112-54.ams2.redhat.com [10.36.112.54]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2GEGwc7001673; Wed, 16 Mar 2016 10:17:23 -0400 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Wed, 16 Mar 2016 15:16:55 +0100 Message-Id: <1458137817-15383-15-git-send-email-pbonzini@redhat.com> In-Reply-To: <1458137817-15383-1-git-send-email-pbonzini@redhat.com> References: <1458137817-15383-1-git-send-email-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: famz@redhat.com, stefanha@redhat.com Subject: [Qemu-devel] [PATCH v2 14/16] iothread: release AioContext around aio_poll 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+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP This is the first step towards having fine-grained critical sections in dataplane threads, which will resolve lock ordering problems between address_space_* functions (which need the BQL when doing MMIO, even after we complete RCU-based dispatch) and the AioContext. Because AioContext does not use contention callbacks anymore, the unit test has to be changed. Previously applied as a0710f7995f914e3044e5899bd8ff6c43c62f916 and then reverted. Reviewed-by: Fam Zheng Signed-off-by: Paolo Bonzini --- async.c | 22 +++------------------- docs/multiple-iothreads.txt | 40 +++++++++++++++++++++++----------------- include/block/aio.h | 3 --- iothread.c | 11 ++--------- tests/test-aio.c | 22 ++++++++++++++-------- 5 files changed, 42 insertions(+), 56 deletions(-) diff --git a/async.c b/async.c index d4dd2cc..0084d57 100644 --- a/async.c +++ b/async.c @@ -85,8 +85,8 @@ int aio_bh_poll(AioContext *ctx) * aio_notify again if necessary. */ if (!bh->deleted && atomic_xchg(&bh->scheduled, 0)) { - /* Idle BHs and the notify BH don't count as progress */ - if (!bh->idle && bh != ctx->notify_dummy_bh) { + /* Idle BHs don't count as progress */ + if (!bh->idle) { ret = 1; } bh->idle = 0; @@ -238,7 +238,6 @@ aio_ctx_finalize(GSource *source) { AioContext *ctx = (AioContext *) source; - qemu_bh_delete(ctx->notify_dummy_bh); thread_pool_free(ctx->thread_pool); qemu_mutex_lock(&ctx->bh_lock); @@ -305,19 +304,6 @@ static void aio_timerlist_notify(void *opaque) aio_notify(opaque); } -static void aio_rfifolock_cb(void *opaque) -{ - AioContext *ctx = opaque; - - /* Kick owner thread in case they are blocked in aio_poll() */ - qemu_bh_schedule(ctx->notify_dummy_bh); -} - -static void notify_dummy_bh(void *opaque) -{ - /* Do nothing, we were invoked just to force the event loop to iterate */ -} - static void event_notifier_dummy_cb(EventNotifier *e) { } @@ -346,11 +332,9 @@ AioContext *aio_context_new(Error **errp) event_notifier_dummy_cb); ctx->thread_pool = NULL; qemu_mutex_init(&ctx->bh_lock); - rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx); + rfifolock_init(&ctx->lock, NULL, NULL); timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx); - ctx->notify_dummy_bh = aio_bh_new(ctx, notify_dummy_bh, NULL); - return ctx; fail: g_source_destroy(&ctx->source); diff --git a/docs/multiple-iothreads.txt b/docs/multiple-iothreads.txt index 40b8419..0e7cdb2 100644 --- a/docs/multiple-iothreads.txt +++ b/docs/multiple-iothreads.txt @@ -105,13 +105,10 @@ a BH in the target AioContext beforehand and then call qemu_bh_schedule(). No acquire/release or locking is needed for the qemu_bh_schedule() call. But be sure to acquire the AioContext for aio_bh_new() if necessary. -The relationship between AioContext and the block layer -------------------------------------------------------- -The AioContext originates from the QEMU block layer because it provides a -scoped way of running event loop iterations until all work is done. This -feature is used to complete all in-flight block I/O requests (see -bdrv_drain_all()). Nowadays AioContext is a generic event loop that can be -used by any QEMU subsystem. +AioContext and the block layer +------------------------------ +The AioContext originates from the QEMU block layer, even though nowadays +AioContext is a generic event loop that can be used by any QEMU subsystem. The block layer has support for AioContext integrated. Each BlockDriverState is associated with an AioContext using bdrv_set_aio_context() and @@ -122,13 +119,22 @@ Block layer code must therefore expect to run in an IOThread and avoid using old APIs that implicitly use the main loop. See the "How to program for IOThreads" above for information on how to do that. -If main loop code such as a QMP function wishes to access a BlockDriverState it -must first call aio_context_acquire(bdrv_get_aio_context(bs)) to ensure the -IOThread does not run in parallel. - -Long-running jobs (usually in the form of coroutines) are best scheduled in the -BlockDriverState's AioContext to avoid the need to acquire/release around each -bdrv_*() call. Be aware that there is currently no mechanism to get notified -when bdrv_set_aio_context() moves this BlockDriverState to a different -AioContext (see bdrv_detach_aio_context()/bdrv_attach_aio_context()), so you -may need to add this if you want to support long-running jobs. +If main loop code such as a QMP function wishes to access a BlockDriverState +it must first call aio_context_acquire(bdrv_get_aio_context(bs)) to ensure +that callbacks in the IOThread do not run in parallel. + +Code running in the monitor typically needs to ensure that past +requests from the guest are completed. When a block device is running +in an IOThread, the IOThread can also process requests from the guest +(via ioeventfd). To achieve both objects, wrap the code between +bdrv_drained_begin() and bdrv_drained_end(), thus creating a "drained +section". The functions must be called between aio_context_acquire() +and aio_context_release(). You can freely release and re-acquire the +AioContext within a drained section. + +Long-running jobs (usually in the form of coroutines) are best scheduled in +the BlockDriverState's AioContext to avoid the need to acquire/release around +each bdrv_*() call. The functions bdrv_add/remove_aio_context_notifier, +or alternatively blk_add/remove_aio_context_notifier if you use BlockBackends, +can be used to get a notification whenever bdrv_set_aio_context() moves a +BlockDriverState to a different AioContext. diff --git a/include/block/aio.h b/include/block/aio.h index 9434665..ba1ee81 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -114,9 +114,6 @@ struct AioContext { bool notified; EventNotifier notifier; - /* Scheduling this BH forces the event loop it iterate */ - QEMUBH *notify_dummy_bh; - /* Thread pool for performing work and receiving completion callbacks */ struct ThreadPool *thread_pool; diff --git a/iothread.c b/iothread.c index 8d40bb0..f66ec95 100644 --- a/iothread.c +++ b/iothread.c @@ -39,7 +39,6 @@ bool aio_context_in_iothread(AioContext *ctx) static void *iothread_run(void *opaque) { IOThread *iothread = opaque; - bool blocking; rcu_register_thread(); @@ -49,14 +48,8 @@ static void *iothread_run(void *opaque) qemu_cond_signal(&iothread->init_done_cond); qemu_mutex_unlock(&iothread->init_done_lock); - while (!iothread->stopping) { - aio_context_acquire(iothread->ctx); - blocking = true; - while (!iothread->stopping && aio_poll(iothread->ctx, blocking)) { - /* Progress was made, keep going */ - blocking = false; - } - aio_context_release(iothread->ctx); + while (!atomic_read(&iothread->stopping)) { + aio_poll(iothread->ctx, true); } rcu_unregister_thread(); diff --git a/tests/test-aio.c b/tests/test-aio.c index a109bd0..17a8ce9 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -100,6 +100,7 @@ static void event_ready_cb(EventNotifier *e) typedef struct { QemuMutex start_lock; + EventNotifier notifier; bool thread_acquired; } AcquireTestData; @@ -111,6 +112,11 @@ static void *test_acquire_thread(void *opaque) qemu_mutex_lock(&data->start_lock); qemu_mutex_unlock(&data->start_lock); + /* event_notifier_set might be called either before or after + * the main thread's call to poll(). The test case's outcome + * should be the same in either case. + */ + event_notifier_set(&data->notifier); aio_context_acquire(ctx); aio_context_release(ctx); @@ -125,20 +131,19 @@ static void set_event_notifier(AioContext *ctx, EventNotifier *notifier, aio_set_event_notifier(ctx, notifier, false, handler); } -static void dummy_notifier_read(EventNotifier *unused) +static void dummy_notifier_read(EventNotifier *n) { - g_assert(false); /* should never be invoked */ + event_notifier_test_and_clear(n); } static void test_acquire(void) { QemuThread thread; - EventNotifier notifier; AcquireTestData data; /* Dummy event notifier ensures aio_poll() will block */ - event_notifier_init(¬ifier, false); - set_event_notifier(ctx, ¬ifier, dummy_notifier_read); + event_notifier_init(&data.notifier, false); + set_event_notifier(ctx, &data.notifier, dummy_notifier_read); g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */ qemu_mutex_init(&data.start_lock); @@ -152,12 +157,13 @@ static void test_acquire(void) /* Block in aio_poll(), let other thread kick us and acquire context */ aio_context_acquire(ctx); qemu_mutex_unlock(&data.start_lock); /* let the thread run */ - g_assert(!aio_poll(ctx, true)); + g_assert(aio_poll(ctx, true)); + g_assert(!data.thread_acquired); aio_context_release(ctx); qemu_thread_join(&thread); - set_event_notifier(ctx, ¬ifier, NULL); - event_notifier_cleanup(¬ifier); + set_event_notifier(ctx, &data.notifier, NULL); + event_notifier_cleanup(&data.notifier); g_assert(data.thread_acquired); }