From patchwork Mon Jul 1 12:18:37 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 256110 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)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 5A7DD2C007C for ; Mon, 1 Jul 2013 22:27:52 +1000 (EST) Received: from localhost ([::1]:59374 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtdCv-0001LZ-DS for incoming@patchwork.ozlabs.org; Mon, 01 Jul 2013 08:27:49 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtdCZ-0001LP-1P for qemu-devel@nongnu.org; Mon, 01 Jul 2013 08:27:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Utd4a-0000pU-Ei for qemu-devel@nongnu.org; Mon, 01 Jul 2013 08:19:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48861) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utd4a-0000pP-2t for qemu-devel@nongnu.org; Mon, 01 Jul 2013 08:19:12 -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 (8.14.4/8.14.4) with ESMTP id r61CJBGq019147 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 1 Jul 2013 08:19:11 -0400 Received: from localhost (ovpn-112-23.ams2.redhat.com [10.36.112.23]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r61CJ6AS023525; Mon, 1 Jul 2013 08:19:07 -0400 From: Stefan Hajnoczi To: Date: Mon, 1 Jul 2013 14:18:37 +0200 Message-Id: <1372681129-18528-7-git-send-email-stefanha@redhat.com> In-Reply-To: <1372681129-18528-1-git-send-email-stefanha@redhat.com> References: <1372681129-18528-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 , Paolo Bonzini , Stefan Hajnoczi Subject: [Qemu-devel] [PATCH v5 06/18] aio: stop using .io_flush() 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 Now that aio_poll() users check their termination condition themselves, it is no longer necessary to call .io_flush() handlers. The behavior of aio_poll() changes as follows: 1. .io_flush() is no longer invoked and file descriptors are *always* monitored. Previously returning 0 from .io_flush() would skip this file descriptor. Due to this change it is essential to check that requests are pending before calling qemu_aio_wait(). Failure to do so means we block, for example, waiting for an idle iSCSI socket to become readable when there are no requests. Currently all qemu_aio_wait()/aio_poll() callers check before calling. 2. aio_poll() now returns true if progress was made (BH or fd handlers executed) and false otherwise. Previously it would return true whenever 'busy', which means that .io_flush() returned true. The 'busy' concept no longer exists so just progress is returned. Due to this change we need to update tests/test-aio.c which asserts aio_poll() return values. Note that QEMU doesn't actually rely on these return values so only tests/test-aio.c cares. Note that ctx->notifier, the EventNotifier fd used for aio_notify(), is now handled as a special case. This is a little ugly but maintains aio_poll() semantics, i.e. aio_notify() does not count as 'progress' and aio_poll() avoids blocking when the user has not set any fd handlers yet. Patches after this remove .io_flush() handler code until we can finally drop the io_flush arguments to aio_set_fd_handler() and friends. Reviewed-by: Paolo Bonzini Signed-off-by: Stefan Hajnoczi --- aio-posix.c | 29 +++++++++-------------------- aio-win32.c | 34 ++++++++++++++-------------------- tests/test-aio.c | 10 +++++----- 3 files changed, 28 insertions(+), 45 deletions(-) diff --git a/aio-posix.c b/aio-posix.c index b68eccd..7d66048 100644 --- a/aio-posix.c +++ b/aio-posix.c @@ -23,7 +23,6 @@ struct AioHandler GPollFD pfd; IOHandler *io_read; IOHandler *io_write; - AioFlushHandler *io_flush; int deleted; int pollfds_idx; void *opaque; @@ -84,7 +83,6 @@ void aio_set_fd_handler(AioContext *ctx, /* Update handler with latest information */ node->io_read = io_read; node->io_write = io_write; - node->io_flush = io_flush; node->opaque = opaque; node->pollfds_idx = -1; @@ -147,7 +145,11 @@ static bool aio_dispatch(AioContext *ctx) (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) && node->io_read) { node->io_read(node->opaque); - progress = true; + + /* aio_notify() does not count as progress */ + if (node->opaque != &ctx->notifier) { + progress = true; + } } if (!node->deleted && (revents & (G_IO_OUT | G_IO_ERR)) && @@ -173,7 +175,7 @@ bool aio_poll(AioContext *ctx, bool blocking) { AioHandler *node; int ret; - bool busy, progress; + bool progress; progress = false; @@ -200,20 +202,8 @@ bool aio_poll(AioContext *ctx, bool blocking) g_array_set_size(ctx->pollfds, 0); /* fill pollfds */ - busy = false; QLIST_FOREACH(node, &ctx->aio_handlers, node) { node->pollfds_idx = -1; - - /* If there aren't pending AIO operations, don't invoke callbacks. - * Otherwise, if there are no AIO requests, qemu_aio_wait() would - * wait indefinitely. - */ - if (!node->deleted && node->io_flush) { - if (node->io_flush(node->opaque) == 0) { - continue; - } - busy = true; - } if (!node->deleted && node->pfd.events) { GPollFD pfd = { .fd = node->pfd.fd, @@ -226,8 +216,8 @@ bool aio_poll(AioContext *ctx, bool blocking) ctx->walking_handlers--; - /* No AIO operations? Get us out of here */ - if (!busy) { + /* early return if we only have the aio_notify() fd */ + if (ctx->pollfds->len == 1) { return progress; } @@ -250,6 +240,5 @@ bool aio_poll(AioContext *ctx, bool blocking) } } - assert(progress || busy); - return true; + return progress; } diff --git a/aio-win32.c b/aio-win32.c index 38723bf..4309c16 100644 --- a/aio-win32.c +++ b/aio-win32.c @@ -23,7 +23,6 @@ struct AioHandler { EventNotifier *e; EventNotifierHandler *io_notify; - AioFlushEventNotifierHandler *io_flush; GPollFD pfd; int deleted; QLIST_ENTRY(AioHandler) node; @@ -73,7 +72,6 @@ void aio_set_event_notifier(AioContext *ctx, } /* Update handler with latest information */ node->io_notify = io_notify; - node->io_flush = io_flush; } aio_notify(ctx); @@ -96,7 +94,7 @@ bool aio_poll(AioContext *ctx, bool blocking) { AioHandler *node; HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; - bool busy, progress; + bool progress; int count; progress = false; @@ -126,7 +124,11 @@ bool aio_poll(AioContext *ctx, bool blocking) if (node->pfd.revents && node->io_notify) { node->pfd.revents = 0; node->io_notify(node->e); - progress = true; + + /* aio_notify() does not count as progress */ + if (node->opaque != &ctx->notifier) { + progress = true; + } } tmp = node; @@ -147,19 +149,8 @@ bool aio_poll(AioContext *ctx, bool blocking) ctx->walking_handlers++; /* fill fd sets */ - busy = false; count = 0; QLIST_FOREACH(node, &ctx->aio_handlers, node) { - /* If there aren't pending AIO operations, don't invoke callbacks. - * Otherwise, if there are no AIO requests, qemu_aio_wait() would - * wait indefinitely. - */ - if (!node->deleted && node->io_flush) { - if (node->io_flush(node->e) == 0) { - continue; - } - busy = true; - } if (!node->deleted && node->io_notify) { events[count++] = event_notifier_get_handle(node->e); } @@ -167,8 +158,8 @@ bool aio_poll(AioContext *ctx, bool blocking) ctx->walking_handlers--; - /* No AIO operations? Get us out of here */ - if (!busy) { + /* early return if we only have the aio_notify() fd */ + if (count == 1) { return progress; } @@ -196,7 +187,11 @@ bool aio_poll(AioContext *ctx, bool blocking) event_notifier_get_handle(node->e) == events[ret - WAIT_OBJECT_0] && node->io_notify) { node->io_notify(node->e); - progress = true; + + /* aio_notify() does not count as progress */ + if (node->opaque != &ctx->notifier) { + progress = true; + } } tmp = node; @@ -214,6 +209,5 @@ bool aio_poll(AioContext *ctx, bool blocking) events[ret - WAIT_OBJECT_0] = events[--count]; } - assert(progress || busy); - return true; + return progress; } diff --git a/tests/test-aio.c b/tests/test-aio.c index 20bf5e6..1251952 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -254,7 +254,7 @@ static void test_wait_event_notifier(void) EventNotifierTestData data = { .n = 0, .active = 1 }; event_notifier_init(&data.e, false); aio_set_event_notifier(ctx, &data.e, event_ready_cb, event_active_cb); - g_assert(aio_poll(ctx, false)); + g_assert(!aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 0); g_assert_cmpint(data.active, ==, 1); @@ -279,7 +279,7 @@ static void test_flush_event_notifier(void) EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true }; event_notifier_init(&data.e, false); aio_set_event_notifier(ctx, &data.e, event_ready_cb, event_active_cb); - g_assert(aio_poll(ctx, false)); + g_assert(!aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 0); g_assert_cmpint(data.active, ==, 10); @@ -313,7 +313,7 @@ static void test_wait_event_notifier_noflush(void) /* Until there is an active descriptor, aio_poll may or may not call * event_ready_cb. Still, it must not block. */ event_notifier_set(&data.e); - g_assert(!aio_poll(ctx, true)); + g_assert(aio_poll(ctx, true)); data.n = 0; /* An active event notifier forces aio_poll to look at EventNotifiers. */ @@ -323,13 +323,13 @@ static void test_wait_event_notifier_noflush(void) event_notifier_set(&data.e); g_assert(aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 1); - g_assert(aio_poll(ctx, false)); + g_assert(!aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 1); event_notifier_set(&data.e); g_assert(aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 2); - g_assert(aio_poll(ctx, false)); + g_assert(!aio_poll(ctx, false)); g_assert_cmpint(data.n, ==, 2); event_notifier_set(&dummy.e);