cgroup: drastically simplify caching of cgroups members mask

Previously we tried to be smart: when a new unit appeared and it only
added controllers to the cgroup mask we'd update the cached members mask
in all parents by ORing in the controller flags in their cached values.
Unfortunately this was quite broken, as we missed some conditions when
this cache had to be reset (for example, when a unit got unloaded),
moreover the optimization doesn't work when a controller is removed
anyway (as in that case there's no other way for the parent to iterate
though all children if any other, remaining child unit still needs it).
Hence, let's simplify the logic substantially: instead of updating the
cache on the right events (which we didn't get right), let's simply
invalidate the cache, and generate it lazily when we encounter it later.
This should actually result in better behaviour as we don't have to
calculate the new members mask for a whole subtree whever we have the
suspicion something changed, but can delay it to the point where we
actually need the members mask.

This allows us to simplify things quite a bit, which is good, since
validating this cache for correctness is hard enough.

Fixes: #9512

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 3803231..598b396 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -1388,53 +1388,14 @@ CGroupMask unit_get_enable_mask(Unit *u) {
         return mask;
 }
 
-/* Recurse from a unit up through its containing slices, propagating
- * mask bits upward. A unit is also member of itself. */
-void unit_update_cgroup_members_masks(Unit *u) {
-        CGroupMask m;
-        bool more;
-
+void unit_invalidate_cgroup_members_masks(Unit *u) {
         assert(u);
 
-        /* Calculate subtree mask */
-        m = unit_get_subtree_mask(u);
-
-        /* See if anything changed from the previous invocation. If
-         * not, we're done. */
-        if (u->cgroup_subtree_mask_valid && m == u->cgroup_subtree_mask)
-                return;
-
-        more =
-                u->cgroup_subtree_mask_valid &&
-                ((m & ~u->cgroup_subtree_mask) != 0) &&
-                ((~m & u->cgroup_subtree_mask) == 0);
-
-        u->cgroup_subtree_mask = m;
-        u->cgroup_subtree_mask_valid = true;
-
-        if (UNIT_ISSET(u->slice)) {
-                Unit *s = UNIT_DEREF(u->slice);
-
-                if (more)
-                        /* There's more set now than before. We
-                         * propagate the new mask to the parent's mask
-                         * (not caring if it actually was valid or
-                         * not). */
-
-                        s->cgroup_members_mask |= m;
-
-                else
-                        /* There's less set now than before (or we
-                         * don't know), we need to recalculate
-                         * everything, so let's invalidate the
-                         * parent's members mask */
+        /* Recurse invalidate the member masks cache all the way up the tree */
+        u->cgroup_members_mask_valid = false;
 
-                        s->cgroup_members_mask_valid = false;
-
-                /* And now make sure that this change also hits our
-                 * grandparents */
-                unit_update_cgroup_members_masks(s);
-        }
+        if (UNIT_ISSET(u->slice))
+                unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice));
 }
 
 const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask) {

diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 64dec40..828b6f0 100644 (file)
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -154,7 +154,7 @@ CGroupMask unit_get_subtree_mask(Unit *u);
 CGroupMask unit_get_target_mask(Unit *u);
 CGroupMask unit_get_enable_mask(Unit *u);
 
-void unit_update_cgroup_members_masks(Unit *u);
+void unit_invalidate_cgroup_members_masks(Unit *u);
 
 void unit_add_to_cgroup_realize_queue(Unit *u);
 

diff --git a/src/core/dbus-mount.c b/src/core/dbus-mount.c
index 3f98d3e..b6d6162 100644 (file)
--- a/src/core/dbus-mount.c
+++ b/src/core/dbus-mount.c
@@ -145,7 +145,7 @@ int bus_mount_set_property(
 int bus_mount_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c
index 5d9fe98..bb807df 100644 (file)
--- a/src/core/dbus-scope.c
+++ b/src/core/dbus-scope.c
@@ -186,7 +186,7 @@ int bus_scope_set_property(
 int bus_scope_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c
index fdf6120..10f53ef 100644 (file)
--- a/src/core/dbus-service.c
+++ b/src/core/dbus-service.c
@@ -424,7 +424,7 @@ int bus_service_set_property(
 int bus_service_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/dbus-slice.c b/src/core/dbus-slice.c
index 722a568..effd5fa 100644 (file)
--- a/src/core/dbus-slice.c
+++ b/src/core/dbus-slice.c
@@ -28,7 +28,7 @@ int bus_slice_set_property(
 int bus_slice_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c
index 4ea5b6c..3819653 100644 (file)
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -461,7 +461,7 @@ int bus_socket_set_property(
 int bus_socket_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/dbus-swap.c b/src/core/dbus-swap.c
index b272d10..353fa20 100644 (file)
--- a/src/core/dbus-swap.c
+++ b/src/core/dbus-swap.c
@@ -63,7 +63,7 @@ int bus_swap_set_property(
 int bus_swap_commit_properties(Unit *u) {
         assert(u);
 
-        unit_update_cgroup_members_masks(u);
+        unit_invalidate_cgroup_members_masks(u);
         unit_realize_cgroup(u);
 
         return 0;

diff --git a/src/core/unit.c b/src/core/unit.c
index 392cc2d..a8c0f08 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1547,7 +1547,8 @@ int unit_load(Unit *u) {
                 if (u->job_running_timeout != USEC_INFINITY && u->job_running_timeout > u->job_timeout)
                         log_unit_warning(u, "JobRunningTimeoutSec= is greater than JobTimeoutSec=, it has no effect.");
 
-                unit_update_cgroup_members_masks(u);
+                /* We finished loading, let's ensure our parents recalculate the members mask */
+                unit_invalidate_cgroup_members_masks(u);
         }
 
         assert((u->load_state != UNIT_MERGED) == !u->merged_into);

diff --git a/src/core/unit.h b/src/core/unit.h
index 180f852..613d7b3 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -250,7 +250,6 @@ typedef struct Unit {
         CGroupMask cgroup_realized_mask;           /* In which hierarchies does this unit's cgroup exist? (only relevant on cgroupsv1) */
         CGroupMask cgroup_enabled_mask;            /* Which controllers are enabled (or more correctly: enabled for the children) for this unit's cgroup? (only relevant on cgroupsv2) */
         CGroupMask cgroup_invalidated_mask;        /* A mask specifiying controllers which shall be considered invalidated, and require re-realization */
-        CGroupMask cgroup_subtree_mask;
         CGroupMask cgroup_members_mask;            /* A cache for the controllers required by all children of this cgroup (only relevant for slice units) */
         int cgroup_inotify_wd;
 
@@ -330,7 +329,6 @@ typedef struct Unit {
 
         bool cgroup_realized:1;
         bool cgroup_members_mask_valid:1;
-        bool cgroup_subtree_mask_valid:1;
 
         /* Reset cgroup accounting next time we fork something off */
         bool reset_accounting:1;