From 37199e725d1f2bb3ec1cdb582172401697bd4b7c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Feb 2017 17:52:58 +0100 Subject: [PATCH] cgroup: change cg_unified() to possibly return errors again We use our cgroup APIs in various contexts, including from our libraries sd-login, sd-bus. As we don#t control those environments we can't rely that the unified cgroup setup logic succeeds, and hence really shouldn't assert on it. This more or less reverts 415fc41ceaeada2e32639f24f134b1c248b9e43f. --- src/basic/cgroup-util.c | 127 ++++++++++++++++++++++++++++++---------- src/basic/cgroup-util.h | 6 +- src/core/cgroup.c | 57 +++++++++++------- 3 files changed, 136 insertions(+), 54 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 48c21a99b..8cd064f0b 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -210,7 +210,13 @@ int cg_rmdir(const char *controller, const char *path) { if (r < 0 && errno != ENOENT) return -errno; - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + r = cg_hybrid_unified(); + if (r < 0) + return r; + if (r == 0) + return 0; + + if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_rmdir(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); if (r < 0) log_warning_errno(r, "Failed to remove compat systemd cgroup %s: %m", path); @@ -555,7 +561,7 @@ static const char *controller_to_dirname(const char *controller) { * hierarchies, if it is specified. */ if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { - if (cg_hybrid_unified()) + if (cg_hybrid_unified() > 0) controller = SYSTEMD_CGROUP_CONTROLLER_HYBRID; else controller = SYSTEMD_CGROUP_CONTROLLER_LEGACY; @@ -642,7 +648,10 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch if (!cg_controller_is_valid(controller)) return -EINVAL; - if (cg_all_unified()) + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) r = join_path_unified(path, suffix, fs); else r = join_path_legacy(controller, path, suffix, fs); @@ -654,6 +663,7 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch } static int controller_is_accessible(const char *controller) { + int r; assert(controller); @@ -665,7 +675,10 @@ static int controller_is_accessible(const char *controller) { if (!cg_controller_is_valid(controller)) return -EINVAL; - if (cg_all_unified()) { + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) { /* We don't support named hierarchies if we are using * the unified hierarchy. */ @@ -742,7 +755,10 @@ int cg_trim(const char *controller, const char *path, bool delete_root) { return -errno; } - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + q = cg_hybrid_unified(); + if (q < 0) + return q; + if (q > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { q = cg_trim(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, delete_root); if (q < 0) log_warning_errno(q, "Failed to trim compat systemd cgroup %s: %m", path); @@ -771,7 +787,11 @@ int cg_create(const char *controller, const char *path) { return -errno; } - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + r = cg_hybrid_unified(); + if (r < 0) + return r; + + if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_create(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); if (r < 0) log_warning_errno(r, "Failed to create compat systemd cgroup %s: %m", path); @@ -818,7 +838,11 @@ int cg_attach(const char *controller, const char *path, pid_t pid) { if (r < 0) return r; - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + r = cg_hybrid_unified(); + if (r < 0) + return r; + + if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_attach(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, pid); if (r < 0) log_warning_errno(r, "Failed to attach %d to compat systemd cgroup %s: %m", pid, path); @@ -878,7 +902,10 @@ int cg_set_group_access( if (r < 0) return r; - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + r = cg_hybrid_unified(); + if (r < 0) + return r; + if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_set_group_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); if (r < 0) log_warning_errno(r, "Failed to set group access on compat systemd cgroup %s: %m", path); @@ -913,14 +940,20 @@ int cg_set_task_access( if (r < 0) return r; - if (!cg_unified(controller)) { + r = cg_unified(controller); + if (r < 0) + return r; + if (r == 0) { /* Compatibility, Always keep values for "tasks" in sync with * "cgroup.procs" */ if (cg_get_path(controller, path, "tasks", &procs) >= 0) (void) chmod_and_chown(procs, mode, uid, gid); } - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { + r = cg_hybrid_unified(); + if (r < 0) + return r; + if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); if (r < 0) log_warning_errno(r, "Failed to set task access on compat systemd cgroup %s: %m", path); @@ -972,7 +1005,7 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { char line[LINE_MAX]; const char *fs, *controller_str; size_t cs = 0; - bool unified; + int unified; assert(path); assert(pid >= 0); @@ -984,7 +1017,9 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { controller = SYSTEMD_CGROUP_CONTROLLER; unified = cg_unified(controller); - if (!unified) { + if (unified < 0) + return unified; + if (unified == 0) { if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) controller_str = SYSTEMD_CGROUP_CONTROLLER_LEGACY; else @@ -1059,7 +1094,10 @@ int cg_install_release_agent(const char *controller, const char *agent) { assert(agent); - if (cg_unified(controller)) /* doesn't apply to unified hierarchy */ + r = cg_unified(controller); + if (r < 0) + return r; + if (r > 0) /* doesn't apply to unified hierarchy */ return -EOPNOTSUPP; r = cg_get_path(controller, NULL, "release_agent", &fs); @@ -1107,7 +1145,10 @@ int cg_uninstall_release_agent(const char *controller) { _cleanup_free_ char *fs = NULL; int r; - if (cg_unified(controller)) /* Doesn't apply to unified hierarchy */ + r = cg_unified(controller); + if (r < 0) + return r; + if (r > 0) /* Doesn't apply to unified hierarchy */ return -EOPNOTSUPP; r = cg_get_path(controller, NULL, "notify_on_release", &fs); @@ -1160,7 +1201,10 @@ int cg_is_empty_recursive(const char *controller, const char *path) { if (controller && (isempty(path) || path_equal(path, "/"))) return false; - if (cg_unified(controller)) { + r = cg_unified(controller); + if (r < 0) + return r; + if (r > 0) { _cleanup_free_ char *t = NULL; /* On the unified hierarchy we can check empty state @@ -2087,7 +2131,10 @@ int cg_create_everywhere(CGroupMask supported, CGroupMask mask, const char *path return r; /* If we are in the unified hierarchy, we are done now */ - if (cg_all_unified()) + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) return 0; /* Otherwise, do the same in the other hierarchies */ @@ -2114,7 +2161,10 @@ int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid, cg_m if (r < 0) return r; - if (cg_all_unified()) + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) return 0; for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { @@ -2155,7 +2205,7 @@ int cg_attach_many_everywhere(CGroupMask supported, const char *path, Set* pids, int cg_migrate_everywhere(CGroupMask supported, const char *from, const char *to, cg_migrate_callback_t to_callback, void *userdata) { CGroupController c; - int r = 0; + int r = 0, q; if (!path_equal(from, to)) { r = cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, from, SYSTEMD_CGROUP_CONTROLLER, to, CGROUP_REMOVE); @@ -2163,7 +2213,10 @@ int cg_migrate_everywhere(CGroupMask supported, const char *from, const char *to return r; } - if (cg_all_unified()) + q = cg_all_unified(); + if (q < 0) + return q; + if (q > 0) return r; for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { @@ -2187,13 +2240,16 @@ int cg_migrate_everywhere(CGroupMask supported, const char *from, const char *to int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root) { CGroupController c; - int r; + int r, q; r = cg_trim(SYSTEMD_CGROUP_CONTROLLER, path, delete_root); if (r < 0) return r; - if (cg_all_unified()) + q = cg_all_unified(); + if (q < 0) + return q; + if (q > 0) return r; for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { @@ -2217,7 +2273,10 @@ int cg_mask_supported(CGroupMask *ret) { * includes controllers we can make sense of and that are * actually accessible. */ - if (cg_all_unified()) { + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) { _cleanup_free_ char *root = NULL, *controllers = NULL, *path = NULL; const char *c; @@ -2401,9 +2460,12 @@ static int cg_update_unified(void) { return 0; } -bool cg_unified(const char *controller) { +int cg_unified(const char *controller) { + int r; - assert(cg_update_unified() >= 0); + r = cg_update_unified(); + if (r < 0) + return r; if (unified_cache == CGROUP_UNIFIED_NONE) return false; @@ -2414,15 +2476,17 @@ bool cg_unified(const char *controller) { return streq_ptr(controller, SYSTEMD_CGROUP_CONTROLLER); } -bool cg_all_unified(void) { - +int cg_all_unified(void) { return cg_unified(NULL); } #if 0 /// UNNEEDED by elogind -bool cg_hybrid_unified(void) { +int cg_hybrid_unified(void) { + int r; - assert(cg_update_unified() >= 0); + r = cg_update_unified(); + if (r < 0) + return r; return unified_cache == CGROUP_UNIFIED_SYSTEMD && !unified_systemd_v232; } @@ -2443,7 +2507,10 @@ int cg_enable_everywhere(CGroupMask supported, CGroupMask mask, const char *p) { if (supported == 0) return 0; - if (!cg_all_unified()) /* on the legacy hiearchy there's no joining of controllers defined */ + r = cg_all_unified(); + if (r < 0) + return r; + if (r == 0) /* on the legacy hiearchy there's no joining of controllers defined */ return 0; r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, p, "cgroup.subtree_control", &fs); @@ -2486,7 +2553,7 @@ bool cg_is_unified_wanted(void) { /* If the hierarchy is already mounted, then follow whatever * was chosen for it. */ if (cg_unified_flush() >= 0) - return (wanted = cg_all_unified()); + return (wanted = unified_cache >= CGROUP_UNIFIED_ALL); /* Otherwise, let's see what the kernel command line has to say. * Since checking is expensive, cache a non-error result. */ diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index 33b7ba7f4..942f48228 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -254,9 +254,9 @@ bool cg_ns_supported(void); #endif // 0 #if 0 /// UNNEEDED by elogind -bool cg_all_unified(void); -bool cg_hybrid_unified(void); -bool cg_unified(const char *controller); +int cg_all_unified(void); +int cg_hybrid_unified(void); +int cg_unified(const char *controller); int cg_unified_flush(void); bool cg_is_unified_wanted(void); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 1e14d9fc4..bc5ff23a1 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -860,8 +860,7 @@ static void cgroup_context_apply(Unit *u, CGroupMask mask, ManagerState state) { if ((mask & CGROUP_MASK_MEMORY) && !is_root) { if (cg_all_unified() > 0) { - uint64_t max; - uint64_t swap_max = CGROUP_LIMIT_MAX; + uint64_t max, swap_max = CGROUP_LIMIT_MAX; if (cgroup_context_has_unified_memory_config(c)) { max = c->memory_max; @@ -1264,7 +1263,7 @@ int unit_watch_cgroup(Unit *u) { /* Only applies to the unified hierarchy */ r = cg_unified(SYSTEMD_CGROUP_CONTROLLER); if (r < 0) - return log_unit_error_errno(u, r, "Failed detect whether the unified hierarchy is used: %m"); + return log_error_errno(r, "Failed to determine whether the name=systemd hierarchy is unified: %m"); if (r == 0) return 0; @@ -1675,6 +1674,8 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) { } int unit_watch_all_pids(Unit *u) { + int r; + assert(u); /* Adds all PIDs from our cgroup to the set of PIDs we @@ -1685,7 +1686,10 @@ int unit_watch_all_pids(Unit *u) { if (!u->cgroup_path) return -ENOENT; - if (cg_unified(SYSTEMD_CGROUP_CONTROLLER) > 0) /* On unified we can use proper notifications */ + r = cg_unified(SYSTEMD_CGROUP_CONTROLLER); + if (r < 0) + return r; + if (r > 0) /* On unified we can use proper notifications */ return 0; return unit_watch_pids_in_path(u, u->cgroup_path); @@ -1759,7 +1763,7 @@ static int on_cgroup_inotify_event(sd_event_source *s, int fd, uint32_t revents, int manager_setup_cgroup(Manager *m) { _cleanup_free_ char *path = NULL; CGroupController c; - int r, all_unified, systemd_unified; + int r, all_unified; char *e; assert(m); @@ -1800,25 +1804,30 @@ int manager_setup_cgroup(Manager *m) { if (r < 0) return log_error_errno(r, "Cannot find cgroup mount point: %m"); - all_unified = cg_all_unified(); - systemd_unified = cg_unified(SYSTEMD_CGROUP_CONTROLLER); - - if (all_unified < 0 || systemd_unified < 0) - return log_error_errno(all_unified < 0 ? all_unified : systemd_unified, - "Couldn't determine if we are running in the unified hierarchy: %m"); + r = cg_unified_flush(); + if (r < 0) + return log_error_errno(r, "Couldn't determine if we are running in the unified hierarchy: %m"); - if (all_unified > 0) + all_unified = cg_all_unified(); + if (r < 0) + return log_error_errno(r, "Couldn't determine whether we are in all unified mode: %m"); + if (r > 0) log_debug("Unified cgroup hierarchy is located at %s.", path); - else if (systemd_unified > 0) - log_debug("Unified cgroup hierarchy is located at %s. Controllers are on legacy hierarchies.", path); - else - log_debug("Using cgroup controller " SYSTEMD_CGROUP_CONTROLLER ". File system hierarchy is at %s.", path); + else { + r = cg_unified(SYSTEMD_CGROUP_CONTROLLER); + if (r < 0) + return log_error_errno(r, "Failed to determine whether systemd's own controller is in unified mode: %m"); + if (r > 0) + log_debug("Unified cgroup hierarchy is located at %s. Controllers are on legacy hierarchies.", path); + else + log_debug("Using cgroup controller " SYSTEMD_CGROUP_CONTROLLER_LEGACY ". File system hierarchy is at %s.", path); + } if (!m->test_run) { const char *scope_path; /* 3. Install agent */ - if (systemd_unified) { + if (cg_unified(SYSTEMD_CGROUP_CONTROLLER) > 0) { /* In the unified hierarchy we can get * cgroup empty notifications via inotify. */ @@ -2047,10 +2056,13 @@ int unit_get_memory_current(Unit *u, uint64_t *ret) { if ((u->cgroup_realized_mask & CGROUP_MASK_MEMORY) == 0) return -ENODATA; - if (cg_all_unified() <= 0) - r = cg_get_attribute("memory", u->cgroup_path, "memory.usage_in_bytes", &v); - else + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) r = cg_get_attribute("memory", u->cgroup_path, "memory.current", &v); + else + r = cg_get_attribute("memory", u->cgroup_path, "memory.usage_in_bytes", &v); if (r == -ENOENT) return -ENODATA; if (r < 0) @@ -2092,7 +2104,10 @@ static int unit_get_cpu_usage_raw(Unit *u, nsec_t *ret) { if (!u->cgroup_path) return -ENODATA; - if (cg_all_unified() > 0) { + r = cg_all_unified(); + if (r < 0) + return r; + if (r > 0) { const char *keys[] = { "usage_usec", NULL }; _cleanup_free_ char *val = NULL; uint64_t us; -- 2.30.2