From d219a2b07cc5dc8ffd5010f08561fab2780d8616 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 18:37:25 +0200 Subject: [PATCH] core: convert Slice= into a proper dependency (and add a back dependency) The slice a unit is assigned to is currently a UnitRef reference. Let's turn it into a proper dependency, to simplify and clean up code a bit. Now that new dep types are cheaper, deps should generally be preferable over everything else, if the concept applies. This brings one major benefit: we often have to iterate through all unit a slice contains. So far we iterated through all Before= dependencies of the slice unit to achieve that, filtering out unrelated units, and taking benefit of the fact that slice units are implicitly ordered Before= the units they contain. By making Slice= a proper dependency, and having an accompanying SliceOf= dependency type, this is much simpler and nicer as we can directly enumerate the units a slice contains. The forward dependency is actually called InSlice internally, since we already used the UNIT_SLICE name as UnitType field. However, since we don't intend to expose the dependency to users as dep anyway (we already have the regular Slice D-Bus property for this) this shouldn't matter. The SliceOf= implicit dependency type (the erverse of Slice=/InSlice=) is exported over the bus, to make things a bit nicer to debug and discoverable. --- man/org.freedesktop.systemd1.xml | 6 +++++ src/basic/unit-def.c | 2 ++ src/basic/unit-def.h | 4 ++++ src/core/cgroup.c | 23 ++++-------------- src/core/dbus-unit.c | 3 ++- src/core/load-fragment.c | 2 +- src/core/slice.c | 21 ++++------------- src/core/unit-dependency-atom.c | 8 +++++++ src/core/unit-dependency-atom.h | 4 +++- src/core/unit.c | 40 +++++++++++++++++++++++--------- src/core/unit.h | 7 ++---- 11 files changed, 66 insertions(+), 54 deletions(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 3d6f1a39b3b..de724126405 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -1640,6 +1640,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as JoinsNamespaceOf = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as SliceOf = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as RequiresMountsFor = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as Documentation = ['...', ...]; @@ -1775,6 +1777,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + @@ -1911,6 +1915,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 5cfabca83fb..56516203178 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -282,6 +282,8 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_JOINS_NAMESPACE_OF] = "JoinsNamespaceOf", [UNIT_REFERENCES] = "References", [UNIT_REFERENCED_BY] = "ReferencedBy", + [UNIT_IN_SLICE] = "InSlice", + [UNIT_SLICE_OF] = "SliceOf", }; DEFINE_STRING_TABLE_LOOKUP(unit_dependency, UnitDependency); diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index c30135f4d6c..7d76576de10 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -245,6 +245,10 @@ typedef enum UnitDependency { UNIT_REFERENCES, /* Inverse of 'references' is 'referenced_by' */ UNIT_REFERENCED_BY, + /* Slice= */ + UNIT_IN_SLICE, + UNIT_SLICE_OF, + _UNIT_DEPENDENCY_MAX, _UNIT_DEPENDENCY_INVALID = -EINVAL, } UnitDependency; diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 97820eb2640..5b00faa6f62 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1702,12 +1702,8 @@ CGroupMask unit_get_members_mask(Unit *u) { if (u->type == UNIT_SLICE) { Unit *member; - UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != u) - continue; - + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_SLICE_OF) u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ - } } u->cgroup_members_mask_valid = true; @@ -2362,13 +2358,10 @@ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { if (u->type != UNIT_SLICE) return 0; - UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_SLICE_OF) { CGroupMask target_mask, enable_mask, new_target_mask, new_enable_mask; int r; - if (UNIT_DEREF(m->slice) != u) - continue; - /* The cgroup for this unit might not actually be fully realised yet, in which case it isn't * holding any controllers open anyway. */ if (!m->cgroup_realized) @@ -2530,11 +2523,7 @@ void unit_add_family_to_cgroup_realize_queue(Unit *u) { /* Children of u likely changed when we're called */ u->cgroup_members_mask_valid = false; - UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { - - /* Skip units that have a dependency on the slice but aren't actually in it. */ - if (UNIT_DEREF(m->slice) != u) - continue; + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_SLICE_OF) { /* No point in doing cgroup application for units without active processes. */ if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(m))) @@ -3799,12 +3788,8 @@ void unit_invalidate_cgroup_bpf(Unit *u) { if (u->type == UNIT_SLICE) { Unit *member; - UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != u) - continue; - + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_SLICE_OF) unit_invalidate_cgroup_bpf(member); - } } } diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index e8818c919c0..a1278dd6e98 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -870,6 +870,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JoinsNamespaceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("SliceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RequiresMountsFor", "as", property_get_requires_mounts_for, offsetof(Unit, requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Documentation", "as", NULL, offsetof(Unit, documentation), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Description", "s", property_get_description, 0, SD_BUS_VTABLE_PROPERTY_CONST), @@ -2237,7 +2238,7 @@ static int bus_unit_set_transient_property( return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Unit name '%s' is not a slice", s); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_set_slice(u, slice); + r = unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); if (r < 0) return r; diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index cd373145426..72378a40827 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3574,7 +3574,7 @@ int config_parse_unit_slice( return 0; } - r = unit_set_slice(u, slice); + r = unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); if (r < 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to assign slice %s to unit %s, ignoring: %m", slice->id, u->id); return 0; diff --git a/src/core/slice.c b/src/core/slice.c index 5d836a11e6b..595f704a17a 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -47,7 +47,7 @@ static void slice_set_state(Slice *t, SliceState state) { } static int slice_add_parent_slice(Slice *s) { - Unit *u = UNIT(s), *parent; + Unit *u = UNIT(s); _cleanup_free_ char *a = NULL; int r; @@ -60,12 +60,7 @@ static int slice_add_parent_slice(Slice *s) { if (r <= 0) /* 0 means root slice */ return r; - r = manager_load_unit(u->manager, a, NULL, NULL, &parent); - if (r < 0) - return r; - - unit_ref_set(&u->slice, u, parent); - return 0; + return unit_add_dependency_by_name(u, UNIT_IN_SLICE, a, true, UNIT_DEPENDENCY_IMPLICIT); } static int slice_add_default_dependencies(Slice *s) { @@ -346,14 +341,11 @@ static void slice_enumerate_perpetual(Manager *m) { static bool slice_freezer_action_supported_by_children(Unit *s) { Unit *member; + int r; assert(s); - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { - int r; - - if (UNIT_DEREF(member->slice) != s) - continue; + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { if (member->type == UNIT_SLICE) { r = slice_freezer_action_supported_by_children(member); @@ -380,10 +372,7 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { return 0; } - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != s) - continue; - + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { if (action == FREEZER_FREEZE) r = UNIT_VTABLE(member)->freeze(member); else diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index 4b012ed265a..cfd5acd33b4 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -78,6 +78,8 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { [UNIT_JOINS_NAMESPACE_OF] = UNIT_ATOM_JOINS_NAMESPACE_OF, [UNIT_REFERENCES] = UNIT_ATOM_REFERENCES, [UNIT_REFERENCED_BY] = UNIT_ATOM_REFERENCED_BY, + [UNIT_IN_SLICE] = UNIT_ATOM_IN_SLICE, + [UNIT_SLICE_OF] = UNIT_ATOM_SLICE_OF, /* These are dependency types without effect on our state engine. We maintain them only to make * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they @@ -190,6 +192,12 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { case UNIT_ATOM_REFERENCED_BY: return UNIT_REFERENCED_BY; + case UNIT_ATOM_IN_SLICE: + return UNIT_IN_SLICE; + + case UNIT_ATOM_SLICE_OF: + return UNIT_SLICE_OF; + default: return _UNIT_DEPENDENCY_INVALID; } diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index d04d6134a0c..01482215118 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -69,7 +69,9 @@ typedef enum UnitDependencyAtom { UNIT_ATOM_JOINS_NAMESPACE_OF = UINT64_C(1) << 29, UNIT_ATOM_REFERENCES = UINT64_C(1) << 30, UNIT_ATOM_REFERENCED_BY = UINT64_C(1) << 31, - _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 32) - 1, + UNIT_ATOM_IN_SLICE = UINT64_C(1) << 32, + UNIT_ATOM_SLICE_OF = UINT64_C(1) << 33, + _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 34) - 1, _UNIT_DEPENDENCY_ATOM_INVALID = -EINVAL, } UnitDependencyAtom; diff --git a/src/core/unit.c b/src/core/unit.c index f3eaa595708..c94f4e75236 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -659,11 +659,10 @@ Unit* unit_free(Unit *u) { job_free(j); } - unit_clear_dependencies(u); - /* 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. */ slice = UNIT_GET_SLICE(u); + unit_clear_dependencies(u); if (slice) unit_add_family_to_cgroup_realize_queue(slice); @@ -689,7 +688,6 @@ Unit* unit_free(Unit *u) { unit_unwatch_all_pids(u); - unit_ref_unset(&u->slice); while (u->refs_by_target) unit_ref_unset(u->refs_by_target); @@ -2881,6 +2879,8 @@ int unit_add_dependency( [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, [UNIT_RELOAD_PROPAGATED_FROM] = UNIT_PROPAGATES_RELOAD_TO, [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, /* symmetric! 👓 */ + [UNIT_IN_SLICE] = UNIT_SLICE_OF, + [UNIT_SLICE_OF] = UNIT_IN_SLICE, }; Unit *original_u = u, *original_other = other; UnitDependencyAtom a; @@ -2924,6 +2924,21 @@ int unit_add_dependency( return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), "Requested dependency TriggeredBy=%s refused (%s units cannot trigger other units).", other->id, unit_type_to_string(other->type)); + if (FLAGS_SET(a, UNIT_ATOM_IN_SLICE) && other->type != UNIT_SLICE) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency Slice=%s refused (%s is not a slice unit).", other->id, other->id); + if (FLAGS_SET(a, UNIT_ATOM_SLICE_OF) && u->type != UNIT_SLICE) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency SliceOf=%s refused (%s is not a slice unit).", other->id, u->id); + + if (FLAGS_SET(a, UNIT_ATOM_IN_SLICE) && !UNIT_HAS_CGROUP_CONTEXT(u)) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency Slice=%s refused (%s is not a cgroup unit).", other->id, u->id); + + if (FLAGS_SET(a, UNIT_ATOM_SLICE_OF) && !UNIT_HAS_CGROUP_CONTEXT(other)) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency SliceOf=%s refused (%s is not a cgroup unit).", other->id, other->id); + r = unit_add_dependency_hashmap(&u->dependencies, d, other, mask, 0); if (r < 0) return r; @@ -3102,15 +3117,15 @@ reset: return r; } -int unit_set_slice(Unit *u, Unit *slice) { +int unit_set_slice(Unit *u, Unit *slice, UnitDependencyMask mask) { + int r; + assert(u); assert(slice); - /* Sets the unit slice if it has not been set before. Is extra - * careful, to only allow this for units that actually have a - * cgroup context. Also, we don't allow to set this for slices - * (since the parent slice is derived from the name). Make - * sure the unit we set is actually a slice. */ + /* Sets the unit slice if it has not been set before. Is extra careful, to only allow this for units + * that actually have a cgroup context. Also, we don't allow to set this for slices (since the parent + * slice is derived from the name). Make sure the unit we set is actually a slice. */ if (!UNIT_HAS_CGROUP_CONTEXT(u)) return -EOPNOTSUPP; @@ -3135,7 +3150,10 @@ int unit_set_slice(Unit *u, Unit *slice) { if (UNIT_GET_SLICE(u) && u->cgroup_realized) return -EBUSY; - unit_ref_set(&u->slice, u, slice); + r = unit_add_dependency(u, UNIT_IN_SLICE, slice, true, mask); + if (r < 0) + return r; + return 1; } @@ -3185,7 +3203,7 @@ int unit_set_default_slice(Unit *u) { if (r < 0) return r; - return unit_set_slice(u, slice); + return unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); } const char *unit_slice_name(Unit *u) { diff --git a/src/core/unit.h b/src/core/unit.h index cf110ebb55f..6b3f48596f1 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -202,8 +202,6 @@ typedef struct Unit { dual_timestamp active_exit_timestamp; dual_timestamp inactive_enter_timestamp; - UnitRef slice; - /* Per type list */ LIST_FIELDS(Unit, units_by_type); @@ -707,8 +705,7 @@ static inline Unit* UNIT_TRIGGER(Unit *u) { } static inline Unit* UNIT_GET_SLICE(const Unit *u) { - assert(u); - return u->slice.target; + return unit_has_dependency(u, UNIT_ATOM_IN_SLICE, NULL); } Unit* unit_new(Manager *m, size_t size); @@ -751,7 +748,7 @@ Unit *unit_follow_merge(Unit *u) _pure_; int unit_load_fragment_and_dropin(Unit *u, bool fragment_required); int unit_load(Unit *unit); -int unit_set_slice(Unit *u, Unit *slice); +int unit_set_slice(Unit *u, Unit *slice, UnitDependencyMask mask); int unit_set_default_slice(Unit *u); const char *unit_description(Unit *u) _pure_;