From 5af8805872809e6de4cc4d9495cb1a904772ab4e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Nov 2018 01:07:34 +0100 Subject: [PATCH] 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 --- src/core/cgroup.c | 49 +++++------------------------------------ src/core/cgroup.h | 2 +- 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 | 3 ++- src/core/unit.h | 2 -- 10 files changed, 14 insertions(+), 54 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 38032311416..598b396186b 100644 --- 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 64dec408ccd..828b6f07951 100644 --- 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 3f98d3ecf0d..b6d61627ebb 100644 --- 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 5d9fe98857a..bb807df2e92 100644 --- 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 fdf61206105..10f53ef4016 100644 --- 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 722a5688a52..effd5fa5d76 100644 --- 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 4ea5b6c6e59..3819653908f 100644 --- 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 b272d10113c..353fa201321 100644 --- 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 392cc2d7c57..a8c0f08e956 100644 --- 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 180f852a25f..613d7b32e68 100644 --- 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; -- 2.39.5