apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: [PATCH] pollset_poll() improvements
Date Thu, 12 Aug 2004 16:39:10 GMT
On Thu, Aug 12, 2004 at 03:55:41PM +0100, Joe Orton wrote:
> On Thu, Aug 12, 2004 at 10:44:29AM +0100, Joe Orton wrote:
> > warnings on LP64 platforms.  Is there a cleaner alternative?  epoll
> > stuff looks good, I'll commit that.
> 
> Actually there's a problem: this change breaks apr_pollset_remove();
> when the query_set array is shuffled up after removing a descriptor, any
> epoll descriptors may still reference the old indexes into query_set
> from data.u32.
...
> The best fix for this would probably be to have apr_pollset_destroy
> leave holes in the query_set array which apr_pollset_add will fill in, I 
> think?

e.g. how does this look?  I've only tested epoll and poll with these
changes.

Index: poll/unix/poll.c
===================================================================
RCS file: /home/cvs/apr/poll/unix/poll.c,v
retrieving revision 1.46
diff -u -r1.46 poll.c
--- poll/unix/poll.c	7 Jul 2004 07:40:12 -0000	1.46
+++ poll/unix/poll.c	12 Aug 2004 16:35:15 -0000
@@ -355,10 +355,12 @@
     apr_uint32_t nalloc;
 #ifdef HAVE_KQUEUE
     int kqueue_fd;
+    apr_uint32_t nextfree;
     struct kevent kevent;
     struct kevent *ke_set;
 #elif defined(HAVE_EPOLL)
     int epoll_fd;
+    apr_uint32_t nextfree;
     struct epoll_event *pollset;
 #elif defined(HAVE_POLL)
     struct pollfd *pollset;
@@ -409,11 +411,13 @@
     if ((*pollset)->kqueue_fd == -1) {
          return APR_ENOMEM;
     }
+    (*pollset)->nextfree = 0;
     apr_pool_cleanup_register(p, (void*)(*pollset), backend_cleanup, 
         apr_pool_cleanup_null);
 #elif defined(HAVE_EPOLL)
     (*pollset)->epoll_fd = epoll_create(size);
     (*pollset)->pollset = apr_palloc(p, size * sizeof(struct epoll_event));
+    (*pollset)->nextfree = 0;
     apr_pool_cleanup_register(p, (void*)(*pollset), backend_cleanup, 
         apr_pool_cleanup_null);
 #elif defined(HAVE_POLL)
@@ -445,24 +449,15 @@
 APR_DECLARE(apr_status_t) apr_pollset_add(apr_pollset_t *pollset,
                                           const apr_pollfd_t *descriptor)
 {
-#ifdef HAVE_KQUEUE
-    apr_os_sock_t fd;
-#elif defined(HAVE_EPOLL)
-    struct epoll_event ev;
-    int ret = -1;
-#else
-#if !defined(HAVE_POLL)
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
     apr_os_sock_t fd;
+    apr_uint32_t idx;
 #endif
+#ifdef HAVE_EPOLL
+    struct epoll_event ev;
 #endif
 
-    if (pollset->nelts == pollset->nalloc) {
-        return APR_ENOMEM;
-    }
-
-    pollset->query_set[pollset->nelts] = *descriptor;
-
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
     if (descriptor->desc_type == APR_POLL_SOCKET) {
         fd = descriptor->desc.s->socketdes;
     }
@@ -470,8 +465,32 @@
         fd = descriptor->desc.f->filedes;
     }
 
+    idx = pollset->nextfree;
+    if (idx == pollset->nalloc) {
+        return APR_ENOMEM;
+    }
+
+    /* Find the next unused descriptor (else nextfree == nelts). */
+    do {
+        pollset->nextfree++;
+    } while (pollset->nextfree < pollset->nelts
+             && pollset->query_set[pollset->nextfree].desc_type != APR_NO_DESC);
+
+    if (idx == pollset->nelts) {
+        pollset->nelts++;
+    }
+
+    pollset->query_set[idx] = *descriptor;
+#else
+    if (pollset->nelts == pollset->nalloc) {
+        return APR_ENOMEM;
+    }
+    pollset->query_set[pollset->nelts] = *descriptor;
+#endif
+
+#if defined(HAVE_KQUEUE)
     if (descriptor->reqevents & APR_POLLIN) {
-        EV_SET(&pollset->kevent, fd, EVFILT_READ, EV_ADD, 0, 0, NULL);
+        EV_SET(&pollset->kevent, fd, EVFILT_READ, EV_ADD, 0, 0, (void*)idx);
 
         if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
                    NULL) == -1) {
@@ -480,9 +499,9 @@
     }
 
     if (descriptor->reqevents & APR_POLLOUT) {
-        EV_SET(&pollset->kevent, fd, EVFILT_WRITE, EV_ADD, 0, 0, NULL);
+        EV_SET(&pollset->kevent, fd, EVFILT_WRITE, EV_ADD, 0, 0, (void *)idx);
 
-        if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
+        if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0, 
                    NULL) == -1) {
             return APR_ENOMEM;
         }
@@ -490,18 +509,9 @@
 
 #elif defined(HAVE_EPOLL)
     ev.events = get_epoll_event(descriptor->reqevents);
-    if (descriptor->desc_type == APR_POLL_SOCKET) {
-        ev.data.fd = descriptor->desc.s->socketdes;
-        ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD,
-                        descriptor->desc.s->socketdes, &ev);
-    }
-    else {
-        ev.data.fd = descriptor->desc.f->filedes;
-        ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD,
-                        descriptor->desc.f->filedes, &ev);
-    }
-    if (0 != ret) {
-        return APR_EBADF;
+    ev.data.u32 = idx;
+    if (epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD, fd, &ev)) {
+        return errno;
     }
 #elif defined(HAVE_POLL)
 
@@ -513,7 +523,8 @@
     }
 
     pollset->pollset[pollset->nelts].events = get_event(descriptor->reqevents);
-#else
+    pollset->nelts++;
+#else /* !(HAVE_POLL || HAVE_EPOLL || HAVE_KQUEUE) */
     if (descriptor->desc_type == APR_POLL_SOCKET) {
 #ifdef NETWARE
         /* NetWare can't handle mixed descriptor types in select() */
@@ -563,8 +574,8 @@
     if ((int)fd > pollset->maxfd) {
         pollset->maxfd = (int)fd;
     }
-#endif
     pollset->nelts++;
+#endif
     return APR_SUCCESS;
 }
 
@@ -572,96 +583,67 @@
                                              const apr_pollfd_t *descriptor)
 {
     apr_uint32_t i;
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
     apr_os_sock_t fd;
-#elif defined(HAVE_EPOLL)
+    int found = 0;
+#endif
+#if defined(HAVE_EPOLL)
     struct epoll_event ev;
-    int ret = -1;
 #elif !defined(HAVE_POLL)
     apr_os_sock_t fd;
 #endif
 
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
+    if (descriptor->desc_type == APR_POLL_SOCKET) {
+        fd = descriptor->desc.s->socketdes;
+    }
+    else {
+        fd = descriptor->desc.f->filedes;
+    }
+
     for (i = 0; i < pollset->nelts; i++) {
         if (descriptor->desc.s == pollset->query_set[i].desc.s) {
-            /* Found an instance of the fd: remove this and any other copies  */
-            apr_uint32_t dst = i;
-            apr_uint32_t old_nelts = pollset->nelts;
-            pollset->nelts--;
-            for (i++; i < old_nelts; i++) {
-                if (descriptor->desc.s == pollset->query_set[i].desc.s) {
-                    pollset->nelts--;
-                }
-                else {
-                    pollset->query_set[dst] = pollset->query_set[i];
-                    dst++;
-                }
-            }
-
-            if (descriptor->desc_type == APR_POLL_SOCKET) {
-                fd = descriptor->desc.s->socketdes;
-            }
-            else {
-                fd = descriptor->desc.f->filedes;
-            }
-
-            if (descriptor->reqevents & APR_POLLIN) {
-                EV_SET(&pollset->kevent, fd,
-                       EVFILT_READ, EV_DELETE, 0, 0, NULL);
-
-                if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
-                          NULL) == -1) {
-                    return APR_EBADF;
-                }
+            if (i < pollset->nextfree) {
+                pollset->nextfree = i;
             }
+            pollset->query_set[i].desc_type = APR_NO_DESC;
+            found = 1;
+        }
+    }
 
-            if (descriptor->reqevents & APR_POLLOUT) {
-                EV_SET(&pollset->kevent, fd,
-                       EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
-
-                if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
-                          NULL) == -1) {
-                    return APR_EBADF;
-                }
-            }
+    if (!found) {
+        return APR_NOTFOUND;
+    }
+#endif
 
-            return APR_SUCCESS;
+#if defined(HAVE_KQUEUE)
+    if (descriptor->reqevents & APR_POLLIN) {
+        EV_SET(&pollset->kevent, fd,
+               EVFILT_READ, EV_DELETE, 0, 0, NULL);
+        
+        if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0, 
+                   NULL) == -1) {
+            return APR_EBADF;
         }
     }
-#elif defined(HAVE_EPOLL)
-    for (i = 0; i < pollset->nelts; i++) {
-        if (descriptor->desc.s == pollset->query_set[i].desc.s) {
-            /* Found an instance of the fd: remove this and any other copies  */
-            apr_uint32_t dst = i;
-            apr_uint32_t old_nelts = pollset->nelts;
-            pollset->nelts--;
-            for (i++; i < old_nelts; i++) {
-                if (descriptor->desc.s == pollset->query_set[i].desc.s) {
-                    pollset->nelts--;
-                }
-                else {
-                    pollset->query_set[dst] = pollset->query_set[i];
-                    dst++;
-                }
-            }
-            ev.events = get_epoll_event(descriptor->reqevents);
-            if (descriptor->desc_type == APR_POLL_SOCKET) {
-                ev.data.fd = descriptor->desc.s->socketdes;
-                ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL,
-                                descriptor->desc.s->socketdes, &ev);
-            }
-            else {
-                ev.data.fd = descriptor->desc.f->filedes;
-                ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL,
-                                descriptor->desc.f->filedes, &ev);
-            }
-            if (ret < 0) {
-                return APR_EBADF;
-            }
-
-            return APR_SUCCESS;
+    
+    if (descriptor->reqevents & APR_POLLOUT) {
+        EV_SET(&pollset->kevent, fd,
+               EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
+        
+        if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
+                   NULL) == -1) {
+            return APR_EBADF;
         }
     }
+    
+    return APR_SUCCESS;
+#elif defined(HAVE_EPOLL)
+    /* event argument is ignored but must not be NULL */
+    if (epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL, fd, &ev) < 0) {
+        return errno;
+    }
+    return APR_SUCCESS;
 #elif defined(HAVE_POLL)
     for (i = 0; i < pollset->nelts; i++) {
         if (descriptor->desc.s == pollset->query_set[i].desc.s) {
@@ -682,7 +664,7 @@
             return APR_SUCCESS;
         }
     }
-
+    return APR_NOTFOUND;
 #else /* no poll */
     if (descriptor->desc_type == APR_POLL_SOCKET) {
         fd = descriptor->desc.s->socketdes;
@@ -719,9 +701,8 @@
             return APR_SUCCESS;
         }
     }
-#endif /* no poll */
-
     return APR_NOTFOUND;
+#endif /* no poll */
 }
 #ifdef HAVE_KQUEUE
 APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
@@ -788,8 +769,7 @@
                                            apr_int32_t *num,
                                            const apr_pollfd_t **descriptors)
 {
-    int rv;
-    apr_uint32_t i, j, k;
+    int rv, i;
 
     if (timeout > 0) {
         timeout /= 1000;
@@ -799,43 +779,27 @@
                     timeout);
     (*num) = rv;
     if (rv < 0) {
-        return apr_get_netos_error();
+        return errno;
     }
     if (rv == 0) {
         return APR_TIMEUP;
     }
-    j = 0;
-    for (i = 0; i < pollset->nelts; i++) {
-        if (pollset->pollset[i].events != 0) {
-            /* TODO: Is there a better way to re-associate our data? */
-            for (k = 0; k < pollset->nelts; k++) {
-                if (pollset->query_set[k].desc_type == APR_POLL_SOCKET &&
-                    pollset->query_set[k].desc.s->socketdes ==
-                        pollset->pollset[i].data.fd) {
-                    pollset->result_set[j] = pollset->query_set[k];
-                    pollset->result_set[j].rtnevents =
-                        get_epoll_revent(pollset->pollset[i].events);
-                    j++;
-                    break;
-                }
-                else if (pollset->query_set[k].desc_type == APR_POLL_FILE 
-                         && pollset->query_set[k].desc.f->filedes ==
-                            pollset->pollset[i].data.fd) {
-                    pollset->result_set[j] = pollset->query_set[k];
-                    pollset->result_set[j].rtnevents =
-                        get_epoll_revent(pollset->pollset[i].events);
-                    j++;
-                    break;
-                }
-            }
-        }
+
+    for (i = 0; i < rv; i++) {
+        pollset->result_set[i] = 
+            pollset->query_set[pollset->pollset[i].data.u32];
+        pollset->result_set[i].rtnevents =
+            get_epoll_revent(pollset->pollset[i].events);
     }
+
     if (descriptors) {
         *descriptors = pollset->result_set;
     }
     return APR_SUCCESS;
 }
+
 #elif defined(HAVE_POLL)
+
 APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
                                            apr_interval_time_t timeout,
                                            apr_int32_t *num,

Mime
View raw message