[Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Zavadovsky Yan]

Calling SuspendThread() is not enough to suspend Win32 thread.
We need to call GetThreadContext() after SuspendThread()
to make sure that OS have really suspended target thread.
But GetThreadContext() needs for THREAD_GET_CONTEXT
access right on thread object.

This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
and change 'while(GetThreadContext() == SUCCESS)' to
'while(GetThreadContext() == FAILED)'.
So this 'while' loop will stop only after successful grabbing
of thread context(i.e. when thread is really suspended).
Not after the one failed GetThreadContext() call.

Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
---
 cpus.c                   | 2 +-
 util/qemu-thread-win32.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cpus.c b/cpus.c
index b85fb5f..83d5eb5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
          * suspended until we can get the context.
          */
         tcgContext.ContextFlags = CONTEXT_CONTROL;
-        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
+        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
             continue;
         }
 
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 406b52f..823eca1 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
 
     EnterCriticalSection(&data->cs);
     if (!data->exited) {
-        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
-                            thread->tid);
+        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME | THREAD_GET_CONTEXT,
+                            FALSE, thread->tid);
     } else {
         handle = NULL;
     }
-- 
2.4.4.windows.2

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Stefan Weil]

Am 22.06.2015 um 23:54 schrieb Zavadovsky Yan:
> Calling SuspendThread() is not enough to suspend Win32 thread.
> We need to call GetThreadContext() after SuspendThread()
> to make sure that OS have really suspended target thread.
> But GetThreadContext() needs for THREAD_GET_CONTEXT
> access right on thread object.
>
> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
> and change 'while(GetThreadContext() == SUCCESS)' to
> 'while(GetThreadContext() == FAILED)'.
> So this 'while' loop will stop only after successful grabbing
> of thread context(i.e. when thread is really suspended).
> Not after the one failed GetThreadContext() call.
>
> Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
> ---
>   cpus.c                   | 2 +-
>   util/qemu-thread-win32.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index b85fb5f..83d5eb5 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>            * suspended until we can get the context.
>            */
>           tcgContext.ContextFlags = CONTEXT_CONTROL;
> -        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
> +        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>               continue;
>           }
>   
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 406b52f..823eca1 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>   
>       EnterCriticalSection(&data->cs);
>       if (!data->exited) {
> -        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
> -                            thread->tid);
> +        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME | THREAD_GET_CONTEXT,
> +                            FALSE, thread->tid);
>       } else {
>           handle = NULL;
>       }


I added the contributers of the original code to the cc list.

The modifications look reasonable - if GetThreadContext is needed at all.
We should add an URL to reliable documentation which supports that
claim.

Is it a good idea to run a busy waiting loop? Or would a Sleep(0) in
the loop be better (it allows other threads to run, maybe it helps
them to suspend, too).

Regards
Stefan

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Fabien Chouteau]

On 06/23/2015 08:02 AM, Stefan Weil wrote:
> Am 22.06.2015 um 23:54 schrieb Zavadovsky Yan:
>> Calling SuspendThread() is not enough to suspend Win32 thread.
>> We need to call GetThreadContext() after SuspendThread()
>> to make sure that OS have really suspended target thread.
>> But GetThreadContext() needs for THREAD_GET_CONTEXT
>> access right on thread object.
>>
>> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
>> and change 'while(GetThreadContext() == SUCCESS)' to
>> 'while(GetThreadContext() == FAILED)'.
>> So this 'while' loop will stop only after successful grabbing
>> of thread context(i.e. when thread is really suspended).
>> Not after the one failed GetThreadContext() call.
>>
>> Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
>> ---
>>   cpus.c                   | 2 +-
>>   util/qemu-thread-win32.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index b85fb5f..83d5eb5 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>            * suspended until we can get the context.
>>            */
>>           tcgContext.ContextFlags = CONTEXT_CONTROL;
>> -        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
>> +        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>>               continue;

This looks like a reasonable change, right now I don't understand why I
did it the other way...

>>           }
>>   diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>> index 406b52f..823eca1 100644
>> --- a/util/qemu-thread-win32.c
>> +++ b/util/qemu-thread-win32.c
>> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>>         EnterCriticalSection(&data->cs);
>>       if (!data->exited) {
>> -        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
>> -                            thread->tid);
>> +        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME | THREAD_GET_CONTEXT,
>> +                            FALSE, thread->tid);
>>       } else {
>>           handle = NULL;
>>       }
> 
> 
> I added the contributers of the original code to the cc list.
> 
> The modifications look reasonable - if GetThreadContext is needed at all.
> We should add an URL to reliable documentation which supports that
> claim.
>

The reason we need this call is on multi-processor host, when the TCG
thread and the IO-loop thread don't run on the same CPU.

So in this situation the function SuspendThread can return even before
the thread (running on another CPU) is effectively suspended.

Unfortunately this is not really documented by Microsoft an we found
that information somewhere on Internet (if you want I can search the
source again but there's nothing official) after countless hours of
debugging a very nasty race condition caused by this undocumented
behavior.

Maybe this is not explicit enough and the comments need to be updated.


> Is it a good idea to run a busy waiting loop? Or would a Sleep(0) in
> the loop be better (it allows other threads to run, maybe it helps
> them to suspend, too).
>

Maybe we can, but the "while" will only loop when threads are running on
different CPU, so the other thread is already running and calling sleep
will not help I think.

I hope this is clear, as I said we spent a huge amount of time debugging
this about a year and a half ago. The bug would append once every
several thousands tests. QEMU thread code is very "sensitive" on Windows
so we should be careful.

Yan, if you didn't already, I recommend you extensively test this
modification. By extensively, I mean running QEMU several thousands of
time on an SMP host (with many CPUs like 8 or 16 if possible).

Regards,

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Ян Завадовский]

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

Hello.

On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:

> Am 22.06.2015 um 23:54 schrieb Zavadovsky Yan:
>
>> Calling SuspendThread() is not enough to suspend Win32 thread.
>> We need to call GetThreadContext() after SuspendThread()
>> to make sure that OS have really suspended target thread.
>> But GetThreadContext() needs for THREAD_GET_CONTEXT
>> access right on thread object.
>>
>> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
>> and change 'while(GetThreadContext() == SUCCESS)' to
>> 'while(GetThreadContext() == FAILED)'.
>> So this 'while' loop will stop only after successful grabbing
>> of thread context(i.e. when thread is really suspended).
>> Not after the one failed GetThreadContext() call.
>>
>> Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
>> ---
>>   cpus.c                   | 2 +-
>>   util/qemu-thread-win32.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index b85fb5f..83d5eb5 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>            * suspended until we can get the context.
>>            */
>>           tcgContext.ContextFlags = CONTEXT_CONTROL;
>> -        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
>> +        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>>               continue;
>>           }
>>   diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>> index 406b52f..823eca1 100644
>> --- a/util/qemu-thread-win32.c
>> +++ b/util/qemu-thread-win32.c
>> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>>         EnterCriticalSection(&data->cs);
>>       if (!data->exited) {
>> -        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
>> -                            thread->tid);
>> +        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME |
>> THREAD_GET_CONTEXT,
>> +                            FALSE, thread->tid);
>>       } else {
>>           handle = NULL;
>>       }
>>
>
>
> I added the contributers of the original code to the cc list.
>
> The modifications look reasonable - if GetThreadContext is needed at all.
>
GetThreadContext is needed to avoid races on multicore system. As it wrote
in comment from original contributors of this code.

We should add an URL to reliable documentation which supports that
> claim.
>
Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
designed for debuggers. Don't use in applications.":
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
And nothing more useful.
So when I found this piece of code with Suspend/Resume and failed
GetContext I did some googling.
And found this article:
http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
In which:
>The Suspend­Thread function tells the scheduler to suspend the thread but
does not wait for an acknowledgment from the scheduler that the suspension
has actually occurred
>If you want to make sure the thread really is suspended, you need to
perform a synchronous operation that is dependent on the fact that the
thread is suspended
>The traditional way of doing this is to call Get­Thread­Context, since
this requires the kernel to read from the context of the suspended thread,
which has as a prerequisite that the context be saved in the first place,
which has as a prerequisite that the thread be suspended


> Is it a good idea to run a busy waiting loop?

I think no. Because infinite loop is possible. If someone make regress in
future.
Maybe better use same handling as Suspend/Resume uses?
'if (GetThreadContext() == FAILED) { print_error; error_exit; }'
GetThreadContext either works or not. So if it fails first call it will
fail all next calls.


> Or would a Sleep(0)

No. Sleep is "hardcode" without any guarantees.

[-- Attachment #2: Type: text/html, Size: 5745 bytes --]

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Ян Завадовский]

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

On Tue, Jun 23, 2015 at 12:49 PM, Fabien Chouteau <chouteau@adacore.com>
wrote:
>
> Maybe we can, but the "while" will only loop when threads are running on
> different CPU, so the other thread is already running and calling sleep
> will not help I think.
>
What you think about this:
diff --git a/cpus.c b/cpus.c
index 83d5eb5..2e71221 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1097,8 +1097,10 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
          * suspended until we can get the context.
          */
         tcgContext.ContextFlags = CONTEXT_CONTROL;
-        while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
-            continue;
+        if (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
+            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
+                    GetLastError());
+            exit(1);
         }

         cpu_signal(0);
Without THREAD_GET_CONTEXT flag this code instantly fails with error
message.
So if GetThreadContext will fail sometime in future we will get useful
stderr.txt.
Also GetThreadContext is synchronized function(at least this is wrotten by
authoritative MS man - see the link from my previous post). No need to ping
GetThreadContext until it works.


> Yan, if you didn't already, I recommend you extensively test this
> modification. By extensively, I mean running QEMU several thousands of
> time on an SMP host (with many CPUs like 8 or 16 if possible).
>
Temporarily now I have only my home 4-cores CPU.
But this mistake I found in March. Fix it and run Qemu many times. Mostly
with OVMF BIOS.

[-- Attachment #2: Type: text/html, Size: 2453 bytes --]

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Peter Maydell]

On 23 June 2015 at 10:55, Ян Завадовский <zavadovsky.yan@gmail.com> wrote:
> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:
>> We should add an URL to reliable documentation which supports that
>> claim.
>
> Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
> designed for debuggers. Don't use in applications.":
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
> And nothing more useful.
> So when I found this piece of code with Suspend/Resume and failed GetContext
> I did some googling.
> And found this article:
> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx

Personally I am happy to treat a Raymond Chen blog post as "reliable
documentation"...

-- PMM

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Paolo Bonzini]

On 23/06/2015 12:30, Peter Maydell wrote:
> On 23 June 2015 at 10:55, Ян Завадовский <zavadovsky.yan@gmail.com> wrote:
>> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:
>>> We should add an URL to reliable documentation which supports that
>>> claim.
>>
>> Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
>> designed for debuggers. Don't use in applications.":
>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
>> And nothing more useful.
>> So when I found this piece of code with Suspend/Resume and failed GetContext
>> I did some googling.
>> And found this article:
>> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
> 
> Personally I am happy to treat a Raymond Chen blog post as "reliable
> documentation"...

Me too. :)

SuspendThread was pretty much the only way to emulate signals.
Initially I used SetThreadContext to redirect execution to cpu_signal;
that was more complicated, but in retrospect it would have avoided the
problems with memory barriers and with asynchronous SuspendThread.  It
certainly would have saved the AdaCore people a lot of debugging time. :(

For 2.5, however, I wonder if SuspendThread/ResumeThread is needed at
all now that cpu_exit doesn't have to undo block chaining anymore.  Even
on POSIX platforms the signal might not be necessary anymore.

Paolo

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 12:46:59PM +0200, Paolo Bonzini wrote:
> 
> 
> On 23/06/2015 12:30, Peter Maydell wrote:
> > On 23 June 2015 at 10:55, Ян Завадовский <zavadovsky.yan@gmail.com> wrote:
> >> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:
> >>> We should add an URL to reliable documentation which supports that
> >>> claim.
> >>
> >> Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
> >> designed for debuggers. Don't use in applications.":
> >> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
> >> And nothing more useful.
> >> So when I found this piece of code with Suspend/Resume and failed GetContext
> >> I did some googling.
> >> And found this article:
> >> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
> > 
> > Personally I am happy to treat a Raymond Chen blog post as "reliable
> > documentation"...
> 
> Me too. :)
> 
> SuspendThread was pretty much the only way to emulate signals.
> Initially I used SetThreadContext to redirect execution to cpu_signal;
> that was more complicated, but in retrospect it would have avoided the
> problems with memory barriers and with asynchronous SuspendThread.  It
> certainly would have saved the AdaCore people a lot of debugging time. :(
> 
> For 2.5, however, I wonder if SuspendThread/ResumeThread is needed at
> all now that cpu_exit doesn't have to undo block chaining anymore.  Even
> on POSIX platforms the signal might not be necessary anymore.

If you don't have that signal / SuspendThread/ResumtThread requirement,
might that enable QEMU to just depend on the winpthreads library that
is provided by Mingw project, and not bother reinventing the wheel for
thread library portabilty ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Peter Maydell]

On 23 June 2015 at 11:46, Paolo Bonzini <pbonzini@redhat.com> wrote:
> SuspendThread was pretty much the only way to emulate signals.
> Initially I used SetThreadContext to redirect execution to cpu_signal;
> that was more complicated, but in retrospect it would have avoided the
> problems with memory barriers and with asynchronous SuspendThread.  It
> certainly would have saved the AdaCore people a lot of debugging time. :(
>
> For 2.5, however, I wonder if SuspendThread/ResumeThread is needed at
> all now that cpu_exit doesn't have to undo block chaining anymore.  Even
> on POSIX platforms the signal might not be necessary anymore.

Yeah, I was wondering that too. All we're really doing is setting
three flag variables, so the suspend/resume or signal is just getting
us atomicity of those flag changes. It might be possible to redesign
things a bit to not require the atomicity part.

-- PMM

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Paolo Bonzini]

On 23/06/2015 13:18, Daniel P. Berrange wrote:
> > For 2.5, however, I wonder if SuspendThread/ResumeThread is needed at
> > all now that cpu_exit doesn't have to undo block chaining anymore.  Even
> > on POSIX platforms the signal might not be necessary anymore.
> 
> If you don't have that signal / SuspendThread/ResumtThread requirement,

That was independent of QEMU reinventing the wheel for mutexes/condvars.

> might that enable QEMU to just depend on the winpthreads library that
> is provided by Mingw project, and not bother reinventing the wheel for
> thread library portabilty ?

We can and should just reuse glib these days as much as we can (probably
not entirely because glib doesn't have detached threads).  At least a
few years ago, winpthreads was much slower than native Win32, which is
why everyone reinvents the wheel.

I was planning to do it in 2.5.

Paolo

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 01:32:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 23/06/2015 13:18, Daniel P. Berrange wrote:
> > > For 2.5, however, I wonder if SuspendThread/ResumeThread is needed at
> > > all now that cpu_exit doesn't have to undo block chaining anymore.  Even
> > > on POSIX platforms the signal might not be necessary anymore.
> > 
> > If you don't have that signal / SuspendThread/ResumtThread requirement,
> 
> That was independent of QEMU reinventing the wheel for mutexes/condvars.
> 
> > might that enable QEMU to just depend on the winpthreads library that
> > is provided by Mingw project, and not bother reinventing the wheel for
> > thread library portabilty ?
> 
> We can and should just reuse glib these days as much as we can (probably
> not entirely because glib doesn't have detached threads).  At least a
> few years ago, winpthreads was much slower than native Win32, which is
> why everyone reinvents the wheel.

Are you sure that was wrt the (new) winpthreads library maintained by
Mingw64 team, and not the confusingly similar pthreads-win32 library ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Paolo Bonzini]

On 23/06/2015 13:43, Daniel P. Berrange wrote:
> > We can and should just reuse glib these days as much as we can (probably
> > not entirely because glib doesn't have detached threads).  At least a
> > few years ago, winpthreads was much slower than native Win32, which is
> > why everyone reinvents the wheel.
> 
> Are you sure that was wrt the (new) winpthreads library maintained by
> Mingw64 team, and not the confusingly similar pthreads-win32 library ?

None of them use native Win32 condition variables, which is the main
reason to switch to glib's version.

Paolo

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Stefan Weil]

Am 23.06.2015 um 12:46 schrieb Paolo Bonzini:
> On 23/06/2015 12:30, Peter Maydell wrote:
>> On 23 June 2015 at 10:55, Ян Завадовский <zavadovsky.yan@gmail.com> wrote:
>>> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:
>>>> We should add an URL to reliable documentation which supports that
>>>> claim.
>>> Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
>>> designed for debuggers. Don't use in applications.":
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
>>> And nothing more useful.
>>> So when I found this piece of code with Suspend/Resume and failed GetContext
>>> I did some googling.
>>> And found this article:
>>> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
>> Personally I am happy to treat a Raymond Chen blog post as "reliable
>> documentation"...
> Me too. :)

+1

Fabien, I wonder why nobody noticed that the current
code did not do what it was written for. As far as I see
the threads were created with the wrong options, so
GetThreadContext always failed and therefore was only
executed once, so there was no waiting for thread
suspension.

Removing the code would have given identical results.

Is that in an indicator that the SuspendThread is not
needed at all, as it was discussed in the other e-mails
here?

Stefan

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Fabien Chouteau]

On 06/23/2015 07:07 PM, Stefan Weil wrote:
> Am 23.06.2015 um 12:46 schrieb Paolo Bonzini:
>> On 23/06/2015 12:30, Peter Maydell wrote:
>>> On 23 June 2015 at 10:55, Ян Завадовский <zavadovsky.yan@gmail.com> wrote:
>>>> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <sw@weilnetz.de> wrote:
>>>>> We should add an URL to reliable documentation which supports that
>>>>> claim.
>>>> Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
>>>> designed for debuggers. Don't use in applications.":
>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
>>>> And nothing more useful.
>>>> So when I found this piece of code with Suspend/Resume and failed GetContext
>>>> I did some googling.
>>>> And found this article:
>>>> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
>>> Personally I am happy to treat a Raymond Chen blog post as "reliable
>>> documentation"...
>> Me too. :)
> 
> +1
> 
> Fabien, I wonder why nobody noticed that the current
> code did not do what it was written for. As far as I see
> the threads were created with the wrong options, so
> GetThreadContext always failed and therefore was only
> executed once, so there was no waiting for thread
> suspension.
> 

I'm surprised as well, but we run several hundred thousands of tests
every day (one QEMU instance for each test) and before this fix we had a
few instances freezing for no reason. We identified a possible race
condition on SMP host and the bug disappeared after this fix.

Even if the call was erroneous, adding a call to GetThreadContext
probably gave more time or forced the suspend request to be effective,
it's the only explanation I have right now.

But clearly there was a bug, and the call to GetThreadContext fixed it.
I found other pieces of code that uses this technique but calling
GetThreadContext only once (not in a loop like we did), so maybe it's
enough to call it once and the loop is superfluous...

> Removing the code would have given identical results.
>

Considering we are talking about thread synchronization on Windows and
SMP host, I would not make that assumption :)

> Is that in an indicator that the SuspendThread is not
> needed at all, as it was discussed in the other e-mails
> here?

If we completely change the thread synchronization on Windows, maybe
SuspendeThread is not needed anymore, but with the current scheme (at
least what I know of it), I don't see how we can remove it.

As I said before we must be very careful with this piece of code.

Regards,

Re: [Qemu-devel] [PATCH] thread-win32: fix GetThreadContext() permanently fails

[Peter Maydell]

On 24 June 2015 at 10:09, Fabien Chouteau <chouteau@adacore.com> wrote:
> If we completely change the thread synchronization on Windows, maybe
> SuspendeThread is not needed anymore, but with the current scheme (at
> least what I know of it), I don't see how we can remove it.
>
> As I said before we must be very careful with this piece of code.

Agreed; maybe for 2.5 we can consider a design fix that would
let us drop this suspend/resume stuff. For 2.4 we should just
make this correction to the current design, I think.

-- PMM