From 0d2d6fbf15c842b6f6579232ae5b11e491eccf0e Mon Sep 17 00:00:00 2001 From: Chris Down Date: Tue, 27 Nov 2018 15:16:35 +0000 Subject: [PATCH 1/4] cgroup: Move attribute application into unit_create_cgroup We always end up doing these together, so just colocate them and require manager state for unit_create_cgroup. --- src/core/cgroup.c | 55 ++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 11f9611b71..8d8256f155 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -375,6 +375,26 @@ int cgroup_add_device_allow(CGroupContext *c, const char *dev, const char *mode) return 0; } +static void cgroup_xattr_apply(Unit *u) { + char ids[SD_ID128_STRING_MAX]; + int r; + + assert(u); + + if (!MANAGER_IS_SYSTEM(u->manager)) + return; + + if (sd_id128_is_null(u->invocation_id)) + return; + + r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, + "trusted.invocation_id", + sd_id128_to_string(u->invocation_id, ids), 32, + 0); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to set invocation ID on control group %s, ignoring: %m", u->cgroup_path); +} + static int lookup_block_device(const char *p, dev_t *ret) { struct stat st = {}; int r; @@ -1597,7 +1617,8 @@ int unit_pick_cgroup_path(Unit *u) { static int unit_create_cgroup( Unit *u, CGroupMask target_mask, - CGroupMask enable_mask) { + CGroupMask enable_mask, + ManagerState state) { bool created; int r; @@ -1665,6 +1686,10 @@ static int unit_create_cgroup( log_unit_warning_errno(u, r, "Failed to migrate cgroup from to %s, ignoring: %m", u->cgroup_path); } + /* Set attributes */ + cgroup_context_apply(u, target_mask, state); + cgroup_xattr_apply(u); + return 0; } @@ -1806,26 +1831,6 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { return r; } -static void cgroup_xattr_apply(Unit *u) { - char ids[SD_ID128_STRING_MAX]; - int r; - - assert(u); - - if (!MANAGER_IS_SYSTEM(u->manager)) - return; - - if (sd_id128_is_null(u->invocation_id)) - return; - - r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, - "trusted.invocation_id", - sd_id128_to_string(u->invocation_id, ids), 32, - 0); - if (r < 0) - log_unit_debug_errno(u, r, "Failed to set invocation ID on control group %s, ignoring: %m", u->cgroup_path); -} - static bool unit_has_mask_realized( Unit *u, CGroupMask target_mask, @@ -1901,15 +1906,11 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { return r; } - /* And then do the real work */ - r = unit_create_cgroup(u, target_mask, enable_mask); + /* Now actually deal with the cgroup we were trying to realise and set attributes */ + r = unit_create_cgroup(u, target_mask, enable_mask, state); if (r < 0) return r; - /* Finally, apply the necessary attributes. */ - cgroup_context_apply(u, target_mask, state); - cgroup_xattr_apply(u); - /* Now, reset the invalidation mask */ u->cgroup_invalidated_mask = 0; return 0; From a57669d2903d9277bc099879e72be8f7a68ab5e5 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Mon, 26 Nov 2018 13:45:26 +0000 Subject: [PATCH 2/4] cgroup: Rework unit_realize_cgroup_now to explicitly be breadth-first systemd currently doesn't really expend much effort in disabling controllers. unit_realize_cgroup_now *may* be able to disable a controller in the basic case when using cgroup v2, but generally won't manage as downstream dependents may still use it. This code doesn't add any logic to fix that, but it starts the process of moving to have a breadth-first version of unit_realize_cgroup_now for enabling, and a depth-first version of unit_realize_cgroup_now for disabling. --- src/core/cgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 8d8256f155..387fffc667 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1859,6 +1859,23 @@ static bool unit_has_mask_realized( u->cgroup_invalidated_mask == 0; } +static bool unit_has_mask_enables_realized( + Unit *u, + CGroupMask target_mask, + CGroupMask enable_mask) { + + assert(u); + + /* Returns true if all controllers which should be enabled are indeed enabled. + * + * Unlike unit_has_mask_realized, we don't care about the controllers that are not present, only that anything + * we want to add is already added. */ + + return u->cgroup_realized && + ((u->cgroup_realized_mask | target_mask) & CGROUP_MASK_V1) == u->cgroup_realized_mask && + ((u->cgroup_enabled_mask | enable_mask) & CGROUP_MASK_V2) == u->cgroup_enabled_mask; +} + void unit_add_to_cgroup_realize_queue(Unit *u) { assert(u); @@ -1879,10 +1896,83 @@ static void unit_remove_from_cgroup_realize_queue(Unit *u) { u->in_cgroup_realize_queue = false; } +/* Controllers can only be enabled breadth-first, from the root of the + * hierarchy downwards to the unit in question. */ +static int unit_realize_cgroup_now_enable(Unit *u, ManagerState state) { + CGroupMask target_mask, enable_mask, new_target_mask, new_enable_mask; + int r; + + assert(u); + + /* First go deal with this unit's parent, or we won't be able to enable + * any new controllers at this layer. */ + if (UNIT_ISSET(u->slice)) { + r = unit_realize_cgroup_now_enable(UNIT_DEREF(u->slice), state); + if (r < 0) + return r; + } + + target_mask = unit_get_target_mask(u); + enable_mask = unit_get_enable_mask(u); + + /* We can only enable in this direction, don't try to disable anything. + */ + if (unit_has_mask_enables_realized(u, target_mask, enable_mask)) + return 0; + + new_target_mask = u->cgroup_realized_mask | target_mask; + new_enable_mask = u->cgroup_enabled_mask | enable_mask; + + r = unit_create_cgroup(u, new_target_mask, new_enable_mask, state); + if (r < 0) + return r; + + return 0; +} + + /* Check if necessary controllers and attributes for a unit are in place. * - * If so, do nothing. - * If not, create paths, move processes over, and set attributes. + * - If so, do nothing. + * - If not, create paths, move processes over, and set attributes. + * + * Controllers can only be *enabled* in a breadth-first way, and *disabled* in + * a depth-first way. As such the process looks like this: + * + * Suppose we have a cgroup hierarchy which looks like this: + * + * root + * / \ + * / \ + * / \ + * a b + * / \ / \ + * / \ / \ + * c d e f + * / \ / \ / \ / \ + * h i j k l m n o + * + * 1. We want to realise cgroup "d" now. + * 2. cgroup "a" has DisableController=cpu in the associated unit. + * 3. cgroup "k" just started requesting the memory controller. + * + * To make this work we must do the following in order: + * + * 1. Disable CPU controller in k, j + * 2. Disable CPU controller in d + * 3. Enable memory controller in root + * 4. Enable memory controller in a + * 5. Enable memory controller in d + * 6. Enable memory controller in k + * + * Notice that we need to touch j in one direction, but not the other. We also + * don't go beyond d when disabling -- it's up to "a" to get realized if it + * wants to disable further. The basic rules are therefore: + * + * - If you're disabling something, you need to realise all of the cgroups from + * your recursive descendants to the root. This starts from the leaves. + * - If you're enabling something, you need to realise from the root cgroup + * downwards, but you don't need to iterate your recursive descendants. * * Returns 0 on success and < 0 on failure. */ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { @@ -1899,9 +1989,9 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { if (unit_has_mask_realized(u, target_mask, enable_mask)) return 0; - /* First, realize parents */ + /* Enable controllers above us */ if (UNIT_ISSET(u->slice)) { - r = unit_realize_cgroup_now(UNIT_DEREF(u->slice), state); + r = unit_realize_cgroup_now_enable(UNIT_DEREF(u->slice), state); if (r < 0) return r; } From 4f6f62e468db358a25abf51668c881105b35dc24 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Tue, 27 Nov 2018 15:49:41 +0000 Subject: [PATCH 3/4] cgroup: Traverse leaves to realised cgroup to release controllers This adds a depth-first version of unit_realize_cgroup_now which can only do depth-first disabling of controllers, in preparation for the DisableController= directive. --- src/core/cgroup.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 387fffc667..13c595bbcd 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1416,6 +1416,31 @@ CGroupMask unit_get_siblings_mask(Unit *u) { return unit_get_subtree_mask(u); /* we are the top-level slice */ } +CGroupMask unit_get_disable_mask(Unit *u) { + CGroupContext *c; + + c = unit_get_cgroup_context(u); + if (!c) + return 0; + + return c->disable_controllers; +} + +CGroupMask unit_get_ancestor_disable_mask(Unit *u) { + CGroupMask mask; + + assert(u); + mask = unit_get_disable_mask(u); + + /* Returns the mask of controllers which are marked as forcibly + * disabled in any ancestor unit or the unit in question. */ + + if (UNIT_ISSET(u->slice)) + mask |= unit_get_ancestor_disable_mask(UNIT_DEREF(u->slice)); + + return mask; +} + CGroupMask unit_get_subtree_mask(Unit *u) { /* Returns the mask of this subtree, meaning of the group @@ -1859,6 +1884,23 @@ static bool unit_has_mask_realized( u->cgroup_invalidated_mask == 0; } +static bool unit_has_mask_disables_realized( + Unit *u, + CGroupMask target_mask, + CGroupMask enable_mask) { + + assert(u); + + /* Returns true if all controllers which should be disabled are indeed disabled. + * + * Unlike unit_has_mask_realized, we don't care what was enabled, only that anything we want to remove is + * already removed. */ + + return !u->cgroup_realized || + (FLAGS_SET(u->cgroup_realized_mask, target_mask & CGROUP_MASK_V1) && + FLAGS_SET(u->cgroup_enabled_mask, enable_mask & CGROUP_MASK_V2)); +} + static bool unit_has_mask_enables_realized( Unit *u, CGroupMask target_mask, @@ -1930,6 +1972,54 @@ static int unit_realize_cgroup_now_enable(Unit *u, ManagerState state) { return 0; } +/* Controllers can only be disabled depth-first, from the leaves of the + * hierarchy upwards to the unit in question. */ +static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { + Iterator i; + Unit *m; + void *v; + + assert(u); + + if (u->type != UNIT_SLICE) + return 0; + + HASHMAP_FOREACH_KEY(v, m, u->dependencies[UNIT_BEFORE], i) { + 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_path) + continue; + + /* We must disable those below us first in order to release the + * controller. */ + if (m->type == UNIT_SLICE) + (void) unit_realize_cgroup_now_disable(m, state); + + target_mask = unit_get_target_mask(m); + enable_mask = unit_get_enable_mask(m); + + /* We can only disable in this direction, don't try to enable + * anything. */ + if (unit_has_mask_disables_realized(m, target_mask, enable_mask)) + continue; + + new_target_mask = m->cgroup_realized_mask & target_mask; + new_enable_mask = m->cgroup_enabled_mask & enable_mask; + + r = unit_create_cgroup(m, new_target_mask, new_enable_mask, state); + if (r < 0) + return r; + } + + return 0; +} /* Check if necessary controllers and attributes for a unit are in place. * @@ -1989,7 +2079,12 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { if (unit_has_mask_realized(u, target_mask, enable_mask)) return 0; - /* Enable controllers above us */ + /* Disable controllers below us, if there are any */ + r = unit_realize_cgroup_now_disable(u, state); + if (r < 0) + return r; + + /* Enable controllers above us, if there are any */ if (UNIT_ISSET(u->slice)) { r = unit_realize_cgroup_now_enable(UNIT_DEREF(u->slice), state); if (r < 0) From c72703e26d21cb4994f21ae50c4e18675b02ded3 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Mon, 3 Dec 2018 14:38:06 +0000 Subject: [PATCH 4/4] cgroup: Add DisableControllers= directive to disable controller in subtree Some controllers (like the CPU controller) have a performance cost that is non-trivial on certain workloads. While this can be mitigated and improved to an extent, there will for some controllers always be some overheads associated with the benefits gained from the controller. Inside Facebook, the fix applied has been to disable the CPU controller forcibly with `cgroup_disable=cpu` on the kernel command line. This presents a problem: to disable or reenable the controller, a reboot is required, but this is quite cumbersome and slow to do for many thousands of machines, especially machines where disabling/enabling a stateful service on a machine is a matter of several minutes. Currently systemd provides some configuration knobs for these in the form of `[Default]CPUAccounting`, `[Default]MemoryAccounting`, and the like. The limitation of these is that Default*Accounting is overrideable by individual services, of which any one could decide to reenable a controller within the hierarchy at any point just by using a controller feature implicitly (eg. `CPUWeight`), even if the use of that CPU feature could just be opportunistic. Since many services are provided by the distribution, or by upstream teams at a particular organisation, it's not a sustainable solution to simply try to find and remove offending directives from these units. This commit presents a more direct solution -- a DisableControllers= directive that forcibly disallows a controller from being enabled within a subtree. --- man/systemd.resource-control.xml | 21 +++++++++++ src/core/cgroup.c | 16 ++++----- src/core/cgroup.h | 5 +++ src/core/dbus-cgroup.c | 50 ++++++++++++++++++--------- src/core/load-fragment-gperf.gperf.m4 | 1 + src/core/load-fragment.c | 35 +++++++++++++++++++ src/core/load-fragment.h | 1 + src/test/test-cgroup-mask.c | 14 +++++++- test/meson.build | 2 ++ test/nomem.slice | 5 +++ test/nomemleaf.service | 9 +++++ 11 files changed, 133 insertions(+), 26 deletions(-) create mode 100644 test/nomem.slice create mode 100644 test/nomemleaf.service diff --git a/man/systemd.resource-control.xml b/man/systemd.resource-control.xml index 4e282dad3d..aa7d9bcd59 100644 --- a/man/systemd.resource-control.xml +++ b/man/systemd.resource-control.xml @@ -736,6 +736,27 @@ + + DisableControllers= + + + Disables controllers from being enabled for a unit's children. If a controller listed is already in use + in its subtree, the controller will be removed from the subtree. This can be used to avoid child units being + able to implicitly or explicitly enable a controller. Defaults to not disabling any controllers. + + It may not be possible to successfully disable a controller if the unit or any child of the unit in + question delegates controllers to its children, as any delegated subtree of the cgroup hierarchy is unmanaged + by systemd. + + Multiple controllers may be specified, separated by spaces. You may also pass + DisableControllers= multiple times, in which case each new instance adds another controller + to disable. Passing DisableControllers= by itself with no controller name present resets + the disabled controller list. + + Valid controllers are , , , + , , , and . + + diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 13c595bbcd..b585e4bd2b 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1346,7 +1346,7 @@ CGroupMask unit_get_own_mask(Unit *u) { if (!c) return 0; - return cgroup_context_get_mask(c) | unit_get_bpf_mask(u) | unit_get_delegate_mask(u); + return (cgroup_context_get_mask(c) | unit_get_bpf_mask(u) | unit_get_delegate_mask(u)) & ~unit_get_ancestor_disable_mask(u); } CGroupMask unit_get_delegate_mask(Unit *u) { @@ -1461,6 +1461,7 @@ CGroupMask unit_get_target_mask(Unit *u) { mask = unit_get_own_mask(u) | unit_get_members_mask(u) | unit_get_siblings_mask(u); mask &= u->manager->cgroup_supported; + mask &= ~unit_get_ancestor_disable_mask(u); return mask; } @@ -1475,6 +1476,7 @@ CGroupMask unit_get_enable_mask(Unit *u) { mask = unit_get_members_mask(u); mask &= u->manager->cgroup_supported; + mask &= ~unit_get_ancestor_disable_mask(u); return mask; } @@ -1914,8 +1916,8 @@ static bool unit_has_mask_enables_realized( * we want to add is already added. */ return u->cgroup_realized && - ((u->cgroup_realized_mask | target_mask) & CGROUP_MASK_V1) == u->cgroup_realized_mask && - ((u->cgroup_enabled_mask | enable_mask) & CGROUP_MASK_V2) == u->cgroup_enabled_mask; + ((u->cgroup_realized_mask | target_mask) & CGROUP_MASK_V1) == (u->cgroup_realized_mask & CGROUP_MASK_V1) && + ((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) { @@ -1965,11 +1967,7 @@ static int unit_realize_cgroup_now_enable(Unit *u, ManagerState state) { new_target_mask = u->cgroup_realized_mask | target_mask; new_enable_mask = u->cgroup_enabled_mask | enable_mask; - r = unit_create_cgroup(u, new_target_mask, new_enable_mask, state); - if (r < 0) - return r; - - return 0; + return unit_create_cgroup(u, new_target_mask, new_enable_mask, state); } /* Controllers can only be disabled depth-first, from the leaves of the @@ -2043,7 +2041,7 @@ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { * h i j k l m n o * * 1. We want to realise cgroup "d" now. - * 2. cgroup "a" has DisableController=cpu in the associated unit. + * 2. cgroup "a" has DisableControllers=cpu in the associated unit. * 3. cgroup "k" just started requesting the memory controller. * * To make this work we must do the following in order: diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 828b6f0795..6d094e9ecd 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -118,6 +118,8 @@ struct CGroupContext { bool delegate; CGroupMask delegate_controllers; + + CGroupMask disable_controllers; }; /* Used when querying IP accounting data */ @@ -151,6 +153,9 @@ CGroupMask unit_get_delegate_mask(Unit *u); CGroupMask unit_get_members_mask(Unit *u); CGroupMask unit_get_siblings_mask(Unit *u); CGroupMask unit_get_subtree_mask(Unit *u); +CGroupMask unit_get_disable_mask(Unit *u); +CGroupMask unit_get_ancestor_disable_mask(Unit *u); + CGroupMask unit_get_target_mask(Unit *u); CGroupMask unit_get_enable_mask(Unit *u); diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index a2f532076d..53890bcafb 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -17,6 +17,38 @@ static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_cgroup_device_policy, cgroup_device_policy, CGroupDevicePolicy); +static int property_get_cgroup_mask( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + CGroupMask *mask = userdata; + CGroupController ctrl; + int r; + + assert(bus); + assert(reply); + + r = sd_bus_message_open_container(reply, 'a', "s"); + if (r < 0) + return r; + + for (ctrl = 0; ctrl < _CGROUP_CONTROLLER_MAX; ctrl++) { + if ((*mask & CGROUP_CONTROLLER_TO_MASK(ctrl)) == 0) + continue; + + r = sd_bus_message_append(reply, "s", cgroup_controller_to_string(ctrl)); + if (r < 0) + return r; + } + + return sd_bus_message_close_container(reply); +} + static int property_get_delegate_controllers( sd_bus *bus, const char *path, @@ -27,8 +59,6 @@ static int property_get_delegate_controllers( sd_bus_error *error) { CGroupContext *c = userdata; - CGroupController cc; - int r; assert(bus); assert(reply); @@ -37,20 +67,7 @@ static int property_get_delegate_controllers( if (!c->delegate) return sd_bus_message_append(reply, "as", 0); - r = sd_bus_message_open_container(reply, 'a', "s"); - if (r < 0) - return r; - - for (cc = 0; cc < _CGROUP_CONTROLLER_MAX; cc++) { - if ((c->delegate_controllers & CGROUP_CONTROLLER_TO_MASK(cc)) == 0) - continue; - - r = sd_bus_message_append(reply, "s", cgroup_controller_to_string(cc)); - if (r < 0) - return r; - } - - return sd_bus_message_close_container(reply); + return property_get_cgroup_mask(bus, path, interface, property, reply, &c->delegate_controllers, error); } static int property_get_io_device_weight( @@ -342,6 +359,7 @@ const sd_bus_vtable bus_cgroup_vtable[] = { SD_BUS_PROPERTY("IPAccounting", "b", bus_property_get_bool, offsetof(CGroupContext, ip_accounting), 0), SD_BUS_PROPERTY("IPAddressAllow", "a(iayu)", property_get_ip_address_access, offsetof(CGroupContext, ip_address_allow), 0), SD_BUS_PROPERTY("IPAddressDeny", "a(iayu)", property_get_ip_address_access, offsetof(CGroupContext, ip_address_deny), 0), + SD_BUS_PROPERTY("DisableControllers", "as", property_get_cgroup_mask, offsetof(CGroupContext, disable_controllers), 0), SD_BUS_VTABLE_END }; diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 97a707c144..cdbc67f885 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -192,6 +192,7 @@ $1.BlockIOWriteBandwidth, config_parse_blockio_bandwidth, 0, $1.TasksAccounting, config_parse_bool, 0, offsetof($1, cgroup_context.tasks_accounting) $1.TasksMax, config_parse_tasks_max, 0, offsetof($1, cgroup_context.tasks_max) $1.Delegate, config_parse_delegate, 0, offsetof($1, cgroup_context) +$1.DisableControllers, config_parse_disable_controllers, 0, offsetof($1, cgroup_context) $1.IPAccounting, config_parse_bool, 0, offsetof($1, cgroup_context.ip_accounting) $1.IPAddressAllow, config_parse_ip_address_access, 0, offsetof($1, cgroup_context.ip_address_allow) $1.IPAddressDeny, config_parse_ip_address_access, 0, offsetof($1, cgroup_context.ip_address_deny) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index d1988190c2..041b622314 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4323,6 +4323,41 @@ int config_parse_exit_status( return 0; } +int config_parse_disable_controllers( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { + + int r; + CGroupContext *c = data; + CGroupMask disabled_mask; + + /* 1. If empty, make all controllers eligible for use again. + * 2. If non-empty, merge all listed controllers, space separated. */ + + if (isempty(rvalue)) { + c->disable_controllers = 0; + return 0; + } + + r = cg_mask_from_string(rvalue, &disabled_mask); + if (r < 0 || disabled_mask <= 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Invalid cgroup string: %s, ignoring", rvalue); + return 0; + } + + c->disable_controllers |= disabled_mask; + + return 0; +} + #define FOLLOW_MAX 8 static int open_follow(char **filename, FILE **_f, Set *names, char **_final) { diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index 71d94a7762..e0d3b4ec3b 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -105,6 +105,7 @@ CONFIG_PARSER_PROTOTYPE(config_parse_log_extra_fields); CONFIG_PARSER_PROTOTYPE(config_parse_collect_mode); CONFIG_PARSER_PROTOTYPE(config_parse_pid_file); CONFIG_PARSER_PROTOTYPE(config_parse_exit_status); +CONFIG_PARSER_PROTOTYPE(config_parse_disable_controllers); /* gperf prototypes */ const struct ConfigPerfItem* load_fragment_gperf_lookup(const char *key, GPERF_LEN_TYPE length); diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index 143e7aa424..7f6c0c2772 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -30,7 +30,7 @@ static void log_cgroup_mask(CGroupMask got, CGroupMask expected) { static int test_cgroup_mask(void) { _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; _cleanup_(manager_freep) Manager *m = NULL; - Unit *son, *daughter, *parent, *root, *grandchild, *parent_deep; + Unit *son, *daughter, *parent, *root, *grandchild, *parent_deep, *nomem_parent, *nomem_leaf; int r; CGroupMask cpu_accounting_mask = get_cpu_accounting_mask(); @@ -68,11 +68,15 @@ static int test_cgroup_mask(void) { assert_se(manager_load_startable_unit_or_warn(m, "daughter.service", NULL, &daughter) >= 0); assert_se(manager_load_startable_unit_or_warn(m, "grandchild.service", NULL, &grandchild) >= 0); assert_se(manager_load_startable_unit_or_warn(m, "parent-deep.slice", NULL, &parent_deep) >= 0); + assert_se(manager_load_startable_unit_or_warn(m, "nomem.slice", NULL, &nomem_parent) >= 0); + assert_se(manager_load_startable_unit_or_warn(m, "nomemleaf.service", NULL, &nomem_leaf) >= 0); assert_se(UNIT_DEREF(son->slice) == parent); assert_se(UNIT_DEREF(daughter->slice) == parent); assert_se(UNIT_DEREF(parent_deep->slice) == parent); assert_se(UNIT_DEREF(grandchild->slice) == parent_deep); + assert_se(UNIT_DEREF(nomem_leaf->slice) == nomem_parent); root = UNIT_DEREF(parent->slice); + assert_se(UNIT_DEREF(nomem_parent->slice) == root); /* Verify per-unit cgroups settings. */ ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(son), CGROUP_MASK_CPU); @@ -80,6 +84,8 @@ static int test_cgroup_mask(void) { ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(grandchild), 0); ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(parent_deep), CGROUP_MASK_MEMORY); ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(parent), (CGROUP_MASK_IO | CGROUP_MASK_BLKIO)); + ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(nomem_parent), 0); + ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(nomem_leaf), (CGROUP_MASK_IO | CGROUP_MASK_BLKIO)); ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(root), 0); /* Verify aggregation of member masks */ @@ -88,6 +94,8 @@ static int test_cgroup_mask(void) { ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(grandchild), 0); ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(parent_deep), 0); ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(parent), (CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_MEMORY)); + ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(nomem_parent), (CGROUP_MASK_IO | CGROUP_MASK_BLKIO)); + ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(nomem_leaf), 0); ASSERT_CGROUP_MASK_JOINED(unit_get_members_mask(root), (CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY)); /* Verify aggregation of sibling masks. */ @@ -96,6 +104,8 @@ static int test_cgroup_mask(void) { ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(grandchild), 0); ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(parent_deep), (CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_MEMORY)); ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(parent), (CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY)); + ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(nomem_parent), (CGROUP_MASK_CPU | CGROUP_MASK_CPUACCT | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY)); + ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(nomem_leaf), (CGROUP_MASK_IO | CGROUP_MASK_BLKIO)); ASSERT_CGROUP_MASK_JOINED(unit_get_siblings_mask(root), (CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY)); /* Verify aggregation of target masks. */ @@ -104,6 +114,8 @@ static int test_cgroup_mask(void) { ASSERT_CGROUP_MASK(unit_get_target_mask(grandchild), 0); ASSERT_CGROUP_MASK(unit_get_target_mask(parent_deep), (CGROUP_MASK_EXTEND_JOINED(CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_MEMORY) & m->cgroup_supported)); ASSERT_CGROUP_MASK(unit_get_target_mask(parent), (CGROUP_MASK_EXTEND_JOINED(CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY) & m->cgroup_supported)); + ASSERT_CGROUP_MASK(unit_get_target_mask(nomem_parent), (CGROUP_MASK_EXTEND_JOINED(CGROUP_MASK_CPU | CGROUP_MASK_CPUACCT | CGROUP_MASK_IO | CGROUP_MASK_BLKIO) & m->cgroup_supported)); + ASSERT_CGROUP_MASK(unit_get_target_mask(nomem_leaf), (CGROUP_MASK_EXTEND_JOINED(CGROUP_MASK_IO | CGROUP_MASK_BLKIO) & m->cgroup_supported)); ASSERT_CGROUP_MASK(unit_get_target_mask(root), (CGROUP_MASK_EXTEND_JOINED(CGROUP_MASK_CPU | cpu_accounting_mask | CGROUP_MASK_IO | CGROUP_MASK_BLKIO | CGROUP_MASK_MEMORY) & m->cgroup_supported)); return 0; diff --git a/test/meson.build b/test/meson.build index bf02e39f43..d98bfb80d2 100644 --- a/test/meson.build +++ b/test/meson.build @@ -18,6 +18,8 @@ test_data_files = ''' hwdb/10-bad.hwdb journal-data/journal-1.txt journal-data/journal-2.txt + nomem.slice + nomemleaf.service parent-deep.slice parent.slice sched_idle_bad.service diff --git a/test/nomem.slice b/test/nomem.slice new file mode 100644 index 0000000000..9c5d208cb4 --- /dev/null +++ b/test/nomem.slice @@ -0,0 +1,5 @@ +[Unit] +Description=Nomem Parent Slice + +[Slice] +DisableControllers=memory diff --git a/test/nomemleaf.service b/test/nomemleaf.service new file mode 100644 index 0000000000..3cbaccb82f --- /dev/null +++ b/test/nomemleaf.service @@ -0,0 +1,9 @@ +[Unit] +Description=Nomem Leaf Service + +[Service] +Slice=nomem.slice +Type=oneshot +ExecStart=/bin/true +IOWeight=200 +MemoryAccounting=true