cgroup: Introduce family queueing instead of siblings

The unit_add_siblings_to_cgroup_realize_queue does more than mere
siblings queueing, hence define a family of a unit as (immediate)
children of the unit and immediate children of all ancestors.

Working with this abstraction simplifies the queuing calls and it
shouldn't change the functionality.

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index c1593107482c5d9ab4b50ab38a21f98b47e8b9ea..143809068a2bb33db536404d7ab762cea8a4e7ba 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -2313,37 +2313,34 @@ unsigned manager_dispatch_cgroup_realize_queue(Manager *m) {
         return n;
 }
 
-void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
-        Unit *slice;
-        Unit *p = u;
+void unit_add_family_to_cgroup_realize_queue(Unit *u) {
+        assert(u);
+        assert(u->type == UNIT_SLICE);
 
-        /* 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).
-         * The function must invalidate cgroup_members_mask of all ancestors in order to calculate up to date
-         * masks.
+        /* Family of a unit for is defined as (immediate) children of the unit and immediate children of all
+         * its ancestors.
+         *
+         * Ideally we would enqueue ancestor path only (bottom up). However, on cgroup-v1 scheduling becomes
+         * very weird if two units that own processes reside in the same slice, but one is realized in the
+         * "cpu" hierarchy and one is not (for example because one has CPUWeight= set and the other does
+         * not), because that means individual processes need to be scheduled against whole cgroups. Let's
+         * avoid this asymmetry by always ensuring that siblings of a unit are always realized in their v1
+         * controller hierarchies too (if unit requires the controller to be realized).
          *
-         * 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
-         * realized in the "cpu" hierarchy and one is not (for example because one has CPUWeight= set and the
-         * other does not), because that means individual processes need to be scheduled against whole
-         * cgroups. Let's avoid this asymmetry by always ensuring that units below a slice that are realized
-         * 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(p->slice))) {
+         * The function must invalidate cgroup_members_mask of all ancestors in order to calculate up to date
+         * masks. */
+
+        do {
                 Iterator i;
                 Unit *m;
                 void *v;
 
-                /* Children of slice likely changed when we're called */
-                slice->cgroup_members_mask_valid = false;
+                /* Children of u likely changed when we're called */
+                u->cgroup_members_mask_valid = false;
 
-                HASHMAP_FOREACH_KEY(v, m, slice->dependencies[UNIT_BEFORE], i) {
+                HASHMAP_FOREACH_KEY(v, m, u->dependencies[UNIT_BEFORE], i) {
                         /* 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)
+                        if (UNIT_DEREF(m->slice) != u)
                                 continue;
 
                         /* No point in doing cgroup application for units without active processes. */
@@ -2365,12 +2362,9 @@ void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
                         unit_add_to_cgroup_realize_queue(m);
                 }
 
-                p = slice;
-        }
-
-        /* Root slice comes last */
-        if (p != u)
-                unit_add_to_cgroup_realize_queue(p);
+                /* Parent comes after children */
+                unit_add_to_cgroup_realize_queue(u);
+        } while ((u = UNIT_DEREF(u->slice)));
 }
 
 int unit_realize_cgroup(Unit *u) {
@@ -2379,20 +2373,17 @@ int unit_realize_cgroup(Unit *u) {
         if (!UNIT_HAS_CGROUP_CONTEXT(u))
                 return 0;
 
-        /* So, here's the deal: when realizing the cgroups for this
-         * unit, we need to first create all parents, but there's more
-         * actually: for the weight-based controllers we also need to
-         * make sure that all our siblings (i.e. units that are in the
-         * same slice as we are) have cgroups, too. Otherwise, things
-         * would become very uneven as each of their processes would
-         * get as much resources as all our group together. This call
-         * will synchronously create the parent cgroups, but will
-         * defer work on the siblings to the next event loop
-         * iteration. When removing a realized controller, it may become unnecessary in ancestors,
-         * so we also ensure deferred bottom up (de)realization of ancestors.
-         */
+        /* So, here's the deal: when realizing the cgroups for this unit, we need to first create all
+         * parents, but there's more actually: for the weight-based controllers we also need to make sure
+         * that all our siblings (i.e. units that are in the same slice as we are) have cgroups, too.  On the
+         * other hand, when a controller is removed from realized set, it may become unnecessary in siblings
+         * and ancestors and they should be (de)realized too.
+         *
+         * This call will defer work on the siblings and derealized ancestors to the next event loop
+         * iteration and synchronously creates the parent cgroups (unit_realize_cgroup_now). */
 
-        unit_add_siblings_to_cgroup_realize_queue(u);
+        if (UNIT_ISSET(u->slice))
+                unit_add_family_to_cgroup_realize_queue(UNIT_DEREF(u->slice));
 
         /* And realize this one now (and apply the values) */
         return unit_realize_cgroup_now(u, manager_state(u->manager));

diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index d6e170cdc6c4daf138e3d99cd1c7642961c3342a..be9632d8b6600d196dcbc08947dc47edcc86b8ad 100644 (file)
--- 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_siblings_to_cgroup_realize_queue(Unit *u);
+void unit_add_family_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 12465742fdb642711c04b27ee5d77fca1d9350a9..5d6854b91a241ce25db2ef3cbc8db1f53c4e1e41 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -661,11 +661,10 @@ 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. */
+        /* A unit is being dropped from the tree, make sure our family is 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_add_siblings_to_cgroup_realize_queue(u);
+                unit_add_family_to_cgroup_realize_queue(UNIT_DEREF(u->slice));
 
         if (u->on_console)
                 manager_unref_console(u->manager);