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 380323114165bee48db3ef5c914791e2c2d5f736..598b396186b83b6d0080e5d8b3e2e4d916a0a47f 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 64dec408ccdc1413f529923a23964ee269f86293..828b6f07951ab77411f6a634275aab4f3dee4d62 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 3f98d3ecf0dd0ca91ddef300953659446abeff84..b6d61627ebb13b446c269856d42db10aa5acc5c3 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 5d9fe98857a40ae54289f44fcfd97c52eeef2427..bb807df2e928357a5f4cf3985d5473042fbad771 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 fdf612061056ec88996002ba5df87531ab0d3593..10f53ef4016c05ea1174898a1b04028979a3b9e6 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 722a5688a52161eeb13c6cae168d64d56c4860b1..effd5fa5d76b7de0613f657bb42e68be76e226ae 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 4ea5b6c6e599e9f907350d3a287f7ec0e47f59db..3819653908fd850558f9c4cc21d86b5d2a5e0252 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 b272d10113cffe2c51389516787548dd629a2450..353fa201321a900d57f6f9afbc9fb463dfeae2b4 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 392cc2d7c57070897e26f901a01ab86fdaa179c9..a8c0f08e95651973f2a15c53f8165e8e90bb92af 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 180f852a25f3a86268b9f02c35d5694b4386f202..613d7b32e68040c5ece9583055f4d34cca974346 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;