[PATCH 0/2] MR3929: ntdll: Remove entries from queue in RtlWakeAddressAll().
This avoids one case of spurious wakeup of WaitOnAddress (which could be triggered by prior race on consequent WakeAddressAll and a thread trying to wake on that, leaving the waking thread in an alerted state). And fixes Resident Evil games randomly crashing due to unhandled spurious SleepConditionVariableCS wakeups. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/sync.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index fa64917029a..1e2019e52c4 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -925,8 +925,8 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size void WINAPI RtlWakeAddressAll( const void *addr ) { struct futex_queue *queue = get_futex_queue( addr ); + struct futex_entry *entry, *next; unsigned int count = 0, i; - struct futex_entry *entry; DWORD tids[256]; TRACE("%p\n", addr); @@ -938,10 +938,12 @@ void WINAPI RtlWakeAddressAll( const void *addr ) if (!queue->queue.next) list_init(&queue->queue); - LIST_FOR_EACH_ENTRY( entry, &queue->queue, struct futex_entry, entry ) + LIST_FOR_EACH_ENTRY_SAFE( entry, next, &queue->queue, struct futex_entry, entry ) { if (entry->addr == addr) { + entry->addr = NULL; + list_remove( &entry->entry ); /* Try to buffer wakes, so that we don't make a system call while * holding a spinlock. */ if (count < ARRAY_SIZE(tids)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3929
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/sync.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 1e2019e52c4..3b1e4a721e2 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -907,11 +907,14 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size ret = NtWaitForAlertByThreadId( NULL, timeout ); - spin_lock( &queue->lock ); - /* We may have already been removed by a call to RtlWakeAddressSingle(). */ + /* We may have already been removed by a call to RtlWakeAddressSingle() or RtlWakeAddressAll(). */ if (entry.addr) - list_remove( &entry.entry ); - spin_unlock( &queue->lock ); + { + spin_lock( &queue->lock ); + if (entry.addr) + list_remove( &entry.entry ); + spin_unlock( &queue->lock ); + } TRACE("returning %#lx\n", ret); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3929
I made a small perf text just in case (attached), which of course can't catch the difference of RtlWakeAddressAll() time due to pre-existing much greater variance in the execution time. The change is very subtle perf wise. Unrelated here, but the test shows some things interesting by itsef. E. g., the time of RtlWakeAddressAll with 6 threads is more than 2 times bigger than on Windows, and as far as my further profiling went, the vast majority of time is spent in Linux futex_wake() (not even in multiple NtAlerthThreadById calls through dispacther, hacking those in one call instead of 6 improves things aon average a bit but not significantly). [waitonaddr.c](/uploads/f4ac4129f4fb5ccfb0d364313936c90c/waitonaddr.c) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929#note_46366
As an unrelated note (that issue is the same before and after these patches), the fact we add a stack variable to the queue list looks not great: if a thread gets force terminated the queue will be left with bad data in the list. As unfortunate as it sounds, we probably have to allocate queue entry elsewise using some cache? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929#note_47494
As an unrelated note (that issue is the same before and after these patches), the fact we add a stack variable to the queue list looks not great: if a thread gets force terminated the queue will be left with bad data in the list. As unfortunate as it sounds, we probably have to allocate queue entry elsewise using some cache?
Anyone using TerminateThread() is asking for trouble. Unless we specifically run into an application that expects to be able to terminate a thread mid-wait, I don't think there's anything that needs to be done here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929#note_48670
Unless we specifically run into an application that expects to be able to terminate a thread mid-wait, I don't think there's anything that needs to be done here.
Yeah, at least such an issue will be relatively easy to spot. Unlike the one fixed by this MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929#note_48671
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929
Sorry this never got approved... not sure why I let it fall on the floor. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3929#note_68669
participants (4)
-
Elizabeth Figura (＠zfigura) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)