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

[Paolo Bonzini]

> On Thu, Jun 29, 2017 at 5:34 AM, Paolo Bonzini  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-block] [RFC v1 4/4] util/oslib-win32: Recursivly pass the timeout

[Alistair Francis]

On Thu, Jun 29, 2017 at 5:34 AM, Paolo Bonzini  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-block] [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-block] [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 
> Acked-by: Edgar E. Iglesias 
> ---
> 
>  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

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

[Alistair Francis]

Signed-off-by: Alistair Francis 
Acked-by: Edgar E. Iglesias 
---

 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