[PATCH v2 1/2] kernelbase/tests: Relax tests for WaitOnAddress().

[Zebediah Figura]

Application should be expected to handle spurious wakeups, which the
following implementation will give in WakeByAddressSingle(). Avoid testing
that WakeByAddressSingle() only wakes one thread.

Signed-off-by: Zebediah Figura 
---
 dlls/kernelbase/tests/sync.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/dlls/kernelbase/tests/sync.c b/dlls/kernelbase/tests/sync.c
index 99cbbc43bc..7a335d1af8 100644
--- a/dlls/kernelbase/tests/sync.c
+++ b/dlls/kernelbase/tests/sync.c
@@ -31,19 +31,23 @@ static void (WINAPI *pWakeByAddressAll)(void *);
 static void (WINAPI *pWakeByAddressSingle)(void *);

static LONG64 address;
-static LONG64 compare;
 static DWORD WINAPI test_WaitOnAddress_func(void *arg)
 {
     BOOL ret = FALSE;
-    DWORD gle;
-    while (address == compare)
+    LONG64 compare;
+
+    do
     {
-        SetLastError(0xdeadbeef);
-        ret = pWaitOnAddress(&address, &compare, sizeof(compare), INFINITE);
-        gle = GetLastError();
-        ok(gle == 0xdeadbeef || broken(gle == ERROR_SUCCESS) /* Win 8 */, "got %d\n", gle);
-    }
-    ok(ret, "got %d\n", ret);
+        while (!(compare = address))
+        {
+            SetLastError(0xdeadbeef);
+            ret = pWaitOnAddress(&address, &compare, sizeof(compare), INFINITE);
+            ok(ret, "wait failed\n");
+            ok(GetLastError() == 0xdeadbeef || broken(GetLastError() == ERROR_SUCCESS) /* Win 8 */,
+                    "got error %d\n", GetLastError());
+        }
+    } while (InterlockedCompareExchange64(&address, compare - 1, compare) != compare);
+
     return 0;
 }

@@ -51,6 +55,7 @@ static void test_WaitOnAddress(void)
 {
     DWORD gle, val, nthreads;
     HANDLE threads[8];
+    LONG64 compare;
     BOOL ret;
     int i;

@@ -135,31 +140,28 @@ static void test_WaitOnAddress(void)

/* WakeByAddressAll */
     address = 0;
-    compare = 0;
     for (i = 0; i < ARRAY_SIZE(threads); i++)
         threads[i] = CreateThread(NULL, 0, test_WaitOnAddress_func, NULL, 0, NULL);

Sleep(100);
-    address = ~0;
+    address = ARRAY_SIZE(threads);
     pWakeByAddressAll(&address);
     val = WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, 5000);
     ok(val == WAIT_OBJECT_0, "got %d\n", val);
     for (i = 0; i < ARRAY_SIZE(threads); i++)
         CloseHandle(threads[i]);
+    ok(!address, "got unexpected value %s\n", wine_dbgstr_longlong(address));

/* WakeByAddressSingle */
     address = 0;
     for (i = 0; i < ARRAY_SIZE(threads); i++)
-        threads[i] = CreateThread(NULL, 0, test_WaitOnAddress_func, NULL, 0, NULL);
+            threads[i] = CreateThread(NULL, 0, test_WaitOnAddress_func, NULL, 0, NULL);

Sleep(100);
-    address = 1;
     nthreads = ARRAY_SIZE(threads);
+    address = ARRAY_SIZE(threads);
     while (nthreads)
     {
-        val = WaitForMultipleObjects(nthreads, threads, FALSE, 0);
-        ok(val == STATUS_TIMEOUT, "got %u\n", val);
-
         pWakeByAddressSingle(&address);
         val = WaitForMultipleObjects(nthreads, threads, FALSE, 2000);
         ok(val < WAIT_OBJECT_0 + nthreads, "got %u\n", val);
@@ -167,7 +169,7 @@ static void test_WaitOnAddress(void)
         memmove(&threads[val], &threads[val+1], (nthreads - val - 1) * sizeof(threads[0]));
         nthreads--;
     }
-
+    ok(!address, "got unexpected value %s\n", wine_dbgstr_longlong(address));
 }

START_TEST(sync)

[PATCH v2 2/2] ntdll: Add a futex-based implementation of WaitOnAddress().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
v2: fix timeout calculation.

dlls/ntdll/sync.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index 62cb3cd5ec..39e4ca02f0 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -26,7 +26,11 @@

#include <assert.h>
 #include <errno.h>
+#include <limits.h>
 #include <signal.h>
+#ifdef HAVE_SYS_SYSCALL_H
+#include <sys/syscall.h>
+#endif
 #ifdef HAVE_SYS_TIME_H
 # include <sys/time.h>
 #endif
@@ -63,6 +67,8 @@ HANDLE keyed_event = NULL;

static const LARGE_INTEGER zero_timeout;

+#define TICKSPERSEC 10000000
+
 static inline int interlocked_dec_if_nonzero( int *dest )
 {
     int val, tmp;
@@ -74,6 +80,41 @@ static inline int interlocked_dec_if_nonzero( int *dest )
     return val;
 }

+
+#ifdef __linux__
+
+static int wait_op = 128; /*FUTEX_WAIT|FUTEX_PRIVATE_FLAG*/
+static int wake_op = 129; /*FUTEX_WAKE|FUTEX_PRIVATE_FLAG*/
+
+static inline int futex_wait( const int *addr, int val, struct timespec *timeout )
+{
+    return syscall( __NR_futex, addr, wait_op, val, timeout, 0, 0 );
+}
+
+static inline int futex_wake( const int *addr, int val )
+{
+    return syscall( __NR_futex, addr, wake_op, val, NULL, 0, 0 );
+}
+
+static inline int use_futexes(void)
+{
+    static int supported = -1;
+
+    if (supported == -1)
+    {
+        futex_wait( &supported, 10, NULL );
+        if (errno == ENOSYS)
+        {
+            wait_op = 0; /*FUTEX_WAIT*/
+            wake_op = 1; /*FUTEX_WAKE*/
+            futex_wait( &supported, 10, NULL );
+        }
+        supported = (errno != ENOSYS);
+    }
+    return supported;
+}
+#endif
+
 /* creates 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 )
@@ -1987,6 +2028,90 @@ static BOOL compare_addr( const void *addr, const void *cmp, SIZE_T size )
     return FALSE;
 }

+#ifdef __linux__
+static int addr_futex_table[256];
+
+static inline int *hash_addr( const void *addr )
+{
+    ULONG_PTR val = (ULONG_PTR)addr;
+
+    return &addr_futex_table[(val >> 2) & 255];
+}
+
+static inline NTSTATUS fast_wait_addr( const void *addr, const void *cmp, SIZE_T size,
+                                       const LARGE_INTEGER *timeout )
+{
+    int *futex;
+    int val;
+    LARGE_INTEGER now;
+    timeout_t diff;
+    struct timespec timespec;
+    int ret;
+
+    if (!use_futexes())
+        return STATUS_NOT_IMPLEMENTED;
+
+    futex = hash_addr( addr );
+
+    /* We must read the previous value of the futex before checking the value
+     * of the address being waited on. That way, if we receive a wake between
+     * now and waiting on the futex, we know that val will have changed.
+     * Use an atomic load so that memory accesses are ordered between this read
+     * and the increment below. */
+    val = interlocked_cmpxchg( futex, 0, 0 );
+    if (!compare_addr( addr, cmp, size ))
+        return STATUS_SUCCESS;
+
+    if (timeout)
+    {
+        if (timeout->QuadPart > 0)
+        {
+            NtQuerySystemTime( &now );
+            diff = timeout->QuadPart - now.QuadPart;
+        }
+        else
+            diff = -timeout->QuadPart;
+
+        timespec.tv_sec  = diff / TICKSPERSEC;
+        timespec.tv_nsec = (diff % TICKSPERSEC) * 100;
+
+        ret = futex_wait( futex, val, &timespec );
+    }
+    else
+        ret = futex_wait( futex, val, NULL );
+
+    if (ret == -1 && errno == ETIMEDOUT)
+        return STATUS_TIMEOUT;
+    return STATUS_SUCCESS;
+}
+
+static inline NTSTATUS fast_wake_addr( const void *addr )
+{
+    int *futex;
+
+    if (!use_futexes())
+        return STATUS_NOT_IMPLEMENTED;
+
+    futex = hash_addr( addr );
+
+    interlocked_xchg_add( futex, 1 );
+
+    futex_wake( futex, INT_MAX );
+    return STATUS_SUCCESS;
+}
+#else
+static inline NTSTATUS fast_wait_addr( const void *addr, const void *cmp, SIZE_T size,
+                                       const LARGE_INTEGER *timeout )
+{
+    return STATUS_NOT_IMPLEMENTED;
+}
+
+static inline NTSTATUS fast_wake_addr( const void *addr )
+{
+    return STATUS_NOT_IMPLEMENTED;
+}
+#endif
+
 /***********************************************************************
  *           RtlWaitOnAddress   (NTDLL.@)
  */
@@ -2005,6 +2130,9 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
     if (size != 1 && size != 2 && size != 4 && size != 8)
         return STATUS_INVALID_PARAMETER;

+    if ((ret = fast_wait_addr( addr, cmp, size, timeout )) != STATUS_NOT_IMPLEMENTED)
+        return ret;
+
     select_op.keyed_event.op     = SELECT_KEYED_EVENT_WAIT;
     select_op.keyed_event.handle = wine_server_obj_handle( keyed_event );
     select_op.keyed_event.key    = wine_server_client_ptr( addr );
@@ -2059,6 +2187,9 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
  */
 void WINAPI RtlWakeAddressAll( const void *addr )
 {
+    if (fast_wake_addr( addr ) != STATUS_NOT_IMPLEMENTED)
+        return;
+
     RtlEnterCriticalSection( &addr_section );
     while (NtReleaseKeyedEvent( 0, addr, 0, &zero_timeout ) == STATUS_SUCCESS) {}
     RtlLeaveCriticalSection( &addr_section );
@@ -2069,6 +2200,9 @@ void WINAPI RtlWakeAddressAll( const void *addr )
  */
 void WINAPI RtlWakeAddressSingle( const void *addr )
 {
+    if (fast_wake_addr( addr ) != STATUS_NOT_IMPLEMENTED)
+        return;
+
     RtlEnterCriticalSection( &addr_section );
     NtReleaseKeyedEvent( 0, addr, 0, &zero_timeout );
     RtlLeaveCriticalSection( &addr_section );

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45490

Your paranoid android.

=== wxppro (32 bit report) ===

kernelbase:
sync: Timeout

[Zebediah Figura]

On 12/05/2018 10:19 AM, Marvin wrote:
> Hi,
>
> While running your changed tests on Windows, I think I found new failures.
> Being a bot and all I'm not very good at pattern recognition, so I might be
> wrong, but could you please double-check?
>
> Full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=45490
>
> Your paranoid android.
>
> === wxppro (32 bit report) ===
>
> kernelbase:
> sync: Timeout

This occurs because InterlockedCompareExchange64() is missing from 
kernel32. As far as I'm aware, though, kernelbase isn't present on 
Windows XP, so why does the TestBot even try to run these tests?

[Greg]

Hi Zeb,

I see that this pair of patches weren't included in wine 4.0. Now that 
the code freeze is over, what is the procedure for getting them 
resubmitted?  Please tell me if there is any way that I may help. I've 
been applying these patches to all vanilla git tagged releases and they 
continue work well and provide good performance results.

As far as Marvin's concern goes: I am not sure why the test is getting 
executed on XP at all - WaitOnAddress and friends are win8+ according to 
the MSDN docs.

Regards,

Greg

On 6/12/18 4:05 am, Zebediah Figura wrote:
> On 12/05/2018 10:19 AM, Marvin wrote:
> > Hi,
> >
> > While running your changed tests on Windows, I think I found new 
> > failures.
> > Being a bot and all I'm not very good at pattern recognition, so I 
> > might be
> > wrong, but could you please double-check?
> >
> > Full results can be found at:
> > https://testbot.winehq.org/JobDetails.pl?Key=45490
> >
> > Your paranoid android.
> >
> > === wxppro (32 bit report) ===
> >
> > kernelbase:
> > sync: Timeout
>
> This occurs because InterlockedCompareExchange64() is missing from 
> kernel32. As far as I'm aware, though, kernelbase isn't present on 
> Windows XP, so why does the TestBot even try to run these tests?