[PATCH] kernelbase: Fix relationship between Sleep() and GetTickCount()
Sleep() and GetTickCount() work on Windows in 15.6ms increments. Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping. Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang. This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls). This fixes Doom II from Steam. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49564 Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/kernelbase/sync.c | 20 ++++++++++++++++---- dlls/ntdll/tests/time.c | 2 +- server/fd.c | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/dlls/kernelbase/sync.c b/dlls/kernelbase/sync.c index 13a9938e7c..673c5b60d9 100644 --- a/dlls/kernelbase/sync.c +++ b/dlls/kernelbase/sync.c @@ -51,13 +51,25 @@ static inline BOOL is_version_nt(void) } /* helper for kernel32->ntdll timeout format conversion */ -static inline LARGE_INTEGER *get_nt_timeout( LARGE_INTEGER *time, DWORD timeout ) +static inline LARGE_INTEGER *get_aligned_nt_timeout( LARGE_INTEGER *time, DWORD timeout, DWORD alignment ) { + ULONGLONG quad ; + if (timeout == INFINITE) return NULL; - time->QuadPart = (ULONGLONG)timeout * -10000; + + quad = (ULONGLONG)timeout * 10000; + quad = ((quad + alignment - 1) / alignment) * alignment; + + time->QuadPart = quad * -1; + return time; } +static inline LARGE_INTEGER *get_nt_timeout( LARGE_INTEGER *time, DWORD timeout) +{ + return get_aligned_nt_timeout(time, timeout, 10000); +} + /*********************************************************************** * BaseGetNamedObjectDirectory (kernelbase.@) @@ -269,7 +281,7 @@ void WINAPI DECLSPEC_HOTPATCH Sleep( DWORD timeout ) { LARGE_INTEGER time; - NtDelayExecution( FALSE, get_nt_timeout( &time, timeout ) ); + NtDelayExecution( FALSE, get_aligned_nt_timeout( &time, timeout, 156000 ) ); } @@ -281,7 +293,7 @@ DWORD WINAPI DECLSPEC_HOTPATCH SleepEx( DWORD timeout, BOOL alertable ) NTSTATUS status; LARGE_INTEGER time; - status = NtDelayExecution( alertable, get_nt_timeout( &time, timeout ) ); + status = NtDelayExecution( alertable, get_aligned_nt_timeout( &time, timeout, 156000 ) ); if (status == STATUS_USER_APC) return WAIT_IO_COMPLETION; return 0; } diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index a00d507e4e..0e0efe6384 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -261,7 +261,7 @@ static void test_user_shared_data_time(void) t2 = GetTickCount(); if (t1 != t2) changed++; } - todo_wine ok(changed >= 90, "tick count isn't updated after sleeping one millisecond (%d%% correct)\n", changed); + ok(changed >= 90, "tick count isn't updated after sleeping one millisecond (%d%% correct)\n", changed); } START_TEST(time) diff --git a/server/fd.c b/server/fd.c index 7ea8ac273e..1bdce6c70c 100644 --- a/server/fd.c +++ b/server/fd.c @@ -377,7 +377,7 @@ timeout_t current_time; timeout_t monotonic_time; struct _KUSER_SHARED_DATA *user_shared_data = NULL; -static const int user_shared_data_timeout = 16; +static const int user_shared_data_timeout = 15; static void set_user_shared_data_time(void) { -- 2.28.0
Hi, While running your changed tests, 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=76758 Your paranoid android. === w1064v1809_he (32 bit report) === ntdll: time.c:264: Test failed: tick count isn't updated after sleeping one millisecond (87% correct)
On Mon, Aug 10, 2020 at 06:02:46AM -0500, Marvin wrote:
Hi,
While running your changed tests, 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=76758
Your paranoid android.
=== w1064v1809_he (32 bit report) ===
ntdll: time.c:264: Test failed: tick count isn't updated after sleeping one millisecond (87% correct)
Interesting to see it sporadically failing on Windows, just confirms how non-deterministic GetTickCount() <-> Sleep() relationship is. I am not sure what to do about that. I did also few round of a binary that prints deltas if anyone is interested: https://testbot.winehq.org/JobDetails.pl?Key=76749&f101=exe64.report&f107=ex... -- Cheers, Arek
On 8/10/20 13:55, Arkadiusz Hiler wrote:
Sleep() and GetTickCount() work on Windows in 15.6ms increments.
Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping.
Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang.
This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls).
This fixes Doom II from Steam.
I also did notice that Sleep on Windows use to work with 15.6ms quantum some time ago. But the important part which is missed here is that Sleep behaviour is affected (at least) by timeBeginPeriod() / timeEndPeriod() winmm functions (as I recalled now after rerunning my old test program and wondering why I see 1ms sleep quantum). E. g., if I call timeBeingPeriod(1) before testing Sleep actual time and GetTickCount change, the Sleep starts to behave very similar to how it works now in Wine: sleeps with 1ms quantum and GetTickCount() is not necessarily updated on wake. I think we can't ignore that behaviour, I did see the games playing with winmm functions, Sleep etc. I was also initially going to say that maybe we would need to explicitly check if GetTickCount has changed during sleep, but the failing test on Win10 suggests that maybe Windows doesn't guarantee that also, just sleeps with 15.6ms quantum by default (unless changed by winmm).
On 8/10/20 14:32, Paul Gofman wrote:
On 8/10/20 13:55, Arkadiusz Hiler wrote:
Sleep() and GetTickCount() work on Windows in 15.6ms increments.
Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping.
Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang.
This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls).
This fixes Doom II from Steam.
I also did notice that Sleep on Windows use to work with 15.6ms quantum some time ago. But the important part which is missed here is that Sleep behaviour is affected (at least) by timeBeginPeriod() / timeEndPeriod() winmm functions (as I recalled now after rerunning my old test program and wondering why I see 1ms sleep quantum). E. g., if I call timeBeingPeriod(1) before testing Sleep actual time and GetTickCount change, the Sleep starts to behave very similar to how it works now in Wine: sleeps with 1ms quantum and GetTickCount() is not necessarily updated on wake. I think we can't ignore that behaviour, I did see the games playing with winmm functions, Sleep etc.
I was also initially going to say that maybe we would need to explicitly check if GetTickCount has changed during sleep, but the failing test on Win10 suggests that maybe Windows doesn't guarantee that also, just sleeps with 15.6ms quantum by default (unless changed by winmm).
PS If timeBeginPeriod() indeed sets the global time resolution for all processes as the docs suggest, I can guess there should be a field in KSHARED_USER_DATA which holds that resolution (there are a few related to timers, I did not ever test what exactly do they hold), and we should probably use those to store and retrieve timer resolution.
On Mon, Aug 10, 2020 at 02:45:49PM +0300, Paul Gofman wrote:
On 8/10/20 14:32, Paul Gofman wrote:
On 8/10/20 13:55, Arkadiusz Hiler wrote:
Sleep() and GetTickCount() work on Windows in 15.6ms increments.
Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping.
Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang.
This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls).
This fixes Doom II from Steam.
I also did notice that Sleep on Windows use to work with 15.6ms quantum some time ago. But the important part which is missed here is that Sleep behaviour is affected (at least) by timeBeginPeriod() / timeEndPeriod() winmm functions (as I recalled now after rerunning my old test program and wondering why I see 1ms sleep quantum). E. g., if I call timeBeingPeriod(1) before testing Sleep actual time and GetTickCount change, the Sleep starts to behave very similar to how it works now in Wine: sleeps with 1ms quantum and GetTickCount() is not necessarily updated on wake. I think we can't ignore that behaviour, I did see the games playing with winmm functions, Sleep etc.
I was also initially going to say that maybe we would need to explicitly check if GetTickCount has changed during sleep, but the failing test on Win10 suggests that maybe Windows doesn't guarantee that also, just sleeps with 15.6ms quantum by default (unless changed by winmm).
PS If timeBeginPeriod() indeed sets the global time resolution for all processes as the docs suggest, I can guess there should be a field in KSHARED_USER_DATA which holds that resolution (there are a few related to timers, I did not ever test what exactly do they hold), and we should probably use those to store and retrieve timer resolution.
Indeed timeBeginPeriod() seems to affect the resolution of Sleep() https://hiler.eu/p/0701eab8d523.txt https://testbot.winehq.org/JobDetails.pl?Key=76759&f109=exe64.report#k109 This is completely unexpected... Thanks for pointing this out! I guess I have to investigate whether it's truly global and if there is a field for that in KSHARED_USER_DATA. That would be a bit funny - anything that depends on the default behavior will be broken while that process is running, and if it crashes/forgets to call timeEndPeriod() upon exit we are left in that broken state forever :-) -- Cheers, Arek
On 8/10/20 14:55, Arkadiusz Hiler wrote:
On Mon, Aug 10, 2020 at 02:45:49PM +0300, Paul Gofman wrote:
On 8/10/20 14:32, Paul Gofman wrote:
On 8/10/20 13:55, Arkadiusz Hiler wrote:
Sleep() and GetTickCount() work on Windows in 15.6ms increments.
Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping.
Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang.
This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls).
This fixes Doom II from Steam. I also did notice that Sleep on Windows use to work with 15.6ms quantum some time ago. But the important part which is missed here is that Sleep behaviour is affected (at least) by timeBeginPeriod() / timeEndPeriod() winmm functions (as I recalled now after rerunning my old test program and wondering why I see 1ms sleep quantum). E. g., if I call timeBeingPeriod(1) before testing Sleep actual time and GetTickCount change, the Sleep starts to behave very similar to how it works now in Wine: sleeps with 1ms quantum and GetTickCount() is not necessarily updated on wake. I think we can't ignore that behaviour, I did see the games playing with winmm functions, Sleep etc.
I was also initially going to say that maybe we would need to explicitly check if GetTickCount has changed during sleep, but the failing test on Win10 suggests that maybe Windows doesn't guarantee that also, just sleeps with 15.6ms quantum by default (unless changed by winmm).
PS If timeBeginPeriod() indeed sets the global time resolution for all processes as the docs suggest, I can guess there should be a field in KSHARED_USER_DATA which holds that resolution (there are a few related to timers, I did not ever test what exactly do they hold), and we should probably use those to store and retrieve timer resolution. Indeed timeBeginPeriod() seems to affect the resolution of Sleep()
https://hiler.eu/p/0701eab8d523.txt https://testbot.winehq.org/JobDetails.pl?Key=76759&f109=exe64.report#k109
This is completely unexpected... Thanks for pointing this out!
I guess I have to investigate whether it's truly global and if there is a field for that in KSHARED_USER_DATA.
That would be a bit funny - anything that depends on the default behavior will be broken while that process is running, and if it crashes/forgets to call timeEndPeriod() upon exit we are left in that broken state forever :-)
Please mind also NtSetTimerResolution() / NtQueryTimerResolution() functions (which is currently a stub in Wine). This all needs testing, but it would looks natural if winmm would actually call that one. All that probably means that it is NtDelayExecution() which needs to be fixed to favour current timer resolution (besides a load of functions related to various timers; but maybe those can be treated separately from these changes?). Unfortunately that makes things more complicated because NtDelayExecution is widely used by Wine itself and those places might rely on a (relatively) precise delays not to introduce great performance loss, so maybe those usages needs to be checked before changing that.
On Mon, Aug 10, 2020 at 03:02:32PM +0300, Paul Gofman wrote:
On 8/10/20 14:55, Arkadiusz Hiler wrote:
On Mon, Aug 10, 2020 at 02:45:49PM +0300, Paul Gofman wrote:
On 8/10/20 14:32, Paul Gofman wrote:
On 8/10/20 13:55, Arkadiusz Hiler wrote:
Sleep() and GetTickCount() work on Windows in 15.6ms increments.
Some programs (DOSBox) are depending on this behavior and are assuming that the return value of GetTickCount() will change during sleeping.
Currently we are updating shared counters used by GetTickCount() every 16ms + on each server request, and our Sleep() implementation has resolution of 1ms, which causes DOSBox to hang.
This patch changes Sleep() (and SleepEx()) to behave the same way as on Windows and makes sure that GetTickCount() will be updated during sleeping by decreasing the update interval to 15ms (worst case, without any server calls).
This fixes Doom II from Steam. I also did notice that Sleep on Windows use to work with 15.6ms quantum some time ago. But the important part which is missed here is that Sleep behaviour is affected (at least) by timeBeginPeriod() / timeEndPeriod() winmm functions (as I recalled now after rerunning my old test program and wondering why I see 1ms sleep quantum). E. g., if I call timeBeingPeriod(1) before testing Sleep actual time and GetTickCount change, the Sleep starts to behave very similar to how it works now in Wine: sleeps with 1ms quantum and GetTickCount() is not necessarily updated on wake. I think we can't ignore that behaviour, I did see the games playing with winmm functions, Sleep etc.
I was also initially going to say that maybe we would need to explicitly check if GetTickCount has changed during sleep, but the failing test on Win10 suggests that maybe Windows doesn't guarantee that also, just sleeps with 15.6ms quantum by default (unless changed by winmm).
PS If timeBeginPeriod() indeed sets the global time resolution for all processes as the docs suggest, I can guess there should be a field in KSHARED_USER_DATA which holds that resolution (there are a few related to timers, I did not ever test what exactly do they hold), and we should probably use those to store and retrieve timer resolution. Indeed timeBeginPeriod() seems to affect the resolution of Sleep()
https://hiler.eu/p/0701eab8d523.txt https://testbot.winehq.org/JobDetails.pl?Key=76759&f109=exe64.report#k109
This is completely unexpected... Thanks for pointing this out!
I guess I have to investigate whether it's truly global and if there is a field for that in KSHARED_USER_DATA.
That would be a bit funny - anything that depends on the default behavior will be broken while that process is running, and if it crashes/forgets to call timeEndPeriod() upon exit we are left in that broken state forever :-)
Please mind also NtSetTimerResolution() / NtQueryTimerResolution() functions (which is currently a stub in Wine). This all needs testing, but it would looks natural if winmm would actually call that one. All that probably means that it is NtDelayExecution() which needs to be fixed to favour current timer resolution (besides a load of functions related to various timers; but maybe those can be treated separately from these changes?). Unfortunately that makes things more complicated because NtDelayExecution is widely used by Wine itself and those places might rely on a (relatively) precise delays not to introduce great performance loss, so maybe those usages needs to be checked before changing that.
Lazy/temporary solution would to to bring back the old behavior of GetTickCount() - the 1ms resolution. This can be easily done by calling NtQueryPerformanceTimer() as it still uses monotic_counter() through unixlib. Most things seemed to be fine with this approach, not sure what would be the performance hit here though. Also any programs reading counters directly from KSHARED_USER_DATA may still be broken. I think I'll start exploring timeBeginPeriod() and NtSetTimerResolution() and implement the changes limited to Sleep*() for now. Then, if this is fine with the reviewers we can see how hard it would be to extend this behavior over to NtDelayExecution().
participants (3)
-
Arkadiusz Hiler -
Marvin -
Paul Gofman