epoll patch - status?

[Shachar Shemesh]

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;
>     }
>     else if (pollfd[user].fd != -1 || !pollfd[user].events)
>     {
>+        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

-- 
Shachar Shemesh
Lingnu Open Source Consulting ltd.
http://www.lingnu.com/