From 45230b51db703933dc6108c526a9eb872416981a Mon Sep 17 00:00:00 2001 From: Zebediah Figura Date: Fri, 12 Nov 2021 00:25:51 -0600 Subject: [PATCH] ntdll-NtAlertThreadByThreadId: Avoid spurious wakeups by performing the comparison inside of the futex queue spinlock. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50680 --- ...t-Win32-futexes-on-top-of-thread-ID-.patch | 91 +++++-------------- 1 file changed, 25 insertions(+), 66 deletions(-) diff --git a/patches/ntdll-NtAlertThreadByThreadId/0007-ntdll-Reimplement-Win32-futexes-on-top-of-thread-ID-.patch b/patches/ntdll-NtAlertThreadByThreadId/0007-ntdll-Reimplement-Win32-futexes-on-top-of-thread-ID-.patch index 96cd4cb5..0b080860 100644 --- a/patches/ntdll-NtAlertThreadByThreadId/0007-ntdll-Reimplement-Win32-futexes-on-top-of-thread-ID-.patch +++ b/patches/ntdll-NtAlertThreadByThreadId/0007-ntdll-Reimplement-Win32-futexes-on-top-of-thread-ID-.patch @@ -1,4 +1,4 @@ -From 08cec34b6dccea0cf7e8f880911786fd385ed78a Mon Sep 17 00:00:00 2001 +From 4b2ad3bf49e77c6cb0e541329ac851d054e6f649 Mon Sep 17 00:00:00 2001 From: Zebediah Figura Date: Mon, 7 Jun 2021 16:26:18 -0500 Subject: [PATCH] ntdll: Reimplement Win32 futexes on top of thread-ID alerts. @@ -6,14 +6,14 @@ Subject: [PATCH] ntdll: Reimplement Win32 futexes on top of thread-ID alerts. Signed-off-by: Zebediah Figura Signed-off-by: Zebediah Figura --- - dlls/ntdll/sync.c | 214 ++++++++++++++++++++++++++++++++++++++- + dlls/ntdll/sync.c | 173 ++++++++++++++++++++++++++++++++++++++- dlls/ntdll/unix/loader.c | 3 - - dlls/ntdll/unix/sync.c | 162 ----------------------------- + dlls/ntdll/unix/sync.c | 162 ------------------------------------ dlls/ntdll/unixlib.h | 6 +- - 4 files changed, 212 insertions(+), 173 deletions(-) + 4 files changed, 171 insertions(+), 173 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c -index bfb30661864..db68a466d8a 100644 +index bfb30661864..adc03c9ff7a 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -34,11 +34,18 @@ @@ -35,7 +35,7 @@ index bfb30661864..db68a466d8a 100644 /****************************************************************** * RtlRunOnceInitialize (NTDLL.@) */ -@@ -863,13 +870,142 @@ NTSTATUS WINAPI RtlSleepConditionVariableSRW( RTL_CONDITION_VARIABLE *variable, +@@ -863,13 +870,111 @@ NTSTATUS WINAPI RtlSleepConditionVariableSRW( RTL_CONDITION_VARIABLE *variable, return status; } @@ -72,22 +72,6 @@ index bfb30661864..db68a466d8a 100644 + return &futex_queues[(val >> 4) % ARRAY_SIZE(futex_queues)]; +} + -+static void spin_rdlock( LONG *lock ) -+{ -+ for (;;) -+ { -+ LONG old = *lock; -+ if (old != -1 && InterlockedCompareExchange( lock, old + 1, old ) == old) -+ return; -+ YieldProcessor(); -+ } -+} -+ -+static void spin_rdunlock( LONG *lock ) -+{ -+ InterlockedDecrement( lock ); -+} -+ +static void spin_wrlock( LONG *lock ) +{ + while (InterlockedCompareExchange( lock, -1, 0 )) @@ -136,35 +120,20 @@ index bfb30661864..db68a466d8a 100644 + entry.tid = GetCurrentThreadId(); + + spin_wrlock( &queue->lock ); ++ ++ if (!compare_addr( addr, cmp, size )) ++ { ++ spin_wrunlock( &queue->lock ); ++ return STATUS_SUCCESS; ++ } ++ + if (!queue->queue.next) -+ list_init(&queue->queue); ++ list_init( &queue->queue ); + list_add_tail( &queue->queue, &entry.entry ); ++ + spin_wrunlock( &queue->lock ); + -+ /* Ensure that the store above is ordered before the comparison below. -+ * This barrier is paired with another in RtlWakeByAddress*(). -+ * -+ * In more detail, given the following sequence: -+ * -+ * Thread A Thread B -+ * ----------------------------------------------------------------- -+ * RtlWaitOnAddress( &val ); val = 1; -+ * queue thread RtlWakeByAddress( &val ); -+ * MemoryBarrier(); <---- paired with ----> MemoryBarrier(); -+ * compare_addr( &val ); if (thread is queued) -+ * -+ * We must ensure that the thread is queued [through the above -+ * InterlockedExchangePointer()] before reading "val", and that writes to -+ * "val" by the application happen before we check for queued threads. -+ * Otherwise, thread A can deadlock: "val" may appear unchanged, while -+ * thread B observed that thread A was not queued. -+ */ -+ MemoryBarrier(); -+ -+ if (compare_addr( addr, cmp, size )) -+ ret = NtWaitForAlertByThreadId( NULL, timeout ); -+ else -+ ret = STATUS_SUCCESS; ++ ret = NtWaitForAlertByThreadId( NULL, timeout ); + + spin_wrlock( &queue->lock ); + /* We may have already been removed by a call to RtlWakeAddressSingle(). */ @@ -179,7 +148,7 @@ index bfb30661864..db68a466d8a 100644 } /*********************************************************************** -@@ -877,7 +1013,42 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size +@@ -877,7 +982,37 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size */ void WINAPI RtlWakeAddressAll( const void *addr ) { @@ -193,11 +162,6 @@ index bfb30661864..db68a466d8a 100644 + + if (!addr) return; + -+ /* Ensure that memory stores to "addr" are ordered before reading the -+ * array below. Paired with another barrier in RtlWaitOnAddress() [q.v.]. -+ */ -+ MemoryBarrier(); -+ + spin_wrlock( &queue->lock ); + + if (!queue->queue.next) @@ -223,7 +187,7 @@ index bfb30661864..db68a466d8a 100644 } /*********************************************************************** -@@ -885,5 +1056,42 @@ void WINAPI RtlWakeAddressAll( const void *addr ) +@@ -885,5 +1020,37 @@ void WINAPI RtlWakeAddressAll( const void *addr ) */ void WINAPI RtlWakeAddressSingle( const void *addr ) { @@ -236,11 +200,6 @@ index bfb30661864..db68a466d8a 100644 + + if (!addr) return; + -+ /* Ensure that memory stores to "addr" are ordered before reading the -+ * array below. Paired with another barrier in RtlWaitOnAddress() [q.v.]. -+ */ -+ MemoryBarrier(); -+ + spin_wrlock( &queue->lock ); + + if (!queue->queue.next) @@ -268,10 +227,10 @@ index bfb30661864..db68a466d8a 100644 + if (tid) NtAlertThreadByThreadId( (HANDLE)(DWORD_PTR)tid ); } diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c -index 7f36dc42579..05ae264297a 100644 +index a411b01a389..59bc978022c 100644 --- a/dlls/ntdll/unix/loader.c +++ b/dlls/ntdll/unix/loader.c -@@ -2145,9 +2145,6 @@ static struct unix_funcs unix_funcs = +@@ -2146,9 +2146,6 @@ static struct unix_funcs unix_funcs = NtCurrentTeb, #endif RtlGetSystemTimePrecise, @@ -282,10 +241,10 @@ index 7f36dc42579..05ae264297a 100644 fast_RtlpUnWaitCriticalSection, fast_RtlDeleteCriticalSection, diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c -index cdc63c8a6ef..47a0a03b45f 100644 +index ce78c11fd4b..c37dbc5e96e 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c -@@ -73,10 +73,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(sync); +@@ -74,10 +74,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(sync); HANDLE keyed_event = 0; @@ -296,7 +255,7 @@ index cdc63c8a6ef..47a0a03b45f 100644 static const char *debugstr_timeout( const LARGE_INTEGER *timeout ) { if (!timeout) return "(infinite)"; -@@ -186,24 +182,6 @@ static void timespec_from_timeout( struct timespec *timespec, const LARGE_INTEGE +@@ -187,24 +183,6 @@ static void timespec_from_timeout( struct timespec *timespec, const LARGE_INTEGE #endif @@ -321,7 +280,7 @@ index cdc63c8a6ef..47a0a03b45f 100644 /* create a struct security_descriptor and contained information in one contiguous piece of memory */ NTSTATUS alloc_object_attributes( const OBJECT_ATTRIBUTES *attr, struct object_attributes **ret, data_size_t *ret_len ) -@@ -2977,71 +2955,6 @@ NTSTATUS CDECL fast_RtlWakeConditionVariable( RTL_CONDITION_VARIABLE *variable, +@@ -3035,71 +3013,6 @@ NTSTATUS CDECL fast_RtlWakeConditionVariable( RTL_CONDITION_VARIABLE *variable, return STATUS_SUCCESS; } @@ -393,7 +352,7 @@ index cdc63c8a6ef..47a0a03b45f 100644 #else NTSTATUS CDECL fast_RtlTryAcquireSRWLockExclusive( RTL_SRWLOCK *lock ) -@@ -3084,79 +2997,4 @@ NTSTATUS CDECL fast_wait_cv( RTL_CONDITION_VARIABLE *variable, const void *value +@@ -3142,79 +3055,4 @@ NTSTATUS CDECL fast_wait_cv( RTL_CONDITION_VARIABLE *variable, const void *value return STATUS_NOT_IMPLEMENTED; }