[PATCH v5 0/1] MR7642: server: Fall back to epoll_wait if epoll_pwait2 isn't available at run-time.
This fixes a real problem and improves behavior at the same time. - Fixes: hanging when trying to run Wine built with epoll_pwait2 headers available but epoll_pwait2 is missing in the run-time kernel (i.e. build 5.11->run 5.10) - Improvement: bring the improved timeout resolution to Wine builds which didn't have epoll_pwait2 at compile time, if the run-time kernel supports it (i.e. build 5.10->run 6.14) This last point is especially important in my opinion, as it applies to official Proton builds. Proton is (currently) built in Debian 11 with kernel 5.10, but SteamOS is a moving target with a much newer kernel version being used to run these Wine builds. These builds use the lower resolution timeouts if epoll_pwait2 is only checked for at compile-time, when it can cheaply and easily be done at run-time instead. Fixes: 87ca5db40e2c1b37423bdc25101a5c5e39e67e6f An alternative to !7640 that kills two birds with one stone. -- v5: server: Use dlsym() to check for epoll_pwait2 at run-time. https://gitlab.winehq.org/wine/wine/-/merge_requests/7642
From: William Horvath <william(a)horvath.blog> This brings the improved timeout resolution to Wine builds which didn't have epoll_pwait2 at compile time, if the run-time kernel supports it (i.e. build 5.10->run 6.14). --- configure.ac | 1 - server/fd.c | 31 ++++++++++++++++++------------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/configure.ac b/configure.ac index 9e32070f610..05157e80d55 100644 --- a/configure.ac +++ b/configure.ac @@ -2072,7 +2072,6 @@ AC_CHECK_FUNCS(\ dladdr1 \ dlinfo \ epoll_create \ - epoll_pwait2 \ fstatfs \ futimens \ futimes \ diff --git a/server/fd.c b/server/fd.c index bb0e90d99d0..640f9e2a79d 100644 --- a/server/fd.c +++ b/server/fd.c @@ -100,6 +100,7 @@ #include "ddk/wdm.h" #if defined(HAVE_SYS_EPOLL_H) && defined(HAVE_EPOLL_CREATE) +# include <dlfcn.h> # include <sys/epoll.h> # define USE_EPOLL #endif /* HAVE_SYS_EPOLL_H && HAVE_EPOLL_CREATE */ @@ -561,14 +562,25 @@ static inline void remove_epoll_user( struct fd *fd, int user ) } } +static int (*pepoll_pwait2)( int, struct epoll_event *, int, const struct timespec *, const sigset_t * ); + +/* dispatch to the highest resolution epoll available */ +static inline int do_epoll_wait( struct epoll_event *events, int maxevents, const struct timespec *timeout_ts, int timeout_ms ) +{ + if (pepoll_pwait2) + { + int ret = pepoll_pwait2( epoll_fd, events, maxevents, timeout_ts, NULL ); + if (ret == -1 && errno == ENOSYS) pepoll_pwait2 = NULL; /* broken, fall back to epoll_wait */ + else return ret; + } + return epoll_wait( epoll_fd, events, maxevents, timeout_ms ); +} + static inline void main_loop_epoll(void) { int i, ret, timeout; struct timespec ts; struct epoll_event events[128]; -#ifdef HAVE_EPOLL_PWAIT2 - static int failed_epoll_pwait2 = 0; -#endif assert( POLLIN == EPOLLIN ); assert( POLLOUT == EPOLLOUT ); @@ -577,6 +589,8 @@ static inline void main_loop_epoll(void) if (epoll_fd == -1) return; + pepoll_pwait2 = dlsym( RTLD_DEFAULT, "epoll_pwait2" ); + while (active_users) { timeout = get_next_timeout( &ts ); @@ -584,16 +598,7 @@ static inline void main_loop_epoll(void) if (!active_users) break; /* last user removed by a timeout */ if (epoll_fd == -1) break; /* an error occurred with epoll */ -#ifdef HAVE_EPOLL_PWAIT2 - if (!failed_epoll_pwait2) - { - ret = epoll_pwait2( epoll_fd, events, ARRAY_SIZE( events ), timeout == -1 ? NULL : &ts, NULL ); - if (ret == -1 && errno == ENOSYS) - failed_epoll_pwait2 = 1; - } - if (failed_epoll_pwait2) -#endif - ret = epoll_wait( epoll_fd, events, ARRAY_SIZE( events ), timeout ); + ret = do_epoll_wait( events, ARRAY_SIZE( events ), timeout == -1 ? NULL : &ts, timeout ); set_current_time(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7642
Rebased and reworded. I think this is still useful even with the hanging already fixed by @Alcaro's patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7642#note_98959
dlsym(2) doesn't work for this purpose. On older kernels, the function *will* exist (when bundled with newer glibc) but calling it will return errno=ENOSYS. Please use ENOSYS instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7642#note_98965
On Mon Mar 24 23:15:36 2025 +0000, Jinoh Kang wrote:
dlsym(2) doesn't work for this purpose. On older kernels, the function *will* exist (when bundled with newer glibc) but calling it will return errno=ENOSYS. Please use ENOSYS instead. Are you sure you looked at the patch fully? I don't see how I'm doing anything opposed to what you're saying.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7642#note_98966
On Mon Mar 24 23:15:36 2025 +0000, William Horvath wrote:
Are you sure you looked at the patch fully? I don't see how I'm doing anything opposed to what you're saying. One could argue the ENOSYS check should be right beside the dlsym call, not beside the regular invocation. That change would remove a branch from the loop, and make it a little more clear what the code is doing; but it will add a syscall to the setup step, and make it more code in total.
I have no opinion on which is better. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7642#note_98969
On Mon Mar 24 23:18:57 2025 +0000, Alfred Agrell wrote:
One could argue the ENOSYS check should be right beside the dlsym call, not beside the regular invocation. That change would remove a branch from the loop, and make it a little more clear what the code is doing; but it will add a syscall to the setup step, and make it more code in total. I have no opinion on which is better. Okay, so it aims for forward compatibility while compiling with older arches.
I'm not sure it's convincing still, the way we usually do it is to just use syscall(2) and account for arch differences. (No, we don't support x32 ABI.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7642#note_98971
participants (4)
-
Alfred Agrell (＠Alcaro) -
Jinoh Kang (＠iamahuman)
-
William Horvath
-
William Horvath (＠whrvt)