Hi all, Attached is a non-perfect patch for review. This is a migration of the wineserver to use epoll instead of poll (if it's available). current known issue with this patch: 1. Will not compile if HAVE_SYS_EPOLL_H is not 1 (i.e. - won't compile if epoll not available at compile time) 2. Segfaults on wine exit. 3. Lots of debug asserts. Comments welcome. Shachar -- Shachar Shemesh Lingnu Open Source Consulting ltd. http://www.lingnu.com/ Index: configure =================================================================== RCS file: /home/sun/sources/cvs/wine/configure,v retrieving revision 1.573 diff -u -r1.573 configure --- configure 17 Jul 2004 00:52:37 -0000 1.573 +++ configure 24 Aug 2004 12:03:32 -0000 @@ -6727,6 +6727,79 @@ fi +echo "$as_me:$LINENO: checking for epoll_create in -lepoll" >&5 +echo $ECHO_N "checking for epoll_create in -lepoll... $ECHO_C" >&6 +if test "${ac_cv_lib_epoll_epoll_create+set}" = set; then + echo $ECHO_N "(cached) $ECHO_C" >&6 +else + ac_check_lib_save_LIBS=$LIBS +LIBS="-lepoll $LIBS" +cat >conftest.$ac_ext <<_ACEOF +/* confdefs.h. */ +_ACEOF +cat confdefs.h >>conftest.$ac_ext +cat >>conftest.$ac_ext <<_ACEOF +/* end confdefs.h. */ + +/* Override any gcc2 internal prototype to avoid an error. */ +#ifdef __cplusplus +extern "C" +#endif +/* We use char because int might match the return type of a gcc2 + builtin and then its argument prototype would still apply. */ +char epoll_create (); +int +main () +{ +epoll_create (); + ; + return 0; +} +_ACEOF +rm -f conftest.$ac_objext conftest$ac_exeext +if { (eval echo "$as_me:$LINENO: \"$ac_link\"") >&5 + (eval $ac_link) 2>conftest.er1 + ac_status=$? + grep -v '^ *+' conftest.er1 >conftest.err + rm -f conftest.er1 + cat conftest.err >&5 + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); } && + { ac_try='test -z "$ac_c_werror_flag" || test ! -s conftest.err' + { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5 + (eval $ac_try) 2>&5 + ac_status=$? + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); }; } && + { ac_try='test -s conftest$ac_exeext' + { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5 + (eval $ac_try) 2>&5 + ac_status=$? + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); }; }; then + ac_cv_lib_epoll_epoll_create=yes +else + echo "$as_me: failed program was:" >&5 +sed 's/^/| /' conftest.$ac_ext >&5 + +ac_cv_lib_epoll_epoll_create=no +fi +rm -f conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS +fi +echo "$as_me:$LINENO: result: $ac_cv_lib_epoll_epoll_create" >&5 +echo "${ECHO_T}$ac_cv_lib_epoll_epoll_create" >&6 +if test $ac_cv_lib_epoll_epoll_create = yes; then + cat >>confdefs.h <<_ACEOF +#define HAVE_LIBEPOLL 1 +_ACEOF + + LIBS="-lepoll $LIBS" + +fi + + echo "$as_me:$LINENO: checking for res_9_init in -lresolv" >&5 echo $ECHO_N "checking for res_9_init in -lresolv... $ECHO_C" >&6 if test "${ac_cv_lib_resolv_res_9_init+set}" = set; then @@ -16190,6 +16263,7 @@ + for ac_func in \ _lwp_create \ _lwp_self \ @@ -16202,6 +16276,7 @@ _vsnprintf \ chsize \ clone \ + epoll_create \ finite \ fpclass \ fstatfs \ @@ -16435,6 +16510,7 @@ + for ac_header in \ arpa/inet.h \ arpa/nameser.h \ @@ -16495,6 +16571,7 @@ sys/msg.h \ sys/param.h \ sys/poll.h \ + sys/epoll.h \ sys/ptrace.h \ sys/reg.h \ sys/scsiio.h \ Index: configure.ac =================================================================== RCS file: /home/sun/sources/cvs/wine/configure.ac,v retrieving revision 1.285 diff -u -r1.285 configure.ac --- configure.ac 6 Jul 2004 21:01:19 -0000 1.285 +++ configure.ac 24 Aug 2004 12:01:22 -0000 @@ -140,6 +140,8 @@ AC_CHECK_LIB(xpg4,_xpg4_setrunelocale) dnl Check for -lpoll for Mac OS X/Darwin AC_CHECK_LIB(poll,poll) +dnl Check for -lepoll +AC_CHECK_LIB(epoll,epoll_create) dnl Check for -lresolv for Mac OS X/Darwin AC_CHECK_LIB(resolv,res_9_init) dnl Check for -lpthread @@ -1036,6 +1038,7 @@ _vsnprintf \ chsize \ clone \ + epoll_create \ finite \ fpclass \ fstatfs \ @@ -1146,6 +1149,7 @@ sys/msg.h \ sys/param.h \ sys/poll.h \ + sys/epoll.h \ sys/ptrace.h \ sys/reg.h \ sys/scsiio.h \ Index: include/config.h.in =================================================================== RCS file: /home/sun/sources/cvs/wine/include/config.h.in,v retrieving revision 1.190 diff -u -r1.190 config.h.in --- include/config.h.in 18 Jun 2004 19:36:26 -0000 1.190 +++ include/config.h.in 24 Aug 2004 12:05:42 -0000 @@ -80,6 +80,9 @@ /* Define to 1 if you have the <elf.h> header file. */ #undef HAVE_ELF_H +/* Define to 1 if you have the `epoll_create' function. */ +#undef HAVE_EPOLL_CREATE + /* Define to 1 if you have the `finite' function. */ #undef HAVE_FINITE @@ -248,6 +251,9 @@ /* Define if you have the curses library (-lcurses) */ #undef HAVE_LIBCURSES +/* Define to 1 if you have the `epoll' library (-lepoll). */ +#undef HAVE_LIBEPOLL + /* Define if you have libgif/libungif including devel headers */ #undef HAVE_LIBGIF @@ -608,6 +614,9 @@ /* Define to 1 if you have the <sys/elf32.h> header file. */ #undef HAVE_SYS_ELF32_H +/* Define to 1 if you have the <sys/epoll.h> header file. */ +#undef HAVE_SYS_EPOLL_H + /* Define to 1 if you have the <sys/errno.h> header file. */ #undef HAVE_SYS_ERRNO_H Index: server/fd.c =================================================================== RCS file: /home/sun/sources/cvs/wine/server/fd.c,v retrieving revision 1.21 diff -u -r1.21 fd.c --- server/fd.c 22 May 2004 03:15:04 -0000 1.21 +++ server/fd.c 24 Aug 2004 21:20:53 -0000 @@ -33,6 +33,12 @@ #ifdef HAVE_SYS_POLL_H #include <sys/poll.h> #endif +#ifdef HAVE_SYS_EPOLL_H +#include <sys/epoll.h> +#endif +#ifdef HAVE_SYS_SYSCALL_H +#include <sys/syscall.h> +#endif #include <sys/stat.h> #include <sys/time.h> #include <sys/types.h> @@ -48,6 +54,9 @@ #include "winreg.h" #include "winternl.h" +#undef NDEBUG +#include <assert.h> + /* Because of the stupid Posix locking semantics, we need to keep * track of all file descriptors referencing a given file, and not * close a single one until all the locks are gone (sigh). @@ -255,51 +264,107 @@ /****************************************************************/ -/* poll support */ +/* poll and epoll support */ + +/* Vars used by both */ +static int active_users; /* current number of active users */ +/* Vars used by poll */ static struct fd **poll_users; /* users array */ static struct pollfd *pollfd; /* poll fd array */ static int nb_users; /* count of array entries actually in use */ -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 */ +/* Vars used by epoll */ +static int epoll_fd=-1; /* File descriptor for the epoll */ +static const int EPOLL_SIZE=1024; /* "Maximal size" passed to epoll_create. This is an optimization, not a real maximum */ +static struct epoll_event *epoll_revents; /* Array for event results */ +static int epoll_revents_size; /* Number of elements in epoll_revents */ + +#if defined(HAVE_EPOLL_CREATE) && defined(HAVE_SYS_SYSCALL_H) +/* To prevent wine from not loading on non-epoll supporting platforms, + * we need to dynamically call the epoll functions */ +static inline int _epoll_create( int size ) +{ + return syscall(__NR_epoll_create, size); +} + +static inline int _epoll_ctl(int epfd, int op, int fd, struct epoll_event *event) +{ + return syscall(__NR_epoll_ctl, epfd, op, fd, event); +} + +static inline int _epoll_wait(int epfd, struct epoll_event *events, int maxevents, int timeout) +{ + return syscall(__NR_epoll_wait, epfd, events, maxevents, timeout); +} +#else +static inline int _epoll_create( int size ) +{ + errno=ENOSYS; + return -1; +} + +static inline int _epoll_ctl(int epfd, int op, int fd, struct epoll_event *event) +{ + errno=ENOSYS; + return -1; +} + +static inline int _epoll_wait(int epfd, struct epoll_event *events, int maxevents, int timeout) +{ + errno=ENOSYS; + return -1; +} +#endif + + /* add a user in the poll array and return its index, or -1 on failure */ static int add_poll_user( struct fd *fd ) { int ret; - if (freelist) + if( epoll_fd>=0 ) /* We are using the epoll interface */ { - ret = freelist - poll_users; - freelist = (struct fd **)poll_users[ret]; + /* No poll array when using epoll - always return 1 */ + ret=1; } else - { - if (nb_users == allocated_users) + { /* We are using the poll interface */ + if (freelist) { - struct fd **newusers; - struct pollfd *newpoll; - int new_count = allocated_users ? (allocated_users + allocated_users / 2) : 16; - if (!(newusers = realloc( poll_users, new_count * sizeof(*poll_users) ))) return -1; - if (!(newpoll = realloc( pollfd, new_count * sizeof(*pollfd) ))) + ret = freelist - poll_users; + freelist = (struct fd **)poll_users[ret]; + } + else + { + if (nb_users == allocated_users) { - if (allocated_users) - poll_users = newusers; - else - free( newusers ); - return -1; + struct fd **newusers; + struct pollfd *newpoll; + int new_count = allocated_users ? (allocated_users + allocated_users / 2) : 16; + if (!(newusers = realloc( poll_users, new_count * sizeof(*poll_users) ))) return -1; + if (!(newpoll = realloc( pollfd, new_count * sizeof(*pollfd) ))) + { + if (allocated_users) + poll_users = newusers; + else + free( newusers ); + return -1; + } + poll_users = newusers; + pollfd = newpoll; + allocated_users = new_count; } - poll_users = newusers; - pollfd = newpoll; - allocated_users = new_count; - } - ret = nb_users++; - } - pollfd[ret].fd = -1; - pollfd[ret].events = 0; - pollfd[ret].revents = 0; - poll_users[ret] = fd; + ret = nb_users++; + } + pollfd[ret].fd = -1; + pollfd[ret].events = 0; + pollfd[ret].revents = 0; + poll_users[ret] = fd; + } active_users++; + return ret; } @@ -307,15 +372,37 @@ static void remove_poll_user( struct fd *fd, int user ) { assert( user >= 0 ); - assert( poll_users[user] == fd ); - pollfd[user].fd = -1; - pollfd[user].events = 0; - pollfd[user].revents = 0; - poll_users[user] = (struct fd *)freelist; - freelist = &poll_users[user]; + if( epoll_fd>=0 ) + { + if( fd->unix_fd!=-1 ) + { + struct epoll_event event; /* This struct needn't actually contain anything meaningful, as we are merely calling DEL */ + if(!(_epoll_ctl(epoll_fd, EPOLL_CTL_DEL, fd->unix_fd, &event)==0 || errno==ENOENT)) + { + int err; + err=errno; + } + } + } + else + { + assert( poll_users[user] == fd ); + pollfd[user].fd = -1; + pollfd[user].events = 0; + pollfd[user].revents = 0; + poll_users[user] = (struct fd *)freelist; + freelist = &poll_users[user]; + } active_users--; } +/* initialize the fd related state */ +void init_fd() +{ + /* If this function fails, epoll_fd will be -1, which will signal + * the rest of the functions to use poll instead */ + epoll_fd=_epoll_create(EPOLL_SIZE); +} /* server main poll() loop */ void main_loop(void) @@ -341,16 +428,44 @@ } if (!active_users) break; /* last user removed by a timeout */ } - ret = poll( pollfd, nb_users, diff ); - if (ret > 0) + if( epoll_fd>=0 ) { - int i; - for (i = 0; i < nb_users; i++) + /* if the event handlers added a lot of new events, we may want to increase the returned + * events space */ + if( epoll_revents_size<active_users ) { - if (pollfd[i].revents) + /* If epoll_revents_size is below active_users, there is a (very slim) potential for starvation. + * It is not, strictly speaking, an error though. As such, if the allocation fails, we just go on + * with the smaller array size. */ + struct epoll_event *new_revents=malloc(sizeof(struct epoll_event)*active_users); + if( new_revents!=NULL ) { - fd_poll_event( poll_users[i], pollfd[i].revents ); - if (!--ret) break; + free(epoll_revents); + epoll_revents=new_revents; + epoll_revents_size=active_users; + } + } + + ret = epoll_wait(epoll_fd, epoll_revents, epoll_revents_size, diff ); + while( (ret--)>0 ) + { + fd_poll_event((struct fd *)epoll_revents[ret].data.ptr, epoll_revents[ret].events); + } + } + else + { + /* No epoll support - use poll */ + ret = poll( pollfd, nb_users, diff ); + if (ret > 0) + { + int i; + for (i = 0; i < nb_users; i++) + { + if (pollfd[i].revents) + { + fd_poll_event( poll_users[i], pollfd[i].revents ); + if (!--ret) break; + } } } } @@ -835,18 +950,46 @@ /* set the events that select waits for on this fd */ void set_fd_events( struct fd *fd, int events ) { - int user = fd->poll_index; - assert( poll_users[user] == fd ); - if (events == -1) /* stop waiting on this fd completely */ + if( epoll_fd>=0 ) { - pollfd[user].fd = -1; - pollfd[user].events = POLLERR; - pollfd[user].revents = 0; + struct epoll_event event; + event.events=events; + event.data.ptr=fd; + + assert(fd->unix_fd>=0); + + if( events != -1 ) + { + int epoll_res; + if( (epoll_res=_epoll_ctl(epoll_fd, EPOLL_CTL_MOD, fd->unix_fd, &event )!=0) && errno==ENOENT ) + { + /* For one reason or another this file descriptor isn't being watched at the moment. Just add it */ + assert(_epoll_ctl(epoll_fd, EPOLL_CTL_ADD, fd->unix_fd, &event )==0); + } + else + assert(epoll_res==0); + } + else + { + /* Stop waiting on this fd completely */ + assert(_epoll_ctl(epoll_fd, EPOLL_CTL_DEL, fd->unix_fd, &event )==0); + } } - else if (pollfd[user].fd != -1 || !pollfd[user].events) + else { - pollfd[user].fd = fd->unix_fd; - pollfd[user].events = events; + int user = fd->poll_index; + assert( poll_users[user] == fd ); + if (events == -1) /* stop waiting on this fd completely */ + { + pollfd[user].fd = -1; + pollfd[user].events = POLLERR; + pollfd[user].revents = 0; + } + else if (pollfd[user].fd != -1 || !pollfd[user].events) + { + pollfd[user].fd = fd->unix_fd; + pollfd[user].events = events; + } } } Index: server/file.h =================================================================== RCS file: /home/sun/sources/cvs/wine/server/file.h,v retrieving revision 1.16 diff -u -r1.16 file.h --- server/file.h 16 Apr 2004 04:31:35 -0000 1.16 +++ server/file.h 24 Aug 2004 13:34:21 -0000 @@ -44,6 +44,7 @@ /* file descriptor functions */ +extern void init_fd(); extern struct fd *alloc_fd( const struct fd_ops *fd_user_ops, struct object *user ); extern struct fd *open_fd( struct fd *fd, const char *name, int flags, mode_t *mode, unsigned int access, unsigned int sharing, unsigned int options ); Index: server/main.c =================================================================== RCS file: /home/sun/sources/cvs/wine/server/main.c,v retrieving revision 1.31 diff -u -r1.31 main.c --- server/main.c 26 Mar 2003 01:32:18 -0000 1.31 +++ server/main.c 24 Aug 2004 15:49:33 -0000 @@ -122,6 +122,7 @@ signal( SIGTERM, sigterm_handler ); signal( SIGABRT, sigterm_handler ); + init_fd(); sock_init(); open_master_socket(); sync_namespace = create_namespace( 37, TRUE );
Shachar Shemesh wrote:
Hi all,
Attached is a non-perfect patch for review. This is a migration of the wineserver to use epoll instead of poll (if it's available).
current known issue with this patch: 1. Will not compile if HAVE_SYS_EPOLL_H is not 1 (i.e. - won't compile if epoll not available at compile time) 2. Segfaults on wine exit. 3. Lots of debug asserts.
Comments welcome. Shachar
One more thing. I don't think it matters much, but this patch is against 20040716. -- Shachar Shemesh Lingnu Open Source Consulting ltd. http://www.lingnu.com/
Shachar Shemesh wrote:
Hi all,
Attached is a non-perfect patch for review. This is a migration of the wineserver to use epoll instead of poll (if it's available).
current known issue with this patch: 1. Will not compile if HAVE_SYS_EPOLL_H is not 1 (i.e. - won't compile if epoll not available at compile time) 2. Segfaults on wine exit. 3. Lots of debug asserts.
Comments welcome. Shachar
If you're going to use syscalls instead of epoll_create etc, then you don't need to check whether epoll functions exist in glibc, since you're not using them anyway. In any case, seeing that getting your patch accepted will take some effort, it's probably a good idea to use the glibc version functions first, and deal with the problem of missing epoll functions in older glibcs in a seperate patch. Mike
Shachar Shemesh wrote:
Hi all,
Attached is a non-perfect patch for review. This is a migration of the wineserver to use epoll instead of poll (if it's available).
current known issue with this patch: 1. Will not compile if HAVE_SYS_EPOLL_H is not 1 (i.e. - won't compile if epoll not available at compile time) 2. Segfaults on wine exit. 3. Lots of debug asserts.
Comments welcome. Shachar
The patch is not yet ready for commit, but I do have preliminary benchmarks: The attached program (compiled as winelib) was used. ulimit -n was raised to 10240. With the current wine code: real 0m41.076s user 0m5.070s sys 0m7.722s the main wine process was taking 10% CPU time, and wineserver was taking over 60% cpu time. load average was over 2 With my preliminary path: real 0m20.985s user 0m5.316s sys 0m11.421s main wine process was still taking 10%, but so was wineserver. load average was about 1.5. We can see that there is a significant drop in actual execution time, even though there is an INCREASE in user+system processing time. I believe this is not a fluke (results are pretty consistent), but rather that the poll behavior was taking a huge toll on the system in areas where it wasn't attributed towards wine, even though it was wine related. I would take the real time measurement as the indicative one. Ideas for better benchmarks would be greatly appreciated. Shachar -- Shachar Shemesh Lingnu Open Source Consulting ltd. http://www.lingnu.com/ ### Generated by Winemaker SRCDIR = . SUBDIRS = DLLS = EXES = threadtest.exe ### Common settings #CEXTRA = -mno-cygwin #CXXEXTRA = -mno-cygwin CEXTRA = CXXEXTRA = RCEXTRA = INCLUDE_PATH = -I/home/sun/sources/wine/wine/include DLL_PATH = LIBRARY_PATH = LIBRARIES = ### threadtest.exe sources and settings threadtest_exe_MODULE = threadtest.exe threadtest_exe_C_SRCS = main.c threadtest_exe_CXX_SRCS= threadtest_exe_RC_SRCS= #threadtest_exe_LDFLAGS= -mconsole \ # -mno-cygwin threadtest_exe_LDFLAGS= -mconsole threadtest_exe_DLL_PATH= threadtest_exe_DLLS = threadtest_exe_LIBRARY_PATH= -L/home/sun/sources/wine/wine/libs -L/home/sun/sources/wine/wine/dlls threadtest_exe_LIBRARIES= uuid threadtest_exe_OBJS = $(threadtest_exe_C_SRCS:.c=.o) \ $(threadtest_exe_CXX_SRCS:.cpp=.o) \ $(threadtest_exe_RC_SRCS:.rc=.res) ### Global source lists C_SRCS = $(threadtest_exe_C_SRCS) CXX_SRCS = $(threadtest_exe_CXX_SRCS) RC_SRCS = $(threadtest_exe_RC_SRCS) ### Tools CC = winegcc CXX = winegcc RC = wrc WINEBUILD = winebuild ### Generic targets all: $(SUBDIRS) $(DLLS:%=%.so) $(EXES:%=%.so) ### Build rules .PHONY: all clean dummy $(SUBDIRS): dummy @cd $@ && $(MAKE) # Implicit rules .SUFFIXES: .cpp .rc .res DEFINCL = $(INCLUDE_PATH) $(DEFINES) $(OPTIONS) .c.o: $(CC) -c $(CFLAGS) $(CEXTRA) $(DEFINCL) -o $@ $< .cpp.o: $(CXX) -c $(CXXFLAGS) $(CXXEXTRA) $(DEFINCL) -o $@ $< .cxx.o: $(CXX) -c $(CXXFLAGS) $(CXXEXTRA) $(DEFINCL) -o $@ $< .rc.res: $(RC) $(RCFLAGS) $(RCEXTRA) $(DEFINCL) -fo$@ $< # Rules for cleaning CLEAN_FILES = *.dbg.c y.tab.c y.tab.h lex.yy.c \ core *.orig *.rej \ \\\#*\\\# *~ *% .\\\#* clean:: $(SUBDIRS:%=%/__clean__) $(EXTRASUBDIRS:%=%/__clean__) $(RM) $(CLEAN_FILES) $(RC_SRCS:.rc=.res) $(C_SRCS:.c=.o) $(CXX_SRCS:.cpp=.o) $(RM) $(DLLS:%=%.dbg.o) $(DLLS:%=%.so) $(RM) $(EXES:%=%.dbg.o) $(EXES:%=%.so) $(EXES:%.exe=%) $(SUBDIRS:%=%/__clean__): dummy cd `dirname $@` && $(MAKE) clean $(EXTRASUBDIRS:%=%/__clean__): dummy -cd `dirname $@` && $(RM) $(CLEAN_FILES) ### Target specific build rules $(threadtest_exe_MODULE).dbg.c: $(threadtest_exe_C_SRCS) $(threadtest_exe_CXX_SRCS) $(WINEBUILD) -o $@ --debug -C$(SRCDIR) $(threadtest_exe_C_SRCS) $(threadtest_exe_CXX_SRCS) $(threadtest_exe_MODULE).so: $(threadtest_exe_MODULE).dbg.o $(threadtest_exe_OBJS) $(CXX) $(threadtest_exe_LDFLAGS) -o $@ $(threadtest_exe_OBJS) $(threadtest_exe_MODULE).dbg.o $(threadtest_exe_LIBRARY_PATH) $(LIBRARY_PATH) $(threadtest_exe_DLLS:%=-l%) $(threadtest_exe_LIBRARIES:%=-l%)
participants (2)
-
Mike McCormack -
Shachar Shemesh