From: Michal Koutný <mkoutny@suse.com>
Date: Mon, 1 Jun 2020 15:30:35 +0000 (+0200)
Subject: cgroup: Eager realization in unit_free
X-Git-Tag: v247-rc1~366^2~4
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fb46fca7e0c484026bbc9e196aee118800186768;p=thirdparty%2Fsystemd.git

cgroup: Eager realization in unit_free

unit_free(u) realizes direct parent and invalidates members mask of all
ancestors. This isn't sufficient in v1 controller hierarchies since
siblings of the freed unit may have existed only because of the removed
unit.

We cannot be lazy about the siblings because if parent(u) is also
removed, it'd migrate and rmdir cgroups for siblings(u). However,
realized masks of siblings(u) won't reflect this change.

This was a non-issue earlier, because we weren't removing cgroup
directories properly (effectively matching the stale realized mask),
removal failed because of tasks left by missing migration (see previous
commit).

Therefore, ensure realization of all units necessary to clean up after
the free'd unit.

Fixes: #14149
---

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 6750bbbe1d4..84512963a42 100644
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -2105,7 +2105,7 @@ static bool unit_has_mask_enables_realized(
                 ((u->cgroup_enabled_mask | enable_mask) & CGROUP_MASK_V2) == (u->cgroup_enabled_mask & CGROUP_MASK_V2);
 }
 
-void unit_add_to_cgroup_realize_queue(Unit *u) {
+static void unit_add_to_cgroup_realize_queue(Unit *u) {
         assert(u);
 
         if (u->in_cgroup_realize_queue)
@@ -2313,10 +2313,12 @@ unsigned manager_dispatch_cgroup_realize_queue(Manager *m) {
         return n;
 }
 
-static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
+void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
         Unit *slice;
+        Unit *p = u;
 
         /* This adds the path from the specified unit to root slice to the queue and siblings at each level.
+         * The unit itself is excluded (assuming it's handled separately).
          *
          * Propagation of realization "side-ways" (i.e. towards siblings) is relevant on cgroup-v1 where
          * scheduling becomes very weird if two units that own processes reside in the same slice, but one is
@@ -2326,7 +2328,7 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
          * at all are always realized in *all* their hierarchies, and it is sufficient for a unit's sibling
          * to be realized for the unit itself to be realized too. */
 
-        while ((slice = UNIT_DEREF(u->slice))) {
+        while ((slice = UNIT_DEREF(p->slice))) {
                 Iterator i;
                 Unit *m;
                 void *v;
@@ -2335,6 +2337,9 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
                         /* Skip units that have a dependency on the slice but aren't actually in it. */
                         if (UNIT_DEREF(m->slice) != slice)
                                 continue;
+                        /* The origin must be handled separately by caller */
+                        if (m == u)
+                                continue;
 
                         /* No point in doing cgroup application for units without active processes. */
                         if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(m)))
@@ -2355,11 +2360,12 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
                         unit_add_to_cgroup_realize_queue(m);
                 }
 
-                u = slice;
+                p = slice;
         }
 
         /* Root slice comes last */
-        unit_add_to_cgroup_realize_queue(u);
+        if (p != u)
+                unit_add_to_cgroup_realize_queue(p);
 }
 
 int unit_realize_cgroup(Unit *u) {
diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 52d028e740f..d6e170cdc6c 100644
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -212,7 +212,7 @@ CGroupMask unit_get_enable_mask(Unit *u);
 
 void unit_invalidate_cgroup_members_masks(Unit *u);
 
-void unit_add_to_cgroup_realize_queue(Unit *u);
+void unit_add_siblings_to_cgroup_realize_queue(Unit *u);
 
 const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask);
 char *unit_default_cgroup_path(const Unit *u);
diff --git a/src/core/unit.c b/src/core/unit.c
index 2c09def06f3..c16e7562d88 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -620,14 +620,6 @@ void unit_free(Unit *u) {
         if (!u)
                 return;
 
-        if (UNIT_ISSET(u->slice)) {
-                /* A unit is being dropped from the tree, make sure our parent slice recalculates the member mask */
-                unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice));
-
-                /* And make sure the parent is realized again, updating cgroup memberships */
-                unit_add_to_cgroup_realize_queue(UNIT_DEREF(u->slice));
-        }
-
         u->transient_file = safe_fclose(u->transient_file);
 
         if (!MANAGER_IS_RELOADING(u->manager))
@@ -669,6 +661,14 @@ void unit_free(Unit *u) {
         for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
                 bidi_set_free(u, u->dependencies[d]);
 
+        /* A unit is being dropped from the tree, make sure our siblings and ancestor slices are realized
+         * properly. Do this after we detach the unit from slice tree in order to eliminate its effect on
+         * controller masks. */
+        if (UNIT_ISSET(u->slice)) {
+                unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice));
+                unit_add_siblings_to_cgroup_realize_queue(u);
+        }
+
         if (u->on_console)
                 manager_unref_console(u->manager);