From 2c66983a6d46cbd863b079a1c92c916b41efe0ba Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 24 Oct 2010 11:51:14 -0400 Subject: [PATCH] Simplify the logic for choosing EPOLL_CTL_ADD vs EPOLL_CTL_MOD Previously, we chose "ADD" whenever old_events==new_events, (since we expected the add to fail with EEXIST), or whenever old_events was==0, and MOD otherwise (i.e., when old_events was nonzero and not equal to new_events). But now that we retry failed MOD events as ADD *and* failed ADD events as MOD, the important thing is now to try to guess right the largest amount of the time, since guessing right means we do only one syscall, but guessing wrong means we do two. When old_events is 0, ADD is probably right (unless we're hitting the dup bug, when we'll fall back). And when old_events is set and != new_events, MOD is almost certainly right for the same reasons as before. But when old_events is equal to new events, then MOD will work fine unless we closed and reopened the fd, in which case we'll have to fall back to the ADD case. (Redundant del/add pairs are more common than closes for most use cases.) This change lets us avoid calculating new_events, which ought to save a little time in epoll.c --- epoll.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/epoll.c b/epoll.c index 9c8f0e99e7..672025f50a 100644 --- a/epoll.c +++ b/epoll.c @@ -169,43 +169,46 @@ epoll_apply_changes(struct event_base *base) */ + /* TODO: Turn this into a switch or a table lookup. */ + if ((ch->read_change & EV_CHANGE_ADD) || (ch->write_change & EV_CHANGE_ADD)) { /* If we are adding anything at all, we'll want to do * either an ADD or a MOD. */ - short new_events = ch->old_events; events = 0; op = EPOLL_CTL_ADD; if (ch->read_change & EV_CHANGE_ADD) { events |= EPOLLIN; - new_events |= EV_READ; } else if (ch->read_change & EV_CHANGE_DEL) { - new_events &= ~EV_READ; + ; } else if (ch->old_events & EV_READ) { events |= EPOLLIN; } if (ch->write_change & EV_CHANGE_ADD) { events |= EPOLLOUT; - new_events |= EV_WRITE; } else if (ch->write_change & EV_CHANGE_DEL) { - new_events &= ~EV_WRITE; + ; } else if (ch->old_events & EV_WRITE) { events |= EPOLLOUT; } if ((ch->read_change|ch->write_change) & EV_ET) events |= EPOLLET; - if (new_events == ch->old_events) { - /* - If the changelist has an "add" operation, - but no visible change to the events enabled - on the fd, we need to try the ADD anyway, in - case the fd was closed at some in the - middle. If it wasn't, the ADD operation - will fail with EEXIST; and we retry it as a - MOD. */ - op = EPOLL_CTL_ADD; - } else if (ch->old_events) { + if (ch->old_events) { + /* If MOD fails, we retry as an ADD, and if + * ADD fails we will retry as a MOD. So the + * only hard part here is to guess which one + * will work. As a heuristic, we'll try + * MOD first if we think there were old + * events and ADD if we think there were none. + * + * We can be wrong about the MOD if the file + * has in fact been closed and re-opened. + * + * We can be wrong about the ADD if the + * the fd has been re-created with a dup() + * of the same file that it was before. + */ op = EPOLL_CTL_MOD; } } else if ((ch->read_change & EV_CHANGE_DEL) ||