[PATCH v2 resend 1/2] kernelbase/tests: Relax tests for WaitOnAddress().
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 <z.figura12(a)gmail.com> --- 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) -- 2.14.1
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- 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, ×pec ); + } + 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 ); -- 2.14.1
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=46762 Your paranoid android. === wxppro (32 bit report) === kernelbase: sync: Timeout
On 28/1/19 8:54 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=46762
Your paranoid android.
=== wxppro (32 bit report) ===
kernelbase: sync: Timeout
Thanks for resubmitting this Zeb. Putting aside the mysteries of kernelbase and WaitOnAddress existing in windows XP but notInterlockedCompareExchange64, it seems from my research that InterlockedCompareExchange should exist on windows XP. Would it be feasible and acceptable to change the size of 'compare' to a 32 bits and use InterlockedCompareExchange instead? Regards, Greg
On 1/27/19 10:34 PM, Greg wrote:
On 28/1/19 8:54 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=46762
Your paranoid android.
=== wxppro (32 bit report) ===
kernelbase: sync: Timeout
Thanks for resubmitting this Zeb.
Putting aside the mysteries of kernelbase and WaitOnAddress existing in windows XP but notInterlockedCompareExchange64, it seems from my research that InterlockedCompareExchange should exist on windows XP. Would it be feasible and acceptable to change the size of 'compare' to a 32 bits and use InterlockedCompareExchange instead?
Regards,
Greg
Yes, that is probably the best thing to do. If I receive no other feedback, I will resend the patch with that changed.
On 28/1/19 3:48 pm, Zebediah Figura wrote:
On 1/27/19 10:34 PM, Greg wrote:
On 28/1/19 8:54 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=46762
Your paranoid android.
=== wxppro (32 bit report) ===
kernelbase: sync: Timeout
Thanks for resubmitting this Zeb.
Putting aside the mysteries of kernelbase and WaitOnAddress existing in windows XP but notInterlockedCompareExchange64, it seems from my research that InterlockedCompareExchange should exist on windows XP. Would it be feasible and acceptable to change the size of 'compare' to a 32 bits and use InterlockedCompareExchange instead?
Regards,
Greg
Yes, that is probably the best thing to do. If I receive no other feedback, I will resend the patch with that changed.
Given that you are very busy working things that are much more important than something as trivial as getting a game running better, I dug around and prepared an XP VM for local testing, as well as getting my local environment setup for 32bit cross testing. After I reproduced the original problem, I then changed: - Both definitions of "LONG64 compare" to "DWORD compare"; and - "InterlockedCompareExchange64" to "InterlockedCompareExchange" I followed this by a complete rebuild and retesting locally (ubuntu64+32bit test in LXC instance), then on Win10, WinXP, and debian32. All completed successfully. It turns out that the test is skipped on WinXP due to WaitOnAddress not being present. The problem was that the exe had a reference to InterlockedCompareExchange64 which was being resolved when the test was kicked off, causing the error. Regarding the question about kernelbase tests being run on WinXP: It seems that all crosstest outputs are executed on all platforms. I guess that reduces complexity for the testbot. Thanks for all your help with this minor issue, I really appreciate you taking the time out to do it.
Hello Greg, On 2/5/19 5:40 PM, Greg wrote:
On 28/1/19 3:48 pm, Zebediah Figura wrote:
On 1/27/19 10:34 PM, Greg wrote:
On 28/1/19 8:54 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=46762
Your paranoid android.
=== wxppro (32 bit report) ===
kernelbase: sync: Timeout
Thanks for resubmitting this Zeb.
Putting aside the mysteries of kernelbase and WaitOnAddress existing in windows XP but notInterlockedCompareExchange64, it seems from my research that InterlockedCompareExchange should exist on windows XP. Would it be feasible and acceptable to change the size of 'compare' to a 32 bits and use InterlockedCompareExchange instead?
Regards,
Greg
Yes, that is probably the best thing to do. If I receive no other feedback, I will resend the patch with that changed.
Given that you are very busy working things that are much more important than something as trivial as getting a game running better, I dug around and prepared an XP VM for local testing, as well as getting my local environment setup for 32bit cross testing.
After I reproduced the original problem, I then changed: - Both definitions of "LONG64 compare" to "DWORD compare"; and - "InterlockedCompareExchange64" to "InterlockedCompareExchange"
I followed this by a complete rebuild and retesting locally (ubuntu64+32bit test in LXC instance), then on Win10, WinXP, and debian32. All completed successfully. It turns out that the test is skipped on WinXP due to WaitOnAddress not being present. The problem was that the exe had a reference to InterlockedCompareExchange64 which was being resolved when the test was kicked off, causing the error. Regarding the question about kernelbase tests being run on WinXP: It seems that all crosstest outputs are executed on all platforms. I guess that reduces complexity for the testbot.
Thanks for all your help with this minor issue, I really appreciate you taking the time out to do it.
My sincere apologies. I quite honestly have many things taking up my time, most of which aren't related to Wine, and I had forgotten I had outstanding criticism on this patchset. I've resent it using 4-byte values.
participants (3)
-
Greg -
Marvin -
Zebediah Figura