[PATCH 0/3] Some win32 fixes

[PATCH 0/3] Some win32 fixes

[Marc-André Lureau]

Hi,

Here are a few patches that should fix some busy looping issues
already reported >2y ago
(https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html),
and fixing test-char on win32.

hmm, do we have any automated testing/CI on Windows (beside just
cross-compilation)?

thanks

Marc-André Lureau (3):
  util: WSAEWOULDBLOCK on connect should map to EINPROGRESS
  tests: skip serial test on windows
  win32: fix main-loop busy loop on socket/fd event

 tests/test-char.c  | 4 ++--
 util/async.c       | 6 +++++-
 util/oslib-win32.c | 6 +++++-
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.23.0

[PATCH 1/3] util: WSAEWOULDBLOCK on connect should map to EINPROGRESS

[Marc-André Lureau]

In general, WSAEWOULDBLOCK can be mapped to EAGAIN as done by
socket_error() (or EWOULDBLOCK). But for connect() with non-blocking
sockets, it actually means the operation is in progress:

https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-connect
"The socket is marked as nonblocking and the connection cannot be completed immediately."

(this is also the behaviour implemented by GLib GSocket)

This fixes socket_can_bind_connect() test on win32.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/oslib-win32.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index c62cd4328c..886e400d6a 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -585,7 +585,11 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
     int ret;
     ret = connect(sockfd, addr, addrlen);
     if (ret < 0) {
-        errno = socket_error();
+        if (WSAGetLastError() == WSAEWOULDBLOCK) {
+            errno = EINPROGRESS;
+        } else {
+            errno = socket_error();
+        }
     }
     return ret;
 }
-- 
2.23.0

[PATCH 2/3] tests: skip serial test on windows

[Marc-André Lureau]

Serial test is currently hard-coded to /dev/null.

On Windows, serial chardev expect a COM: device, which may not be
availble.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-char.c b/tests/test-char.c
index d62de1b088..45e42af290 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -1103,7 +1103,7 @@ static void char_socket_server_two_clients_test(gconstpointer opaque)
 }
 
 
-#ifdef HAVE_CHARDEV_SERIAL
+#if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
 static void char_serial_test(void)
 {
     QemuOpts *opts;
@@ -1460,7 +1460,7 @@ int main(int argc, char **argv)
 #endif
 
     g_test_add_func("/char/udp", char_udp_test);
-#ifdef HAVE_CHARDEV_SERIAL
+#if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
     g_test_add_func("/char/serial", char_serial_test);
 #endif
     g_test_add_func("/char/hotswap", char_hotswap_test);
-- 
2.23.0

[PATCH 3/3] win32: fix main-loop busy loop on socket/fd event

[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()).

This is essentially a v2 of a patch that was sent earlier:
https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html

that resurfaced when James investigated Spice performance issues on Windows:
https://gitlab.freedesktop.org/spice/spice/issues/36

In order to test that patch, I simply tried running test-char on
win32, and it hangs. Applying that patch solves it. QIO idle sources
are not dispatched. I haven't investigated much further, I suspect
source priorities and busy looping still come into play.

This version keeps the "notified" field, so event_notifier_poll()
should still work as expected.

Cc: James Le Cuirot <chewi@gentoo.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/async.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/util/async.c b/util/async.c
index 4e4c7af51e..ca83e32c7f 100644
--- a/util/async.c
+++ b/util/async.c
@@ -354,7 +354,11 @@ void aio_notify(AioContext *ctx)
 
 void aio_notify_accept(AioContext *ctx)
 {
-    if (atomic_xchg(&ctx->notified, false)) {
+    if (atomic_xchg(&ctx->notified, false)
+#ifdef WIN32
+        || true
+#endif
+    ) {
         event_notifier_test_and_clear(&ctx->notifier);
     }
 }
-- 
2.23.0

Re: [PATCH 0/3] Some win32 fixes

[Paolo Bonzini]

On 01/10/19 15:26, Marc-André Lureau wrote:
> Hi,
> 
> Here are a few patches that should fix some busy looping issues
> already reported >2y ago
> (https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html),
> and fixing test-char on win32.

Queued thanks (with s/fix/work around/ in the commit subject of
patch 3).

> hmm, do we have any automated testing/CI on Windows (beside just
> cross-compilation)?

Nope. :(

Paolo

Re: [PATCH 0/3] Some win32 fixes

[Peter Maydell]

On Tue, 1 Oct 2019 at 14:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/10/19 15:26, Marc-André Lureau wrote:
> > hmm, do we have any automated testing/CI on Windows (beside just
> > cross-compilation)?
>
> Nope. :(

I did wonder whether it would be possible to get 'make check'
passing under Wine, but never investigated...

thanks
-- PMM

Re: [PATCH 2/3] tests: skip serial test on windows

[Philippe Mathieu-Daudé]

On 10/1/19 3:26 PM, Marc-André Lureau wrote:
> Serial test is currently hard-coded to /dev/null.
> 
> On Windows, serial chardev expect a COM: device, which may not be
> availble.

"available"

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   tests/test-char.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-char.c b/tests/test-char.c
> index d62de1b088..45e42af290 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1103,7 +1103,7 @@ static void char_socket_server_two_clients_test(gconstpointer opaque)
>   }
>   
>   
> -#ifdef HAVE_CHARDEV_SERIAL
> +#if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
>   static void char_serial_test(void)
>   {
>       QemuOpts *opts;
> @@ -1460,7 +1460,7 @@ int main(int argc, char **argv)
>   #endif
>   
>       g_test_add_func("/char/udp", char_udp_test);
> -#ifdef HAVE_CHARDEV_SERIAL
> +#if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
>       g_test_add_func("/char/serial", char_serial_test);
>   #endif
>       g_test_add_func("/char/hotswap", char_hotswap_test);
> 

Re: [PATCH 0/3] Some win32 fixes

[Philippe Mathieu-Daudé]

On 10/1/19 3:44 PM, Peter Maydell wrote:
> On Tue, 1 Oct 2019 at 14:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 01/10/19 15:26, Marc-André Lureau wrote:
>>> hmm, do we have any automated testing/CI on Windows (beside just
>>> cross-compilation)?
>>
>> Nope. :(
> 
> I did wonder whether it would be possible to get 'make check'
> passing under Wine, but never investigated...

Nelson Chen started to use Azure CI:

https://github.com/nelsonjchen/nc-qemu/blob/master/.azure/msys2-build.sh

He posted some notes here:

https://mindflakes.com/projects/nc-qemu/

I have this link in my CI TODO to try to upstream it and run 'make 
check' but it is not a high priority.

Re: [PATCH 1/3] util: WSAEWOULDBLOCK on connect should map to EINPROGRESS

[Philippe Mathieu-Daudé]

On 10/1/19 3:26 PM, Marc-André Lureau wrote:
> In general, WSAEWOULDBLOCK can be mapped to EAGAIN as done by
> socket_error() (or EWOULDBLOCK). But for connect() with non-blocking
> sockets, it actually means the operation is in progress:
> 
> https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-connect
> "The socket is marked as nonblocking and the connection cannot be completed immediately."
> 
> (this is also the behaviour implemented by GLib GSocket)
> 
> This fixes socket_can_bind_connect() test on win32.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   util/oslib-win32.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index c62cd4328c..886e400d6a 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -585,7 +585,11 @@ int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
>       int ret;
>       ret = connect(sockfd, addr, addrlen);
>       if (ret < 0) {
> -        errno = socket_error();
> +        if (WSAGetLastError() == WSAEWOULDBLOCK) {
> +            errno = EINPROGRESS;
> +        } else {
> +            errno = socket_error();
> +        }
>       }
>       return ret;
>   }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 0/3] Some win32 fixes

[Alex Bennée]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi,
>
> Here are a few patches that should fix some busy looping issues
> already reported >2y ago
> (https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html),
> and fixing test-char on win32.
>
> hmm, do we have any automated testing/CI on Windows (beside just
> cross-compilation)?

Not that I'm aware of. I did briefly look at CircleCI but never beyond
playing with my own experiments. Ultimately I find it hard to care about
Windows as I don't have a copy myself. Maybe someone else does care and
would like to explore further?

FWIW I believe Travis has Windows support now and there is also
AppVeyor. It's probably worth spreading the love around given how loaded
our Travis jobs are.

>
> thanks
>
> Marc-André Lureau (3):
>   util: WSAEWOULDBLOCK on connect should map to EINPROGRESS
>   tests: skip serial test on windows
>   win32: fix main-loop busy loop on socket/fd event
>
>  tests/test-char.c  | 4 ++--
>  util/async.c       | 6 +++++-
>  util/oslib-win32.c | 6 +++++-
>  3 files changed, 12 insertions(+), 4 deletions(-)


--
Alex Bennée

Re: [PATCH 0/3] Some win32 fixes

[Thomas Huth]

On 01/10/2019 15.26, Marc-André Lureau wrote:
> Hi,
> 
> Here are a few patches that should fix some busy looping issues
> already reported >2y ago
> (https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html),
> and fixing test-char on win32.
> 
> hmm, do we have any automated testing/CI on Windows (beside just
> cross-compilation)?

AFAIK we do not have any automated CI on Windows yet. But seems like
both, Travis and Cirrus, have Windows support, so you could add a test
to our .travis.yml or .cirrus.yml file, I think.

 Thomas

Re: [PATCH 0/3] Some win32 fixes

[Stefan Weil]

Am 01.10.2019 um 15:44 schrieb Peter Maydell:
> On Tue, 1 Oct 2019 at 14:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 01/10/19 15:26, Marc-André Lureau wrote:
>>> hmm, do we have any automated testing/CI on Windows (beside just
>>> cross-compilation)?
>> Nope. :(
> I did wonder whether it would be possible to get 'make check'
> passing under Wine, but never investigated...
>
> thanks
> -- PMM


Running "make check" under Wine had worked for me (well, at least
partially) in the past. Of course it is not a full replacement for tests
under Windows, but it should be sufficient to cover most of the
functionality. Usually I also use Wine to examine bug reports for Windows.

Are there test servers with Wine available?

Regards,
Stefan Weil

Re: [PATCH 3/3] win32: fix main-loop busy loop on socket/fd event

[James Le Cuirot]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On Tue,  1 Oct 2019 17:26:09 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> 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()).
> 
> This is essentially a v2 of a patch that was sent earlier:
> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00420.html
> 
> that resurfaced when James investigated Spice performance issues on Windows:
> https://gitlab.freedesktop.org/spice/spice/issues/36
> 
> In order to test that patch, I simply tried running test-char on
> win32, and it hangs. Applying that patch solves it. QIO idle sources
> are not dispatched. I haven't investigated much further, I suspect
> source priorities and busy looping still come into play.
> 
> This version keeps the "notified" field, so event_notifier_poll()
> should still work as expected.
> 
> Cc: James Le Cuirot <chewi@gentoo.org>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/async.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/util/async.c b/util/async.c
> index 4e4c7af51e..ca83e32c7f 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -354,7 +354,11 @@ void aio_notify(AioContext *ctx)
>  
>  void aio_notify_accept(AioContext *ctx)
>  {
> -    if (atomic_xchg(&ctx->notified, false)) {
> +    if (atomic_xchg(&ctx->notified, false)
> +#ifdef WIN32
> +        || true
> +#endif
> +    ) {
>          event_notifier_test_and_clear(&ctx->notifier);
>      }
>  }

I can confirm that this updated patch fixes my performance issue. The
idle CPU usage drops from around 35% to around 2%. Moving the mouse now
makes the usage go up, not down. :) Many thanks!

Regards,
-- 
James Le Cuirot (chewi)
Gentoo Linux Developer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]