From 877dbfc0561dfc5297cf446fe653cc3095d20b46 Mon Sep 17 00:00:00 2001 From: Daniel Pinto Date: Thu, 12 Nov 2020 19:40:07 +0000 Subject: [PATCH 1/2] 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(-) create mode 100644 changes/ticket17927 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 #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) { From 328f38a59f5e1b049350d05dcb29e7c511cffd79 Mon Sep 17 00:00:00 2001 From: Daniel Pinto Date: Mon, 30 Nov 2020 02:54:13 +0000 Subject: [PATCH 2/2] Use atomic ops to access lock_owner in WIN32 tor_mutex_t #17927 --- src/lib/lock/compat_mutex.h | 2 +- src/lib/lock/compat_mutex_winthreads.c | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/lib/lock/compat_mutex.h b/src/lib/lock/compat_mutex.h index 14bdf70ce5..518ba96b53 100644 --- a/src/lib/lock/compat_mutex.h +++ b/src/lib/lock/compat_mutex.h @@ -46,7 +46,7 @@ typedef struct tor_mutex_t { NON_RECURSIVE = 0, RECURSIVE } type; - DWORD lock_owner; // id of the thread that owns the lock + LONG 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 diff --git a/src/lib/lock/compat_mutex_winthreads.c b/src/lib/lock/compat_mutex_winthreads.c index 03cf5082e0..151a7b80f7 100644 --- a/src/lib/lock/compat_mutex_winthreads.c +++ b/src/lib/lock/compat_mutex_winthreads.c @@ -58,13 +58,15 @@ tor_mutex_uninit(tor_mutex_t *m) static void tor_mutex_acquire_recursive(tor_mutex_t *m) { - DWORD thread_id = GetCurrentThreadId(); - if (thread_id == m->lock_owner) { + LONG thread_id = GetCurrentThreadId(); + // use InterlockedCompareExchange to perform an atomic read + LONG lock_owner = InterlockedCompareExchange(&m->lock_owner, 0, 0); + if (thread_id == lock_owner) { ++m->lock_count; return; } AcquireSRWLockExclusive(&m->mutex); - m->lock_owner = thread_id; + InterlockedExchange(&m->lock_owner, thread_id); m->lock_count = 1; } @@ -91,7 +93,7 @@ tor_mutex_release_recursive(tor_mutex_t *m) if (--m->lock_count) { return; } - m->lock_owner = 0; + InterlockedExchange(&m->lock_owner, 0); ReleaseSRWLockExclusive(&m->mutex); }