[PATCH 0/1] MR10177: ntdll: Fix threadpool timer behavior on system time change.
Threadpool timers are not handling system clock changes correctly. Observed while trying to start delayed-start service on DST change (service started after 1h02m instead of 2m). These patch changes threadpool timers to use NtCreateTimers internally. It matches with native implementation as documented here: https://github.com/SafeBreach-Labs/PoolParty. While it moves implementation closer to what native does I didn't work on making it binary compatible. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10177
From: Piotr Caban <piotr@codeweavers.com> --- dlls/kernel32/tests/timer.c | 73 ++++++++- dlls/ntdll/threadpool.c | 284 ++++++++++++++++++++++++------------ 2 files changed, 260 insertions(+), 97 deletions(-) diff --git a/dlls/kernel32/tests/timer.c b/dlls/kernel32/tests/timer.c index 47cfd11062e..2106c5b35c2 100644 --- a/dlls/kernel32/tests/timer.c +++ b/dlls/kernel32/tests/timer.c @@ -203,9 +203,78 @@ static DWORD WINAPI thread_SetTimer(void *arg) return 0; } +static void WINAPI timer_callback(PTP_CALLBACK_INSTANCE inst, void* ctx, PTP_TIMER timer) +{ + HANDLE event = ctx; + SetEvent(event); +} + +static DWORD WINAPI thread_SetThreadpoolTimer_rel(void *arg) +{ + LARGE_INTEGER li; + TP_TIMER *timer; + HANDLE event; + FILETIME ft; + DWORD t, r; + + event = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(event != NULL, "CreateEvent failed\n"); + timer = CreateThreadpoolTimer(timer_callback, event, NULL); + ok(timer != NULL, "CreateThreadpoolTimer failed\n"); + + li.QuadPart = -3 * TICKSPERSEC; + ft.dwHighDateTime = li.HighPart; + ft.dwLowDateTime = li.LowPart; + t = GetTickCount(); + SetThreadpoolTimer(timer, &ft, 0, 0); + + r = WaitForSingleObject(event, INFINITE); + ok(r == WAIT_OBJECT_0, "WaitForSingleObject returned %ld\n", r); + t = GetTickCount() - t; + ok(t > 2000, "t = %ld\n", t); + + WaitForThreadpoolTimerCallbacks(timer, FALSE); + CloseThreadpoolTimer(timer); + CloseHandle(event); + return 0; +} + +static DWORD WINAPI thread_SetThreadpoolTimer_abs(void *arg) +{ + LARGE_INTEGER li; + TP_TIMER *timer; + HANDLE event; + FILETIME ft; + DWORD t, r; + + event = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(event != NULL, "CreateEvent failed\n"); + timer = CreateThreadpoolTimer(timer_callback, event, NULL); + ok(timer != NULL, "CreateThreadpoolTimer failed\n"); + + GetSystemTimeAsFileTime(&ft); + li.HighPart = ft.dwHighDateTime; + li.LowPart = ft.dwLowDateTime; + li.QuadPart += 3 * TICKSPERSEC; + ft.dwHighDateTime = li.HighPart; + ft.dwLowDateTime = li.LowPart; + t = GetTickCount(); + SetThreadpoolTimer(timer, &ft, 0, 0); + + r = WaitForSingleObject(event, INFINITE); + ok(r == WAIT_OBJECT_0, "WaitForSingleObject returned %ld\n", r); + t = GetTickCount() - t; + ok(t < 2000, "t = %ld\n", t); + + WaitForThreadpoolTimerCallbacks(timer, FALSE); + CloseThreadpoolTimer(timer); + CloseHandle(event); + return 0; +} + static void test_timeouts(void) { - HANDLE threads[7]; + HANDLE threads[9]; DWORD i; if (!adjust_system_time(1)) @@ -222,6 +291,8 @@ static void test_timeouts(void) threads[4] = CreateThread(NULL, 0, thread_WaitableTimer_abs, NULL, 0, NULL); threads[5] = CreateThread(NULL, 0, thread_WaitableTimer_period, NULL, 0, NULL); threads[6] = CreateThread(NULL, 0, thread_SetTimer, NULL, 0, NULL); + threads[7] = CreateThread(NULL, 0, thread_SetThreadpoolTimer_rel, NULL, 0, NULL); + threads[8] = CreateThread(NULL, 0, thread_SetThreadpoolTimer_abs, NULL, 0, NULL); for(i=0; i<ARRAY_SIZE(threads); i++) ok(threads[i] != NULL, "CreateThread failed\n"); diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 64a25694467..0aacfa6449f 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -240,21 +240,29 @@ struct threadpool_group /* global timerqueue object */ static RTL_CRITICAL_SECTION_DEBUG timerqueue_debug; +enum +{ + ABS_TIMER, + REL_TIMER +}; + static struct { CRITICAL_SECTION cs; LONG objcount; BOOL thread_running; - struct list pending_timers; - RTL_CONDITION_VARIABLE update_event; + HANDLE timers[2]; + struct list abs_timers; + struct list rel_timers; } timerqueue = { { &timerqueue_debug, -1, 0, 0, 0, 0 }, /* cs */ 0, /* objcount */ FALSE, /* thread_running */ - LIST_INIT( timerqueue.pending_timers ), /* pending_timers */ - RTL_CONDITION_VARIABLE_INIT /* update_event */ + { 0, 0 }, /* timers */ + LIST_INIT( timerqueue.abs_timers ), /* abs_timers */ + LIST_INIT( timerqueue.rel_timers ) /* rel_timers */ }; static RTL_CRITICAL_SECTION_DEBUG timerqueue_debug = @@ -1052,90 +1060,140 @@ NTSTATUS WINAPI RtlDeleteTimer(HANDLE TimerQueue, HANDLE Timer, } /*********************************************************************** - * timerqueue_thread_proc (internal) + * submit_expired_timers (internal) + * + * timerqueue.cs held by caller. */ -static void CALLBACK timerqueue_thread_proc( void *param ) +static void submit_expired_timers( struct list *queue, ULONGLONG queue_now, ULONGLONG rel_now ) { - ULONGLONG timeout_lower, timeout_upper, new_timeout; struct threadpool_object *other_timer; - LARGE_INTEGER now, timeout; struct list *ptr; - TRACE( "starting timer queue thread\n" ); - set_thread_name(L"wine_threadpool_timerqueue"); - - RtlEnterCriticalSection( &timerqueue.cs ); - for (;;) + while ((ptr = list_head( queue ))) { - NtQuerySystemTime( &now ); + struct threadpool_object *timer = LIST_ENTRY( ptr, struct threadpool_object, u.timer.timer_entry ); + assert( timer->type == TP_OBJECT_TYPE_TIMER ); + assert( timer->u.timer.timer_pending ); + if (timer->u.timer.timeout > queue_now) + return; + + /* Queue a new callback in one of the worker threads. */ + list_remove( &timer->u.timer.timer_entry ); + timer->u.timer.timer_pending = FALSE; + tp_object_submit( timer, FALSE ); - /* Check for expired timers. */ - while ((ptr = list_head( &timerqueue.pending_timers ))) + /* Insert the timer back into the queue, except it's marked for shutdown. */ + if (timer->u.timer.period && !timer->shutdown) { - struct threadpool_object *timer = LIST_ENTRY( ptr, struct threadpool_object, u.timer.timer_entry ); - assert( timer->type == TP_OBJECT_TYPE_TIMER ); - assert( timer->u.timer.timer_pending ); - if (timer->u.timer.timeout > now.QuadPart) - break; + /* Update timeout when moving timer to relative queue */ + if (queue == &timerqueue.abs_timers) + timer->u.timer.timeout = rel_now; - /* Queue a new callback in one of the worker threads. */ - list_remove( &timer->u.timer.timer_entry ); - timer->u.timer.timer_pending = FALSE; - tp_object_submit( timer, FALSE ); + timer->u.timer.timeout += (ULONGLONG)timer->u.timer.period * 10000; + if (timer->u.timer.timeout <= rel_now) + timer->u.timer.timeout = rel_now + 1; - /* Insert the timer back into the queue, except it's marked for shutdown. */ - if (timer->u.timer.period && !timer->shutdown) + LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.rel_timers, + struct threadpool_object, u.timer.timer_entry ) { - timer->u.timer.timeout += (ULONGLONG)timer->u.timer.period * 10000; - if (timer->u.timer.timeout <= now.QuadPart) - timer->u.timer.timeout = now.QuadPart + 1; - - LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers, - struct threadpool_object, u.timer.timer_entry ) - { - assert( other_timer->type == TP_OBJECT_TYPE_TIMER ); - if (timer->u.timer.timeout < other_timer->u.timer.timeout) - break; - } - list_add_before( &other_timer->u.timer.timer_entry, &timer->u.timer.timer_entry ); - timer->u.timer.timer_pending = TRUE; + assert( other_timer->type == TP_OBJECT_TYPE_TIMER ); + if (timer->u.timer.timeout < other_timer->u.timer.timeout) + break; } + list_add_before( &other_timer->u.timer.timer_entry, &timer->u.timer.timer_entry ); + timer->u.timer.timer_pending = TRUE; } + } +} - timeout_lower = timeout_upper = MAXLONGLONG; +/*********************************************************************** + * get_next_timeout (internal) + * + * timerqueue.cs held by caller. + */ +static ULONGLONG get_next_timeout( struct list *list ) +{ + ULONGLONG timeout_lower, timeout_upper, new_timeout; + struct threadpool_object *other_timer; - /* Determine next timeout and use the window length to optimize wakeup times. */ - LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers, - struct threadpool_object, u.timer.timer_entry ) - { - assert( other_timer->type == TP_OBJECT_TYPE_TIMER ); - if (other_timer->u.timer.timeout >= timeout_upper) - break; + timeout_lower = timeout_upper = MAXLONGLONG; - timeout_lower = other_timer->u.timer.timeout; - new_timeout = timeout_lower + (ULONGLONG)other_timer->u.timer.window_length * 10000; - if (new_timeout < timeout_upper) - timeout_upper = new_timeout; - } + /* Determine next timeout and use the window length to optimize wakeup times. */ + LIST_FOR_EACH_ENTRY( other_timer, list, struct threadpool_object, u.timer.timer_entry ) + { + assert( other_timer->type == TP_OBJECT_TYPE_TIMER ); + if (other_timer->u.timer.timeout >= timeout_upper) + break; - /* Wait for timer update events or until the next timer expires. */ - if (timerqueue.objcount) - { - timeout.QuadPart = timeout_lower; - RtlSleepConditionVariableCS( &timerqueue.update_event, &timerqueue.cs, &timeout ); - continue; - } + timeout_lower = other_timer->u.timer.timeout; + new_timeout = timeout_lower + (ULONGLONG)other_timer->u.timer.window_length * 10000; + if (new_timeout < timeout_upper) + timeout_upper = new_timeout; + } + return timeout_lower; +} +/*********************************************************************** + * update_timers (internal) + * + * timerqueue.cs held by caller. + */ +static void update_timers( ULONGLONG rel_now ) +{ + LARGE_INTEGER timeout; + + timeout.QuadPart = get_next_timeout( &timerqueue.abs_timers ); + NtSetTimer( timerqueue.timers[ABS_TIMER], &timeout, NULL, NULL, FALSE, 0, NULL ); + + if (timerqueue.objcount) + { + timeout.QuadPart = get_next_timeout( &timerqueue.rel_timers ); + if (timeout.QuadPart > rel_now) + timeout.QuadPart = rel_now - timeout.QuadPart; + else + timeout.QuadPart = 0; + } + else + { /* All timers have been destroyed, if no new timers are created * within some amount of time, then we can shutdown this thread. */ timeout.QuadPart = (ULONGLONG)THREADPOOL_WORKER_TIMEOUT * -10000; - if (RtlSleepConditionVariableCS( &timerqueue.update_event, &timerqueue.cs, - &timeout ) == STATUS_TIMEOUT && !timerqueue.objcount) - { + } + NtSetTimer( timerqueue.timers[REL_TIMER], &timeout, NULL, NULL, FALSE, 0, NULL ); +} + +/*********************************************************************** + * timerqueue_thread_proc (internal) + */ +static void CALLBACK timerqueue_thread_proc( void *param ) +{ + LARGE_INTEGER abs_now, rel_now; + + TRACE( "starting timer queue thread\n" ); + set_thread_name(L"wine_threadpool_timerqueue"); + + RtlEnterCriticalSection( &timerqueue.cs ); + for (;;) + { + NtQuerySystemTime( &abs_now ); + NtQueryPerformanceCounter( &rel_now, NULL ); + + submit_expired_timers( &timerqueue.abs_timers, abs_now.QuadPart, rel_now.QuadPart ); + submit_expired_timers( &timerqueue.rel_timers, rel_now.QuadPart, rel_now.QuadPart ); + update_timers( rel_now.QuadPart ); + + RtlLeaveCriticalSection( &timerqueue.cs ); + NtWaitForMultipleObjects( ARRAY_SIZE(timerqueue.timers), + timerqueue.timers, WaitAny, FALSE, NULL ); + RtlEnterCriticalSection( &timerqueue.cs ); + + if (!timerqueue.objcount) break; - } } + NtClose( timerqueue.timers[ABS_TIMER] ); + NtClose( timerqueue.timers[REL_TIMER] ); + timerqueue.thread_running = FALSE; RtlLeaveCriticalSection( &timerqueue.cs ); @@ -1173,7 +1231,6 @@ static NTSTATUS tp_new_worker_thread( struct threadpool *pool ) */ static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer ) { - NTSTATUS status = STATUS_SUCCESS; assert( timer->type == TP_OBJECT_TYPE_TIMER ); timer->u.timer.timer_initialized = FALSE; @@ -1188,24 +1245,44 @@ static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer ) /* Make sure that the timerqueue thread is running. */ if (!timerqueue.thread_running) { + NTSTATUS status; HANDLE thread; + + status = NtCreateTimer( &timerqueue.timers[ABS_TIMER], TIMER_ALL_ACCESS, + NULL, NotificationTimer ); + if (status != STATUS_SUCCESS) + { + RtlLeaveCriticalSection( &timerqueue.cs ); + return status; + } + + status = NtCreateTimer( &timerqueue.timers[REL_TIMER], TIMER_ALL_ACCESS, + NULL, NotificationTimer ); + if (status != STATUS_SUCCESS) + { + NtClose( timerqueue.timers[ABS_TIMER] ); + RtlLeaveCriticalSection( &timerqueue.cs ); + return status; + } + status = RtlCreateUserThread( GetCurrentProcess(), NULL, FALSE, 0, 0, 0, timerqueue_thread_proc, NULL, &thread, NULL ); - if (status == STATUS_SUCCESS) + if (status != STATUS_SUCCESS) { - timerqueue.thread_running = TRUE; - NtClose( thread ); + NtClose( timerqueue.timers[ABS_TIMER] ); + NtClose( timerqueue.timers[REL_TIMER] ); + RtlLeaveCriticalSection( &timerqueue.cs ); + return status; } + timerqueue.thread_running = TRUE; + NtClose( thread ); } - if (status == STATUS_SUCCESS) - { - timer->u.timer.timer_initialized = TRUE; - timerqueue.objcount++; - } + timer->u.timer.timer_initialized = TRUE; + timerqueue.objcount++; RtlLeaveCriticalSection( &timerqueue.cs ); - return status; + return STATUS_SUCCESS; } /*********************************************************************** @@ -1227,11 +1304,15 @@ static void tp_timerqueue_unlock( struct threadpool_object *timer ) timer->u.timer.timer_pending = FALSE; } - /* If the last timer object was destroyed, then wake up the thread. */ + /* If the last timer object was destroyed, then update timeout. */ if (!--timerqueue.objcount) { - assert( list_empty( &timerqueue.pending_timers ) ); - RtlWakeAllConditionVariable( &timerqueue.update_event ); + LARGE_INTEGER rel_now; + + assert( list_empty( &timerqueue.abs_timers ) ); + assert( list_empty( &timerqueue.rel_timers ) ); + NtQueryPerformanceCounter( &rel_now, NULL ); + update_timers( rel_now.QuadPart ); } timer->u.timer.timer_initialized = FALSE; @@ -2997,6 +3078,7 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO { struct threadpool_object *this = impl_from_TP_TIMER( timer ); struct threadpool_object *other_timer; + struct list *pending_timers; BOOL submit_timer = FALSE; ULONGLONG timestamp; @@ -3011,23 +3093,29 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO * of zero, which means that the timer is submitted immediately. */ if (timeout) { - timestamp = timeout->QuadPart; - if ((LONGLONG)timestamp < 0) + if (timeout->QuadPart > 0) { - LARGE_INTEGER now; - NtQuerySystemTime( &now ); - timestamp = now.QuadPart - timestamp; + timestamp = timeout->QuadPart; + pending_timers = &timerqueue.abs_timers; } - else if (!timestamp) + else if (timeout->QuadPart < 0) { - if (!period) - timeout = NULL; - else - { - LARGE_INTEGER now; - NtQuerySystemTime( &now ); - timestamp = now.QuadPart + (ULONGLONG)period * 10000; - } + LARGE_INTEGER rel_now; + NtQueryPerformanceCounter( &rel_now, NULL ); + timestamp = rel_now.QuadPart - timeout->QuadPart; + pending_timers = &timerqueue.rel_timers; + } + else if (!period) + { + timeout = NULL; + submit_timer = TRUE; + } + else + { + LARGE_INTEGER rel_now; + NtQueryPerformanceCounter( &rel_now, NULL ); + timestamp = rel_now.QuadPart + (ULONGLONG)period * 10000; + pending_timers = &timerqueue.rel_timers; submit_timer = TRUE; } } @@ -3046,8 +3134,8 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO this->u.timer.period = period; this->u.timer.window_length = window_length; - LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers, - struct threadpool_object, u.timer.timer_entry ) + LIST_FOR_EACH_ENTRY( other_timer, pending_timers, struct threadpool_object, + u.timer.timer_entry ) { assert( other_timer->type == TP_OBJECT_TYPE_TIMER ); if (this->u.timer.timeout < other_timer->u.timer.timeout) @@ -3055,9 +3143,13 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO } list_add_before( &other_timer->u.timer.timer_entry, &this->u.timer.timer_entry ); - /* Wake up the timer thread when the timeout has to be updated. */ - if (list_head( &timerqueue.pending_timers ) == &this->u.timer.timer_entry ) - RtlWakeAllConditionVariable( &timerqueue.update_event ); + /* Update timeout if needed. */ + if (list_head( pending_timers ) == &this->u.timer.timer_entry ) + { + LARGE_INTEGER rel_now; + NtQueryPerformanceCounter( &rel_now, NULL ); + update_timers( rel_now.QuadPart ); + } this->u.timer.timer_pending = TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10177
participants (2)
-
Piotr Caban -
Piotr Caban (@piotr)