[PATCH v3 0/2] MR7392: server: Add an epoll_pwait2 path for server_main_loop if supported.
This mitigates a side effect of the global current_time and monotonic_time being updated on every server call's timeout, where the end time of any unrelated server call is moved into the future (depending on the frequency of server calls). By using a more granular timeout, the overall frequency of server calls doesn't have as great of an effect on each individual timeout, as we don't have to wait for an entire millisecond (which was due to the ceiling operation in get_next_timeout). This [issue](https://bugs.winehq.org/show_bug.cgi?id=57849) gives a few examples of this causing significant degradation in the behavior of some games, which use alertable (server) waits for their FPS limiter. -- v3: server: Use epoll_pwait2 if supported. server: Convert to and use a timespec directly for timeouts if supported. https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
From: William Horvath <william(a)horvath.blog> Instead of first converting to milliseconds, then back to a nanosecond-precision timespec. This mitigates a side effect of the global current_time and monotonic_time being updated on every server call's timeout, where the end time of any unrelated server call is moved into the future (depending on the frequency of server calls). By using a more granular timeout, the overall frequency of server calls doesn't have as great of an effect on each individual timeout, as we don't have to wait for an entire millisecond (which was due to the ceiling operation in get_next_timeout). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57849 --- server/fd.c | 72 ++++++++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/server/fd.c b/server/fd.c index ce32e7f8397..05df1215f7e 100644 --- a/server/fd.c +++ b/server/fd.c @@ -496,6 +496,7 @@ static int active_users; /* current number of active users */ static int allocated_users; /* count of allocated entries in the array */ static struct fd **freelist; /* list of free entries in the array */ +static struct timespec *get_next_timeout_ts(void); static int get_next_timeout(void); static inline void fd_poll_event( struct fd *fd, int event ) @@ -662,27 +663,20 @@ static inline void remove_epoll_user( struct fd *fd, int user ) static inline void main_loop_epoll(void) { - int i, ret, timeout; + int i, ret; + struct timespec *ts; struct kevent events[128]; if (kqueue_fd == -1) return; while (active_users) { - timeout = get_next_timeout(); + ts = get_next_timeout_ts(); if (!active_users) break; /* last user removed by a timeout */ if (kqueue_fd == -1) break; /* an error occurred with kqueue */ - if (timeout != -1) - { - struct timespec ts; - - ts.tv_sec = timeout / 1000; - ts.tv_nsec = (timeout % 1000) * 1000000; - ret = kevent( kqueue_fd, NULL, 0, events, ARRAY_SIZE( events ), &ts ); - } - else ret = kevent( kqueue_fd, NULL, 0, events, ARRAY_SIZE( events ), NULL ); + ret = kevent( kqueue_fd, NULL, 0, events, ARRAY_SIZE( events ), ts ); set_current_time(); @@ -764,28 +758,21 @@ static inline void remove_epoll_user( struct fd *fd, int user ) static inline void main_loop_epoll(void) { - int i, nget, ret, timeout; + int i, nget, ret; + struct timespec *ts; port_event_t events[128]; if (port_fd == -1) return; while (active_users) { - timeout = get_next_timeout(); + ts = get_next_timeout_ts(); nget = 1; if (!active_users) break; /* last user removed by a timeout */ if (port_fd == -1) break; /* an error occurred with event completion */ - if (timeout != -1) - { - struct timespec ts; - - ts.tv_sec = timeout / 1000; - ts.tv_nsec = (timeout % 1000) * 1000000; - ret = port_getn( port_fd, events, ARRAY_SIZE( events ), &nget, &ts ); - } - else ret = port_getn( port_fd, events, ARRAY_SIZE( events ), &nget, NULL ); + ret = port_getn( port_fd, events, ARRAY_SIZE( events ), &nget, ts ); if (ret == -1) break; /* an error occurred with event completion */ @@ -876,10 +863,11 @@ static void remove_poll_user( struct fd *fd, int user ) active_users--; } -/* process pending timeouts and return the time until the next timeout, in milliseconds */ -static int get_next_timeout(void) +/* process pending timeouts and return the time until the next timeout, in a struct timespec (nanoseconds) */ +static struct timespec *get_next_timeout_ts(void) { - int ret = user_shared_data ? user_shared_data_timeout : -1; + static struct timespec ts; + timeout_t next_timeout = user_shared_data ? user_shared_data_timeout : -1; if (!list_empty( &abs_timeout_list ) || !list_empty( &rel_timeout_list )) { @@ -924,22 +912,38 @@ static int get_next_timeout(void) if ((ptr = list_head( &abs_timeout_list )) != NULL) { struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry ); - timeout_t diff = (timeout->when - current_time + 9999) / 10000; - if (diff > INT_MAX) diff = INT_MAX; - else if (diff < 0) diff = 0; - if (ret == -1 || diff < ret) ret = diff; + timeout_t diff = timeout->when - current_time; + if (diff < 0) diff = 0; + if (next_timeout == -1 || diff < next_timeout) next_timeout = diff; } if ((ptr = list_head( &rel_timeout_list )) != NULL) { struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry ); - timeout_t diff = (-timeout->when - monotonic_time + 9999) / 10000; - if (diff > INT_MAX) diff = INT_MAX; - else if (diff < 0) diff = 0; - if (ret == -1 || diff < ret) ret = diff; + timeout_t diff = -timeout->when - monotonic_time; + if (diff < 0) diff = 0; + if (next_timeout == -1 || diff < next_timeout) next_timeout = diff; } } - return ret; + + /* infinite */ + if (next_timeout == -1) return NULL; + + ts.tv_sec = next_timeout / 10000000; + ts.tv_nsec = (next_timeout % 10000000) * 100; + return &ts; +} + +/* process pending timeouts and return the time until the next timeout, in milliseconds */ +static int get_next_timeout(void) +{ + struct timespec *ts = get_next_timeout_ts(); + + if (!ts) return -1; + if (ts->tv_sec >= INT_MAX / 1000) return INT_MAX; + + /* ceil to avoid spinning */ + return ts->tv_sec * 1000 + (ts->tv_nsec + 999999) / 1000000; } /* server main poll() loop */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
From: William Horvath <william(a)horvath.blog> This allows higher precision waits on Linux, to match the other platforms where we can already use a timespec directly. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57849 --- configure.ac | 1 + server/fd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/configure.ac b/configure.ac index dd139950e10..d483096fae6 100644 --- a/configure.ac +++ b/configure.ac @@ -2061,6 +2061,7 @@ AC_CHECK_FUNCS(\ dladdr1 \ dlinfo \ epoll_create \ + epoll_pwait2 \ fstatfs \ futimens \ futimes \ diff --git a/server/fd.c b/server/fd.c index 05df1215f7e..b4d44d3f7f1 100644 --- a/server/fd.c +++ b/server/fd.c @@ -102,6 +102,9 @@ #if defined(HAVE_SYS_EPOLL_H) && defined(HAVE_EPOLL_CREATE) # include <sys/epoll.h> # define USE_EPOLL +# ifdef HAVE_EPOLL_PWAIT2 +# define USE_EPOLL_PWAIT2 +# endif #endif /* HAVE_SYS_EPOLL_H && HAVE_EPOLL_CREATE */ #if defined(HAVE_PORT_H) && defined(HAVE_PORT_CREATE) @@ -562,6 +565,50 @@ static inline void remove_epoll_user( struct fd *fd, int user ) } } +#ifdef USE_EPOLL_PWAIT2 + +static inline void main_loop_epoll(void) +{ + int i, ret; + struct timespec *ts; + struct epoll_event events[128]; + + assert( POLLIN == EPOLLIN ); + assert( POLLOUT == EPOLLOUT ); + assert( POLLERR == EPOLLERR ); + assert( POLLHUP == EPOLLHUP ); + + if (epoll_fd == -1) return; + + while (active_users) + { + ts = get_next_timeout_ts(); + + if (!active_users) break; /* last user removed by a timeout */ + if (epoll_fd == -1) break; /* an error occurred with epoll */ + + ret = epoll_pwait2( epoll_fd, events, ARRAY_SIZE( events ), ts, NULL ); + + set_current_time(); + + /* put the events into the pollfd array first, like poll does */ + for (i = 0; i < ret; i++) + { + int user = events[i].data.u32; + pollfd[user].revents = events[i].events; + } + + /* read events from the pollfd array, as set_fd_events may modify them */ + for (i = 0; i < ret; i++) + { + int user = events[i].data.u32; + if (pollfd[user].revents) fd_poll_event( poll_users[user], pollfd[user].revents ); + } + } +} + +#else + static inline void main_loop_epoll(void) { int i, ret, timeout; @@ -600,6 +647,8 @@ static inline void main_loop_epoll(void) } } +#endif /* USE_EPOLL_PWAIT2 */ + #elif defined(HAVE_KQUEUE) static int kqueue_fd = -1; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
On Thu Feb 20 21:43:29 2025 +0000, William Horvath wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/7392/diffs?diff_id=159231&start_sha=3a7f17d00c1ce2e53fdb9458ee4fdf01eddbeb42#21721cceaf2ea82f93539de2c6e01cc016bbac65_823_818) Ah, I tried this before but somehow missed the fact that the `timespec` struct needs to be `static`. That looks much cleaner now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95434
v2: return a `struct timespec*` directly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95435
On Thu Feb 20 21:41:59 2025 +0000, William Horvath wrote:
Well, for one, I'd like to see an example of a real game that doesn't call timeBeginPeriod(1), but that's besides the point. We already simulate a 1ms timer period by having the tick count increase every millisecond, and this is absolutely not intended to change that. What this is addressing is unrelated server calls from other threads pushing the **global** `current_time` and `monotonic_time` (updated each timeout) forward. That causes the timeouts for alertable `SleepEx` (used in those games' frame limiters) to expire for up to 1ms from that last update, as it's ceil'd to the next millisecond to avoid spinning with a 0 timeout. This causes, on average, an additional 0.5ms waited, depending on where we land on the tick boundary. Sorry, I still don't understand what's going on with the game. timeBeginPeriod() can only set 1ms resolution as best (does game do that?), and thus SleepEx() will go with 1ms sleep granularity, as I understand from your message, that's also the case on Wine? If the change helps the game but it is not what happens on Windows the actual problem might be elsewhere.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95436
On Thu Feb 20 21:43:51 2025 +0000, William Horvath wrote:
Ah, I tried this before but somehow missed the fact that the `timespec` struct needs to be `static`. That looks much cleaner now. Ah no, that's IMO a bit ugly. I meant rather something like that:
``` static struct timespec *get_next_timeout_ts( struct timespec *ts ) { /* ... */ if (next_timeout == -1) return NULL; return ts; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95437
On Thu Feb 20 21:47:59 2025 +0000, Paul Gofman wrote:
Sorry, I still don't understand what's going on with the game. timeBeginPeriod() can only set 1ms resolution as best (does game do that?), and thus SleepEx() will go with 1ms sleep granularity, as I understand from your message, that's also the case on Wine? If the change helps the game but it is not what happens on Windows the actual problem might be elsewhere. Due to the `timeout_t diff = (-timeout->when - monotonic_time + 9999) / 10000;` in `get_next_timeout()`, the `SleepEx(1, TRUE)` calls can last longer due to this `monotonic_time` variable being updated by other threads' server calls, which puts it into the future by up to 1ms. That makes the sleep calls last "unexpectedly" longer depending on how fast you move the mouse, and not just because of increased load.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95438
On Thu Feb 20 21:50:43 2025 +0000, Rémi Bernon wrote:
Ah no, that's IMO a bit ugly. I meant rather something like that: ``` static struct timespec *get_next_timeout_ts( struct timespec *ts ) { /* ... */ if (next_timeout == -1) return NULL; ts->tv_sec = next_timeout / 10000000; ts->tv_nsec = (next_timeout % 10000000) * 100; return ts; } ``` I don't have enough experience to prefer either, but this looks good too, so I will change that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95439
On Thu Feb 20 21:52:18 2025 +0000, William Horvath wrote:
Due to the `timeout_t diff = (-timeout->when - monotonic_time + 9999) / 10000;` in `get_next_timeout()`, the `SleepEx(1, TRUE)` calls can last longer due to this `monotonic_time` variable being updated by other threads' server calls, which puts it into the future by up to 1ms. That makes the sleep calls last "unexpectedly" longer depending on how fast you move the mouse, and not just because of increased load. So it is making Sleep() not adding up to extra 1ms but it can sleep, e. g., 10ms instead?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95441
On Thu Feb 20 22:02:44 2025 +0000, Paul Gofman wrote:
So it is making Sleep() not adding up to extra 1ms but it can sleep, e. g., 10ms instead? If not, that it is suspicious that additional 0.5ms waited has such an effect. That can probably be checked on Windows by making a background program which does timeBeginPeriod(16) periodically (so it sleeps much longer), does it have the same effect on Windows? And does the game actually sets the timer resolution?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95448
On Thu Feb 20 22:06:55 2025 +0000, Paul Gofman wrote:
If not, that it is suspicious that additional 0.5ms waited has such an effect. That can probably be checked on Windows by making a background program which does timeBeginPeriod(16) periodically (so it sleeps much longer), does it have the same effect on Windows? And does the game actually sets the timer resolution? Alertable sleeps can last an extra 1ms longer than desired, with an average of 0.5ms, due to the timeout's ceiling acting on a `monotonic_time` or `current_time` variable that is being updated on each server call.
This doesn't happen on Windows. I'll attach a test program here to showcase the issue, notice the aliasing for the error values that happens on Wine. [timeout-test-sleep.c](/uploads/83edfb9bcd0249646c00eb24d76091ff/timeout-test-sleep.c) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95468
participants (4)
-
Paul Gofman (＠gofman) -
Rémi Bernon
-
William Horvath
-
William Horvath (＠whrvt)