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, Ян Завадовский <address@hidden> wrote:
>>>> On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <address@hidden> 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,