From bc432dc7eb62c5671f2b741a86a66393adb350dc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 14 Feb 2014 19:11:07 +0100 Subject: [PATCH] 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. --- src/core/cgroup.c | 106 +++++++++++++++++++++++++++++------- src/core/cgroup.h | 9 ++- src/core/dbus-mount.c | 2 + src/core/dbus-scope.c | 2 + src/core/dbus-service.c | 2 + src/core/dbus-slice.c | 2 + src/core/dbus-socket.c | 2 + src/core/dbus-swap.c | 2 + src/core/unit.c | 14 ++--- src/core/unit.h | 5 +- src/test/test-cgroup-mask.c | 48 ++++++++++++---- 11 files changed, 150 insertions(+), 44 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index c2b1b7d38..f0420ebc0 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 25f40f7b6..be717ad87 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 28f0e9071..e64d3eaa5 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 d02569608..73b7bbf18 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 73cf17541..0451790d8 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 2a48ea455..8bc90b1da 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 bb9d41990..67a562775 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 7bd05db41..93eae53c2 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 0277675f6..b91fcf195 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 c104a8a9d..808929e64 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 99c8ceea7..3414ed4d6 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); -- 2.30.2