Mike McCormack wrote:
I mean I've allocated an array of 10 struct epoll_events to receive events from epoll, and I plan to dynamically allocate it later.
I have greater plans for wineserver :-) As part of those plans, I'm going to ask some kernel gurus whether that's strictly necessary. If it's not, I may actually advocate making it even lower (1, in particular).
No, there's a return in there, so only one of the main loops is entered.
I stand corrected.
Well, the most serious problem with this code is the race you did not handle. I don't like the use of a whole pollfd array, but that's not for me to decide.
OK, I've fixed the race as I mentioned above. I'm trying to use exactly the same data structures as the select() loop where possible so that it's easier to verify the epoll_loop works assuming that the select_loop() works.
Looking forward to comments on the attached patch.
Index: server/fd.c
RCS file: /home/wine/wine/server/fd.c,v retrieving revision 1.25 diff -u -r1.25 fd.c --- server/fd.c 9 Sep 2004 00:28:36 -0000 1.25 +++ server/fd.c 9 Sep 2004 04:51:16 -0000 @@ -2,6 +2,8 @@
- Server-side file descriptor management
- Copyright (C) 2000, 2003 Alexandre Julliard
- Copyright (C) 2004 Shachar Shemesh for Lingnu Open Source Consulting ltd.
- Copyright (C) 2004 Mike McCormack
I was actually talking about adding: * Epoll header copied here is Copyright (C) 2002, 2003 by the Free Software Foundation, Inc.
However, as I see you no longer copy the header into the program, this is no longer necessary.
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
@@ -236,6 +242,106 @@ static int allocated_users; /* count of allocated entries in the array */ static struct fd **freelist; /* list of free entries in the array */
+#ifdef HAVE_SYS_EPOLL_H
+#define EPOLL_SIZE 1024 +#define MAX_EVENTS 10
+static int epoll_fd; +struct epoll_event *epoll_ev_first, *epoll_ev_last;
+/* this should optimize away if the poll constants are the same as the epoll ones */
Hmm. Need to verify that, though.
+static inline int epoll_events( int events ) +{
- int r = 0;
- if( (POLLIN == EPOLLIN) && (POLLOUT == EPOLLOUT) &&
(POLLERR == EPOLLERR) && (POLLHUP == EPOLLHUP) )return events;- if( POLLIN&events ) r |= EPOLLIN;
- if( POLLOUT&events ) r |= EPOLLOUT;
- if( POLLERR&events ) r |= EPOLLERR;
- if( POLLHUP&events ) r |= EPOLLHUP;
- return r;
+}
+static inline void do_epoll_add( int fd, int events, unsigned int user ) +{
- struct epoll_event eev;
- int r;
- if (epoll_fd < 0)
return;- if (fd < 0)
return;- if (!events)
return;- eev.events = events;
- eev.data.u32 = user;
- r = epoll_ctl( epoll_fd, EPOLL_CTL_ADD, fd, &eev );
- assert( 0 == r );
+}
+static inline void do_epoll_mod( int fd, int events, unsigned int user ) +{
- struct epoll_event eev;
- int r;
- if (epoll_fd < 0)
return;- if (fd < 0)
return;- eev.events = events;
- eev.data.u32 = user;
- r = epoll_ctl( epoll_fd, EPOLL_CTL_MOD, fd, &eev );
- assert( 0 == r );
+}
+static inline void do_epoll_remove( int fd, int old_events, unsigned int user ) +{
- struct epoll_event eev, *i;
- int r;
- if (epoll_fd < 0)
return;- if (fd < 0)
return;- if (!old_events)
return;- r = epoll_ctl( epoll_fd, EPOLL_CTL_DEL, fd, &eev );
- assert( 0 == r );
This assert still depends on the removal semantics. If it's ok for the users to close the Unix fd before removing it from the poll array, you may find that it's already gone and the assert will fail.
If you like, make that: assert( 0 == r || errno == ENOENT );
- /* remove any unprocessed events for this fd */
- for( i=epoll_ev_first; i<epoll_ev_last; i++ )
if (i->data.u32 == user)i->events = 0;+}
+void init_fd() +{
- epoll_fd = epoll_create(EPOLL_SIZE);
+}
+#else
+static inline void do_epoll_add( int fd, int events, unsigned int user ) +{ +}
+static inline void do_epoll_mod( int fd, int events, unsigned int user ) +{ +}
+static inline void do_epoll_remove( int fd, int old_events, unsigned int user ) +{ +}
+void init_fd() +{
+}
+#endif
/* add a user in the poll array and return its index, or -1 on failure */ static int add_poll_user( struct fd *fd ) { @@ -280,6 +386,7 @@ { assert( user >= 0 ); assert( poll_users[user] == fd );
- do_epoll_remove( pollfd[user].fd, pollfd[user].events, user ); pollfd[user].fd = -1; pollfd[user].events = 0; pollfd[user].revents = 0;
@@ -288,56 +395,95 @@ active_users--; }
-/* server main poll() loop */ -void main_loop(void) +static inline int next_delta() {
- int ret;
- long diff = -1;
- struct list expired_list, *ptr;
- struct timeval now;
- while (active_users)
- if (list_empty(&timeout_list))
return diff;- gettimeofday( &now, NULL );
- /* first remove all expired timers from the list */
- list_init( &expired_list );
- while ((ptr = list_head( &timeout_list )) != NULL) {
long diff = -1;if (!list_empty( &timeout_list ))
struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );if (!time_before( &now, &timeout->when )) {
struct list expired_list, *ptr;struct timeval now;gettimeofday( &now, NULL );
list_remove( &timeout->entry );list_add_tail( &expired_list, &timeout->entry );}else break;- }
/* first remove all expired timers from the list */
- /* now call the callback for all the removed timers */
list_init( &expired_list );while ((ptr = list_head( &timeout_list )) != NULL){struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );
- while ((ptr = list_head( &expired_list )) != NULL)
- {
struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );list_remove( &timeout->entry );timeout->callback( timeout->private );free( timeout );- }
if (!time_before( &now, &timeout->when )){list_remove( &timeout->entry );list_add_tail( &expired_list, &timeout->entry );}else break;}
- if (!active_users) return diff; /* last user removed by a timeout */
/* now call the callback for all the removed timers */
- if ((ptr = list_head( &timeout_list )) != NULL)
- {
struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );diff = (timeout->when.tv_sec - now.tv_sec) * 1000+ (timeout->when.tv_usec - now.tv_usec) / 1000;if (diff < 0) diff = 0;- }
while ((ptr = list_head( &expired_list )) != NULL){struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );list_remove( &timeout->entry );timeout->callback( timeout->private );free( timeout );}
- return diff;
+}
if (!active_users) break; /* last user removed by a timeout */+#ifdef HAVE_SYS_EPOLL_H
if ((ptr = list_head( &timeout_list )) != NULL)+/* server main poll() loop using epoll */ +static void epoll_loop(void) +{
- struct epoll_event ev[MAX_EVENTS];
- while (active_users)
- {
int ret, i, user, diff = next_delta();if (!active_users) break; /* last user removed by a timeout */ret = epoll_wait( epoll_fd, &ev[0], MAX_EVENTS, diff );for (i = 0; i < ret; i++){user = ev[i].data.u32;assert( user < nb_users );epoll_ev_last = &ev[ret];
There is no need to do that every time.
if (ev[i].events) {
struct timeout_user *timeout = LIST_ENTRY( ptr, struct timeout_user, entry );diff = (timeout->when.tv_sec - now.tv_sec) * 1000+ (timeout->when.tv_usec - now.tv_usec) / 1000;if (diff < 0) diff = 0;
epoll_ev_first = &ev[i+1];pollfd[user].revents = ev[i].events;fd_poll_event( poll_users[user], ev[i].events );
Why not just get rid of epoll_ev_first and epoll_ev_last, and check that pollfd[user]!=0?
} }
epoll_ev_first = NULL;epoll_ev_last = NULL;- }
+} +#endif
+/* server main poll() loop using select */ +static void select_loop(void) +{
int ret;
while (active_users)
{
int diff = next_delta();if (!active_users) break; /* last user removed by a timeout */ ret = poll( pollfd, nb_users, diff ); if (ret > 0)@@ -355,6 +501,18 @@ } }
+void main_loop(void) +{ +#ifdef HAVE_SYS_EPOLL_H
- if( epoll_fd >= 0 )
- {
epoll_loop();close( epoll_fd );return;- }
+#endif
- select_loop();
+}
/****************************************************************/ /* inode functions */ @@ -837,12 +995,24 @@ assert( poll_users[user] == fd ); if (events == -1) /* stop waiting on this fd completely */ {
do_epoll_remove( pollfd[user].fd, pollfd[user].events, user ); pollfd[user].fd = -1; pollfd[user].events = POLLERR; pollfd[user].revents = 0;/* remove the old fd */if ( (fd->unix_fd != pollfd[user].fd) || !events )do_epoll_remove( pollfd[user].fd, pollfd[user].events, user );if (events){/* add the new fd or update the old one */if ( (pollfd[user].fd == -1) || !pollfd[user].events )do_epoll_add( fd->unix_fd, events, user );else if (pollfd[user].events != events)do_epoll_mod( fd->unix_fd, events, user );} pollfd[user].fd = fd->unix_fd; pollfd[user].events = events;Index: configure.ac
RCS file: /home/wine/wine/configure.ac,v retrieving revision 1.307 diff -u -r1.307 configure.ac @@ -757,6 +759,7 @@ EXTRACFLAGS="$EXTRACFLAGS -Wpointer-arith" fi fi +EXTRACFLAGS="$EXTRACFLAGS -Wsign-compare"
Why is this necessary?
@@ -1629,6 +1634,7 @@ dlls/opengl32/Makefile dlls/psapi/Makefile dlls/psapi/tests/Makefile +dlls/pstorec/Makefile
or this?
Shachar