core: rework how we track which PIDs to watch for a unit

Previously, we'd maintain two hashmaps keyed by PIDs, pointing to Unit
interested in SIGCHLD events for them. This scheme allowed a specific
PID to be watched by exactly 0, 1 or 2 units.

With this rework this is replaced by a single hashmap which is primarily
keyed by the PID and points to a Unit interested in it. However, it
optionally also keyed by the negated PID, in which case it points to a
NULL terminated array of additional Unit objects also interested. This
scheme means arbitrary numbers of Units may now watch the same PID.

Runtime and memory behaviour should not be impact by this change, as for
the common case (i.e. each PID only watched by a single unit) behaviour
stays the same, but for the uncommon case (a PID watched by more than
one unit) we only pay with a single additional memory allocation for the
array.

Why this all? Primarily, because allowing exactly two units to watch a
specific PID is not sufficient for some niche cases, as processes can
belong to more than one unit these days:

1. sd_notify() with MAINPID= can be used to attach a process from a
   different cgroup to multiple units.

2. Similar, the PIDFile= setting in unit files can be used for similar
   setups,

3. By creating a scope unit a main process of a service may join a
   different unit, too.

4. On cgroupsv1 we frequently end up watching all processes remaining in
   a scope, and if a process opens lots of scopes one after the other it
   might thus end up being watch by many of them.

This patch hence removes the 2-unit-per-PID limit. It also makes a
couple of other changes, some of them quite relevant:

- manager_get_unit_by_pid() (and the bus call wrapping it) when there's
  ambiguity will prefer returning the Unit the process belongs to based on
  cgroup membership, and only check the watch-pids hashmap if that
  fails. This change in logic is probably more in line with what people
  expect and makes things more stable as each process can belong to
  exactly one cgroup only.

- Every SIGCHLD event is now dispatched to all units interested in its
  PID. Previously, there was some magic conditionalization: the SIGCHLD
  would only be dispatched to the unit if it was only interested in a
  single PID only, or the PID belonged to the control or main PID or we
  didn't dispatch a signle SIGCHLD to the unit in the current event loop
  iteration yet. These rules were quite arbitrary and also redundant as
  the the per-unit handlers would filter the PIDs anyway a second time.
  With this change we'll hence relax the rules: all we do now is
  dispatch every SIGCHLD event exactly once to each unit interested in
  it, and it's up to the unit to then use or ignore this. We use a
  generation counter in the unit to ensure that we only invoke the unit
  handler once for each event, protecting us from confusion if a unit is
  both associated with a specific PID through cgroup membership and
  through the "watch_pids" logic. It also protects us from being
  confused if the "watch_pids" hashmap is altered while we are
  dispatching to it (which is a very likely case).

- sd_notify() message dispatching has been reworked to be very similar
  to SIGCHLD handling now. A generation counter is used for dispatching
  as well.

This also adds a new test that validates that "watch_pid" registration
and unregstration works correctly.

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 8124704cab094fa387397dbb855b537da2651564..0b69a8c56191d7c3988295b50952e5f0bd821454 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -2223,40 +2223,46 @@ Unit* manager_get_unit_by_cgroup(Manager *m, const char *cgroup) {
 
 Unit *manager_get_unit_by_pid_cgroup(Manager *m, pid_t pid) {
         _cleanup_free_ char *cgroup = NULL;
-        int r;
 
         assert(m);
 
-        if (pid <= 0)
+        if (!pid_is_valid(pid))
                 return NULL;
 
-        r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid, &cgroup);
-        if (r < 0)
+        if (cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid, &cgroup) < 0)
                 return NULL;
 
         return manager_get_unit_by_cgroup(m, cgroup);
 }
 
 Unit *manager_get_unit_by_pid(Manager *m, pid_t pid) {
-        Unit *u;
+        Unit *u, **array;
 
         assert(m);
 
-        if (pid <= 0)
+        /* Note that a process might be owned by multiple units, we return only one here, which is good enough for most
+         * cases, though not strictly correct. We prefer the one reported by cgroup membership, as that's the most
+         * relevant one as children of the process will be assigned to that one, too, before all else. */
+
+        if (!pid_is_valid(pid))
                 return NULL;
 
         if (pid == getpid_cached())
                 return hashmap_get(m->units, SPECIAL_INIT_SCOPE);
 
-        u = hashmap_get(m->watch_pids1, PID_TO_PTR(pid));
+        u = manager_get_unit_by_pid_cgroup(m, pid);
         if (u)
                 return u;
 
-        u = hashmap_get(m->watch_pids2, PID_TO_PTR(pid));
+        u = hashmap_get(m->watch_pids, PID_TO_PTR(pid));
         if (u)
                 return u;
 
-        return manager_get_unit_by_pid_cgroup(m, pid);
+        array = hashmap_get(m->watch_pids, PID_TO_PTR(-pid));
+        if (array)
+                return array[0];
+
+        return NULL;
 }
 #endif // 0
 

diff --git a/src/test/meson.build b/src/test/meson.build
index 72fb33d9aec69cee1d0da4fc8e150cd3f378d228..3674740c0a8c8117d6e2c4d269b58a7636e33a92 100644 (file)
--- a/src/test/meson.build
+++ b/src/test/meson.build
@@ -409,6 +409,17 @@ tests += [
 #           libblkid]],
 #endif // 0
 
+        [['src/test/test-watch-pid.c',
+          'src/test/test-helper.c'],
+         [libcore,
+          libshared],
+         [libmount,
+          threads,
+          librt,
+          libseccomp,
+          libselinux,
+          libblkid]],
+
         [['src/test/test-hashmap.c',
           'src/test/test-hashmap-plain.c',
           test_hashmap_ordered_c],

diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c
new file mode 100644 (file)
index 0000000..37ff2e4
--- /dev/null
+++ b/src/test/test-watch-pid.c
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+
+//#include "log.h"
+//#include "manager.h"
+//#include "rm-rf.h"
+//#include "test-helper.h"
+//#include "tests.h"
+
+int main(int argc, char *argv[]) {
+        _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL;
+        Unit *a, *b, *c, *u;
+        Manager *m;
+        int r;
+
+        log_set_max_level(LOG_DEBUG);
+        log_parse_environment();
+        log_open();
+
+        if (getuid() != 0) {
+                log_notice("Not running as root, skipping kernel related tests.");
+                return EXIT_TEST_SKIP;
+        }
+
+        r = enter_cgroup_subroot();
+        if (r == -ENOMEDIUM) {
+                log_notice("cgroupfs not available, skipping tests");
+                return EXIT_TEST_SKIP;
+        }
+
+        assert_se(set_unit_path(get_testdata_dir("")) >= 0);
+        assert_se(runtime_dir = setup_fake_runtime_dir());
+
+        assert_se(manager_new(UNIT_FILE_USER, true, &m) >= 0);
+        assert_se(manager_startup(m, NULL, NULL) >= 0);
+
+        assert_se(a = unit_new(m, sizeof(Service)));
+        assert_se(unit_add_name(a, "a.service") >= 0);
+        assert_se(set_isempty(a->pids));
+
+        assert_se(b = unit_new(m, sizeof(Service)));
+        assert_se(unit_add_name(b, "b.service") >= 0);
+        assert_se(set_isempty(b->pids));
+
+        assert_se(c = unit_new(m, sizeof(Service)));
+        assert_se(unit_add_name(c, "c.service") >= 0);
+        assert_se(set_isempty(c->pids));
+
+        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(manager_get_unit_by_pid(m, 4711) == a);
+
+        assert_se(unit_watch_pid(a, 4711) >= 0);
+        assert_se(manager_get_unit_by_pid(m, 4711) == a);
+
+        assert_se(unit_watch_pid(b, 4711) >= 0);
+        u = manager_get_unit_by_pid(m, 4711);
+        assert_se(u == a || u == b);
+
+        assert_se(unit_watch_pid(b, 4711) >= 0);
+        u = manager_get_unit_by_pid(m, 4711);
+        assert_se(u == a || u == b);
+
+        assert_se(unit_watch_pid(c, 4711) >= 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);
+        u = manager_get_unit_by_pid(m, 4711);
+        assert_se(u == a || u == b || u == c);
+
+        unit_unwatch_pid(b, 4711);
+        u = manager_get_unit_by_pid(m, 4711);
+        assert_se(u == a || u == c);
+
+        unit_unwatch_pid(b, 4711);
+        u = manager_get_unit_by_pid(m, 4711);
+        assert_se(u == a || u == c);
+
+        unit_unwatch_pid(a, 4711);
+        assert_se(manager_get_unit_by_pid(m, 4711) == c);
+
+        unit_unwatch_pid(a, 4711);
+        assert_se(manager_get_unit_by_pid(m, 4711) == c);
+
+        unit_unwatch_pid(c, 4711);
+        assert_se(manager_get_unit_by_pid(m, 4711) == NULL);
+
+        unit_unwatch_pid(c, 4711);
+        assert_se(manager_get_unit_by_pid(m, 4711) == NULL);
+
+        manager_free(m);
+
+        return 0;
+}