From: Nick Mathewson Date: Fri, 27 Jan 2012 19:30:41 +0000 (-0500) Subject: Restore fast-path event_reinit() for slower backends X-Git-Tag: release-2.1.1-alpha~119 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2c4b5de16a9b80fc564681018956f29077c28c9a;p=libevent Restore fast-path event_reinit() for slower backends We used to use the needs_reinit flag in struct eventop to indicate whether an event backend had shared state across a fork(), and therefore would require us to construct a new event backend. But when we realized that the signal notification fds and the thread notification fds would always be shared across forks, we stopped looking at it. This patch restores the old behavior so that poll, select, and win32select don't need to do a linear scan over all pending fds/signals when they do a reinit. Their life is hard enough already. --- diff --git a/event.c b/event.c index e5a2d9d2..fbea17cd 100644 --- a/event.c +++ b/event.c @@ -860,30 +860,24 @@ event_reinit(struct event_base *base) int res = 0; struct event *ev; int was_notifiable = 0; + int had_signal_added = 0; EVBASE_ACQUIRE_LOCK(base, th_base_lock); evsel = base->evsel; -#if 0 - /* Right now, reinit always takes effect, since even if the - backend doesn't require it, the signal socketpair code does. - - XXX - */ - /* check if this event mechanism requires reinit */ - if (!evsel->need_reinit) - goto done; -#endif - - /* We're going to call event_del() on our notify events (the ones that - * tell about signals and wakeup events). But we don't actually want - * to tell the backend to change its state, since it might still share - * some resource (a kqueue, an epoll fd) with the parent process, and - * we don't want to delete the fds from _that_ backend, we temporarily - * stub out the evsel with a replacement. - */ - base->evsel = &nil_eventop; + /* check if this event mechanism requires reinit on the backend */ + if (evsel->need_reinit) { + /* We're going to call event_del() on our notify events (the + * ones that tell about signals and wakeup events). But we + * don't actually want to tell the backend to change its + * state, since it might still share some resource (a kqueue, + * an epoll fd) with the parent process, and we don't want to + * delete the fds from _that_ backend, we temporarily stub out + * the evsel with a replacement. + */ + base->evsel = &nil_eventop; + } /* We need to re-create a new signal-notification fd and a new * thread-notification fd. Otherwise, we'll still share those with @@ -899,6 +893,7 @@ event_reinit(struct event_base *base) EVUTIL_CLOSESOCKET(base->sig.ev_signal_pair[0]); if (base->sig.ev_signal_pair[1] != -1) EVUTIL_CLOSESOCKET(base->sig.ev_signal_pair[1]); + had_signal_added = 1; base->sig.ev_signal_added = 0; } if (base->th_notify_fd[0] != -1) { @@ -912,37 +907,46 @@ event_reinit(struct event_base *base) event_debug_unassign(&base->th_notify); base->th_notify_fn = NULL; } + /* Replace the original evsel. */ base->evsel = evsel; - /* Reconstruct the backend through brute-force, so that we do not - * share any structures with the parent process. For some backends, - * this is necessary: epoll and kqueue, for instance, have events - * associated with a kernel structure. If didn't reinitialize, we'd - * share that structure with the parent process, and any changes made - * by the parent would affect our backend's behavior (and vice versa). - */ - if (base->evsel->dealloc != NULL) - base->evsel->dealloc(base); - base->evbase = evsel->init(base); - if (base->evbase == NULL) { - event_errx(1, "%s: could not reinitialize event mechanism", - __func__); - res = -1; - goto done; - } - - /* Empty out the changelist (if any): we are starting from a blank - * slate. */ - event_changelist_freemem(&base->changelist); + if (evsel->need_reinit) { + /* Reconstruct the backend through brute-force, so that we do + * not share any structures with the parent process. For some + * backends, this is necessary: epoll and kqueue, for + * instance, have events associated with a kernel + * structure. If didn't reinitialize, we'd share that + * structure with the parent process, and any changes made by + * the parent would affect our backend's behavior (and vice + * versa). + */ + if (base->evsel->dealloc != NULL) + base->evsel->dealloc(base); + base->evbase = evsel->init(base); + if (base->evbase == NULL) { + event_errx(1, + "%s: could not reinitialize event mechanism", + __func__); + res = -1; + goto done; + } - /* Tell the event maps to re-inform the backend about all pending - * events. This will make the signal notification event get re-created - * if necessary. */ - if (evmap_io_reinit(base) < 0) - res = -1; - if (evmap_signal_reinit(base) < 0) - res = -1; + /* Empty out the changelist (if any): we are starting from a + * blank slate. */ + event_changelist_freemem(&base->changelist); + + /* Tell the event maps to re-inform the backend about all + * pending events. This will make the signal notification + * event get re-created if necessary. */ + if (evmap_io_reinit(base) < 0) + res = -1; + if (evmap_signal_reinit(base) < 0) + res = -1; + } else { + if (had_signal_added) + res = evsig_init(base); + } /* If we were notifiable before, and nothing just exploded, become * notifiable again. */ diff --git a/signal.c b/signal.c index 391e8648..eb1fb90f 100644 --- a/signal.c +++ b/signal.c @@ -188,6 +188,9 @@ evsig_init(struct event_base *base) evutil_make_socket_closeonexec(base->sig.ev_signal_pair[0]); evutil_make_socket_closeonexec(base->sig.ev_signal_pair[1]); + if (base->sig.sh_old) { + mm_free(base->sig.sh_old); + } base->sig.sh_old = NULL; base->sig.sh_old_max = 0; @@ -330,6 +333,12 @@ _evsig_restore_handler(struct event_base *base, int evsignal) ev_sighandler_t *sh; #endif + if (evsignal >= sig->sh_old_max) { + /* Can't actually restore. */ + /* XXXX.*/ + return 0; + } + /* restore previous handler */ sh = sig->sh_old[evsignal]; sig->sh_old[evsignal] = NULL;