[Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives

Andrew Baumann posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170324220141.10104-1-Andrew.Baumann@microsoft.com
Test checkpatch passed
Test docker passed
Test s390x passed
include/qemu/thread-win32.h |   7 +--
util/qemu-thread-win32.c    | 136 +++++---------------------------------------
2 files changed, 17 insertions(+), 126 deletions(-)
[Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Andrew Baumann 7 years ago
From: Andrey Shedel <ashedel@microsoft.com>

The multithreaded TCG implementation exposed deadlocks in the win32
condition variables: as implemented, qemu_cond_broadcast waited on
receivers, whereas the pthreads API it was intended to emulate does
not. This was causing a deadlock because broadcast was called while
holding the IO lock, as well as all possible waiters blocked on the
same lock.

This patch replaces all the custom synchronisation code for mutexes
and condition variables with native Windows primitives (SRWlocks and
condition variables) with the same semantics as their POSIX
equivalents. To enable that, it requires a Windows Vista or newer host
OS.

[AB: edited commit message]
Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
The major implication of this patch is that it drops support for
pre-Vista versions of Windows. However, those OSes are past their end
of life, and other OSS projects have dropped support. e.g.; the last
Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
good tradeoff to invest effort in fixing broken code needed to support
them, so hopefully this isn't too controversial.

 include/qemu/thread-win32.h |   7 +--
 util/qemu-thread-win32.c    | 136 +++++---------------------------------------
 2 files changed, 17 insertions(+), 126 deletions(-)

diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
index 5fb6541..4c4a261 100644
--- a/include/qemu/thread-win32.h
+++ b/include/qemu/thread-win32.h
@@ -4,8 +4,7 @@
 #include <windows.h>
 
 struct QemuMutex {
-    CRITICAL_SECTION lock;
-    LONG owner;
+    SRWLOCK lock;
 };
 
 typedef struct QemuRecMutex QemuRecMutex;
@@ -19,9 +18,7 @@ int qemu_rec_mutex_trylock(QemuRecMutex *mutex);
 void qemu_rec_mutex_unlock(QemuRecMutex *mutex);
 
 struct QemuCond {
-    LONG waiters, target;
-    HANDLE sema;
-    HANDLE continue_event;
+    CONDITION_VARIABLE var;
 };
 
 struct QemuSemaphore {
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 29c3e4d..59befd5 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -10,6 +10,11 @@
  * See the COPYING file in the top-level directory.
  *
  */
+
+#ifndef _WIN32_WINNT
+#define _WIN32_WINNT 0x0600
+#endif
+
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/thread.h"
@@ -39,44 +44,30 @@ static void error_exit(int err, const char *msg)
 
 void qemu_mutex_init(QemuMutex *mutex)
 {
-    mutex->owner = 0;
-    InitializeCriticalSection(&mutex->lock);
+    InitializeSRWLock(&mutex->lock);
 }
 
 void qemu_mutex_destroy(QemuMutex *mutex)
 {
-    assert(mutex->owner == 0);
-    DeleteCriticalSection(&mutex->lock);
+    InitializeSRWLock(&mutex->lock);
 }
 
 void qemu_mutex_lock(QemuMutex *mutex)
 {
-    EnterCriticalSection(&mutex->lock);
-
-    /* Win32 CRITICAL_SECTIONs are recursive.  Assert that we're not
-     * using them as such.
-     */
-    assert(mutex->owner == 0);
-    mutex->owner = GetCurrentThreadId();
+    AcquireSRWLockExclusive(&mutex->lock);
 }
 
 int qemu_mutex_trylock(QemuMutex *mutex)
 {
     int owned;
 
-    owned = TryEnterCriticalSection(&mutex->lock);
-    if (owned) {
-        assert(mutex->owner == 0);
-        mutex->owner = GetCurrentThreadId();
-    }
+    owned = TryAcquireSRWLockExclusive(&mutex->lock);
     return !owned;
 }
 
 void qemu_mutex_unlock(QemuMutex *mutex)
 {
-    assert(mutex->owner == GetCurrentThreadId());
-    mutex->owner = 0;
-    LeaveCriticalSection(&mutex->lock);
+    ReleaseSRWLockExclusive(&mutex->lock);
 }
 
 void qemu_rec_mutex_init(QemuRecMutex *mutex)
@@ -107,124 +98,27 @@ void qemu_rec_mutex_unlock(QemuRecMutex *mutex)
 void qemu_cond_init(QemuCond *cond)
 {
     memset(cond, 0, sizeof(*cond));
-
-    cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
-    if (!cond->sema) {
-        error_exit(GetLastError(), __func__);
-    }
-    cond->continue_event = CreateEvent(NULL,    /* security */
-                                       FALSE,   /* auto-reset */
-                                       FALSE,   /* not signaled */
-                                       NULL);   /* name */
-    if (!cond->continue_event) {
-        error_exit(GetLastError(), __func__);
-    }
+    InitializeConditionVariable(&cond->var);
 }
 
 void qemu_cond_destroy(QemuCond *cond)
 {
-    BOOL result;
-    result = CloseHandle(cond->continue_event);
-    if (!result) {
-        error_exit(GetLastError(), __func__);
-    }
-    cond->continue_event = 0;
-    result = CloseHandle(cond->sema);
-    if (!result) {
-        error_exit(GetLastError(), __func__);
-    }
-    cond->sema = 0;
+    InitializeConditionVariable(&cond->var);
 }
 
 void qemu_cond_signal(QemuCond *cond)
 {
-    DWORD result;
-
-    /*
-     * Signal only when there are waiters.  cond->waiters is
-     * incremented by pthread_cond_wait under the external lock,
-     * so we are safe about that.
-     */
-    if (cond->waiters == 0) {
-        return;
-    }
-
-    /*
-     * Waiting threads decrement it outside the external lock, but
-     * only if another thread is executing pthread_cond_broadcast and
-     * has the mutex.  So, it also cannot be decremented concurrently
-     * with this particular access.
-     */
-    cond->target = cond->waiters - 1;
-    result = SignalObjectAndWait(cond->sema, cond->continue_event,
-                                 INFINITE, FALSE);
-    if (result == WAIT_ABANDONED || result == WAIT_FAILED) {
-        error_exit(GetLastError(), __func__);
-    }
+    WakeConditionVariable(&cond->var);
 }
 
 void qemu_cond_broadcast(QemuCond *cond)
 {
-    BOOLEAN result;
-    /*
-     * As in pthread_cond_signal, access to cond->waiters and
-     * cond->target is locked via the external mutex.
-     */
-    if (cond->waiters == 0) {
-        return;
-    }
-
-    cond->target = 0;
-    result = ReleaseSemaphore(cond->sema, cond->waiters, NULL);
-    if (!result) {
-        error_exit(GetLastError(), __func__);
-    }
-
-    /*
-     * At this point all waiters continue. Each one takes its
-     * slice of the semaphore. Now it's our turn to wait: Since
-     * the external mutex is held, no thread can leave cond_wait,
-     * yet. For this reason, we can be sure that no thread gets
-     * a chance to eat *more* than one slice. OTOH, it means
-     * that the last waiter must send us a wake-up.
-     */
-    WaitForSingleObject(cond->continue_event, INFINITE);
+    WakeAllConditionVariable(&cond->var);
 }
 
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 {
-    /*
-     * This access is protected under the mutex.
-     */
-    cond->waiters++;
-
-    /*
-     * Unlock external mutex and wait for signal.
-     * NOTE: we've held mutex locked long enough to increment
-     * waiters count above, so there's no problem with
-     * leaving mutex unlocked before we wait on semaphore.
-     */
-    qemu_mutex_unlock(mutex);
-    WaitForSingleObject(cond->sema, INFINITE);
-
-    /* Now waiters must rendez-vous with the signaling thread and
-     * let it continue.  For cond_broadcast this has heavy contention
-     * and triggers thundering herd.  So goes life.
-     *
-     * Decrease waiters count.  The mutex is not taken, so we have
-     * to do this atomically.
-     *
-     * All waiters contend for the mutex at the end of this function
-     * until the signaling thread relinquishes it.  To ensure
-     * each waiter consumes exactly one slice of the semaphore,
-     * the signaling thread stops until it is told by the last
-     * waiter that it can go on.
-     */
-    if (InterlockedDecrement(&cond->waiters) == cond->target) {
-        SetEvent(cond->continue_event);
-    }
-
-    qemu_mutex_lock(mutex);
+    SleepConditionVariableSRW(&cond->var, &mutex->lock, INFINITE, 0);
 }
 
 void qemu_sem_init(QemuSemaphore *sem, int init)
-- 
2.8.3


Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Paolo Bonzini 7 years ago
> The major implication of this patch is that it drops support for
> pre-Vista versions of Windows. However, those OSes are past their end
> of life, and other OSS projects have dropped support. e.g.; the last
> Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> good tradeoff to invest effort in fixing broken code needed to support
> them, so hopefully this isn't too controversial.

Certainly fine by me!

The original reason to introduce these was that Wine didn't
support SRWLocks, but now it does.

Paolo

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Paolo Bonzini 7 years ago
> From: Andrey Shedel <ashedel@microsoft.com>
> 
> The multithreaded TCG implementation exposed deadlocks in the win32
> condition variables: as implemented, qemu_cond_broadcast waited on
> receivers, whereas the pthreads API it was intended to emulate does
> not. This was causing a deadlock because broadcast was called while
> holding the IO lock, as well as all possible waiters blocked on the
> same lock.
> 
> This patch replaces all the custom synchronisation code for mutexes
> and condition variables with native Windows primitives (SRWlocks and
> condition variables) with the same semantics as their POSIX
> equivalents. To enable that, it requires a Windows Vista or newer host
> OS.
> 
> [AB: edited commit message]
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Oops, just a nit but an important one: there should be a
Signed-off-by for Andrey as well.

Paolo

> ---
> The major implication of this patch is that it drops support for
> pre-Vista versions of Windows. However, those OSes are past their end
> of life, and other OSS projects have dropped support. e.g.; the last
> Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> good tradeoff to invest effort in fixing broken code needed to support
> them, so hopefully this isn't too controversial.
> 
>  include/qemu/thread-win32.h |   7 +--
>  util/qemu-thread-win32.c    | 136
>  +++++---------------------------------------
>  2 files changed, 17 insertions(+), 126 deletions(-)
> 
> diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
> index 5fb6541..4c4a261 100644
> --- a/include/qemu/thread-win32.h
> +++ b/include/qemu/thread-win32.h
> @@ -4,8 +4,7 @@
>  #include <windows.h>
>  
>  struct QemuMutex {
> -    CRITICAL_SECTION lock;
> -    LONG owner;
> +    SRWLOCK lock;
>  };
>  
>  typedef struct QemuRecMutex QemuRecMutex;
> @@ -19,9 +18,7 @@ int qemu_rec_mutex_trylock(QemuRecMutex *mutex);
>  void qemu_rec_mutex_unlock(QemuRecMutex *mutex);
>  
>  struct QemuCond {
> -    LONG waiters, target;
> -    HANDLE sema;
> -    HANDLE continue_event;
> +    CONDITION_VARIABLE var;
>  };
>  
>  struct QemuSemaphore {
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 29c3e4d..59befd5 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -10,6 +10,11 @@
>   * See the COPYING file in the top-level directory.
>   *
>   */
> +
> +#ifndef _WIN32_WINNT
> +#define _WIN32_WINNT 0x0600
> +#endif
> +
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/thread.h"
> @@ -39,44 +44,30 @@ static void error_exit(int err, const char *msg)
>  
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
> -    mutex->owner = 0;
> -    InitializeCriticalSection(&mutex->lock);
> +    InitializeSRWLock(&mutex->lock);
>  }
>  
>  void qemu_mutex_destroy(QemuMutex *mutex)
>  {
> -    assert(mutex->owner == 0);
> -    DeleteCriticalSection(&mutex->lock);
> +    InitializeSRWLock(&mutex->lock);
>  }
>  
>  void qemu_mutex_lock(QemuMutex *mutex)
>  {
> -    EnterCriticalSection(&mutex->lock);
> -
> -    /* Win32 CRITICAL_SECTIONs are recursive.  Assert that we're not
> -     * using them as such.
> -     */
> -    assert(mutex->owner == 0);
> -    mutex->owner = GetCurrentThreadId();
> +    AcquireSRWLockExclusive(&mutex->lock);
>  }
>  
>  int qemu_mutex_trylock(QemuMutex *mutex)
>  {
>      int owned;
>  
> -    owned = TryEnterCriticalSection(&mutex->lock);
> -    if (owned) {
> -        assert(mutex->owner == 0);
> -        mutex->owner = GetCurrentThreadId();
> -    }
> +    owned = TryAcquireSRWLockExclusive(&mutex->lock);
>      return !owned;
>  }
>  
>  void qemu_mutex_unlock(QemuMutex *mutex)
>  {
> -    assert(mutex->owner == GetCurrentThreadId());
> -    mutex->owner = 0;
> -    LeaveCriticalSection(&mutex->lock);
> +    ReleaseSRWLockExclusive(&mutex->lock);
>  }
>  
>  void qemu_rec_mutex_init(QemuRecMutex *mutex)
> @@ -107,124 +98,27 @@ void qemu_rec_mutex_unlock(QemuRecMutex *mutex)
>  void qemu_cond_init(QemuCond *cond)
>  {
>      memset(cond, 0, sizeof(*cond));
> -
> -    cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
> -    if (!cond->sema) {
> -        error_exit(GetLastError(), __func__);
> -    }
> -    cond->continue_event = CreateEvent(NULL,    /* security */
> -                                       FALSE,   /* auto-reset */
> -                                       FALSE,   /* not signaled */
> -                                       NULL);   /* name */
> -    if (!cond->continue_event) {
> -        error_exit(GetLastError(), __func__);
> -    }
> +    InitializeConditionVariable(&cond->var);
>  }
>  
>  void qemu_cond_destroy(QemuCond *cond)
>  {
> -    BOOL result;
> -    result = CloseHandle(cond->continue_event);
> -    if (!result) {
> -        error_exit(GetLastError(), __func__);
> -    }
> -    cond->continue_event = 0;
> -    result = CloseHandle(cond->sema);
> -    if (!result) {
> -        error_exit(GetLastError(), __func__);
> -    }
> -    cond->sema = 0;
> +    InitializeConditionVariable(&cond->var);
>  }
>  
>  void qemu_cond_signal(QemuCond *cond)
>  {
> -    DWORD result;
> -
> -    /*
> -     * Signal only when there are waiters.  cond->waiters is
> -     * incremented by pthread_cond_wait under the external lock,
> -     * so we are safe about that.
> -     */
> -    if (cond->waiters == 0) {
> -        return;
> -    }
> -
> -    /*
> -     * Waiting threads decrement it outside the external lock, but
> -     * only if another thread is executing pthread_cond_broadcast and
> -     * has the mutex.  So, it also cannot be decremented concurrently
> -     * with this particular access.
> -     */
> -    cond->target = cond->waiters - 1;
> -    result = SignalObjectAndWait(cond->sema, cond->continue_event,
> -                                 INFINITE, FALSE);
> -    if (result == WAIT_ABANDONED || result == WAIT_FAILED) {
> -        error_exit(GetLastError(), __func__);
> -    }
> +    WakeConditionVariable(&cond->var);
>  }
>  
>  void qemu_cond_broadcast(QemuCond *cond)
>  {
> -    BOOLEAN result;
> -    /*
> -     * As in pthread_cond_signal, access to cond->waiters and
> -     * cond->target is locked via the external mutex.
> -     */
> -    if (cond->waiters == 0) {
> -        return;
> -    }
> -
> -    cond->target = 0;
> -    result = ReleaseSemaphore(cond->sema, cond->waiters, NULL);
> -    if (!result) {
> -        error_exit(GetLastError(), __func__);
> -    }
> -
> -    /*
> -     * At this point all waiters continue. Each one takes its
> -     * slice of the semaphore. Now it's our turn to wait: Since
> -     * the external mutex is held, no thread can leave cond_wait,
> -     * yet. For this reason, we can be sure that no thread gets
> -     * a chance to eat *more* than one slice. OTOH, it means
> -     * that the last waiter must send us a wake-up.
> -     */
> -    WaitForSingleObject(cond->continue_event, INFINITE);
> +    WakeAllConditionVariable(&cond->var);
>  }
>  
>  void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>  {
> -    /*
> -     * This access is protected under the mutex.
> -     */
> -    cond->waiters++;
> -
> -    /*
> -     * Unlock external mutex and wait for signal.
> -     * NOTE: we've held mutex locked long enough to increment
> -     * waiters count above, so there's no problem with
> -     * leaving mutex unlocked before we wait on semaphore.
> -     */
> -    qemu_mutex_unlock(mutex);
> -    WaitForSingleObject(cond->sema, INFINITE);
> -
> -    /* Now waiters must rendez-vous with the signaling thread and
> -     * let it continue.  For cond_broadcast this has heavy contention
> -     * and triggers thundering herd.  So goes life.
> -     *
> -     * Decrease waiters count.  The mutex is not taken, so we have
> -     * to do this atomically.
> -     *
> -     * All waiters contend for the mutex at the end of this function
> -     * until the signaling thread relinquishes it.  To ensure
> -     * each waiter consumes exactly one slice of the semaphore,
> -     * the signaling thread stops until it is told by the last
> -     * waiter that it can go on.
> -     */
> -    if (InterlockedDecrement(&cond->waiters) == cond->target) {
> -        SetEvent(cond->continue_event);
> -    }
> -
> -    qemu_mutex_lock(mutex);
> +    SleepConditionVariableSRW(&cond->var, &mutex->lock, INFINITE, 0);
>  }
>  
>  void qemu_sem_init(QemuSemaphore *sem, int init)
> --
> 2.8.3
> 
> 
> 

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Andrew Baumann 7 years ago
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Friday, 24 March 2017 15:29

> > From: Andrey Shedel <ashedel@microsoft.com>
> >
> > The multithreaded TCG implementation exposed deadlocks in the win32
> > condition variables: as implemented, qemu_cond_broadcast waited on
> > receivers, whereas the pthreads API it was intended to emulate does
> > not. This was causing a deadlock because broadcast was called while
> > holding the IO lock, as well as all possible waiters blocked on the
> > same lock.
> >
> > This patch replaces all the custom synchronisation code for mutexes
> > and condition variables with native Windows primitives (SRWlocks and
> > condition variables) with the same semantics as their POSIX
> > equivalents. To enable that, it requires a Windows Vista or newer host
> > OS.
> >
> > [AB: edited commit message]
> > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> 
> Oops, just a nit but an important one: there should be a
> Signed-off-by for Andrey as well.

Oops, my fault, since I took his code and prepared the patch submission. We can resend with the signoff, but perhaps I should wait for a review?

Andrew
Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Paolo Bonzini 7 years ago

----- Original Message -----
> From: "Andrew Baumann" <Andrew.Baumann@microsoft.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Andrey Shedel" <ashedel@microsoft.com>, "Stefan Weil" <sw@weilnetz.de>, qemu-devel@nongnu.org
> Sent: Saturday, March 25, 2017 12:14:20 AM
> Subject: Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
> 
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: Friday, 24 March 2017 15:29
> 
> > > From: Andrey Shedel <ashedel@microsoft.com>
> > >
> > > The multithreaded TCG implementation exposed deadlocks in the win32
> > > condition variables: as implemented, qemu_cond_broadcast waited on
> > > receivers, whereas the pthreads API it was intended to emulate does
> > > not. This was causing a deadlock because broadcast was called while
> > > holding the IO lock, as well as all possible waiters blocked on the
> > > same lock.
> > >
> > > This patch replaces all the custom synchronisation code for mutexes
> > > and condition variables with native Windows primitives (SRWlocks and
> > > condition variables) with the same semantics as their POSIX
> > > equivalents. To enable that, it requires a Windows Vista or newer host
> > > OS.
> > >
> > > [AB: edited commit message]
> > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > 
> > Oops, just a nit but an important one: there should be a
> > Signed-off-by for Andrey as well.
> 
> Oops, my fault, since I took his code and prepared the patch submission. We
> can resend with the signoff, but perhaps I should wait for a review?

It's enough if he replies with the Signed-off-by line.

Paolo

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Andrey Shedel 7 years ago
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] 
> Sent: Saturday, March 25, 2017 3:14
>
> ----- Original Message -----
> > From: "Andrew Baumann" <Andrew.Baumann@microsoft.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: "Andrey Shedel" <ashedel@microsoft.com>, "Stefan Weil" 
> > <sw@weilnetz.de>, qemu-devel@nongnu.org
> > Sent: Saturday, March 25, 2017 12:14:20 AM
> > Subject: Re: [Qemu-devel] [PATCH] win32: replace custom mutex and 
> > condition variable with native primitives
> > 
> > > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > > Sent: Friday, 24 March 2017 15:29
> > 
> > > > From: Andrey Shedel <ashedel@microsoft.com>
> > > >
> > > > The multithreaded TCG implementation exposed deadlocks in the 
> > > > win32 condition variables: as implemented, qemu_cond_broadcast 
> > > > waited on receivers, whereas the pthreads API it was intended to 
> > > > emulate does not. This was causing a deadlock because broadcast 
> > > > was called while holding the IO lock, as well as all possible 
> > > > waiters blocked on the same lock.
> > > >
> > > > This patch replaces all the custom synchronisation code for 
> > > > mutexes and condition variables with native Windows primitives 
> > > > (SRWlocks and condition variables) with the same semantics as 
> > > > their POSIX equivalents. To enable that, it requires a Windows 
> > > > Vista or newer host OS.
> > > >
> > > > [AB: edited commit message]
> > > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > > 
> > > Oops, just a nit but an important one: there should be a 
> > > Signed-off-by for Andrey as well.
> > 
> > Oops, my fault, since I took his code and prepared the patch 
> > submission. We can resend with the signoff, but perhaps I should wait for a review?
>
> It's enough if he replies with the Signed-off-by line.
>
> Paolo
>

Signed-off-by: Andrey Shedel <ashedel@microsoft.com>

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Eric Blake 7 years ago
On 03/24/2017 05:01 PM, Andrew Baumann wrote:
> From: Andrey Shedel <ashedel@microsoft.com>
> 
> The multithreaded TCG implementation exposed deadlocks in the win32
> condition variables: as implemented, qemu_cond_broadcast waited on
> receivers, whereas the pthreads API it was intended to emulate does
> not. This was causing a deadlock because broadcast was called while
> holding the IO lock, as well as all possible waiters blocked on the
> same lock.
> 
> This patch replaces all the custom synchronisation code for mutexes
> and condition variables with native Windows primitives (SRWlocks and
> condition variables) with the same semantics as their POSIX
> equivalents. To enable that, it requires a Windows Vista or newer host
> OS.
> 
> [AB: edited commit message]
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> The major implication of this patch is that it drops support for
> pre-Vista versions of Windows. However, those OSes are past their end
> of life, and other OSS projects have dropped support. e.g.; the last
> Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> good tradeoff to invest effort in fixing broken code needed to support
> them, so hopefully this isn't too controversial.

I'll leave the technical review to others, but from the pragmatic view,
I'm in favor of this change.  While we still strive to let qemu run
past-their-prime OSs (like pre-Vista) as guests, it is a different story
for the host, and I'm totally fine with stating that our mingw build
requires a host OS that is not past end-of-life.

Furthermore, our recent thread on which OSs should still be supported
for builds should be evidence that we are willing to drop old code for
hosts that cannot be set up as a build-bot; and I see no reason that
anyone running an out-of-date Windows XP box would want to expose it to
the internet to be a build-bot given that it has known security flaws
that will not be patched.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Cornelia Huck 6 years, 11 months ago
On Fri, 24 Mar 2017 15:01:41 -0700
Andrew Baumann <Andrew.Baumann@microsoft.com> wrote:

> From: Andrey Shedel <ashedel@microsoft.com>
> 
> The multithreaded TCG implementation exposed deadlocks in the win32
> condition variables: as implemented, qemu_cond_broadcast waited on
> receivers, whereas the pthreads API it was intended to emulate does
> not. This was causing a deadlock because broadcast was called while
> holding the IO lock, as well as all possible waiters blocked on the
> same lock.
> 
> This patch replaces all the custom synchronisation code for mutexes
> and condition variables with native Windows primitives (SRWlocks and
> condition variables) with the same semantics as their POSIX
> equivalents. To enable that, it requires a Windows Vista or newer host
> OS.
> 
> [AB: edited commit message]
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> The major implication of this patch is that it drops support for
> pre-Vista versions of Windows. However, those OSes are past their end
> of life, and other OSS projects have dropped support. e.g.; the last
> Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> good tradeoff to invest effort in fixing broken code needed to support
> them, so hopefully this isn't too controversial.

Just a quick question:

I noticed that this breaks builds on an old fc20-based installation I
run some mingw cross builds on (getting an undefined reference to
`_imp__TryAcquireSRWLockExclusive@4'). I should probably take that as a
trigger to update that installation ;), but wanted to confirm that this
is expected.


Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Andrew Baumann via Qemu-devel 6 years, 11 months ago
> From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> Sent: Monday, 3 April 2017 7:20
> 
> On Fri, 24 Mar 2017 15:01:41 -0700
> Andrew Baumann <Andrew.Baumann@microsoft.com> wrote:
> 
> > From: Andrey Shedel <ashedel@microsoft.com>
> >
> > The multithreaded TCG implementation exposed deadlocks in the win32
> > condition variables: as implemented, qemu_cond_broadcast waited on
> > receivers, whereas the pthreads API it was intended to emulate does
> > not. This was causing a deadlock because broadcast was called while
> > holding the IO lock, as well as all possible waiters blocked on the
> > same lock.
> >
> > This patch replaces all the custom synchronisation code for mutexes
> > and condition variables with native Windows primitives (SRWlocks and
> > condition variables) with the same semantics as their POSIX
> > equivalents. To enable that, it requires a Windows Vista or newer host
> > OS.
> >
> > [AB: edited commit message]
> > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > ---
> > The major implication of this patch is that it drops support for
> > pre-Vista versions of Windows. However, those OSes are past their end
> > of life, and other OSS projects have dropped support. e.g.; the last
> > Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> > good tradeoff to invest effort in fixing broken code needed to support
> > them, so hopefully this isn't too controversial.
> 
> Just a quick question:
> 
> I noticed that this breaks builds on an old fc20-based installation I
> run some mingw cross builds on (getting an undefined reference to
> `_imp__TryAcquireSRWLockExclusive@4'). I should probably take that as a
> trigger to update that installation ;), but wanted to confirm that this
> is expected.

Yes, please try updating your mingw headers. I've done x64 cross-builds from Ubuntu 16.04 (WSL) using mingw-w64 4.0.4.

Andrew

Re: [Qemu-devel] [PATCH] win32: replace custom mutex and condition variable with native primitives
Posted by Cornelia Huck 6 years, 11 months ago
On Mon, 3 Apr 2017 18:12:34 +0000
Andrew Baumann <Andrew.Baumann@microsoft.com> wrote:

> > From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> > Sent: Monday, 3 April 2017 7:20
> > 
> > On Fri, 24 Mar 2017 15:01:41 -0700
> > Andrew Baumann <Andrew.Baumann@microsoft.com> wrote:
> > 
> > > From: Andrey Shedel <ashedel@microsoft.com>
> > >
> > > The multithreaded TCG implementation exposed deadlocks in the win32
> > > condition variables: as implemented, qemu_cond_broadcast waited on
> > > receivers, whereas the pthreads API it was intended to emulate does
> > > not. This was causing a deadlock because broadcast was called while
> > > holding the IO lock, as well as all possible waiters blocked on the
> > > same lock.
> > >
> > > This patch replaces all the custom synchronisation code for mutexes
> > > and condition variables with native Windows primitives (SRWlocks and
> > > condition variables) with the same semantics as their POSIX
> > > equivalents. To enable that, it requires a Windows Vista or newer host
> > > OS.
> > >
> > > [AB: edited commit message]
> > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > > ---
> > > The major implication of this patch is that it drops support for
> > > pre-Vista versions of Windows. However, those OSes are past their end
> > > of life, and other OSS projects have dropped support. e.g.; the last
> > > Cygwin release supporting XP was in Jun 2016. It doesn't seem like a
> > > good tradeoff to invest effort in fixing broken code needed to support
> > > them, so hopefully this isn't too controversial.
> > 
> > Just a quick question:
> > 
> > I noticed that this breaks builds on an old fc20-based installation I
> > run some mingw cross builds on (getting an undefined reference to
> > `_imp__TryAcquireSRWLockExclusive@4'). I should probably take that as a
> > trigger to update that installation ;), but wanted to confirm that this
> > is expected.
> 
> Yes, please try updating your mingw headers. I've done x64 cross-builds from Ubuntu 16.04 (WSL) using mingw-w64 4.0.4.

Yes, another, more modern cross-build setup works fine. I just was a
bit surprised that this mingw setup did not go back to Vista level.