[tor-commits] [tor/master] Use SRWLocks to implement locking on Windows #17927

[ahf@xxxxxxxxxxxxxx]

commit 877dbfc0561dfc5297cf446fe653cc3095d20b46
Author: Daniel Pinto <danielpinto52@xxxxxxxxx>
Date:   Thu Nov 12 19:40:07 2020 +0000

    Use SRWLocks to implement locking on Windows #17927
    
    Replace the Windows locking implementation which used critical
    sections with the faster SRWLocks available since Vista.
---
 changes/ticket17927                    |  4 ++
 src/lib/lock/compat_mutex.h            | 11 ++++-
 src/lib/lock/compat_mutex_winthreads.c | 76 +++++++++++++++++++++++++++++++---
 src/lib/thread/compat_winthreads.c     |  8 +++-
 4 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/changes/ticket17927 b/changes/ticket17927
new file mode 100644
index 0000000000..532416dac4
--- /dev/null
+++ b/changes/ticket17927
@@ -0,0 +1,4 @@
+  o Minor features (performance, windows):
+    - Use SRWLocks to implement locking on Windows. Replaces the critical
+      section locking implementation with the faster SRWLocks available
+      since Windows Vista. Closes ticket 17927. Patch by Daniel Pinto.
diff --git a/src/lib/lock/compat_mutex.h b/src/lib/lock/compat_mutex.h
index 5631993cc4..14bdf70ce5 100644
--- a/src/lib/lock/compat_mutex.h
+++ b/src/lib/lock/compat_mutex.h
@@ -39,8 +39,15 @@
 /** A generic lock structure for multithreaded builds. */
 typedef struct tor_mutex_t {
 #if defined(USE_WIN32_THREADS)
-  /** Windows-only: on windows, we implement locks with CRITICAL_SECTIONS. */
-  CRITICAL_SECTION mutex;
+  /** Windows-only: on windows, we implement locks with SRW locks. */
+  SRWLOCK mutex;
+  /** For recursive lock support (SRW locks are not recursive) */
+  enum mutex_type_t {
+    NON_RECURSIVE = 0,
+    RECURSIVE
+  } type;
+  DWORD lock_owner; // id of the thread that owns the lock
+  int lock_count; // number of times the lock is held recursively
 #elif defined(USE_PTHREADS)
   /** Pthreads-only: with pthreads, we implement locks with
    * pthread_mutex_t. */
diff --git a/src/lib/lock/compat_mutex_winthreads.c b/src/lib/lock/compat_mutex_winthreads.c
index 5fe6870a93..03cf5082e0 100644
--- a/src/lib/lock/compat_mutex_winthreads.c
+++ b/src/lib/lock/compat_mutex_winthreads.c
@@ -9,6 +9,23 @@
  * \brief Implement the tor_mutex API using CRITICAL_SECTION.
  **/
 
+#include "orconfig.h"
+
+/* For SRW locks support */
+#ifndef WINVER
+#error "orconfig.h didn't define WINVER"
+#endif
+#ifndef _WIN32_WINNT
+#error "orconfig.h didn't define _WIN32_WINNT"
+#endif
+#if WINVER < 0x0600
+#error "winver too low"
+#endif
+#if _WIN32_WINNT < 0x0600
+#error "winver too low"
+#endif
+
+#include <windows.h>
 #include "lib/lock/compat_mutex.h"
 #include "lib/err/torerr.h"
 
@@ -20,27 +37,76 @@ tor_locking_init(void)
 void
 tor_mutex_init(tor_mutex_t *m)
 {
-  InitializeCriticalSection(&m->mutex);
+  m->type = RECURSIVE;
+  m->lock_owner = 0;
+  m->lock_count = 0;
+  InitializeSRWLock(&m->mutex);
 }
 void
 tor_mutex_init_nonrecursive(tor_mutex_t *m)
 {
-  InitializeCriticalSection(&m->mutex);
+  m->type = NON_RECURSIVE;
+  InitializeSRWLock(&m->mutex);
 }
 
 void
 tor_mutex_uninit(tor_mutex_t *m)
 {
-  DeleteCriticalSection(&m->mutex);
+  (void) m;
+}
+
+static void
+tor_mutex_acquire_recursive(tor_mutex_t *m)
+{
+  DWORD thread_id = GetCurrentThreadId();
+  if (thread_id == m->lock_owner) {
+    ++m->lock_count;
+    return;
+  }
+  AcquireSRWLockExclusive(&m->mutex);
+  m->lock_owner = thread_id;
+  m->lock_count = 1;
+}
+
+static void
+tor_mutex_acquire_nonrecursive(tor_mutex_t *m)
+{
+  AcquireSRWLockExclusive(&m->mutex);
 }
+
 void
 tor_mutex_acquire(tor_mutex_t *m)
 {
   raw_assert(m);
-  EnterCriticalSection(&m->mutex);
+  if (m->type == NON_RECURSIVE) {
+    tor_mutex_acquire_nonrecursive(m);
+  } else {
+    tor_mutex_acquire_recursive(m);
+  }
+}
+
+static void
+tor_mutex_release_recursive(tor_mutex_t *m)
+{
+  if (--m->lock_count) {
+    return;
+  }
+  m->lock_owner = 0;
+  ReleaseSRWLockExclusive(&m->mutex);
 }
+
+static void
+tor_mutex_release_nonrecursive(tor_mutex_t *m)
+{
+  ReleaseSRWLockExclusive(&m->mutex);
+}
+
 void
 tor_mutex_release(tor_mutex_t *m)
 {
-  LeaveCriticalSection(&m->mutex);
+  if (m->type == NON_RECURSIVE) {
+    tor_mutex_release_nonrecursive(m);
+  } else {
+    tor_mutex_release_recursive(m);
+  }
 }
diff --git a/src/lib/thread/compat_winthreads.c b/src/lib/thread/compat_winthreads.c
index fcc9c0279b..a6213aa46a 100644
--- a/src/lib/thread/compat_winthreads.c
+++ b/src/lib/thread/compat_winthreads.c
@@ -144,13 +144,17 @@ tor_threadlocal_set(tor_threadlocal_t *threadlocal, void *value)
 int
 tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock_, const struct timeval *tv)
 {
-  CRITICAL_SECTION *lock = &lock_->mutex;
+  // recursive SRW locks are not supported because they need extra logic for
+  // acquiring and releasing but SleepConditionVariableSRW will use the OS
+  // lock relase function which lacks our extra logic
+  tor_assert(lock_->type == NON_RECURSIVE);
+  SRWLOCK *lock = &lock_->mutex;
   DWORD ms = INFINITE;
   if (tv) {
     ms = tv->tv_sec*1000 + (tv->tv_usec+999)/1000;
   }
 
-  BOOL ok = SleepConditionVariableCS(&cond->cond, lock, ms);
+  BOOL ok = SleepConditionVariableSRW(&cond->cond, lock, ms, 0);
   if (!ok) {
     DWORD err = GetLastError();
     if (err == ERROR_TIMEOUT) {



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits