[PATCH 0/3] MR9557: ntdll: Support iterative barrier usage without re-initialization.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/sync.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index c997edcce91..66d376b0154 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -1382,7 +1382,7 @@ struct barrier_impl LONG spin_count; LONG total_thread_count; volatile LONG reached_thread_count; - volatile LONG waiting_thread_count; + volatile LONG structure_lock_count; volatile LONG wait_barrier_complete; }; @@ -1401,7 +1401,7 @@ NTSTATUS WINAPI RtlInitBarrier( RTL_BARRIER *barrier, LONG thread_count, LONG sp b->total_thread_count = thread_count; b->spin_count = spin_count; b->reached_thread_count = 0; - b->waiting_thread_count = 0; + b->structure_lock_count = 0; b->wait_barrier_complete = 0; return STATUS_SUCCESS; } @@ -1418,14 +1418,14 @@ void WINAPI RtlDeleteBarrier( RTL_BARRIER *barrier ) TRACE( "barrier %p.\n", barrier ); if (!barrier) return; - if (ReadAcquire( &b->reached_thread_count ) < b->total_thread_count && b->waiting_thread_count) + if (ReadAcquire( &b->reached_thread_count ) < b->total_thread_count && b->structure_lock_count) { /* On Windows this case will make RtlDeleteBarrier and the threads joining after wait forever, * unless the threads joining after will have SYNCHRONIZATION_BARRIER_FLAGS_NO_DELETE. */ ERR( "called before the barrier wait is satisfied.\n" ); } - while ((count = ReadAcquire( &b->waiting_thread_count ))) - RtlWaitOnAddress( (void *)&b->waiting_thread_count, &count, sizeof(b->waiting_thread_count), NULL ); + while ((count = ReadAcquire( &b->structure_lock_count ))) + RtlWaitOnAddress( (void *)&b->structure_lock_count, &count, sizeof(b->structure_lock_count), NULL ); } @@ -1450,7 +1450,7 @@ BOOLEAN WINAPI RtlBarrier( RTL_BARRIER *barrier, ULONG flags ) /* Incrementing reached_thread_count may trigger RTL_BARRIER data desrtuction from another thread, * so lock RtlDeleteBarrier with waiting_thread_count before that. */ - if (flags & 0x10000) InterlockedIncrement( &b->waiting_thread_count ); + if (flags & 0x10000) InterlockedIncrement( &b->structure_lock_count ); if (InterlockedIncrement( &b->reached_thread_count ) == b->total_thread_count) { WriteRelease( &b->wait_barrier_complete, 1 ); @@ -1474,12 +1474,12 @@ BOOLEAN WINAPI RtlBarrier( RTL_BARRIER *barrier, ULONG flags ) } done: - if (flags & 0x10000 && !InterlockedDecrement( &b->waiting_thread_count )) + if (flags & 0x10000 && !InterlockedDecrement( &b->structure_lock_count )) { /* Now RTL_BARRIER structure contents may become invalid. Signaling on its address should be fine, the worst * (unlikely) case it will wake something unrelated on reused address but that should be a legitimate spurious * wakeup case. */ - RtlWakeAddressAll( (const void *)&b->waiting_thread_count ); + RtlWakeAddressAll( (const void *)&b->structure_lock_count ); } return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9557
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/sync.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index e781d07dd4d..1d5a3c7ccd7 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1205,17 +1205,22 @@ struct test_barrier_thread_param BOOL wait_skipped; volatile LONG *count; volatile LONG *true_ret_count; + unsigned int iter_count; }; static DWORD WINAPI test_barrier_thread(void *param) { struct test_barrier_thread_param *p = param; + unsigned int i; InterlockedIncrement( p->count ); - if (pRtlBarrier( p->barrier, p->flags )) - InterlockedIncrement( p->true_ret_count ); + for (i = 0; i < p->iter_count; ++i) + { + if (pRtlBarrier( p->barrier, p->flags )) + InterlockedIncrement( p->true_ret_count ); + } if (!p->wait_skipped) - ok( *p->count == p->thread_count, "got %ld.\n", *p->count ); + ok( *p->count == p->thread_count * p->iter_count, "got %ld.\n", *p->count ); return 0; } @@ -1285,7 +1290,12 @@ static void test_barrier(void) status = pRtlInitBarrier( &barrier, 0, -1 ); ok( !status, "got %#lx.\n", status ); - + if (0) + { + /* Waits forever. */ + bval = pRtlBarrier( &barrier, 0 ); + ok( bval == 1, "got %#x.\n", bval ); + } status = pRtlInitBarrier( &barrier, 1, -2 ); ok( !status, "got %#lx.\n", status ); @@ -1317,20 +1327,28 @@ static void test_barrier(void) p.thread_count = ARRAY_SIZE(threads) + 1; p.count = &count; p.true_ret_count = &true_ret_count; + p.iter_count = 100; p.wait_skipped = TRUE; + + status = pRtlInitBarrier( &barrier, p.thread_count, -1 ); + ok( !status, "got %#lx.\n", status ); count = 0; true_ret_count = 0; for (i = 0; i < ARRAY_SIZE(threads); ++i) threads[i] = CreateThread( NULL, 0, test_barrier_thread, &p, 0, NULL ); - InterlockedIncrement( p.count ); - if (pRtlBarrier( p.barrier, p.flags )) - InterlockedIncrement( p.true_ret_count ); + + for (i = 0; i < p.iter_count; ++i) + { + InterlockedIncrement( p.count ); + if (pRtlBarrier( p.barrier, p.flags )) + InterlockedIncrement( p.true_ret_count ); + } for (i = 0; i < ARRAY_SIZE(threads); ++i) { WaitForSingleObject( threads[i], INFINITE ); CloseHandle( threads[i] ); } - ok( true_ret_count == p.thread_count, "got %ld.\n", true_ret_count ); + todo_wine ok( true_ret_count == p.iter_count, "got %ld.\n", true_ret_count ); /* Normal case. */ status = pRtlInitBarrier( &barrier, p.thread_count, -1 ); @@ -1340,6 +1358,7 @@ static void test_barrier(void) pRtlDeleteBarrier( &barrier ); p.flags = 0x10000; p.wait_skipped = FALSE; + p.iter_count = 1; count = 0; true_ret_count = 0; for (i = 0; i < ARRAY_SIZE(threads); ++i) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9557
From: Paul Gofman <pgofman(a)codeweavers.com> https://bugs.winehq.org/show_bug.cgi?id=59017 --- dlls/ntdll/sync.c | 33 ++++++++++++++++++++++++++++----- dlls/ntdll/tests/sync.c | 2 +- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 66d376b0154..18e61e47368 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -1383,6 +1383,7 @@ struct barrier_impl LONG total_thread_count; volatile LONG reached_thread_count; volatile LONG structure_lock_count; + volatile LONG waiting_thread_count; volatile LONG wait_barrier_complete; }; @@ -1402,6 +1403,7 @@ NTSTATUS WINAPI RtlInitBarrier( RTL_BARRIER *barrier, LONG thread_count, LONG sp b->spin_count = spin_count; b->reached_thread_count = 0; b->structure_lock_count = 0; + b->waiting_thread_count = 0; b->wait_barrier_complete = 0; return STATUS_SUCCESS; } @@ -1446,11 +1448,28 @@ BOOLEAN WINAPI RtlBarrier( RTL_BARRIER *barrier, ULONG flags ) if (flags & ~0x10000 && !once++) FIXME( "Unknown flags %#lx.\n", flags ); if (!barrier) return FALSE; - if (ReadAcquire( &b->reached_thread_count ) >= b->total_thread_count) return TRUE; - /* Incrementing reached_thread_count may trigger RTL_BARRIER data desrtuction from another thread, * so lock RtlDeleteBarrier with waiting_thread_count before that. */ if (flags & 0x10000) InterlockedIncrement( &b->structure_lock_count ); + + /* On Windows the long wait doesn't consume CPU with any spin count, so probably the spin count + * is limited or not used at all. */ + spin_count = min( 2000, (unsigned int)b->spin_count ); + + /* Wait for previous wait iteration to complete. */ + count = 0; + while (ReadAcquire( &b->reached_thread_count ) == b->total_thread_count) + { + if (count < spin_count) + { + ++count; + YieldProcessor(); + continue; + } + RtlWaitOnAddress( (void *)&b->reached_thread_count, &b->total_thread_count, + sizeof(b->reached_thread_count), NULL ); + } + InterlockedIncrement( &b->waiting_thread_count ); if (InterlockedIncrement( &b->reached_thread_count ) == b->total_thread_count) { WriteRelease( &b->wait_barrier_complete, 1 ); @@ -1458,9 +1477,6 @@ BOOLEAN WINAPI RtlBarrier( RTL_BARRIER *barrier, ULONG flags ) ret = TRUE; goto done; } - /* On Windows the long wait doesn't consume CPU with any spin count, so probably the spin count - * is limited or not used at all. */ - spin_count = min( 2000, (unsigned int)b->spin_count ); count = 0; while (ReadAcquire( &b->reached_thread_count ) < b->total_thread_count ) { @@ -1474,6 +1490,13 @@ BOOLEAN WINAPI RtlBarrier( RTL_BARRIER *barrier, ULONG flags ) } done: + if (!InterlockedDecrement( &b->waiting_thread_count )) + { + WriteRelease( &b->wait_barrier_complete, 0 ); + WriteRelease( &b->reached_thread_count, 0 ); + RtlWakeAddressAll( (const void *)&b->reached_thread_count ); + } + if (flags & 0x10000 && !InterlockedDecrement( &b->structure_lock_count )) { /* Now RTL_BARRIER structure contents may become invalid. Signaling on its address should be fine, the worst diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 1d5a3c7ccd7..dcbb8b18c3f 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1348,7 +1348,7 @@ static void test_barrier(void) WaitForSingleObject( threads[i], INFINITE ); CloseHandle( threads[i] ); } - todo_wine ok( true_ret_count == p.iter_count, "got %ld.\n", true_ret_count ); + ok( true_ret_count == p.iter_count, "got %ld.\n", true_ret_count ); /* Normal case. */ status = pRtlInitBarrier( &barrier, p.thread_count, -1 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9557
William Horvath (@whrvt) commented about dlls/ntdll/tests/sync.c:
status = pRtlInitBarrier( &barrier, 0, -1 ); ok( !status, "got %#lx.\n", status ); - + if (0)
Accidentally left this here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9557#note_123342
On Sat Nov 22 02:03:07 2025 +0000, William Horvath wrote:
Accidentally left this here? No, this is intentional. It is our common practice to leave tests that would hang or crash in 'if (0)' for documentation purposes and manual checks. Doing such a check in a working test is possible (by using a thread and then force killing it after some wait) but I believe doesn't worth the complication and test time increase in this case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9557#note_123380
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
William Horvath (＠whrvt)