From d42b61d279afbc82a73adee6e2666107cae36e80 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Jan 2024 13:50:17 +0100 Subject: [PATCH 1/5] core: maintain a single table with event source priorities It's hard to oversee the assigned processing priorities of the various event sources we have. Let's unify them in a table (an enum), where we can have a single consisten look at them, and then reference the table entries by expressive symbols. This doesn#t change behaviour in any way, it just gives each priority a nice label, but doesn't change any of the priorities. Prompted by: #30799 --- src/core/cgroup.c | 6 +++--- src/core/core-varlink.c | 4 ++-- src/core/dbus.c | 6 +++--- src/core/manager.c | 16 ++++++++-------- src/core/manager.h | 22 ++++++++++++++++++++++ src/core/mount.c | 2 +- src/core/service.c | 4 ++-- src/core/swap.c | 2 +- src/core/unit.c | 2 +- 9 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index d5df93940b..e7585c3c01 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3858,7 +3858,7 @@ static void unit_add_to_cgroup_oom_queue(Unit *u) { return; } - r = sd_event_source_set_priority(s, SD_EVENT_PRIORITY_NORMAL-8); + r = sd_event_source_set_priority(s, EVENT_PRIORITY_CGROUP_OOM); if (r < 0) { log_error_errno(r, "Failed to set priority of cgroup oom event source: %m"); return; @@ -4064,7 +4064,7 @@ int manager_setup_cgroup(Manager *m) { /* Schedule cgroup empty checks early, but after having processed service notification messages or * SIGCHLD signals, so that a cgroup running empty is always just the last safety net of * notification, and we collected the metadata the notification and SIGCHLD stuff offers first. */ - r = sd_event_source_set_priority(m->cgroup_empty_event_source, SD_EVENT_PRIORITY_NORMAL-5); + r = sd_event_source_set_priority(m->cgroup_empty_event_source, EVENT_PRIORITY_CGROUP_EMPTY); if (r < 0) return log_error_errno(r, "Failed to set priority of cgroup empty event source: %m"); @@ -4093,7 +4093,7 @@ int manager_setup_cgroup(Manager *m) { /* Process cgroup empty notifications early. Note that when this event is dispatched it'll * just add the unit to a cgroup empty queue, hence let's run earlier than that. Also see * handling of cgroup agent notifications, for the classic cgroup hierarchy support. */ - r = sd_event_source_set_priority(m->cgroup_inotify_event_source, SD_EVENT_PRIORITY_NORMAL-9); + r = sd_event_source_set_priority(m->cgroup_inotify_event_source, EVENT_PRIORITY_CGROUP_INOTIFY); if (r < 0) return log_error_errno(r, "Failed to set priority of inotify event source: %m"); diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index cd913817d2..1065042106 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -527,7 +527,7 @@ static int manager_varlink_init_system(Manager *m) { } } - r = varlink_server_attach_event(s, m->event, SD_EVENT_PRIORITY_NORMAL); + r = varlink_server_attach_event(s, m->event, EVENT_PRIORITY_IPC); if (r < 0) return log_error_errno(r, "Failed to attach varlink connection to event loop: %m"); @@ -585,7 +585,7 @@ static int manager_varlink_init_user(Manager *m) { if (r < 0) return r; - r = varlink_attach_event(link, m->event, SD_EVENT_PRIORITY_NORMAL); + r = varlink_attach_event(link, m->event, EVENT_PRIORITY_IPC); if (r < 0) return log_error_errno(r, "Failed to attach varlink connection to event loop: %m"); diff --git a/src/core/dbus.c b/src/core/dbus.c index e24c5bbc53..49a80bbcfa 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -739,7 +739,7 @@ static int bus_on_connection(sd_event_source *s, int fd, uint32_t revents, void log_debug("Accepting direct incoming connection from " PID_FMT " (%s) [%s]", pid, strna(comm), strna(description)); } - r = sd_bus_attach_event(bus, m->event, SD_EVENT_PRIORITY_NORMAL); + r = sd_bus_attach_event(bus, m->event, EVENT_PRIORITY_IPC); if (r < 0) { log_warning_errno(r, "Failed to attach new connection bus to event loop: %m"); return 0; @@ -847,7 +847,7 @@ int bus_init_api(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to connect to API bus: %m"); - r = sd_bus_attach_event(bus, m->event, SD_EVENT_PRIORITY_NORMAL); + r = sd_bus_attach_event(bus, m->event, EVENT_PRIORITY_IPC); if (r < 0) return log_error_errno(r, "Failed to attach API bus to event loop: %m"); @@ -904,7 +904,7 @@ int bus_init_system(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to connect to system bus: %m"); - r = sd_bus_attach_event(bus, m->event, SD_EVENT_PRIORITY_NORMAL); + r = sd_bus_attach_event(bus, m->event, EVENT_PRIORITY_IPC); if (r < 0) return log_error_errno(r, "Failed to attach system bus to event loop: %m"); diff --git a/src/core/manager.c b/src/core/manager.c index 95c5f6381d..e052e36cee 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -397,7 +397,7 @@ static int manager_setup_time_change(Manager *m) { return log_error_errno(r, "Failed to create time change event source: %m"); /* Schedule this slightly earlier than the .timer event sources */ - r = sd_event_source_set_priority(m->time_change_event_source, SD_EVENT_PRIORITY_NORMAL-1); + r = sd_event_source_set_priority(m->time_change_event_source, EVENT_PRIORITY_TIME_CHANGE); if (r < 0) return log_error_errno(r, "Failed to set priority of time change event sources: %m"); @@ -464,7 +464,7 @@ static int manager_setup_timezone_change(Manager *m) { return log_error_errno(r, "Failed to create timezone change event source: %m"); /* Schedule this slightly earlier than the .timer event sources */ - r = sd_event_source_set_priority(new_event, SD_EVENT_PRIORITY_NORMAL-1); + r = sd_event_source_set_priority(new_event, EVENT_PRIORITY_TIME_ZONE); if (r < 0) return log_error_errno(r, "Failed to set priority of timezone change event sources: %m"); @@ -592,7 +592,7 @@ static int manager_setup_signals(Manager *m) { * notify processing can still figure out to which process/service a message belongs, before we reap the * process. Also, process this before handling cgroup notifications, so that we always collect child exit * status information before detecting that there's no process in a cgroup. */ - r = sd_event_source_set_priority(m->signal_event_source, SD_EVENT_PRIORITY_NORMAL-6); + r = sd_event_source_set_priority(m->signal_event_source, EVENT_PRIORITY_SIGNALS); if (r < 0) return r; @@ -736,7 +736,7 @@ static int manager_setup_run_queue(Manager *m) { if (r < 0) return r; - r = sd_event_source_set_priority(m->run_queue_event_source, SD_EVENT_PRIORITY_IDLE); + r = sd_event_source_set_priority(m->run_queue_event_source, EVENT_PRIORITY_RUN_QUEUE); if (r < 0) return r; @@ -759,7 +759,7 @@ static int manager_setup_sigchld_event_source(Manager *m) { if (r < 0) return r; - r = sd_event_source_set_priority(m->sigchld_event_source, SD_EVENT_PRIORITY_NORMAL-7); + r = sd_event_source_set_priority(m->sigchld_event_source, EVENT_PRIORITY_SIGCHLD); if (r < 0) return r; @@ -1113,7 +1113,7 @@ static int manager_setup_notify(Manager *m) { /* Process notification messages a bit earlier than SIGCHLD, so that we can still identify to which * service an exit message belongs. */ - r = sd_event_source_set_priority(m->notify_event_source, SD_EVENT_PRIORITY_NORMAL-8); + r = sd_event_source_set_priority(m->notify_event_source, EVENT_PRIORITY_NOTIFY); if (r < 0) return log_error_errno(r, "Failed to set priority of notify event source: %m"); @@ -1187,7 +1187,7 @@ static int manager_setup_cgroups_agent(Manager *m) { /* Process cgroups notifications early. Note that when the agent notification is received * we'll just enqueue the unit in the cgroup empty queue, hence pick a high priority than * that. Also see handling of cgroup inotify for the unified cgroup stuff. */ - r = sd_event_source_set_priority(m->cgroups_agent_event_source, SD_EVENT_PRIORITY_NORMAL-9); + r = sd_event_source_set_priority(m->cgroups_agent_event_source, EVENT_PRIORITY_CGROUP_AGENT); if (r < 0) return log_error_errno(r, "Failed to set priority of cgroups agent event source: %m"); @@ -1240,7 +1240,7 @@ static int manager_setup_user_lookup_fd(Manager *m) { /* Process even earlier than the notify event source, so that we always know first about valid UID/GID * resolutions */ - r = sd_event_source_set_priority(m->user_lookup_event_source, SD_EVENT_PRIORITY_NORMAL-11); + r = sd_event_source_set_priority(m->user_lookup_event_source, EVENT_PRIORITY_USER_LOOKUP); if (r < 0) return log_error_errno(errno, "Failed to set priority of user lookup event source: %m"); diff --git a/src/core/manager.h b/src/core/manager.h index d7bc6e761d..cfcf1929f5 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -644,3 +644,25 @@ OOMPolicy oom_policy_from_string(const char *s) _pure_; void unit_defaults_init(UnitDefaults *defaults, RuntimeScope scope); void unit_defaults_done(UnitDefaults *defaults); + +enum { + /* most important … */ + EVENT_PRIORITY_USER_LOOKUP = SD_EVENT_PRIORITY_NORMAL-11, + EVENT_PRIORITY_MOUNT_TABLE = SD_EVENT_PRIORITY_NORMAL-10, + EVENT_PRIORITY_SWAP_TABLE = SD_EVENT_PRIORITY_NORMAL-10, + EVENT_PRIORITY_CGROUP_AGENT = SD_EVENT_PRIORITY_NORMAL-9, + EVENT_PRIORITY_CGROUP_INOTIFY = SD_EVENT_PRIORITY_NORMAL-9, + EVENT_PRIORITY_CGROUP_OOM = SD_EVENT_PRIORITY_NORMAL-8, + EVENT_PRIORITY_NOTIFY = SD_EVENT_PRIORITY_NORMAL-8, + EVENT_PRIORITY_SIGCHLD = SD_EVENT_PRIORITY_NORMAL-7, + EVENT_PRIORITY_SIGNALS = SD_EVENT_PRIORITY_NORMAL-6, + EVENT_PRIORITY_CGROUP_EMPTY = SD_EVENT_PRIORITY_NORMAL-5, + EVENT_PRIORITY_EXEC_FD = SD_EVENT_PRIORITY_NORMAL-3, + EVENT_PRIORITY_TIME_CHANGE = SD_EVENT_PRIORITY_NORMAL-1, + EVENT_PRIORITY_TIME_ZONE = SD_EVENT_PRIORITY_NORMAL-1, + EVENT_PRIORITY_IPC = SD_EVENT_PRIORITY_NORMAL, + EVENT_PRIORITY_REWATCH_PIDS = SD_EVENT_PRIORITY_IDLE, + EVENT_PRIORITY_RUN_QUEUE = SD_EVENT_PRIORITY_IDLE, + EVENT_PRIORITY_SERVICE_WATCHDOG = SD_EVENT_PRIORITY_IDLE, + /* … to least important */ +}; diff --git a/src/core/mount.c b/src/core/mount.c index ff97eebb59..048802fa1b 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -2090,7 +2090,7 @@ static void mount_enumerate(Manager *m) { goto fail; } - r = sd_event_source_set_priority(m->mount_event_source, SD_EVENT_PRIORITY_NORMAL-10); + r = sd_event_source_set_priority(m->mount_event_source, EVENT_PRIORITY_MOUNT_TABLE); if (r < 0) { log_error_errno(r, "Failed to adjust mount watch priority: %m"); goto fail; diff --git a/src/core/service.c b/src/core/service.c index a4c9f80e04..79859a2a1e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -291,7 +291,7 @@ static void service_start_watchdog(Service *s) { /* Let's process everything else which might be a sign * of living before we consider a service died. */ - r = sd_event_source_set_priority(s->watchdog_event_source, SD_EVENT_PRIORITY_IDLE); + r = sd_event_source_set_priority(s->watchdog_event_source, EVENT_PRIORITY_SERVICE_WATCHDOG); } if (r < 0) log_unit_warning_errno(UNIT(s), r, "Failed to install watchdog timer: %m"); @@ -1514,7 +1514,7 @@ static int service_allocate_exec_fd_event_source( /* This is a bit lower priority than SIGCHLD, as that carries a lot more interesting failure information */ - r = sd_event_source_set_priority(source, SD_EVENT_PRIORITY_NORMAL-3); + r = sd_event_source_set_priority(source, EVENT_PRIORITY_EXEC_FD); if (r < 0) return log_unit_error_errno(UNIT(s), r, "Failed to adjust priority of exec_fd event source: %m"); diff --git a/src/core/swap.c b/src/core/swap.c index ce35a5c441..9f7c12b0c3 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1358,7 +1358,7 @@ static void swap_enumerate(Manager *m) { /* Dispatch this before we dispatch SIGCHLD, so that * we always get the events from /proc/swaps before * the SIGCHLD of /sbin/swapon. */ - r = sd_event_source_set_priority(m->swap_event_source, SD_EVENT_PRIORITY_NORMAL-10); + r = sd_event_source_set_priority(m->swap_event_source, EVENT_PRIORITY_SWAP_TABLE); if (r < 0) { log_error_errno(r, "Failed to change /proc/swaps priority: %m"); goto fail; diff --git a/src/core/unit.c b/src/core/unit.c index 49b45b2140..d9e6706742 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2992,7 +2992,7 @@ int unit_enqueue_rewatch_pids(Unit *u) { if (r < 0) return log_error_errno(r, "Failed to allocate event source for tidying watched PIDs: %m"); - r = sd_event_source_set_priority(s, SD_EVENT_PRIORITY_IDLE); + r = sd_event_source_set_priority(s, EVENT_PRIORITY_REWATCH_PIDS); if (r < 0) return log_error_errno(r, "Failed to adjust priority of event source for tidying watched PIDs: %m"); From f3a269a9354261d55754963d568112b6ab94ba81 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Jan 2024 14:03:55 +0100 Subject: [PATCH 2/5] manager: process exec_fd (i.e. Type=exec) events before SIGCHLD events We want to make sure we don't confuse the case "process started successfully but then failed quickly" from the case "process failed to start". Hence we need to make sure we take notice of Type=exec before we bother with SIGCHLD. Hence move EVENT_PRIORITY_EXEC_FD to the front. In fact, let's move it even further up than SIGCHLD, i.e. before sd_notify() handling, so that we don't end up processing service state change notifications before we even considered that the service is properly started. This also gives the cgroup OOM handling and the exec_fd handling different priorities, to improve robustness of the system, we should act quickly on OOM, and it doesn't matter if a service started succcessfully if we have to act on OOM anyway. This is based on Andrew Onyshchuk work here: See: #30799 Fixes: #28304 --- src/core/manager.h | 10 +++++----- src/core/service.c | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/manager.h b/src/core/manager.h index cfcf1929f5..4193c10e4b 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -653,11 +653,11 @@ enum { EVENT_PRIORITY_CGROUP_AGENT = SD_EVENT_PRIORITY_NORMAL-9, EVENT_PRIORITY_CGROUP_INOTIFY = SD_EVENT_PRIORITY_NORMAL-9, EVENT_PRIORITY_CGROUP_OOM = SD_EVENT_PRIORITY_NORMAL-8, - EVENT_PRIORITY_NOTIFY = SD_EVENT_PRIORITY_NORMAL-8, - EVENT_PRIORITY_SIGCHLD = SD_EVENT_PRIORITY_NORMAL-7, - EVENT_PRIORITY_SIGNALS = SD_EVENT_PRIORITY_NORMAL-6, - EVENT_PRIORITY_CGROUP_EMPTY = SD_EVENT_PRIORITY_NORMAL-5, - EVENT_PRIORITY_EXEC_FD = SD_EVENT_PRIORITY_NORMAL-3, + EVENT_PRIORITY_EXEC_FD = SD_EVENT_PRIORITY_NORMAL-6, + EVENT_PRIORITY_NOTIFY = SD_EVENT_PRIORITY_NORMAL-5, + EVENT_PRIORITY_SIGCHLD = SD_EVENT_PRIORITY_NORMAL-4, + EVENT_PRIORITY_SIGNALS = SD_EVENT_PRIORITY_NORMAL-3, + EVENT_PRIORITY_CGROUP_EMPTY = SD_EVENT_PRIORITY_NORMAL-2, EVENT_PRIORITY_TIME_CHANGE = SD_EVENT_PRIORITY_NORMAL-1, EVENT_PRIORITY_TIME_ZONE = SD_EVENT_PRIORITY_NORMAL-1, EVENT_PRIORITY_IPC = SD_EVENT_PRIORITY_NORMAL, diff --git a/src/core/service.c b/src/core/service.c index 79859a2a1e..aacfe0d57e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1512,7 +1512,8 @@ static int service_allocate_exec_fd_event_source( if (r < 0) return log_unit_error_errno(UNIT(s), r, "Failed to allocate exec_fd event source: %m"); - /* This is a bit lower priority than SIGCHLD, as that carries a lot more interesting failure information */ + /* This is a bit higher priority than SIGCHLD, to make sure we don't confuse the case "failed to + * start" from the case "succeeded to start, but failed immediately after". */ r = sd_event_source_set_priority(source, EVENT_PRIORITY_EXEC_FD); if (r < 0) From 90c40ea4bff0259fda98295b2e9e9f329b38f6b4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Jan 2024 14:08:38 +0100 Subject: [PATCH 3/5] manager: renumber priorities no actual change, let's just make sure the priorities are densely defined without "holes". Just to make this prettier and less surprising. --- src/core/manager.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/manager.h b/src/core/manager.h index 4193c10e4b..8962c5c66e 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -647,12 +647,12 @@ void unit_defaults_done(UnitDefaults *defaults); enum { /* most important … */ - EVENT_PRIORITY_USER_LOOKUP = SD_EVENT_PRIORITY_NORMAL-11, - EVENT_PRIORITY_MOUNT_TABLE = SD_EVENT_PRIORITY_NORMAL-10, - EVENT_PRIORITY_SWAP_TABLE = SD_EVENT_PRIORITY_NORMAL-10, - EVENT_PRIORITY_CGROUP_AGENT = SD_EVENT_PRIORITY_NORMAL-9, - EVENT_PRIORITY_CGROUP_INOTIFY = SD_EVENT_PRIORITY_NORMAL-9, - EVENT_PRIORITY_CGROUP_OOM = SD_EVENT_PRIORITY_NORMAL-8, + EVENT_PRIORITY_USER_LOOKUP = SD_EVENT_PRIORITY_NORMAL-10, + EVENT_PRIORITY_MOUNT_TABLE = SD_EVENT_PRIORITY_NORMAL-9, + EVENT_PRIORITY_SWAP_TABLE = SD_EVENT_PRIORITY_NORMAL-9, + EVENT_PRIORITY_CGROUP_AGENT = SD_EVENT_PRIORITY_NORMAL-8, /* cgroupv1 */ + EVENT_PRIORITY_CGROUP_INOTIFY = SD_EVENT_PRIORITY_NORMAL-8, /* cgroupv2 */ + EVENT_PRIORITY_CGROUP_OOM = SD_EVENT_PRIORITY_NORMAL-7, EVENT_PRIORITY_EXEC_FD = SD_EVENT_PRIORITY_NORMAL-6, EVENT_PRIORITY_NOTIFY = SD_EVENT_PRIORITY_NORMAL-5, EVENT_PRIORITY_SIGCHLD = SD_EVENT_PRIORITY_NORMAL-4, From 8a69bf0c57ebd99aa225d0a6555df03c0d2bc42a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Jan 2024 14:09:42 +0100 Subject: [PATCH 4/5] manager: order idle jobs Let's put the run queue really the last spot, as we should only start doing more work if we really have nothing else to do anymore. Let's move the service watchdog after the rewatch PID logic for similar logic: it will possibly result in new jobs being enqueued to stop things, and we should really have done all other work first. --- src/core/manager.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/manager.h b/src/core/manager.h index 8962c5c66e..ba7c1e28c9 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -662,7 +662,7 @@ enum { EVENT_PRIORITY_TIME_ZONE = SD_EVENT_PRIORITY_NORMAL-1, EVENT_PRIORITY_IPC = SD_EVENT_PRIORITY_NORMAL, EVENT_PRIORITY_REWATCH_PIDS = SD_EVENT_PRIORITY_IDLE, - EVENT_PRIORITY_RUN_QUEUE = SD_EVENT_PRIORITY_IDLE, - EVENT_PRIORITY_SERVICE_WATCHDOG = SD_EVENT_PRIORITY_IDLE, + EVENT_PRIORITY_SERVICE_WATCHDOG = SD_EVENT_PRIORITY_IDLE+1, + EVENT_PRIORITY_RUN_QUEUE = SD_EVENT_PRIORITY_IDLE+2, /* … to least important */ }; From 9d2a2922462a21b0edfc85ed605eeb325c51ca21 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Jan 2024 14:36:13 +0100 Subject: [PATCH 5/5] test: add integration test for Type=exec robustness This tests for #30799, so that it does not return. --- test/units/testsuite-07.type-exec-parallel.sh | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100755 test/units/testsuite-07.type-exec-parallel.sh diff --git a/test/units/testsuite-07.type-exec-parallel.sh b/test/units/testsuite-07.type-exec-parallel.sh new file mode 100755 index 0000000000..30f80c539b --- /dev/null +++ b/test/units/testsuite-07.type-exec-parallel.sh @@ -0,0 +1,9 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +# Make sure that we never mistake a process starting but failing quickly for a process failing to start, with Type=exec. +# See https://github.com/systemd/systemd/pull/30799 + +seq 25 | xargs -n 1 -P 0 systemd-run -p Type=exec /bin/false