From 4d05154600f64e9d5c9800ff9f61167fe80a08ba Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Mon, 18 Mar 2019 11:48:34 +0100 Subject: [PATCH 1/4] process-util: introduce pid_is_my_child() helper No functional changes. --- src/basic/process-util.c | 14 ++++++++++++++ src/basic/process-util.h | 1 + src/core/cgroup.c | 7 ++----- src/core/service.c | 8 ++------ 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index c41a2aa5c9..98ab8b005c 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -934,6 +934,20 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) { return 0; } +int pid_is_my_child(pid_t pid) { + pid_t ppid; + int r; + + if (pid <= 1) + return false; + + r = get_process_ppid(pid, &ppid); + if (r < 0) + return r; + + return ppid == getpid_cached(); +} + bool pid_is_unwaited(pid_t pid) { /* Checks whether a PID is still valid at all, including a zombie */ diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 9950723996..1571f1a652 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -69,6 +69,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value); bool pid_is_alive(pid_t pid); bool pid_is_unwaited(pid_t pid); +int pid_is_my_child(pid_t pid); int pid_from_same_root_fs(pid_t pid); bool is_main_thread(void); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 059bf29eb9..8db044befa 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2269,7 +2269,7 @@ void unit_prune_cgroup(Unit *u) { int unit_search_main_pid(Unit *u, pid_t *ret) { _cleanup_fclose_ FILE *f = NULL; - pid_t pid = 0, npid, mypid; + pid_t pid = 0, npid; int r; assert(u); @@ -2282,15 +2282,12 @@ int unit_search_main_pid(Unit *u, pid_t *ret) { if (r < 0) return r; - mypid = getpid_cached(); while (cg_read_pid(f, &npid) > 0) { - pid_t ppid; if (npid == pid) continue; - /* Ignore processes that aren't our kids */ - if (get_process_ppid(npid, &ppid) >= 0 && ppid != mypid) + if (pid_is_my_child(npid) == 0) continue; if (pid != 0) diff --git a/src/core/service.c b/src/core/service.c index bbe5532209..4682e86f8e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -143,8 +143,6 @@ static void service_unwatch_pid_file(Service *s) { } static int service_set_main_pid(Service *s, pid_t pid) { - pid_t ppid; - assert(s); if (pid <= 1) @@ -163,12 +161,10 @@ static int service_set_main_pid(Service *s, pid_t pid) { s->main_pid = pid; s->main_pid_known = true; + s->main_pid_alien = pid_is_my_child(pid) == 0; - if (get_process_ppid(pid, &ppid) >= 0 && ppid != getpid_cached()) { + if (s->main_pid_alien) log_unit_warning(UNIT(s), "Supervising process "PID_FMT" which is not our child. We'll most likely not notice when it exits.", pid); - s->main_pid_alien = true; - } else - s->main_pid_alien = false; return 0; } From f75f613d259e9332be8e9657fa37e73f7aabcb8a Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Mon, 18 Mar 2019 20:59:36 +0100 Subject: [PATCH 2/4] core: reduce the number of stalled PIDs from the watched processes list when possible Some PIDs can remain in the watched list even though their processes have exited since a long time. It can easily happen if the main process of a forking service manages to spawn a child before the control process exits for example. However when a pid is about to be mapped to a unit by calling unit_watch_pid(), the caller usually knows if the pid should belong to this unit exclusively: if we just forked() off a child, then we can be sure that its PID is otherwise unused. In this case we take this opportunity to remove any stalled PIDs from the watched process list. If we learnt about a PID in any other form (for example via PID file, via searching, MAINPID= and so on), then we can't assume anything. --- src/core/cgroup.c | 2 +- src/core/dbus-scope.c | 2 +- src/core/manager.c | 10 ++++++++++ src/core/manager.h | 2 ++ src/core/mount.c | 5 ++--- src/core/service.c | 16 ++++++++-------- src/core/socket.c | 7 +++---- src/core/swap.c | 5 ++--- src/core/unit.c | 8 +++++++- src/core/unit.h | 2 +- src/test/test-watch-pid.c | 12 ++++++------ 11 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 8db044befa..ad67ba0438 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2319,7 +2319,7 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) { pid_t pid; while ((r = cg_read_pid(f, &pid)) > 0) { - r = unit_watch_pid(u, pid); + r = unit_watch_pid(u, pid, false); if (r < 0 && ret >= 0) ret = r; } diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index bb807df2e9..8eb915e508 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -106,7 +106,7 @@ static int bus_scope_set_transient_property( return r; if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0 && r != -EEXIST) return r; } diff --git a/src/core/manager.c b/src/core/manager.c index 22d1fda2dc..33785d905f 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2129,6 +2129,16 @@ void manager_clear_jobs(Manager *m) { job_finish_and_invalidate(j, JOB_CANCELED, false, false); } +void manager_unwatch_pid(Manager *m, pid_t pid) { + assert(m); + + /* First let's drop the unit keyed as "pid". */ + (void) hashmap_remove(m->watch_pids, PID_TO_PTR(pid)); + + /* Then, let's also drop the array keyed by -pid. */ + free(hashmap_remove(m->watch_pids, PID_TO_PTR(-pid))); +} + static int manager_dispatch_run_queue(sd_event_source *source, void *userdata) { Manager *m = userdata; Job *j; diff --git a/src/core/manager.h b/src/core/manager.h index bce8020cfd..c43c587a79 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -437,6 +437,8 @@ int manager_get_dump_string(Manager *m, char **ret); void manager_clear_jobs(Manager *m); +void manager_unwatch_pid(Manager *m, pid_t pid); + unsigned manager_dispatch_load_queue(Manager *m); int manager_default_environment(Manager *m); diff --git a/src/core/mount.c b/src/core/mount.c index a991bf8794..4fb4b0f81e 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -704,7 +704,7 @@ static int mount_coldplug(Unit *u) { pid_is_unwaited(m->control_pid) && MOUNT_STATE_WITH_PROCESS(new_state)) { - r = unit_watch_pid(UNIT(m), m->control_pid); + r = unit_watch_pid(UNIT(m), m->control_pid, false); if (r < 0) return r; @@ -810,9 +810,8 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(m), pid); + r = unit_watch_pid(UNIT(m), pid, true); if (r < 0) - /* FIXME: we need to do something here */ return r; *_pid = pid; diff --git a/src/core/service.c b/src/core/service.c index 4682e86f8e..883ca7bbfe 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -990,7 +990,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0) /* FIXME: we need to do something here */ return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", pid); @@ -1020,7 +1020,7 @@ static void service_search_main_pid(Service *s) { if (service_set_main_pid(s, pid) < 0) return; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0) /* FIXME: we need to do something here */ log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" from: %m", pid); @@ -1154,7 +1154,7 @@ static int service_coldplug(Unit *u) { SERVICE_RUNNING, SERVICE_RELOAD, SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL))) { - r = unit_watch_pid(UNIT(s), s->main_pid); + r = unit_watch_pid(UNIT(s), s->main_pid, false); if (r < 0) return r; } @@ -1166,7 +1166,7 @@ static int service_coldplug(Unit *u) { SERVICE_RELOAD, SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; } @@ -1566,8 +1566,8 @@ static int service_spawn( s->exec_fd_event_source = TAKE_PTR(exec_fd_source); s->exec_fd_hot = false; - r = unit_watch_pid(UNIT(s), pid); - if (r < 0) /* FIXME: we need to do something here */ + r = unit_watch_pid(UNIT(s), pid, true); + if (r < 0) return r; *_pid = pid; @@ -3705,7 +3705,7 @@ static void service_notify_message( if (r > 0) { service_set_main_pid(s, new_main_pid); - r = unit_watch_pid(UNIT(s), new_main_pid); + r = unit_watch_pid(UNIT(s), new_main_pid, false); if (r < 0) log_unit_warning_errno(UNIT(s), r, "Failed to watch new main PID "PID_FMT" for service: %m", new_main_pid); @@ -3923,7 +3923,7 @@ static void service_bus_name_owner_change( log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, name, pid); service_set_main_pid(s, pid); - unit_watch_pid(UNIT(s), pid); + unit_watch_pid(UNIT(s), pid, false); } } } diff --git a/src/core/socket.c b/src/core/socket.c index fd550290be..1396842b26 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1852,7 +1852,7 @@ static int socket_coldplug(Unit *u) { SOCKET_FINAL_SIGTERM, SOCKET_FINAL_SIGKILL)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; @@ -1938,9 +1938,8 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) - /* FIXME: we need to do something here */ return r; *_pid = pid; @@ -2009,7 +2008,7 @@ static int socket_chown(Socket *s, pid_t *_pid) { _exit(EXIT_SUCCESS); } - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) goto fail; diff --git a/src/core/swap.c b/src/core/swap.c index b836fdd304..28acef2373 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -550,7 +550,7 @@ static int swap_coldplug(Unit *u) { pid_is_unwaited(s->control_pid) && SWAP_STATE_WITH_PROCESS(new_state)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; @@ -657,9 +657,8 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { if (r < 0) goto fail; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) - /* FIXME: we need to do something here */ goto fail; *_pid = pid; diff --git a/src/core/unit.c b/src/core/unit.c index 445cf6695c..5620fd9017 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2525,7 +2525,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_add_to_gc_queue(u); } -int unit_watch_pid(Unit *u, pid_t pid) { +int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { int r; assert(u); @@ -2533,6 +2533,12 @@ int unit_watch_pid(Unit *u, pid_t pid) { /* Watch a specific PID */ + /* Caller might be sure that this PID belongs to this unit only. Let's take this + * opportunity to remove any stalled references to this PID as they can be created + * easily (when watching a process which is not our direct child). */ + if (exclusive) + manager_unwatch_pid(u->manager, pid); + r = set_ensure_allocated(&u->pids, NULL); if (r < 0) return r; diff --git a/src/core/unit.h b/src/core/unit.h index 35b936771c..28878b5f4f 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -678,7 +678,7 @@ typedef enum UnitNotifyFlags { void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags); -int unit_watch_pid(Unit *u, pid_t pid); +int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); void unit_unwatch_pid(Unit *u, pid_t pid); void unit_unwatch_all_pids(Unit *u); diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c index 2c6ca0a1a2..04d7e1fbd8 100644 --- a/src/test/test-watch-pid.c +++ b/src/test/test-watch-pid.c @@ -42,25 +42,25 @@ int main(int argc, char *argv[]) { assert_se(hashmap_isempty(m->watch_pids)); assert_se(manager_get_unit_by_pid(m, 4711) == NULL); - assert_se(unit_watch_pid(a, 4711) >= 0); + assert_se(unit_watch_pid(a, 4711, false) >= 0); assert_se(manager_get_unit_by_pid(m, 4711) == a); - assert_se(unit_watch_pid(a, 4711) >= 0); + assert_se(unit_watch_pid(a, 4711, false) >= 0); assert_se(manager_get_unit_by_pid(m, 4711) == a); - assert_se(unit_watch_pid(b, 4711) >= 0); + assert_se(unit_watch_pid(b, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b); - assert_se(unit_watch_pid(b, 4711) >= 0); + assert_se(unit_watch_pid(b, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b); - assert_se(unit_watch_pid(c, 4711) >= 0); + assert_se(unit_watch_pid(c, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b || u == c); - assert_se(unit_watch_pid(c, 4711) >= 0); + assert_se(unit_watch_pid(c, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b || u == c); From 846a07b50594e763c2ed57855a6bf97d42cfa32a Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Tue, 19 Mar 2019 10:59:26 +0100 Subject: [PATCH 3/4] core: only watch processes when it's really necessary If we know that main pid is our child then it's unnecessary to watch all other processes of a unit since in this case we will get SIGCHLD when the main process will exit and will act upon accordingly. So let's watch all processes only if the main process is not our child since in this case we need to detect when the cgroup will become empty in order to figure out when the service becomes dead. This is only needed by cgroupv1. --- src/core/service.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 883ca7bbfe..25a1297569 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -3471,8 +3471,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (main_pid_good(s) <= 0) service_enter_stop_post(s, f); - /* If there is still a service - * process around, wait until + /* If there is still a service process around, wait until * that one quit, too */ break; @@ -3494,10 +3493,14 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (notify_dbus) unit_add_to_dbus_queue(u); - /* If we get a SIGCHLD event for one of the processes we were interested in, then we look for others to watch, - * under the assumption that we'll sooner or later get a SIGCHLD for them, as the original process we watched - * was probably the parent of them, and they are hence now our children. */ - (void) unit_enqueue_rewatch_pids(u); + /* We watch the main/control process otherwise we can't retrieve the unit they + * belong to with cgroupv1. But if they are not our direct child, we won't get a + * SIGCHLD for them. Therefore we need to look for others to watch so we can + * detect when the cgroup becomes empty. Note that the control process is always + * our child so it's pointless to watch all other processes. */ + if (!control_pid_good(s)) + if (!s->main_pid_known || s->main_pid_alien) + (void) unit_enqueue_rewatch_pids(u); } static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *userdata) { From 01542056aa0d517bf352e754b550fe125b13c1c7 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Wed, 20 Mar 2019 08:06:09 +0100 Subject: [PATCH 4/4] scope: tiny cleanup: UNIT(s) -> u No functional changes. --- src/core/scope.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/scope.c b/src/core/scope.c index e478661f94..7f83052258 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -333,11 +333,11 @@ static int scope_start(Unit *u) { (void) unit_reset_cpu_accounting(u); (void) unit_reset_ip_accounting(u); - unit_export_state_files(UNIT(s)); + unit_export_state_files(u); - r = unit_attach_pids_to_cgroup(u, UNIT(s)->pids, NULL); + r = unit_attach_pids_to_cgroup(u, u->pids, NULL); if (r < 0) { - log_unit_warning_errno(UNIT(s), r, "Failed to add PIDs to scope's control group: %m"); + log_unit_warning_errno(u, r, "Failed to add PIDs to scope's control group: %m"); scope_enter_dead(s, SCOPE_FAILURE_RESOURCES); return r; } @@ -347,7 +347,7 @@ static int scope_start(Unit *u) { scope_set_state(s, SCOPE_RUNNING); /* Start watching the PIDs currently in the scope */ - (void) unit_enqueue_rewatch_pids(UNIT(s)); + (void) unit_enqueue_rewatch_pids(u); return 1; }