Mike McCormack wrote:
Shachar Shemesh wrote:
After a discussion with Alexandre on IRC I came to the conclusion that nothing I will do will make this patch go in. Mike, please add it to your list of "uncommitted patches".
Hi Shachar,
I'm not sure which Mike the above is addressed at... :)
Mike Hearn addressed the need for a repository of uncommitted patches.
I agree epoll is something worth persuing, so I had a go at refining your patch in accordance with what Alexandre recommended (quoted below). Here's what I came up with... it's not complete as yet, but hopefully it's more conformant with Alexandre's wishes ...
Notes:
- used direct syscalls as my libc doesn't have epoll_* functions
Which Alexandre already asked not to. In any case, if you do, you have to add a copyright notice to the authors of the header file who's content you are copying.
- fixed array of epoll_event structures for starters
I missed that. What did you mean by this?
- do_epoll_remove assert commented as it occasionally triggers
Was that assert in my code? By looking at it, I see no reason for it NOT to be triggered occasionally. Don't forget that the file descriptor may have been closed prior to being removed from the list, in which case the system removed it from the epoll fd itself.
It works for a quick test, but I'm sure everybody can find problems with it...
I'm afraid so. Read on.
Mike
+static inline void do_epoll_add( int fd, int events, 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 );
+}
If the file descriptor is already in there, EPOLL_CTL_ADD will fail. You will then have to call EPOLL_CTL_MOD.
In "add_poll_user", you are allocating a totally useless (if epoll is used) "pollfd" array.
-/* 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;
+}
I'm assuming you pulled the timeout code into a separate function here. If there is anything fd related here, please let me know.
if (!active_users) break; /* last user removed by a timeout */+#ifdef USE_EPOLL
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;}+/* server main poll() loop using epoll */ +static void epoll_loop(void) +{
- int ret, i;
- struct epoll_event ev[MAX_EVENTS];
- assert(POLLIN == EPOLLIN);
- assert(POLLOUT == EPOLLOUT);
- assert(POLLERR == EPOLLERR);
- assert(POLLHUP == EPOLLHUP);
If you really have to, make that: #if POLLIN!=EPOLLIN || .... #error Error in macro definitions #endif
An assert is really not necessary here.
- while (active_users)
- {
int 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++ ){assert( ev[i].data.u32 < nb_users );fd_poll_event( poll_users[ev[i].data.u32], ev[i].events );
Read the comment in the appropriate section of my code. You have a race here that will crash you occasionally.
}
- }
+} +#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 +509,19 @@ } }
+void main_loop(void) +{ +#ifdef USE_EPOLL
- if( epoll_fd >= 0 )
- {
fprintf(stderr,"epoll enabled\n");epoll_loop();close( epoll_fd );return;- }
+#endif
- select_loop();
You are calling both epoll AND poll here.
+}
/****************************************************************/ /* inode functions */ @@ -837,14 +1004,19 @@ assert( poll_users[user] == fd ); if (events == -1) /* stop waiting on this fd completely */ {
if( pollfd[user].events )do_epoll_remove( pollfd[user].fd ); pollfd[user].fd = -1; pollfd[user].events = POLLERR; pollfd[user].revents = 0;if( pollfd[user].events )do_epoll_remove( pollfd[user].fd ); pollfd[user].fd = fd->unix_fd; pollfd[user].events = events;do_epoll_add( pollfd[user].fd, pollfd[user].events, user );
Like I said earlier, EPOLL_CTL_MOD does the same thing.
}}
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.
Shachar