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