From e1951c16a8fbe5b0b9ecc08f4f835a806059d28f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Sekleta=CC=81r?= Date: Fri, 23 Oct 2020 18:29:27 +0200 Subject: [PATCH 1/2] sd-event: split out helper functions for reshuffling prioqs We typically don't just reshuffle a single prioq at once, but always two. Let's add two helper functions that do this, and reuse them everywhere. (Note that this drops one minor optimization: sd_event_source_set_time_accuracy() previously only reshuffled the "latest" prioq, since changing the accuracy has no effect on the earliest time of an event source, just the latest time an event source can run. This optimization is removed to simplify things, given that it's not really worth the effort as prioq_reshuffle() on properly ordered prioqs has practically zero cost O(1)). (Slightly generalized, commented and split out of #17284 by Lennart) --- src/libsystemd/sd-event/sd-event.c | 96 ++++++++++++------------------ 1 file changed, 38 insertions(+), 58 deletions(-) diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 5e770c83b7a..69032661dae 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -701,6 +701,33 @@ static void event_gc_signal_data(sd_event *e, const int64_t *priority, int sig) event_unmask_signal_data(e, d, sig); } +static void event_source_pp_prioq_reshuffle(sd_event_source *s) { + assert(s); + + /* Reshuffles the pending + prepare prioqs. Called whenever the dispatch order changes, i.e. when + * they are enabled/disabled or marked pending and such. */ + + if (s->pending) + prioq_reshuffle(s->event->pending, s, &s->pending_index); + + if (s->prepare) + prioq_reshuffle(s->event->prepare, s, &s->prepare_index); +} + +static void event_source_time_prioq_reshuffle(sd_event_source *s) { + struct clock_data *d; + + assert(s); + assert(EVENT_SOURCE_IS_TIME(s->type)); + + /* Called whenever the event source's timer ordering properties changed, i.e. time, accuracy, + * pending, enable state. Makes sure the two prioq's are ordered properly again. */ + assert_se(d = event_get_clock_data(s->event, s->type)); + prioq_reshuffle(d->earliest, s, &s->time.earliest_index); + prioq_reshuffle(d->latest, s, &s->time.latest_index); + d->needs_rearm = true; +} + static void source_disconnect(sd_event_source *s) { sd_event *event; @@ -907,16 +934,8 @@ static int source_set_pending(sd_event_source *s, bool b) { } else assert_se(prioq_remove(s->event->pending, s, &s->pending_index)); - if (EVENT_SOURCE_IS_TIME(s->type)) { - struct clock_data *d; - - d = event_get_clock_data(s->event, s->type); - assert(d); - - prioq_reshuffle(d->earliest, s, &s->time.earliest_index); - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; - } + if (EVENT_SOURCE_IS_TIME(s->type)) + event_source_time_prioq_reshuffle(s); if (s->type == SOURCE_SIGNAL && !b) { struct signal_data *d; @@ -2207,11 +2226,7 @@ _public_ int sd_event_source_set_priority(sd_event_source *s, int64_t priority) } else s->priority = priority; - if (s->pending) - prioq_reshuffle(s->event->pending, s, &s->pending_index); - - if (s->prepare) - prioq_reshuffle(s->event->prepare, s, &s->prepare_index); + event_source_pp_prioq_reshuffle(s); if (s->type == SOURCE_EXIT) prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index); @@ -2272,18 +2287,10 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { case SOURCE_TIME_BOOTTIME: case SOURCE_TIME_MONOTONIC: case SOURCE_TIME_REALTIME_ALARM: - case SOURCE_TIME_BOOTTIME_ALARM: { - struct clock_data *d; - + case SOURCE_TIME_BOOTTIME_ALARM: s->enabled = m; - d = event_get_clock_data(s->event, s->type); - assert(d); - - prioq_reshuffle(d->earliest, s, &s->time.earliest_index); - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; + event_source_time_prioq_reshuffle(s); break; - } case SOURCE_SIGNAL: s->enabled = m; @@ -2342,18 +2349,10 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { case SOURCE_TIME_BOOTTIME: case SOURCE_TIME_MONOTONIC: case SOURCE_TIME_REALTIME_ALARM: - case SOURCE_TIME_BOOTTIME_ALARM: { - struct clock_data *d; - + case SOURCE_TIME_BOOTTIME_ALARM: s->enabled = m; - d = event_get_clock_data(s->event, s->type); - assert(d); - - prioq_reshuffle(d->earliest, s, &s->time.earliest_index); - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; + event_source_time_prioq_reshuffle(s); break; - } case SOURCE_SIGNAL: @@ -2414,11 +2413,7 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { } } - if (s->pending) - prioq_reshuffle(s->event->pending, s, &s->pending_index); - - if (s->prepare) - prioq_reshuffle(s->event->prepare, s, &s->prepare_index); + event_source_pp_prioq_reshuffle(s); return 0; } @@ -2434,7 +2429,6 @@ _public_ int sd_event_source_get_time(sd_event_source *s, uint64_t *usec) { } _public_ int sd_event_source_set_time(sd_event_source *s, uint64_t usec) { - struct clock_data *d; int r; assert_return(s, -EINVAL); @@ -2448,13 +2442,7 @@ _public_ int sd_event_source_set_time(sd_event_source *s, uint64_t usec) { s->time.next = usec; - d = event_get_clock_data(s->event, s->type); - assert(d); - - prioq_reshuffle(d->earliest, s, &s->time.earliest_index); - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; - + event_source_time_prioq_reshuffle(s); return 0; } @@ -2486,7 +2474,6 @@ _public_ int sd_event_source_get_time_accuracy(sd_event_source *s, uint64_t *use } _public_ int sd_event_source_set_time_accuracy(sd_event_source *s, uint64_t usec) { - struct clock_data *d; int r; assert_return(s, -EINVAL); @@ -2504,12 +2491,7 @@ _public_ int sd_event_source_set_time_accuracy(sd_event_source *s, uint64_t usec s->time.accuracy = usec; - d = event_get_clock_data(s->event, s->type); - assert(d); - - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; - + event_source_time_prioq_reshuffle(s); return 0; } @@ -2888,9 +2870,7 @@ static int process_timer( if (r < 0) return r; - prioq_reshuffle(d->earliest, s, &s->time.earliest_index); - prioq_reshuffle(d->latest, s, &s->time.latest_index); - d->needs_rearm = true; + event_source_time_prioq_reshuffle(s); } return 0; From ddfde737b546c17e54182028153aa7f7e78804e3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Oct 2020 21:21:58 +0200 Subject: [PATCH 2/2] sd-event: split out enable and disable codepaths from sd_event_source_set_enabled() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So far half of sd_event_source_set_enabled() was doing enabling, the other half was doing disabling. Let's split that into two separate calls. (This also adds a new shortcut to sd_event_source_set_enabled(): if the caller toggles between "ON" and "ONESHOT" we'll now shortcut this, since the event source is already enabled in that case and shall remain enabled.) This heavily borrows and is inspired from Michal Sekletár's #17284 refactoring. --- src/libsystemd/sd-event/sd-event.c | 305 +++++++++++++++-------------- 1 file changed, 159 insertions(+), 146 deletions(-) diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 69032661dae..50ee29ae979 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -2252,6 +2252,152 @@ _public_ int sd_event_source_get_enabled(sd_event_source *s, int *m) { return s->enabled != SD_EVENT_OFF; } +static int event_source_disable(sd_event_source *s) { + int r; + + assert(s); + assert(s->enabled != SD_EVENT_OFF); + + /* Unset the pending flag when this event source is disabled */ + if (!IN_SET(s->type, SOURCE_DEFER, SOURCE_EXIT)) { + r = source_set_pending(s, false); + if (r < 0) + return r; + } + + s->enabled = SD_EVENT_OFF; + + switch (s->type) { + + case SOURCE_IO: + source_io_unregister(s); + break; + + case SOURCE_TIME_REALTIME: + case SOURCE_TIME_BOOTTIME: + case SOURCE_TIME_MONOTONIC: + case SOURCE_TIME_REALTIME_ALARM: + case SOURCE_TIME_BOOTTIME_ALARM: + event_source_time_prioq_reshuffle(s); + break; + + case SOURCE_SIGNAL: + event_gc_signal_data(s->event, &s->priority, s->signal.sig); + break; + + case SOURCE_CHILD: + assert(s->event->n_enabled_child_sources > 0); + s->event->n_enabled_child_sources--; + + if (EVENT_SOURCE_WATCH_PIDFD(s)) + source_child_pidfd_unregister(s); + else + event_gc_signal_data(s->event, &s->priority, SIGCHLD); + break; + + case SOURCE_EXIT: + prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index); + break; + + case SOURCE_DEFER: + case SOURCE_POST: + case SOURCE_INOTIFY: + break; + + default: + assert_not_reached("Wut? I shouldn't exist."); + } + + return 0; +} + +static int event_source_enable(sd_event_source *s, int m) { + int r; + + assert(s); + assert(IN_SET(m, SD_EVENT_ON, SD_EVENT_ONESHOT)); + assert(s->enabled == SD_EVENT_OFF); + + /* Unset the pending flag when this event source is enabled */ + if (!IN_SET(s->type, SOURCE_DEFER, SOURCE_EXIT)) { + r = source_set_pending(s, false); + if (r < 0) + return r; + } + + s->enabled = m; + + switch (s->type) { + + case SOURCE_IO: + r = source_io_register(s, m, s->io.events); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + return r; + } + + break; + + case SOURCE_TIME_REALTIME: + case SOURCE_TIME_BOOTTIME: + case SOURCE_TIME_MONOTONIC: + case SOURCE_TIME_REALTIME_ALARM: + case SOURCE_TIME_BOOTTIME_ALARM: + event_source_time_prioq_reshuffle(s); + break; + + case SOURCE_SIGNAL: + r = event_make_signal_data(s->event, s->signal.sig, NULL); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + event_gc_signal_data(s->event, &s->priority, s->signal.sig); + return r; + } + + break; + + case SOURCE_CHILD: + s->event->n_enabled_child_sources++; + + if (EVENT_SOURCE_WATCH_PIDFD(s)) { + /* yes, we have pidfd */ + + r = source_child_pidfd_register(s, s->enabled); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + s->event->n_enabled_child_sources--; + return r; + } + } else { + /* no pidfd, or something other to watch for than WEXITED */ + + r = event_make_signal_data(s->event, SIGCHLD, NULL); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + s->event->n_enabled_child_sources--; + event_gc_signal_data(s->event, &s->priority, SIGCHLD); + return r; + } + } + + break; + + case SOURCE_EXIT: + prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index); + break; + + case SOURCE_DEFER: + case SOURCE_POST: + case SOURCE_INOTIFY: + break; + + default: + assert_not_reached("Wut? I shouldn't exist."); + } + + return 0; +} + _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { int r; @@ -2259,162 +2405,29 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { assert_return(IN_SET(m, SD_EVENT_OFF, SD_EVENT_ON, SD_EVENT_ONESHOT), -EINVAL); assert_return(!event_pid_changed(s->event), -ECHILD); - /* If we are dead anyway, we are fine with turning off - * sources, but everything else needs to fail. */ + /* If we are dead anyway, we are fine with turning off sources, but everything else needs to fail. */ if (s->event->state == SD_EVENT_FINISHED) return m == SD_EVENT_OFF ? 0 : -ESTALE; - if (s->enabled == m) + if (s->enabled == m) /* No change? */ return 0; - if (m == SD_EVENT_OFF) { - - /* Unset the pending flag when this event source is disabled */ - if (!IN_SET(s->type, SOURCE_DEFER, SOURCE_EXIT)) { - r = source_set_pending(s, false); - if (r < 0) - return r; + if (m == SD_EVENT_OFF) + r = event_source_disable(s); + else { + if (s->enabled != SD_EVENT_OFF) { + /* Switching from "on" to "oneshot" or back? If that's the case, we can take a shortcut, the + * event source is already enabled after all. */ + s->enabled = m; + return 0; } - switch (s->type) { - - case SOURCE_IO: - source_io_unregister(s); - s->enabled = m; - break; - - case SOURCE_TIME_REALTIME: - case SOURCE_TIME_BOOTTIME: - case SOURCE_TIME_MONOTONIC: - case SOURCE_TIME_REALTIME_ALARM: - case SOURCE_TIME_BOOTTIME_ALARM: - s->enabled = m; - event_source_time_prioq_reshuffle(s); - break; - - case SOURCE_SIGNAL: - s->enabled = m; - - event_gc_signal_data(s->event, &s->priority, s->signal.sig); - break; - - case SOURCE_CHILD: - s->enabled = m; - - assert(s->event->n_enabled_child_sources > 0); - s->event->n_enabled_child_sources--; - - if (EVENT_SOURCE_WATCH_PIDFD(s)) - source_child_pidfd_unregister(s); - else - event_gc_signal_data(s->event, &s->priority, SIGCHLD); - - break; - - case SOURCE_EXIT: - s->enabled = m; - prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index); - break; - - case SOURCE_DEFER: - case SOURCE_POST: - case SOURCE_INOTIFY: - s->enabled = m; - break; - - default: - assert_not_reached("Wut? I shouldn't exist."); - } - - } else { - - /* Unset the pending flag when this event source is enabled */ - if (s->enabled == SD_EVENT_OFF && !IN_SET(s->type, SOURCE_DEFER, SOURCE_EXIT)) { - r = source_set_pending(s, false); - if (r < 0) - return r; - } - - switch (s->type) { - - case SOURCE_IO: - r = source_io_register(s, m, s->io.events); - if (r < 0) - return r; - - s->enabled = m; - break; - - case SOURCE_TIME_REALTIME: - case SOURCE_TIME_BOOTTIME: - case SOURCE_TIME_MONOTONIC: - case SOURCE_TIME_REALTIME_ALARM: - case SOURCE_TIME_BOOTTIME_ALARM: - s->enabled = m; - event_source_time_prioq_reshuffle(s); - break; - - case SOURCE_SIGNAL: - - s->enabled = m; - - r = event_make_signal_data(s->event, s->signal.sig, NULL); - if (r < 0) { - s->enabled = SD_EVENT_OFF; - event_gc_signal_data(s->event, &s->priority, s->signal.sig); - return r; - } - - break; - - case SOURCE_CHILD: - - if (s->enabled == SD_EVENT_OFF) - s->event->n_enabled_child_sources++; - - s->enabled = m; - - if (EVENT_SOURCE_WATCH_PIDFD(s)) { - /* yes, we have pidfd */ - - r = source_child_pidfd_register(s, s->enabled); - if (r < 0) { - s->enabled = SD_EVENT_OFF; - s->event->n_enabled_child_sources--; - return r; - } - } else { - /* no pidfd, or something other to watch for than WEXITED */ - - r = event_make_signal_data(s->event, SIGCHLD, NULL); - if (r < 0) { - s->enabled = SD_EVENT_OFF; - s->event->n_enabled_child_sources--; - event_gc_signal_data(s->event, &s->priority, SIGCHLD); - return r; - } - } - - break; - - case SOURCE_EXIT: - s->enabled = m; - prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index); - break; - - case SOURCE_DEFER: - case SOURCE_POST: - case SOURCE_INOTIFY: - s->enabled = m; - break; - - default: - assert_not_reached("Wut? I shouldn't exist."); - } + r = event_source_enable(s, m); } + if (r < 0) + return r; event_source_pp_prioq_reshuffle(s); - return 0; }