[Qemu-devel] [PATCH v1 0/3] Windows runtime improvements

[Qemu-devel] [PATCH v1 0/3] Windows runtime improvements

[Alistair Francis]

Changes since RFC:
 - Remove controversial timeout increate patch (util/oslib-win32:
   Recursivly pass the timeout) from the series
 - Small adjustments based on comments
 - Change from RFC to patch series

Original cover letter:

At Xilinx we have started to run our fork of QEMU on Windows and are
starting to distrubute this to customers. As part of this QEMU is
launched from an Eclipse based GUI on Windows hosts. This uncovered a
few performance issues in QEMU when running on CPU restricted machines.

What we see is that when QEMU is run on a machine where there aren't
enough spare CPUs on the host, QEMU is extreamly slow. We especially see
this when running our multi-architecture QEMU setup (MicroBlaze and ARM)
because we are running two QEMU instances as well as what else is
running on the machine.

Generally two instances of QEMU will never reach a Linux login prompt
when run on four host CPUs, but will reach the login prompt when run on
eight CPUs.

We investigated the issue and realised that it is mostly because QEMU
did not block and instead the IO thread just busy looped. This basically
locked up two CPUs (two QEMU instances) with IO threads not leaving
enough resources on the machine.

This patch series fixed the issue for us on our fork of QEMU. Our fork
is based on QEMU 2.8.1 and does not include MTTCG support. I have not
tested this on the mainline QEMU or with MTTCG. It is on my todo list to
see if I can repdouce the same issue on Windows with mainline QEMU. In
the meantime I wanted to send this series to see if anyone else has seen
Windows performance issues and if this helps with the problems. In order
to see the issue we had to do a full Linux boot, smaller baremetal
applications generally don't reproduce the same issue.

Also, most of these patches are editing the QEMU implementation of Glib,
obviously these fixes (if applicable) will need to be ported to Glib and
applied there as well. I just wanted to start here.

Alistair Francis (3):
  util/aio-win32: Only select on what we are actually waiting for
  util/oslib-win32: Remove invalid check
  util/oslib-win32: Remove if conditional

 util/aio-win32.c   | 13 ++++++++++---
 util/oslib-win32.c |  8 +++-----
 2 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v1 1/3] util/aio-win32: Only select on what we are actually waiting for

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
Changes since RFC:
 - Include more bitmasks for the select call

 util/aio-win32.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/util/aio-win32.c b/util/aio-win32.c
index bca496a47a..d6d5e02f00 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -71,6 +71,7 @@ void aio_set_fd_handler(AioContext *ctx,
         }
     } else {
         HANDLE event;
+        long bitmask = 0;
 
         if (node == NULL) {
             /* Alloc and insert if it's not already there */
@@ -95,10 +96,16 @@ void aio_set_fd_handler(AioContext *ctx,
         node->io_write = io_write;
         node->is_external = is_external;
 
+        if (io_read) {
+            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
+        }
+
+        if (io_write) {
+            bitmask |= FD_WRITE | FD_CONNECT;
+        }
+
         event = event_notifier_get_handle(&ctx->notifier);
-        WSAEventSelect(node->pfd.fd, event,
-                       FD_READ | FD_ACCEPT | FD_CLOSE |
-                       FD_CONNECT | FD_WRITE | FD_OOB);
+        WSAEventSelect(node->pfd.fd, event, bitmask);
     }
 
     qemu_lockcnt_unlock(&ctx->list_lock);
-- 
2.11.0

[Qemu-devel] [PATCH v1 2/3] util/oslib-win32: Remove invalid check

[Alistair Francis]

There is no way nhandles can be zero in this section so that part of the
if statement will always be false. Let's just remove it to make the code
easier to read.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Fam Zheng <famz@redhat.com>
---

 util/oslib-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 80e4668935..7ec0f8e083 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
         /* If we have a timeout, or no handles to poll, be satisfied
          * with just noticing we have messages waiting.
          */
-        if (timeout != 0 || nhandles == 0) {
+        if (timeout != 0) {
             return 1;
         }
 
-- 
2.11.0

[Qemu-devel] [PATCH v1 3/3] util/oslib-win32: Remove if conditional

[Alistair Francis]

The original ready < nhandles - 1 can be re-written as ready + 1 <
nhandles which is the same confition that we are checking on the first
itteration of the for loop. This means we can remove the if statement
and let the for loop check the code.

This also has the side effect of removing an invalid check as
WAIT_OBJECT_0 was not subtracted from ready.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 util/oslib-win32.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 7ec0f8e083..d42d695050 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -438,10 +438,8 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
         if (timeout == 0 && nhandles > 1) {
             /* Remove the handle that fired */
             int i;
-            if (ready < nhandles - 1) {
-                for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
-                    handles[i-1] = handles[i];
-                }
+            for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
+                handles[i-1] = handles[i];
             }
             nhandles--;
             recursed_result = poll_rest(FALSE, handles, nhandles, fds, nfds, 0);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v1 1/3] util/aio-win32: Only select on what we are actually waiting for

[Philippe Mathieu-Daudé]

On 06/29/2017 02:16 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> Changes since RFC:
>   - Include more bitmasks for the select call
> 
>   util/aio-win32.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index bca496a47a..d6d5e02f00 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -71,6 +71,7 @@ void aio_set_fd_handler(AioContext *ctx,
>           }
>       } else {
>           HANDLE event;
> +        long bitmask = 0;
>   
>           if (node == NULL) {
>               /* Alloc and insert if it's not already there */
> @@ -95,10 +96,16 @@ void aio_set_fd_handler(AioContext *ctx,
>           node->io_write = io_write;
>           node->is_external = is_external;
>   
> +        if (io_read) {
> +            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
> +        }
> +
> +        if (io_write) {
> +            bitmask |= FD_WRITE | FD_CONNECT;
> +        }
> +
>           event = event_notifier_get_handle(&ctx->notifier);
> -        WSAEventSelect(node->pfd.fd, event,
> -                       FD_READ | FD_ACCEPT | FD_CLOSE |
> -                       FD_CONNECT | FD_WRITE | FD_OOB);
> +        WSAEventSelect(node->pfd.fd, event, bitmask);
>       }
>   
>       qemu_lockcnt_unlock(&ctx->list_lock);
> 

Re: [Qemu-devel] [PATCH v1 2/3] util/oslib-win32: Remove invalid check

[Paolo Bonzini]

On 29/06/2017 19:16, Alistair Francis wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> 
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>          /* If we have a timeout, or no handles to poll, be satisfied
>           * with just noticing we have messages waiting.
>           */
> -        if (timeout != 0 || nhandles == 0) {
> +        if (timeout != 0) {
>              return 1;
>          }
>  

Can you explain this better?

Paolo

Re: [Qemu-devel] [PATCH v1 2/3] util/oslib-win32: Remove invalid check

[Peter Maydell]

On 29 June 2017 at 18:16, Alistair Francis <alistair.francis@xilinx.com> wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
>
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>          /* If we have a timeout, or no handles to poll, be satisfied
>           * with just noticing we have messages waiting.
>           */
> -        if (timeout != 0 || nhandles == 0) {
> +        if (timeout != 0) {
>              return 1;
>          }

The comment and the code now don't match -- does the comment
need updating too?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 0/3] Windows runtime improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v1 0/3]  Windows runtime improvements
Type: series
Message-id: cover.1498756113.git.alistair.francis@xilinx.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1499242883-2184-1-git-send-email-peterx@redhat.com -> patchew/1499242883-2184-1-git-send-email-peterx@redhat.com
Switched to a new branch 'test'
fede661 util/oslib-win32: Remove if conditional
f464b14 util/oslib-win32: Remove invalid check
29fb20d util/aio-win32: Only select on what we are actually waiting for

=== OUTPUT BEGIN ===
Checking PATCH 1/3: util/aio-win32: Only select on what we are actually waiting for...
Checking PATCH 2/3: util/oslib-win32: Remove invalid check...
Checking PATCH 3/3: util/oslib-win32: Remove if conditional...
ERROR: spaces required around that '-' (ctx:VxV)
#30: FILE: util/oslib-win32.c:442:
+                handles[i-1] = handles[i];
                          ^

total: 1 errors, 0 warnings, 12 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org