* [Qemu-devel] [PATCH] win32: fix main-loop busy loop on socket/fd event
@ 2017-01-04 20:59 Marc-André Lureau
2017-01-04 21:19 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Marc-André Lureau @ 2017-01-04 20:59 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Marc-André Lureau
Commit 05e514b1d4d5bd4209e2c8bbc76ff05c85a235f3 introduced an AIO
context optimization to avoid calling event_notifier_test_and_clear() on
ctx->notifier. On Windows, the same notifier is being used to wakeup the
wait on socket events (see commit
d3385eb448e38f828c78f8f68ec5d79c66a58b5d).
The ctx->notifier event is added to the gpoll sources in
aio_set_event_notifier(), aio_ctx_check() should clear the event
regardless of ctx->notified, since Windows sets the event by itself,
bypassing the aio->notified. This fixes qemu not clearing the event
resulting in a busy loop.
Paolo suggested to me on irc to call event_notifier_test_and_clear()
after select() >0 from aio-win32.c's aio_prepare. Unfortunately, not all
fds associated with ctx->notifiers are in AIO fd handlers set.
(qemu_set_nonblock() in util/oslib-win32.c calls qemu_fd_register()).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/block/aio.h | 2 ++
async.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/include/block/aio.h b/include/block/aio.h
index ca551e346f..a6da135bf3 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -100,6 +100,7 @@ struct AioContext {
*/
int walking_bh;
+#ifndef _WIN32
/* Used by aio_notify.
*
* "notified" is used to avoid expensive event_notifier_test_and_clear
@@ -113,6 +114,7 @@ struct AioContext {
* in the docs/aio_notify_accept.promela formal model.
*/
bool notified;
+#endif
EventNotifier notifier;
/* Thread pool for performing work and receiving completion callbacks */
diff --git a/async.c b/async.c
index b2de360c23..8c2a68b6cc 100644
--- a/async.c
+++ b/async.c
@@ -329,15 +329,21 @@ void aio_notify(AioContext *ctx)
smp_mb();
if (ctx->notify_me) {
event_notifier_set(&ctx->notifier);
+#ifndef _WIN32
atomic_mb_set(&ctx->notified, true);
+#endif
}
}
void aio_notify_accept(AioContext *ctx)
{
+#ifndef _WIN32
if (atomic_xchg(&ctx->notified, false)) {
+#endif
event_notifier_test_and_clear(&ctx->notifier);
+#ifndef _WIN32
}
+#endif
}
static void aio_timerlist_notify(void *opaque)
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] win32: fix main-loop busy loop on socket/fd event
2017-01-04 20:59 [Qemu-devel] [PATCH] win32: fix main-loop busy loop on socket/fd event Marc-André Lureau
@ 2017-01-04 21:19 ` Paolo Bonzini
2017-01-04 21:39 ` Marc-André Lureau
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2017-01-04 21:19 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> Paolo suggested to me on irc to call event_notifier_test_and_clear()
> after select() >0 from aio-win32.c's aio_prepare. Unfortunately, not all
> fds associated with ctx->notifiers are in AIO fd handlers set.
> (qemu_set_nonblock() in util/oslib-win32.c calls qemu_fd_register()).
That makes sense. Out of curiosity, what is a practical case of a socket
that is nonblocking but doesn't have an attached handler?
Another possibility (this one requires much more attention to avoid missing
some edge case; however, it should be easy to verify if it fixes the busy
loop) could be to move aio_notify_accept to just before setting ctx->notify_me.
This would work for both aio-posix and aio-win32.
Paolo
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/block/aio.h | 2 ++
> async.c | 6 ++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index ca551e346f..a6da135bf3 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -100,6 +100,7 @@ struct AioContext {
> */
> int walking_bh;
>
> +#ifndef _WIN32
> /* Used by aio_notify.
> *
> * "notified" is used to avoid expensive event_notifier_test_and_clear
> @@ -113,6 +114,7 @@ struct AioContext {
> * in the docs/aio_notify_accept.promela formal model.
> */
> bool notified;
> +#endif
> EventNotifier notifier;
>
> /* Thread pool for performing work and receiving completion callbacks */
> diff --git a/async.c b/async.c
> index b2de360c23..8c2a68b6cc 100644
> --- a/async.c
> +++ b/async.c
> @@ -329,15 +329,21 @@ void aio_notify(AioContext *ctx)
> smp_mb();
> if (ctx->notify_me) {
> event_notifier_set(&ctx->notifier);
> +#ifndef _WIN32
> atomic_mb_set(&ctx->notified, true);
> +#endif
> }
> }
>
> void aio_notify_accept(AioContext *ctx)
> {
> +#ifndef _WIN32
> if (atomic_xchg(&ctx->notified, false)) {
> +#endif
> event_notifier_test_and_clear(&ctx->notifier);
> +#ifndef _WIN32
> }
> +#endif
> }
>
> static void aio_timerlist_notify(void *opaque)
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] win32: fix main-loop busy loop on socket/fd event
2017-01-04 21:19 ` Paolo Bonzini
@ 2017-01-04 21:39 ` Marc-André Lureau
0 siblings, 0 replies; 3+ messages in thread
From: Marc-André Lureau @ 2017-01-04 21:39 UTC (permalink / raw)
To: Paolo Bonzini, Daniel P. Berrange; +Cc: qemu-devel
Hi
On Wed, Jan 4, 2017 at 10:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> > Paolo suggested to me on irc to call event_notifier_test_and_clear()
> > after select() >0 from aio-win32.c's aio_prepare. Unfortunately, not all
> > fds associated with ctx->notifiers are in AIO fd handlers set.
> > (qemu_set_nonblock() in util/oslib-win32.c calls qemu_fd_register()).
>
> That makes sense. Out of curiosity, what is a practical case of a socket
> that is nonblocking but doesn't have an attached handler?
>
>
Good question, qio_channel_socket_set_blocking() calls qemu_set_nonblock().
But it seems to also use its own source handler
qio_channel_create_socket_watch(), so the AIO notifier is probably needless
here.
Another possibility (this one requires much more attention to avoid missing
> some edge case; however, it should be easy to verify if it fixes the busy
> loop) could be to move aio_notify_accept to just before setting
> ctx->notify_me.
> This would work for both aio-posix and aio-win32.
>
>
--- a/async.c
+++ b/async.c
@@ -232,8 +232,8 @@ aio_ctx_check(GSource *source)
AioContext *ctx = (AioContext *) source;
QEMUBH *bh;
- atomic_and(&ctx->notify_me, ~1);
aio_notify_accept(ctx);
+ atomic_and(&ctx->notify_me, ~1);
That? doesn't work here.
thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-01-04 21:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-04 20:59 [Qemu-devel] [PATCH] win32: fix main-loop busy loop on socket/fd event Marc-André Lureau
2017-01-04 21:19 ` Paolo Bonzini
2017-01-04 21:39 ` Marc-André Lureau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.