core: rework cgroup mask propagation

Previously a cgroup setting down tree would result in cgroup membership
additions being propagated up the tree and to the siblings, however a
unit could never lose cgroup memberships again. With this change we'll
make sure that both cgroup additions and removals propagate properly.

11 files changed, 150 insertions, 44 deletions

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index c2b1b7d38d..f0420ebc09 100644
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -335,7 +335,7 @@ CGroupControllerMask cgroup_context_get_mask(CGroupContext *c) {
         return mask;
 }
 
-static CGroupControllerMask unit_get_cgroup_mask(Unit *u) {
+CGroupControllerMask unit_get_cgroup_mask(Unit *u) {
         CGroupContext *c;
 
         c = unit_get_cgroup_context(u);
@@ -345,24 +345,52 @@ static CGroupControllerMask unit_get_cgroup_mask(Unit *u) {
         return cgroup_context_get_mask(c);
 }
 
-static CGroupControllerMask unit_get_members_mask(Unit *u) {
+CGroupControllerMask unit_get_members_mask(Unit *u) {
         assert(u);
+
+        if (u->cgroup_members_mask_valid)
+                return u->cgroup_members_mask;
+
+        u->cgroup_members_mask = 0;
+
+        if (u->type == UNIT_SLICE) {
+                Unit *member;
+                Iterator i;
+
+                SET_FOREACH(member, u->dependencies[UNIT_BEFORE], i) {
+
+                        if (member == u)
+                                continue;
+
+                         if (UNIT_DEREF(member->slice) != u)
+                                continue;
+
+                        u->cgroup_members_mask |=
+                                unit_get_cgroup_mask(member) |
+                                unit_get_members_mask(member);
+                }
+        }
+
+        u->cgroup_members_mask_valid = true;
         return u->cgroup_members_mask;
 }
 
-static CGroupControllerMask unit_get_siblings_mask(Unit *u) {
+CGroupControllerMask unit_get_siblings_mask(Unit *u) {
+        CGroupControllerMask m;
+
         assert(u);
 
-        if (!UNIT_ISSET(u->slice))
-                return 0;
+        if (UNIT_ISSET(u->slice))
+                m = unit_get_members_mask(UNIT_DEREF(u->slice));
+        else
+                m = unit_get_cgroup_mask(u) | unit_get_members_mask(u);
 
         /* Sibling propagation is only relevant for weight-based
          * controllers, so let's mask out everything else */
-        return unit_get_members_mask(UNIT_DEREF(u->slice)) &
-                (CGROUP_CPU|CGROUP_BLKIO|CGROUP_CPUACCT);
+        return m & (CGROUP_CPU|CGROUP_BLKIO|CGROUP_CPUACCT);
 }
 
-static CGroupControllerMask unit_get_target_mask(Unit *u) {
+CGroupControllerMask unit_get_target_mask(Unit *u) {
         CGroupControllerMask mask;
 
         mask = unit_get_cgroup_mask(u) | unit_get_members_mask(u) | unit_get_siblings_mask(u);
@@ -373,19 +401,57 @@ static CGroupControllerMask unit_get_target_mask(Unit *u) {
 
 /* Recurse from a unit up through its containing slices, propagating
  * mask bits upward. A unit is also member of itself. */
-void unit_update_member_masks(Unit *u) {
-        u->cgroup_members_mask |= unit_get_cgroup_mask(u);
+void unit_update_cgroup_members_masks(Unit *u) {
+        CGroupControllerMask m;
+        bool more;
+
+        assert(u);
+
+        /* Calculate subtree mask */
+        m = unit_get_cgroup_mask(u) | unit_get_members_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);
-                s->cgroup_members_mask |= u->cgroup_members_mask;
-                unit_update_member_masks(s);
+
+                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 */
+
+                        s->cgroup_members_mask_valid = false;
+
+                /* And now make sure that this change also hits our
+                 * grandparents */
+                unit_update_cgroup_members_masks(s);
         }
 }
 
 static int unit_create_cgroups(Unit *u, CGroupControllerMask mask) {
         _cleanup_free_ char *path;
-        int r;
         bool was_in_hash = false;
+        int r;
 
         assert(u);
 
@@ -424,13 +490,15 @@ static int unit_create_cgroups(Unit *u, CGroupControllerMask mask) {
         }
 
         u->cgroup_realized = true;
-        u->cgroup_mask = mask;
+        u->cgroup_realized_mask = mask;
 
         return 0;
 }
 
 static bool unit_has_mask_realized(Unit *u, CGroupControllerMask mask) {
-        return u->cgroup_realized && u->cgroup_mask == mask;
+        assert(u);
+
+        return u->cgroup_realized && u->cgroup_realized_mask == mask;
 }
 
 /* Check if necessary controllers and attributes for a unit are in place.
@@ -452,7 +520,6 @@ static int unit_realize_cgroup_now(Unit *u) {
 
         mask = unit_get_target_mask(u);
 
-        /* TODO: Consider skipping this check. It may be redundant. */
         if (unit_has_mask_realized(u, mask))
                 return 0;
 
@@ -541,7 +608,6 @@ static void unit_queue_siblings(Unit *u) {
 
 int unit_realize_cgroup(Unit *u) {
         CGroupContext *c;
-        int r;
 
         assert(u);
 
@@ -564,9 +630,7 @@ int unit_realize_cgroup(Unit *u) {
         unit_queue_siblings(u);
 
         /* And realize this one now (and apply the values) */
-        r = unit_realize_cgroup_now(u);
-
-        return r;
+        return unit_realize_cgroup_now(u);
 }
 
 void unit_destroy_cgroup(Unit *u) {
@@ -586,7 +650,7 @@ void unit_destroy_cgroup(Unit *u) {
         free(u->cgroup_path);
         u->cgroup_path = NULL;
         u->cgroup_realized = false;
-        u->cgroup_mask = 0;
+        u->cgroup_realized_mask = 0;
 
 }
 
diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 25f40f7b62..be717ad877 100644
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -90,12 +90,19 @@ void cgroup_context_init(CGroupContext *c);
 void cgroup_context_done(CGroupContext *c);
 void cgroup_context_dump(CGroupContext *c, FILE* f, const char *prefix);
 void cgroup_context_apply(CGroupContext *c, CGroupControllerMask mask, const char *path);
+
 CGroupControllerMask cgroup_context_get_mask(CGroupContext *c);
 
 void cgroup_context_free_device_allow(CGroupContext *c, CGroupDeviceAllow *a);
 void cgroup_context_free_blockio_device_weight(CGroupContext *c, CGroupBlockIODeviceWeight *w);
 void cgroup_context_free_blockio_device_bandwidth(CGroupContext *c, CGroupBlockIODeviceBandwidth *b);
 
+CGroupControllerMask unit_get_cgroup_mask(Unit *u);
+CGroupControllerMask unit_get_siblings_mask(Unit *u);
+CGroupControllerMask unit_get_members_mask(Unit *u);
+CGroupControllerMask unit_get_target_mask(Unit *u);
+
+void unit_update_cgroup_members_masks(Unit *u);
 int unit_realize_cgroup(Unit *u);
 void unit_destroy_cgroup(Unit *u);
 
@@ -113,5 +120,3 @@ int manager_notify_cgroup_empty(Manager *m, const char *group);
 
 const char* cgroup_device_policy_to_string(CGroupDevicePolicy i) _const_;
 CGroupDevicePolicy cgroup_device_policy_from_string(const char *s) _pure_;
-
-void unit_update_member_masks(Unit *u);
diff --git a/src/core/dbus-mount.c b/src/core/dbus-mount.c
index 28f0e9071e..e64d3eaa5e 100644
--- a/src/core/dbus-mount.c
+++ b/src/core/dbus-mount.c
@@ -143,6 +143,8 @@ int bus_mount_set_property(
 int bus_mount_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_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 d02569608c..73b7bbf18c 100644
--- a/src/core/dbus-scope.c
+++ b/src/core/dbus-scope.c
@@ -187,7 +187,9 @@ int bus_scope_set_property(
 int bus_scope_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_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 73cf17541a..0451790d83 100644
--- a/src/core/dbus-service.c
+++ b/src/core/dbus-service.c
@@ -258,6 +258,8 @@ int bus_service_set_property(
 int bus_service_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_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 2a48ea455e..8bc90b1dad 100644
--- a/src/core/dbus-slice.c
+++ b/src/core/dbus-slice.c
@@ -48,6 +48,8 @@ int bus_slice_set_property(
 int bus_slice_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_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 bb9d41990a..67a562775c 100644
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -145,6 +145,8 @@ int bus_socket_set_property(
 int bus_socket_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_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 7bd05db415..93eae53c29 100644
--- a/src/core/dbus-swap.c
+++ b/src/core/dbus-swap.c
@@ -88,6 +88,8 @@ int bus_swap_set_property(
 int bus_swap_commit_properties(Unit *u) {
         assert(u);
 
+        unit_update_cgroup_members_masks(u);
         unit_realize_cgroup(u);
+
         return 0;
 }
diff --git a/src/core/unit.c b/src/core/unit.c
index 0277675f60..b91fcf1957 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -778,7 +778,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 prefix, strna(unit_slice_name(u)),
                 prefix, strna(u->cgroup_path),
                 prefix, yes_no(u->cgroup_realized),
-                prefix, u->cgroup_mask,
+                prefix, u->cgroup_realized_mask,
                 prefix, u->cgroup_members_mask);
 
         SET_FOREACH(t, u->names, i)
@@ -1056,21 +1056,17 @@ int unit_load(Unit *u) {
                                 goto fail;
                 }
 
-                unit_update_member_masks(u);
-
                 r = unit_add_mount_links(u);
                 if (r < 0)
                         goto fail;
 
-                if (u->on_failure_job_mode == JOB_ISOLATE &&
-                    set_size(u->dependencies[UNIT_ON_FAILURE]) > 1) {
-
-                        log_error_unit(u->id,
-                                       "More than one OnFailure= dependencies specified for %s but OnFailureJobMode=isolate set. Refusing.", u->id);
-
+                if (u->on_failure_job_mode == JOB_ISOLATE && set_size(u->dependencies[UNIT_ON_FAILURE]) > 1) {
+                        log_error_unit(u->id, "More than one OnFailure= dependencies specified for %s but OnFailureJobMode=isolate set. Refusing.", u->id);
                         r = -EINVAL;
                         goto fail;
                 }
+
+                unit_update_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 c104a8a9d5..808929e64a 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -177,7 +177,8 @@ struct Unit {
 
         /* Counterparts in the cgroup filesystem */
         char *cgroup_path;
-        CGroupControllerMask cgroup_mask;
+        CGroupControllerMask cgroup_realized_mask;
+        CGroupControllerMask cgroup_subtree_mask;
         CGroupControllerMask cgroup_members_mask;
 
         UnitRef slice;
@@ -266,6 +267,8 @@ struct Unit {
         bool in_audit:1;
 
         bool cgroup_realized:1;
+        bool cgroup_members_mask_valid:1;
+        bool cgroup_subtree_mask_valid:1;
 };
 
 struct UnitStatusMessageFormats {
diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c
index 99c8ceea76..3414ed4d60 100644
--- a/src/test/test-cgroup-mask.c
+++ b/src/test/test-cgroup-mask.c
@@ -33,7 +33,7 @@
 
 static int test_cgroup_mask(void) {
         Manager *m;
-        Unit *son, *daughter, *parent, *root;
+        Unit *son, *daughter, *parent, *root, *grandchild, *parent_deep;
         FILE *serial = NULL;
         FDSet *fdset = NULL;
         int r;
@@ -53,24 +53,50 @@ static int test_cgroup_mask(void) {
         assert_se(manager_load_unit(m, "parent.slice", NULL, NULL, &parent) >= 0);
         assert_se(manager_load_unit(m, "son.service", NULL, NULL, &son) >= 0);
         assert_se(manager_load_unit(m, "daughter.service", NULL, NULL, &daughter) >= 0);
+        assert_se(manager_load_unit(m, "grandchild.service", NULL, NULL, &grandchild) >= 0);
+        assert_se(manager_load_unit(m, "parent-deep.slice", NULL, NULL, &parent_deep) >= 0);
         assert(parent->load_state == UNIT_LOADED);
         assert(son->load_state == UNIT_LOADED);
         assert(daughter->load_state == UNIT_LOADED);
+        assert(grandchild->load_state == UNIT_LOADED);
+        assert(parent_deep->load_state == UNIT_LOADED);
         assert(UNIT_DEREF(son->slice) == parent);
         assert(UNIT_DEREF(daughter->slice) == parent);
+        assert(UNIT_DEREF(parent_deep->slice) == parent);
+        assert(UNIT_DEREF(grandchild->slice) == parent_deep);
         root = UNIT_DEREF(parent->slice);
 
         /* Verify per-unit cgroups settings. */
-        assert(cgroup_context_get_mask(unit_get_cgroup_context(son)) == (CGROUP_CPU | CGROUP_CPUACCT));
-        assert(cgroup_context_get_mask(unit_get_cgroup_context(daughter)) == 0);
-        assert(cgroup_context_get_mask(unit_get_cgroup_context(parent)) == CGROUP_BLKIO);
-        assert(cgroup_context_get_mask(unit_get_cgroup_context(root)) == 0);
-
-        /* Verify aggregation of controller masks. */
-        assert(son->cgroup_members_mask == (CGROUP_CPU | CGROUP_CPUACCT));
-        assert(daughter->cgroup_members_mask == 0);
-        assert(parent->cgroup_members_mask == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO));
-        assert(root->cgroup_members_mask == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO));
+        assert(unit_get_cgroup_mask(son) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_cgroup_mask(daughter) == 0);
+        assert(unit_get_cgroup_mask(grandchild) == 0);
+        assert(unit_get_cgroup_mask(parent_deep) == CGROUP_MEMORY);
+        assert(unit_get_cgroup_mask(parent) == CGROUP_BLKIO);
+        assert(unit_get_cgroup_mask(root) == 0);
+
+        /* Verify aggregation of member masks */
+        assert(unit_get_members_mask(son) == 0);
+        assert(unit_get_members_mask(daughter) == 0);
+        assert(unit_get_members_mask(grandchild) == 0);
+        assert(unit_get_members_mask(parent_deep) == 0);
+        assert(unit_get_members_mask(parent) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_MEMORY));
+        assert(unit_get_members_mask(root) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO | CGROUP_MEMORY));
+
+        /* Verify aggregation of sibling masks. */
+        assert(unit_get_siblings_mask(son) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_siblings_mask(daughter) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_siblings_mask(grandchild) == 0);
+        assert(unit_get_siblings_mask(parent_deep) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_siblings_mask(parent) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO));
+        assert(unit_get_siblings_mask(root) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO));
+
+        /* Verify aggregation of target masks. */
+        assert(unit_get_target_mask(son) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_target_mask(daughter) == (CGROUP_CPU | CGROUP_CPUACCT));
+        assert(unit_get_target_mask(grandchild) == 0);
+        assert(unit_get_target_mask(parent_deep) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_MEMORY));
+        assert(unit_get_target_mask(parent) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO | CGROUP_MEMORY));
+        assert(unit_get_target_mask(root) == (CGROUP_CPU | CGROUP_CPUACCT | CGROUP_BLKIO | CGROUP_MEMORY));
 
         manager_free(m);