[PATCH v8 0/2] MR7392: server: Don't ceil poll timeouts to the next millisecond if higher resolution timeouts are 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. -- v8: server: Use epoll_pwait2 for the main loop on Linux. server: Use a high precision timespec directly for poll timeouts on supported platforms. 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 | 65 ++++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/server/fd.c b/server/fd.c index ce32e7f8397..9e28c1b9fcf 100644 --- a/server/fd.c +++ b/server/fd.c @@ -496,7 +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 int get_next_timeout(void); +static int get_next_timeout( struct timespec *ts ); static inline void fd_poll_event( struct fd *fd, int event ) { @@ -575,7 +575,7 @@ static inline void main_loop_epoll(void) while (active_users) { - timeout = get_next_timeout(); + timeout = get_next_timeout( NULL ); if (!active_users) break; /* last user removed by a timeout */ if (epoll_fd == -1) break; /* an error occurred with epoll */ @@ -663,26 +663,19 @@ static inline void remove_epoll_user( struct fd *fd, int user ) static inline void main_loop_epoll(void) { int i, ret, timeout; + struct timespec ts; struct kevent events[128]; if (kqueue_fd == -1) return; while (active_users) { - timeout = get_next_timeout(); + timeout = 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 ), timeout == -1 ? NULL : &ts ); set_current_time(); @@ -765,27 +758,20 @@ static inline void remove_epoll_user( struct fd *fd, int user ) static inline void main_loop_epoll(void) { int i, nget, ret, timeout; + struct timespec ts; port_event_t events[128]; if (port_fd == -1) return; while (active_users) { - timeout = get_next_timeout(); + timeout = 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, timeout == -1 ? NULL : &ts ); if (ret == -1) break; /* an error occurred with event completion */ @@ -876,10 +862,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 milliseconds, + * and full nanosecond precision in the timespec parameter if given */ +static int get_next_timeout( struct timespec *ts ) { - int ret = user_shared_data ? user_shared_data_timeout : -1; + timeout_t ret = user_shared_data ? (timeout_t)user_shared_data_timeout * 10000 : -1; if (!list_empty( &abs_timeout_list ) || !list_empty( &rel_timeout_list )) { @@ -924,22 +911,34 @@ 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; + timeout_t diff = timeout->when - current_time; + if (diff < 0) diff = 0; if (ret == -1 || diff < ret) ret = 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; + timeout_t diff = -timeout->when - monotonic_time; + if (diff < 0) diff = 0; if (ret == -1 || diff < ret) ret = diff; } } - return ret; + + /* infinite */ + if (ret == -1) return -1; + + if (ts) + { + ts->tv_sec = ret / TICKS_PER_SEC; + ts->tv_nsec = (ret % TICKS_PER_SEC) * 100; + } + + /* convert to milliseconds, ceil to avoid spinning with 0 timeout */ + ret = (ret + 9999) / 10000; + if (ret > INT_MAX) ret = INT_MAX; + + return (int)ret; } /* server main poll() loop */ @@ -955,7 +954,7 @@ void main_loop(void) while (active_users) { - timeout = get_next_timeout(); + timeout = get_next_timeout( NULL ); if (!active_users) break; /* last user removed by a timeout */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
From: William Horvath <william(a)horvath.blog> This allows higher precision waits, 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 | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 20c2cceadee..18aa26e9e92 100644 --- a/configure.ac +++ b/configure.ac @@ -2062,6 +2062,7 @@ AC_CHECK_FUNCS(\ dladdr1 \ dlinfo \ epoll_create \ + epoll_pwait2 \ fstatfs \ futimens \ futimes \ diff --git a/server/fd.c b/server/fd.c index 9e28c1b9fcf..3fd7f459dc2 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) @@ -564,6 +567,7 @@ static inline void remove_epoll_user( struct fd *fd, int user ) static inline void main_loop_epoll(void) { int i, ret, timeout; + struct timespec ts; struct epoll_event events[128]; assert( POLLIN == EPOLLIN ); @@ -575,12 +579,17 @@ static inline void main_loop_epoll(void) while (active_users) { - timeout = get_next_timeout( NULL ); + timeout = get_next_timeout( &ts ); if (!active_users) break; /* last user removed by a timeout */ if (epoll_fd == -1) break; /* an error occurred with epoll */ +#ifdef USE_EPOLL_PWAIT2 + ret = epoll_pwait2( epoll_fd, events, ARRAY_SIZE( events ), timeout == -1 ? NULL : &ts, NULL ); +#else ret = epoll_wait( epoll_fd, events, ARRAY_SIZE( events ), timeout ); +#endif + set_current_time(); /* put the events into the pollfd array first, like poll does */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
v7: - Clarify a comment and move conditional operator out of return. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95921
Suggestions for alternative solutions are appreciated if this is fundamentally the wrong approach. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_95953
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392
For additional clarification, the issue is that because of the ceiling, every time we check for elapsed timeouts, the ones that are not yet elapsed are going to be checked again at best 1ms after the current check. The more often we check (ie: the more requests there is), the more likely it gets that we wake before timeout elapsed and schedule the next check 1ms away from current time. I suspect this effect should start reversing as soon as there's more than 1000 requests/s, as the request wake ups would then act as a higher resolution timer and we would check for elapsed timers earlier, but it's not really what we want anyway. The ceil is there since 4d1d49b78a731abe46986cfae0083154d3c578b8, and I understand it's only there to avoid getting 0 timeouts / waking up before the timeout are actually elapsed, and spinning unnecessarily. This MR solves this by using higher resolution epoll when available. It doesn't seem to be related to timer resolution in any way, and if there's a resolution we need to implement I think it's probably something orthogonal that should be implemented in the timeout elapse time computation, not something like the current unpredictable and dynamically changing resolution. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7392#note_96097
participants (3)
-
Rémi Bernon -
William Horvath
-
William Horvath (＠whrvt)