From 5ff23accd018a2a8182a1e27167f101731d55d6c Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Wed, 1 May 2024 15:35:31 -0700 Subject: [PATCH 1/6] fix: use documented synch APIs --- CMakeLists.txt | 1 + src/init.c | 2 +- src/nt.h | 29 ----------------------------- src/reflock.c | 20 +++----------------- src/reflock.h | 2 -- 5 files changed, 5 insertions(+), 49 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index dfc13b7..e482540 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,6 +4,7 @@ project(wepoll) include(CMakeParseArguments) link_libraries(ws2_32) +link_libraries(synchronization) if(MSVC) add_compile_options(/Wall /WX /wd4127 /wd4201 /wd4242 /wd4710 /wd4711 /wd4820) diff --git a/src/init.c b/src/init.c index 04f72bc..78b3aab 100644 --- a/src/init.c +++ b/src/init.c @@ -21,7 +21,7 @@ static BOOL CALLBACK init__once_callback(INIT_ONCE* once, /* N.b. that initialization order matters here. */ if (ws_global_init() < 0 || nt_global_init() < 0 || - reflock_global_init() < 0 || epoll_global_init() < 0) + epoll_global_init() < 0) return FALSE; init__done = true; diff --git a/src/nt.h b/src/nt.h index 4e66548..caf4865 100644 --- a/src/nt.h +++ b/src/nt.h @@ -63,11 +63,6 @@ typedef struct _OBJECT_ATTRIBUTES { #define FILE_OPEN 0x00000001UL #endif -#define KEYEDEVENT_WAIT 0x00000001UL -#define KEYEDEVENT_WAKE 0x00000002UL -#define KEYEDEVENT_ALL_ACCESS \ - (STANDARD_RIGHTS_REQUIRED | KEYEDEVENT_WAIT | KEYEDEVENT_WAKE) - #define NT_NTDLL_IMPORT_LIST(X) \ X(NTSTATUS, \ NTAPI, \ @@ -91,14 +86,6 @@ typedef struct _OBJECT_ATTRIBUTES { PVOID EaBuffer, \ ULONG EaLength)) \ \ - X(NTSTATUS, \ - NTAPI, \ - NtCreateKeyedEvent, \ - (PHANDLE KeyedEventHandle, \ - ACCESS_MASK DesiredAccess, \ - POBJECT_ATTRIBUTES ObjectAttributes, \ - ULONG Flags)) \ - \ X(NTSTATUS, \ NTAPI, \ NtDeviceIoControlFile, \ @@ -113,22 +100,6 @@ typedef struct _OBJECT_ATTRIBUTES { PVOID OutputBuffer, \ ULONG OutputBufferLength)) \ \ - X(NTSTATUS, \ - NTAPI, \ - NtReleaseKeyedEvent, \ - (HANDLE KeyedEventHandle, \ - PVOID KeyValue, \ - BOOLEAN Alertable, \ - PLARGE_INTEGER Timeout)) \ - \ - X(NTSTATUS, \ - NTAPI, \ - NtWaitForKeyedEvent, \ - (HANDLE KeyedEventHandle, \ - PVOID KeyValue, \ - BOOLEAN Alertable, \ - PLARGE_INTEGER Timeout)) \ - \ X(ULONG, WINAPI, RtlNtStatusToDosError, (NTSTATUS Status)) #define X(return_type, attributes, name, parameters) \ diff --git a/src/reflock.c b/src/reflock.c index 60a96b5..76af8bb 100644 --- a/src/reflock.c +++ b/src/reflock.c @@ -15,31 +15,17 @@ #define REFLOCK__POISON ((long) 0x300dead0UL) /* clang-format on */ -static HANDLE reflock__keyed_event = NULL; - -int reflock_global_init(void) { - NTSTATUS status = NtCreateKeyedEvent( - &reflock__keyed_event, KEYEDEVENT_ALL_ACCESS, NULL, 0); - if (status != STATUS_SUCCESS) - return_set_error(-1, RtlNtStatusToDosError(status)); - return 0; -} - void reflock_init(reflock_t* reflock) { reflock->state = 0; } static void reflock__signal_event(void* address) { - NTSTATUS status = - NtReleaseKeyedEvent(reflock__keyed_event, address, FALSE, NULL); - if (status != STATUS_SUCCESS) - abort(); + WakeByAddressSingle(address); } static void reflock__await_event(void* address) { - NTSTATUS status = - NtWaitForKeyedEvent(reflock__keyed_event, address, FALSE, NULL); - if (status != STATUS_SUCCESS) + BOOL status = WaitOnAddress(address, address, sizeof(void*), INFINITE); + if (status != TRUE) abort(); } diff --git a/src/reflock.h b/src/reflock.h index 7f6d5a6..a3d276e 100644 --- a/src/reflock.h +++ b/src/reflock.h @@ -25,8 +25,6 @@ typedef struct reflock { volatile long state; /* 32-bit Interlocked APIs operate on `long` values. */ } reflock_t; -WEPOLL_INTERNAL int reflock_global_init(void); - WEPOLL_INTERNAL void reflock_init(reflock_t* reflock); WEPOLL_INTERNAL void reflock_ref(reflock_t* reflock); WEPOLL_INTERNAL void reflock_unref(reflock_t* reflock); From 4cfa6ab9ddde22edcf897690d300222fe305fbfd Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 10 May 2024 09:18:06 -0700 Subject: [PATCH 2/6] Disable C4191 warnings --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e482540..f8d2154 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,7 +7,7 @@ link_libraries(ws2_32) link_libraries(synchronization) if(MSVC) - add_compile_options(/Wall /WX /wd4127 /wd4201 /wd4242 /wd4710 /wd4711 /wd4820) + add_compile_options(/Wall /WX /wd4127 /wd4191 /wd4201 /wd4242 /wd4710 /wd4711 /wd4820) if(MSVC_VERSION GREATER_EQUAL 1900) add_compile_options(/wd5045) endif() From 1ab6cd7dab3c05ecb09b03ea98a70925cbf08a77 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 10 May 2024 09:47:10 -0700 Subject: [PATCH 3/6] Add wait object --- src/reflock.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/reflock.c b/src/reflock.c index 76af8bb..94967e6 100644 --- a/src/reflock.c +++ b/src/reflock.c @@ -15,18 +15,24 @@ #define REFLOCK__POISON ((long) 0x300dead0UL) /* clang-format on */ +static void* reflock__wait_object = NULL; + void reflock_init(reflock_t* reflock) { reflock->state = 0; } static void reflock__signal_event(void* address) { + reflock__wait_object = NULL; WakeByAddressSingle(address); } static void reflock__await_event(void* address) { - BOOL status = WaitOnAddress(address, address, sizeof(void*), INFINITE); - if (status != TRUE) - abort(); + reflock__wait_object = address; + do { + BOOL status = WaitOnAddress(address, reflock__wait_object, sizeof(void*), INFINITE); + if (status != TRUE) + abort(); + } while (reflock__wait_object == address); } void reflock_ref(reflock_t* reflock) { From f392daa61f2a0d17b666cea54bfc00144a6420a9 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Wed, 22 May 2024 10:11:34 -0700 Subject: [PATCH 4/6] Use signal object instead of wait object --- src/reflock.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/reflock.c b/src/reflock.c index 94967e6..7954d8e 100644 --- a/src/reflock.c +++ b/src/reflock.c @@ -15,24 +15,31 @@ #define REFLOCK__POISON ((long) 0x300dead0UL) /* clang-format on */ -static void* reflock__wait_object = NULL; +/* Prevents the possibility of exiting early due to spurious wakeups. */ +static void* reflock__signal_object = NULL; void reflock_init(reflock_t* reflock) { reflock->state = 0; } static void reflock__signal_event(void* address) { - reflock__wait_object = NULL; + reflock__signal_object = address; WakeByAddressSingle(address); } static void reflock__await_event(void* address) { - reflock__wait_object = address; + BOOL status = TRUE; + + if (reflock__signal_object == address) { + reflock__signal_object = NULL; + } + do { - BOOL status = WaitOnAddress(address, reflock__wait_object, sizeof(void*), INFINITE); - if (status != TRUE) - abort(); - } while (reflock__wait_object == address); + status = WaitOnAddress(address, address, sizeof(void*), INFINITE); + } while (reflock__signal_object != address); + + if (status != TRUE) + abort(); } void reflock_ref(reflock_t* reflock) { From c265e8625b6535601831bbd89949c31bec16713c Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 14 Jun 2024 14:18:56 -0700 Subject: [PATCH 5/6] fix reflock test --- src/reflock.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/reflock.c b/src/reflock.c index 7954d8e..474c6b7 100644 --- a/src/reflock.c +++ b/src/reflock.c @@ -11,32 +11,31 @@ #define REFLOCK__REF ((long) 0x00000001UL) #define REFLOCK__REF_MASK ((long) 0x0fffffffUL) #define REFLOCK__DESTROY ((long) 0x10000000UL) -#define REFLOCK__DESTROY_MASK ((long) 0xf0000000UL) +#define REFLOCK__DESTROY_MASK ((long) 0x10000000UL) +#define REFLOCK__SIGNAL ((long) 0x20000000UL) +#define REFLOCK__SIGNAL_MASK ((long) 0x20000000UL) #define REFLOCK__POISON ((long) 0x300dead0UL) /* clang-format on */ -/* Prevents the possibility of exiting early due to spurious wakeups. */ -static void* reflock__signal_object = NULL; - void reflock_init(reflock_t* reflock) { reflock->state = 0; } -static void reflock__signal_event(void* address) { - reflock__signal_object = address; - WakeByAddressSingle(address); +static void reflock__signal_event(reflock_t* reflock) { + long state = InterlockedAdd(&reflock->state, REFLOCK__SIGNAL); + unused_var(state); + + WakeByAddressSingle(reflock); } -static void reflock__await_event(void* address) { +static void reflock__await_event(reflock_t* reflock) { BOOL status = TRUE; - - if (reflock__signal_object == address) { - reflock__signal_object = NULL; - } - do { - status = WaitOnAddress(address, address, sizeof(void*), INFINITE); - } while (reflock__signal_object != address); + status = WaitOnAddress(reflock, reflock, sizeof(reflock_t*), INFINITE); + } while ((reflock->state & REFLOCK__SIGNAL_MASK) == 0); + + long state = InterlockedAdd(&reflock->state, -REFLOCK__SIGNAL); + unused_var(state); if (status != TRUE) abort(); From 48de2a9598fce0a1eda826a2099dac7bfcf6b7a2 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Mon, 22 Jul 2024 15:43:34 -0700 Subject: [PATCH 6/6] Switch to condition variables --- CMakeLists.txt | 1 - src/init.c | 2 +- src/reflock.c | 51 +++++++++++++++++++++++++++++++++++++++----------- src/reflock.h | 5 +++++ 4 files changed, 46 insertions(+), 13 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f8d2154..00ce953 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,7 +4,6 @@ project(wepoll) include(CMakeParseArguments) link_libraries(ws2_32) -link_libraries(synchronization) if(MSVC) add_compile_options(/Wall /WX /wd4127 /wd4191 /wd4201 /wd4242 /wd4710 /wd4711 /wd4820) diff --git a/src/init.c b/src/init.c index 78b3aab..04f72bc 100644 --- a/src/init.c +++ b/src/init.c @@ -21,7 +21,7 @@ static BOOL CALLBACK init__once_callback(INIT_ONCE* once, /* N.b. that initialization order matters here. */ if (ws_global_init() < 0 || nt_global_init() < 0 || - epoll_global_init() < 0) + reflock_global_init() < 0 || epoll_global_init() < 0) return FALSE; init__done = true; diff --git a/src/reflock.c b/src/reflock.c index 474c6b7..e50355a 100644 --- a/src/reflock.c +++ b/src/reflock.c @@ -14,31 +14,58 @@ #define REFLOCK__DESTROY_MASK ((long) 0x10000000UL) #define REFLOCK__SIGNAL ((long) 0x20000000UL) #define REFLOCK__SIGNAL_MASK ((long) 0x20000000UL) -#define REFLOCK__POISON ((long) 0x300dead0UL) +#define REFLOCK__AWAIT ((long) 0x40000000UL) +#define REFLOCK__AWAIT_MASK ((long) 0x40000000UL) +#define REFLOCK__POISON ((long) 0x800dead0UL) /* clang-format on */ +static CRITICAL_SECTION signalMutex; + +int reflock_global_init(void) { + InitializeCriticalSection(&signalMutex); + return 0; +} + void reflock_init(reflock_t* reflock) { reflock->state = 0; + InitializeConditionVariable(&reflock->cv_signal); + InitializeConditionVariable(&reflock->cv_await); } static void reflock__signal_event(reflock_t* reflock) { - long state = InterlockedAdd(&reflock->state, REFLOCK__SIGNAL); - unused_var(state); + BOOL status = TRUE; - WakeByAddressSingle(reflock); + EnterCriticalSection(&signalMutex); + long state = InterlockedOr(&reflock->state, REFLOCK__SIGNAL); + while ((reflock->state & REFLOCK__AWAIT_MASK) == 0) { + status = SleepConditionVariableCS(&reflock->cv_signal, &signalMutex, INFINITE); + } + LeaveCriticalSection(&signalMutex); + + if (status != TRUE) + abort(); + + /* At most one reflock__await_event call per reflock. */ + WakeConditionVariable(&reflock->cv_await); + unused_var(state); } static void reflock__await_event(reflock_t* reflock) { BOOL status = TRUE; - do { - status = WaitOnAddress(reflock, reflock, sizeof(reflock_t*), INFINITE); - } while ((reflock->state & REFLOCK__SIGNAL_MASK) == 0); - long state = InterlockedAdd(&reflock->state, -REFLOCK__SIGNAL); - unused_var(state); + EnterCriticalSection(&signalMutex); + long state = InterlockedOr(&reflock->state, REFLOCK__AWAIT); + while ((reflock->state & REFLOCK__SIGNAL_MASK) == 0) { + status = SleepConditionVariableCS(&reflock->cv_await, &signalMutex, INFINITE); + } + LeaveCriticalSection(&signalMutex); if (status != TRUE) abort(); + + /* Multiple threads could be waiting. */ + WakeAllConditionVariable(&reflock->cv_signal); + unused_var(state); } void reflock_ref(reflock_t* reflock) { @@ -55,7 +82,8 @@ void reflock_unref(reflock_t* reflock) { /* Verify that the lock was referenced and not already destroyed. */ assert((state & REFLOCK__DESTROY_MASK & ~REFLOCK__DESTROY) == 0); - if (state == REFLOCK__DESTROY) + if ((state & REFLOCK__DESTROY_MASK) == REFLOCK__DESTROY && + (state & REFLOCK__REF_MASK) == 0) reflock__signal_event(reflock); } @@ -71,5 +99,6 @@ void reflock_unref_and_destroy(reflock_t* reflock) { reflock__await_event(reflock); state = InterlockedExchange(&reflock->state, REFLOCK__POISON); - assert(state == REFLOCK__DESTROY); + assert((state & REFLOCK__DESTROY_MASK) == REFLOCK__DESTROY); + assert((state & REFLOCK__REF_MASK) == 0); } diff --git a/src/reflock.h b/src/reflock.h index a3d276e..37da44b 100644 --- a/src/reflock.h +++ b/src/reflock.h @@ -2,6 +2,7 @@ #define WEPOLL_REFLOCK_H_ #include "config.h" +#include "win.h" /* A reflock is a special kind of lock that normally prevents a chunk of * memory from being freed, but does allow the chunk of memory to eventually be @@ -23,8 +24,12 @@ typedef struct reflock { volatile long state; /* 32-bit Interlocked APIs operate on `long` values. */ + CONDITION_VARIABLE cv_signal; + CONDITION_VARIABLE cv_await; } reflock_t; +WEPOLL_INTERNAL int reflock_global_init(void); + WEPOLL_INTERNAL void reflock_init(reflock_t* reflock); WEPOLL_INTERNAL void reflock_ref(reflock_t* reflock); WEPOLL_INTERNAL void reflock_unref(reflock_t* reflock);