[Qemu-devel] [RFC v1 0/4] Windows runtime improvements

[Qemu-devel] [RFC v1 0/4] Windows runtime improvements

[Alistair Francis]

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 (4):
  util/aio-win32: Only select on what we are actually waiting for
  util/oslib-win32: Remove invalid check
  util/oslib-win32: Fix up if conditional
  util/oslib-win32: Recursivly pass the timeout

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

-- 
2.11.0

[Qemu-devel] [RFC v1 1/4] 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>
---

 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..949979c2f5 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;
+        }
+
+        if (io_write) {
+            bitmask |= FD_WRITE;
+        }
+
         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] [RFC v1 2/4] 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>
---

 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] [RFC v1 3/4] util/oslib-win32: Fix up if conditional

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 7ec0f8e083..a015e1ac96 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -438,7 +438,7 @@ 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) {
+            if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
                 for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
                     handles[i-1] = handles[i];
                 }
-- 
2.11.0

[Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---

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

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index a015e1ac96..3630e46499 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -432,10 +432,10 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
             }
         }
 
-        /* If no timeout and polling several handles, recurse to poll
-         * the rest of them.
+        /* We only found one and we are waiting on more then one. Let's try
+         * again.
          */
-        if (timeout == 0 && nhandles > 1) {
+        if (nhandles > 1) {
             /* Remove the handle that fired */
             int i;
             if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
@@ -444,7 +444,20 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
                 }
             }
             nhandles--;
-            recursed_result = poll_rest(FALSE, handles, nhandles, fds, nfds, 0);
+
+            /* If we just had a very small timeout let's increase it when we
+             * recurse to ensure we don't just busy wait. This ensures we let
+             * the Windows threads block at least a little. If we previously
+             * had some wait let's set it to zero to avoid blocking for too
+             * long.
+             */
+            if (timeout < 10) {
+                timeout = timeout + 1;
+            } else {
+                timeout = 0;
+            }
+            recursed_result = poll_rest(FALSE, handles, nhandles, fds,
+                                        nfds, timeout);
             return (recursed_result == -1) ? -1 : 1 + recursed_result;
         }
         return 1;
-- 
2.11.0

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

[Philippe Mathieu-Daudé]

On Tue, Jun 27, 2017 at 8:57 PM, 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>

> ---
>
>  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
>
>

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

[Fam Zheng]

On Tue, 06/27 16:57, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> 
>  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..949979c2f5 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;
> +        }
> +
> +        if (io_write) {
> +            bitmask |= FD_WRITE;
> +        }
> +
>          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);

Not sure if it's okay to drop accept/close/connect/oob altogether, Cc'ing Paolo
who knows Windows stuff.

Fam

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

[Fam Zheng]

On Tue, 06/27 16:57, 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>
> ---
> 
>  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
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v1 3/4] util/oslib-win32: Fix up if conditional

[Fam Zheng]

On Tue, 06/27 16:57, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 7ec0f8e083..a015e1ac96 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -438,7 +438,7 @@ 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) {
> +            if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
>                  for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
>                      handles[i-1] = handles[i];
>                  }
> -- 
> 2.11.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Fam Zheng]

On Tue, 06/27 16:57, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> 
>  util/oslib-win32.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index a015e1ac96..3630e46499 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -432,10 +432,10 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>              }
>          }
>  
> -        /* If no timeout and polling several handles, recurse to poll
> -         * the rest of them.
> +        /* We only found one and we are waiting on more then one. Let's try
> +         * again.
>           */
> -        if (timeout == 0 && nhandles > 1) {
> +        if (nhandles > 1) {
>              /* Remove the handle that fired */
>              int i;
>              if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
> @@ -444,7 +444,20 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>                  }
>              }
>              nhandles--;
> -            recursed_result = poll_rest(FALSE, handles, nhandles, fds, nfds, 0);
> +
> +            /* If we just had a very small timeout let's increase it when we
> +             * recurse to ensure we don't just busy wait. This ensures we let
> +             * the Windows threads block at least a little. If we previously
> +             * had some wait let's set it to zero to avoid blocking for too
> +             * long.
> +             */
> +            if (timeout < 10) {
> +                timeout = timeout + 1;
> +            } else {
> +                timeout = 0;
> +            }
> +            recursed_result = poll_rest(FALSE, handles, nhandles, fds,
> +                                        nfds, timeout);
>              return (recursed_result == -1) ? -1 : 1 + recursed_result;
>          }
>          return 1;
> -- 
> 2.11.0
> 

This is a hack, can we fix what is the causing the busy wait instead?

Fam

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

[Paolo Bonzini]

On 28/06/2017 01:57, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> 
>  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..949979c2f5 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;
> +        }
> +
> +        if (io_write) {
> +            bitmask |= FD_WRITE;
> +        }

The read case should also include FD_ACCEPT, and the write case should
also include FD_CONNECT.  FD_CLOSE probably also goes under "read",
since it's signaled when read()/recv() return 0.

FD_OOB is unnecessary, indeed.

Paolo

>          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] [RFC v1 3/4] util/oslib-win32: Fix up if conditional

[Paolo Bonzini]

On 28/06/2017 01:57, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 7ec0f8e083..a015e1ac96 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -438,7 +438,7 @@ 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) {
> +            if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
>                  for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
>                      handles[i-1] = handles[i];
>                  }
> 

WAIT_OBJECT_0 is zero, but I agree it's better to add it.

However, the condition can be rewritten as ready - WAIT_OBJECT_0 + 1 <
nhandles, which is the same as the first iteration's "i < nhandles".  So
I'd just remove the "if" and "unindent" the for loop.

Paolo

Re: [Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Paolo Bonzini]

On 28/06/2017 01:57, Alistair Francis wrote:
> +        /* We only found one and we are waiting on more then one. Let's try
> +         * again.
>           */
> -        if (timeout == 0 && nhandles > 1) {
> +        if (nhandles > 1) {
>              /* Remove the handle that fired */
>              int i;
>              if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
> @@ -444,7 +444,20 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>                  }
>              }
>              nhandles--;
> -            recursed_result = poll_rest(FALSE, handles, nhandles, fds, nfds, 0);
> +
> +            /* If we just had a very small timeout let's increase it when we
> +             * recurse to ensure we don't just busy wait. This ensures we let
> +             * the Windows threads block at least a little. If we previously
> +             * had some wait let's set it to zero to avoid blocking for too
> +             * long.
> +             */
> +            if (timeout < 10) {
> +                timeout = timeout + 1;
> +            } else {
> +                timeout = 0;
> +            }
> +            recursed_result = poll_rest(FALSE, handles, nhandles, fds,
> +                                        nfds, timeout);
>              return (recursed_result == -1) ? -1 : 1 + recursed_result;

I'm not sure I agree with this change, which is effectively delaying the
processing of events.  The question to me is which handles are
triggering so fast that QEMU effectively busy waits.

Maybe your QEMUs can get some breath with commit 12f8def0e0 ("win32:
replace custom mutex and condition variable with native primitives",
2017-03-27), since the native primitives are more efficient and TCG 2.8
used condvars a lot for qemu_io_proceeded_cond.

Paolo

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

[Paolo Bonzini]

On 28/06/2017 01:57, 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>
> ---
> 
>  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;
>          }
>  
> 

Hmm, I think it's possible, poll_msgs is true here.

Paolo

Re: [Qemu-devel] [RFC v1 3/4] util/oslib-win32: Fix up if conditional

[Alistair Francis]

On Thu, Jun 29, 2017 at 5:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 28/06/2017 01:57, Alistair Francis wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 7ec0f8e083..a015e1ac96 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -438,7 +438,7 @@ 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) {
>> +            if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
>>                  for (i = ready - WAIT_OBJECT_0 + 1; i < nhandles; i++) {
>>                      handles[i-1] = handles[i];
>>                  }
>>
>
> WAIT_OBJECT_0 is zero, but I agree it's better to add it.
>
> However, the condition can be rewritten as ready - WAIT_OBJECT_0 + 1 <
> nhandles, which is the same as the first iteration's "i < nhandles".  So
> I'd just remove the "if" and "unindent" the for loop.

Ah, good spot. I'll remove it.

Thanks,
Alistair

>
> Paolo

Re: [Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Alistair Francis]

On Thu, Jun 29, 2017 at 5:34 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 28/06/2017 01:57, Alistair Francis wrote:
>> +        /* We only found one and we are waiting on more then one. Let's try
>> +         * again.
>>           */
>> -        if (timeout == 0 && nhandles > 1) {
>> +        if (nhandles > 1) {
>>              /* Remove the handle that fired */
>>              int i;
>>              if ((ready - WAIT_OBJECT_0) < nhandles - 1) {
>> @@ -444,7 +444,20 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>>                  }
>>              }
>>              nhandles--;
>> -            recursed_result = poll_rest(FALSE, handles, nhandles, fds, nfds, 0);
>> +
>> +            /* If we just had a very small timeout let's increase it when we
>> +             * recurse to ensure we don't just busy wait. This ensures we let
>> +             * the Windows threads block at least a little. If we previously
>> +             * had some wait let's set it to zero to avoid blocking for too
>> +             * long.
>> +             */
>> +            if (timeout < 10) {
>> +                timeout = timeout + 1;
>> +            } else {
>> +                timeout = 0;
>> +            }
>> +            recursed_result = poll_rest(FALSE, handles, nhandles, fds,
>> +                                        nfds, timeout);
>>              return (recursed_result == -1) ? -1 : 1 + recursed_result;
>
> I'm not sure I agree with this change, which is effectively delaying the
> processing of events.  The question to me is which handles are
> triggering so fast that QEMU effectively busy waits.

Yeah, that is what I was trying to figure out, but didn't make much headway.

I kept seeing zero timeouts, which means that the thread never blocks
and this patch helps a lot.

>
> Maybe your QEMUs can get some breath with commit 12f8def0e0 ("win32:
> replace custom mutex and condition variable with native primitives",
> 2017-03-27), since the native primitives are more efficient and TCG 2.8
> used condvars a lot for qemu_io_proceeded_cond.

Ok, I will try that.

Does this mean you don't see the same slowness on QEMU 2.9?

Thanks,
Alistair

>
> Paolo

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

[Alistair Francis]

On Thu, Jun 29, 2017 at 6:32 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 28/06/2017 01:57, 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>
>> ---
>>
>>  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;
>>          }
>>
>>
>
> Hmm, I think it's possible, poll_msgs is true here.

poll_msgs?

If nhandles is 0 then we have already entered an earlier if statement
and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
can't enter this part of the if statement.

Thanks,
Alistair

>
> Paolo

Re: [Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Paolo Bonzini]

> On Thu, Jun 29, 2017 at 5:34 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 28/06/2017 01:57, Alistair Francis wrote:
> > I'm not sure I agree with this change, which is effectively delaying the
> > processing of events.  The question to me is which handles are
> > triggering so fast that QEMU effectively busy waits.
> 
> Yeah, that is what I was trying to figure out, but didn't make much headway.
> 
> I kept seeing zero timeouts, which means that the thread never blocks
> and this patch helps a lot.

Perhaps you can use tracepoints?  There shouldn't be many handles registered,
since on Windows even the GUI actions all go through messages.

> > Maybe your QEMUs can get some breath with commit 12f8def0e0 ("win32:
> > replace custom mutex and condition variable with native primitives",
> > 2017-03-27), since the native primitives are more efficient and TCG 2.8
> > used condvars a lot for qemu_io_proceeded_cond.
> 
> Ok, I will try that.
> 
> Does this mean you don't see the same slowness on QEMU 2.9?

I have not tried, but the patch is only working around the real issue,
as Fam pointed out.

Paolo

Re: [Qemu-devel] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Alistair Francis]

On Thu, Jun 29, 2017 at 12:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On Thu, Jun 29, 2017 at 5:34 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > On 28/06/2017 01:57, Alistair Francis wrote:
>> > I'm not sure I agree with this change, which is effectively delaying the
>> > processing of events.  The question to me is which handles are
>> > triggering so fast that QEMU effectively busy waits.
>>
>> Yeah, that is what I was trying to figure out, but didn't make much headway.
>>
>> I kept seeing zero timeouts, which means that the thread never blocks
>> and this patch helps a lot.
>
> Perhaps you can use tracepoints?  There shouldn't be many handles registered,
> since on Windows even the GUI actions all go through messages.
>
>> > Maybe your QEMUs can get some breath with commit 12f8def0e0 ("win32:
>> > replace custom mutex and condition variable with native primitives",
>> > 2017-03-27), since the native primitives are more efficient and TCG 2.8
>> > used condvars a lot for qemu_io_proceeded_cond.
>>
>> Ok, I will try that.

No luck, I tried applying that and it didn't make any difference.

>>
>> Does this mean you don't see the same slowness on QEMU 2.9?
>
> I have not tried, but the patch is only working around the real issue,
> as Fam pointed out.

Agreed. I'll keep digging and see what I can find.

Thanks,
Alistair

>
> Paolo
>

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

[Paolo Bonzini]

On 29/06/2017 18:37, Alistair Francis wrote:
>> Hmm, I think it's possible, poll_msgs is true here.
> poll_msgs?
> 
> If nhandles is 0 then we have already entered an earlier if statement
> and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
> can't enter this part of the if statement.

No, that's not correct.  The code is:

    if (poll_msgs) {
        /* Wait for either messages or handles
         * -> Use MsgWaitForMultipleObjectsEx
         */
        ready = MsgWaitForMultipleObjectsEx(nhandles, handles, timeout,
                                            QS_ALLINPUT, MWMO_ALERTABLE);

        if (ready == WAIT_FAILED) {
            gchar *emsg = g_win32_error_message(GetLastError());
            g_warning("MsgWaitForMultipleObjectsEx failed: %s", emsg);
            g_free(emsg);
        }
    } else if (nhandles == 0) {
        /* No handles to wait for, just the timeout */
        if (timeout == INFINITE) {
            ready = WAIT_FAILED;
        } else {
            SleepEx(timeout, TRUE);
            ready = WAIT_TIMEOUT;
        }

You can have poll_msgs == TRUE && nhandles == 0.  This happens for

   GPollFD fds[1] = { .fd = G_WIN32_MSG_HANDLE, .events = G_IO_IN };
   g_poll(fds, 1, timeout);

Thanks,

Paolo

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

[Alistair Francis]

On Fri, Jun 30, 2017 at 3:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 29/06/2017 18:37, Alistair Francis wrote:
>>> Hmm, I think it's possible, poll_msgs is true here.
>> poll_msgs?
>>
>> If nhandles is 0 then we have already entered an earlier if statement
>> and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
>> can't enter this part of the if statement.
>
> No, that's not correct.  The code is:
>
>     if (poll_msgs) {
>         /* Wait for either messages or handles
>          * -> Use MsgWaitForMultipleObjectsEx
>          */
>         ready = MsgWaitForMultipleObjectsEx(nhandles, handles, timeout,
>                                             QS_ALLINPUT, MWMO_ALERTABLE);
>
>         if (ready == WAIT_FAILED) {
>             gchar *emsg = g_win32_error_message(GetLastError());
>             g_warning("MsgWaitForMultipleObjectsEx failed: %s", emsg);
>             g_free(emsg);
>         }
>     } else if (nhandles == 0) {
>         /* No handles to wait for, just the timeout */
>         if (timeout == INFINITE) {
>             ready = WAIT_FAILED;
>         } else {
>             SleepEx(timeout, TRUE);
>             ready = WAIT_TIMEOUT;
>         }
>
> You can have poll_msgs == TRUE && nhandles == 0.  This happens for
>
>    GPollFD fds[1] = { .fd = G_WIN32_MSG_HANDLE, .events = G_IO_IN };
>    g_poll(fds, 1, timeout);

Ah. Yeah good point.

Ok, I'll respin the series without this patch then.

Thanks,
Alistair

>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [RFC v1 0/4] Windows runtime improvements

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: Cannot update paths and switch to branch 'test' at the same time.
Did you intend to checkout 'origin/patchew/cover.1498607452.git.alistair.francis@xilinx.com' which can not be resolved as commit?
Traceback (most recent call last):
  File "/home/fam/bin/patchew", line 440, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/fam/bin/patchew", line 53, in git_clone_repo
    cwd=clone)
  File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/cover.1498607452.git.alistair.francis@xilinx.com', '-b', 'test']' returned non-zero exit status 128



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

Re: [Qemu-devel] [RFC v1 0/4] Windows runtime improvements

[Fam Zheng]

On Thu, 07/06 16:48, no-reply@patchew.org wrote:
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> fatal: Cannot update paths and switch to branch 'test' at the same time.
> Did you intend to checkout 'origin/patchew/cover.1498607452.git.alistair.francis@xilinx.com' which can not be resolved as commit?
> Traceback (most recent call last):
>   File "/home/fam/bin/patchew", line 440, in test_one
>     git_clone_repo(clone, r["repo"], r["head"], logf)
>   File "/home/fam/bin/patchew", line 53, in git_clone_repo
>     cwd=clone)
>   File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/cover.1498607452.git.alistair.francis@xilinx.com', '-b', 'test']' returned non-zero exit status 128

Ignore this please, patchew is recovering from a bad state.