From 0c2909a2920a39a9fe2501b562571d913fd8503f Mon Sep 17 00:00:00 2001 From: Martin Mares Date: Fri, 6 Mar 2015 14:50:05 +0100 Subject: [PATCH] Mainloop: Avoid polling for an empty set of events If a main_file was active, but it did not have any handlers set, it could happen that poll/epoll was called with an empty set of requested events. Alas, even in those cases POLLHUP or POLLERR could be delivered, leading to an endless loop of such events. (Remember that we do not use the edge-triggered mode of epoll as we want to be compatible with plain poll semantics in handlers.) From now on, we avoid this situation by removing the fd from the poll set temporarily. Also, this led to a slight unification of code paths. --- ucw/mainloop.c | 107 +++++++++++++++++++++++++++++-------------------- ucw/mainloop.h | 8 ++-- 2 files changed, 67 insertions(+), 48 deletions(-) diff --git a/ucw/mainloop.c b/ucw/mainloop.c index 46604d86..920d0f0b 100644 --- a/ucw/mainloop.c +++ b/ucw/mainloop.c @@ -1,7 +1,7 @@ /* * UCW Library -- Main Loop * - * (c) 2004--2012 Martin Mares + * (c) 2004--2015 Martin Mares * * This software may be freely distributed and used according to the terms * of the GNU Lesser General Public License. @@ -311,6 +311,49 @@ file_want_events(struct main_file *fi) return events; } +static void +file_set_events(struct main_context *m, struct main_file *fi, uint mask) +{ + uint prev_events = fi->want_events; + if (prev_events == mask) + return; + DBG("MAIN: Changing requested events for fd %d to %x", fi->fd, mask); + fi->want_events = mask; + +#ifdef CONFIG_UCW_EPOLL + if (m->epoll_fd < 0) + return; + struct epoll_event evt = { + .events = mask, + .data.ptr = fi, + }; + int op; + if (!prev_events) + { + op = EPOLL_CTL_ADD; + m->poll_cnt++; + } + else if (!mask) + { + op = EPOLL_CTL_DEL; + m->poll_cnt--; + } + else + op = EPOLL_CTL_MOD; + if (epoll_ctl(m->epoll_fd, op, fi->fd, &evt) < 0) + { + // Some clients call file_del() on an already closed descriptor. Trying to be benevolent. + if (errno != EBADF || op != EPOLL_CTL_DEL) + die("epoll_ctl() failed: %m"); + } +#else + if (!prev_events || !mask || !fi->pollfd) + m->poll_table_obsolete = 1; + else + fi->pollfd->events = mask; +#endif +} + void file_add(struct main_file *fi) { @@ -320,17 +363,7 @@ file_add(struct main_file *fi) ASSERT(!file_is_active(fi)); clist_add_tail(&m->file_list, &fi->n); m->file_cnt++; -#ifdef CONFIG_UCW_EPOLL - struct epoll_event evt = { - .events = file_want_events(fi), - .data.ptr = fi, - }; - if (epoll_ctl(m->epoll_fd, EPOLL_CTL_ADD, fi->fd, &evt) < 0) - die("epoll_ctl() failed: %m"); - fi->last_want_events = evt.events; -#else - m->poll_table_obsolete = 1; -#endif + file_set_events(m, fi, file_want_events(fi)); if (fcntl(fi->fd, F_SETFL, O_NONBLOCK) < 0) msg(L_ERROR, "Error setting fd %d to non-blocking mode: %m. Keep fingers crossed.", fi->fd); } @@ -338,13 +371,12 @@ file_add(struct main_file *fi) void file_chg(struct main_file *fi) { + struct main_context *m = main_current(); #ifdef CONFIG_UCW_EPOLL clist_remove(&fi->n); - clist_add_tail(&main_current()->file_recalc_list, &fi->n); + clist_add_tail(&m->file_recalc_list, &fi->n); #else - struct pollfd *p = fi->pollfd; - if (p) - p->events = file_want_events(fi); + file_set_events(m, fi, file_want_events(fi)); #endif } @@ -358,16 +390,7 @@ file_del_ctx(struct main_context *m, struct main_file *fi) return; clist_unlink(&fi->n); m->file_cnt--; -#ifdef CONFIG_UCW_EPOLL - if (m->epoll_fd >= 0 && epoll_ctl(m->epoll_fd, EPOLL_CTL_DEL, fi->fd, NULL) < 0) - { - // Some clients call file_del() on an already closed descriptor. Trying to be benevolent. - if (errno != EBADF) - die("epoll_ctl() failed: %m"); - } -#else - m->poll_table_obsolete = 1; -#endif + file_set_events(m, fi, 0); } void @@ -735,17 +758,7 @@ recalc_files(struct main_context *m) while (fi = clist_remove_head(&m->file_recalc_list)) { - struct epoll_event evt = { - .events = file_want_events(fi), - .data.ptr = fi, - }; - if (evt.events != fi->last_want_events) - { - DBG("MAIN: Changing requested events for fd %d to %x", fi->fd, evt.events); - fi->last_want_events = evt.events; - if (epoll_ctl(main_current()->epoll_fd, EPOLL_CTL_MOD, fi->fd, &evt) < 0) - die("epoll_ctl() failed: %m"); - } + file_set_events(m, fi, file_want_events(fi)); clist_add_tail(&m->file_list, &fi->n); } } @@ -758,15 +771,21 @@ rebuild_poll_table(struct main_context *m) GARY_INIT_OR_RESIZE(m->poll_table, m->file_cnt); GARY_INIT_OR_RESIZE(m->poll_file_table, m->file_cnt); DBG("MAIN: Rebuilding poll table: %d entries", m->file_cnt); + // From this point on, the gary's must not be resized: we keep pointers to their entries struct pollfd *p = m->poll_table; struct main_file **pf = m->poll_file_table; + m->poll_cnt = 0; CLIST_FOR_EACH(struct main_file *, fi, m->file_list) { p->fd = fi->fd; - p->events = file_want_events(fi); - fi->pollfd = p++; - *pf++ = fi; + p->events = fi->want_events; + if (p->events) + { + fi->pollfd = p++; + *pf++ = fi; + m->poll_cnt++; + } } m->poll_table_obsolete = 0; } @@ -807,13 +826,13 @@ main_loop(void) #ifdef CONFIG_UCW_EPOLL recalc_files(m); - DBG("MAIN: Epoll for %d fds and timeout %d ms", m->file_cnt, timeout); + DBG("MAIN: Epoll for %d fds and timeout %d ms", m->poll_cnt, timeout); int n = epoll_wait(m->epoll_fd, m->epoll_events, EPOLL_BUF_SIZE, timeout); #else if (m->poll_table_obsolete) rebuild_poll_table(m); - DBG("MAIN: Poll for %d fds and timeout %d ms", m->file_cnt, timeout); - int n = poll(m->poll_table, m->file_cnt, timeout); + DBG("MAIN: Poll for %d fds and timeout %d ms", m->poll_cnt, timeout); + int n = poll(m->poll_table, m->poll_cnt, timeout); #endif DBG("\t-> %d events", n); @@ -844,7 +863,7 @@ main_loop(void) #else struct pollfd *p = m->poll_table; struct main_file **pf = m->poll_file_table; - for (uint i=0; i < m->file_cnt; i++) + for (uint i=0; i < m->poll_cnt; i++) if (p[i].revents) { struct main_file *fi = pf[i]; diff --git a/ucw/mainloop.h b/ucw/mainloop.h index 6e8ffae6..8fd2e6ed 100644 --- a/ucw/mainloop.h +++ b/ucw/mainloop.h @@ -1,7 +1,7 @@ /* * UCW Library -- Main Loop * - * (c) 2004--2012 Martin Mares + * (c) 2004--2015 Martin Mares * * This software may be freely distributed and used according to the terms * of the GNU Lesser General Public License. @@ -90,6 +90,7 @@ struct main_context { struct pollfd *poll_table; struct main_file **poll_file_table; #endif + uint poll_cnt; struct main_timer **timer_table; /* Growing array containing the heap of timers */ sigset_t want_signals; int sig_pipe_send; @@ -352,9 +353,8 @@ struct main_file { int (*write_handler)(struct main_file *fi); void *data; /* [*] Data for use by the handlers */ uint events; -#ifdef CONFIG_UCW_EPOLL - uint last_want_events; -#else + uint want_events; +#ifndef CONFIG_UCW_EPOLL struct pollfd *pollfd; #endif }; -- 2.39.2