From 0f58036abf34e92313105839658420e95acfdd7e Mon Sep 17 00:00:00 2001 From: Tejun Heo Date: Mon, 21 Nov 2016 14:45:53 -0500 Subject: [PATCH] core: simplify cg_[all_]unified() cg_[all_]unified() test whether a specific controller or all controllers are on the unified hierarchy. While what's being asked is a simple binary question, the callers must assume that the functions may fail any time, which unnecessarily complicates their usages. This complication is unnecessary. Internally, the test result is cached anyway and there are only a few places where the test actually needs to be performed. This patch simplifies cg_[all_]unified(). * cg_[all_]unified() are updated to return bool. If the result can't be decided, assertion failure is triggered. Error handlings from their callers are dropped. * cg_unified_flush() is updated to calculate the new result synchrnously and return whether it succeeded or not. Places which need to flush the test result are updated to test for failure. This ensures that all the following cg_[all_]unified() tests succeed. * Places which expected possible cg_[all_]unified() failures are updated to call and test cg_unified_flush() before calling cg_[all_]unified(). This includes functions used while setting up mounts during boot and manager_setup_cgroup(). --- src/basic/cgroup-util.c | 112 +++++++++------------------------------- src/basic/cgroup-util.h | 1 - src/core/cgroup.c | 54 ++++++++++--------- 3 files changed, 52 insertions(+), 115 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 323720cae..bcb022896 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -210,12 +210,6 @@ 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_rmdir(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); - if (r < 0) - log_warning_errno(r, "Failed to remove compat systemd cgroup %s: %m", path); - } - return 0; } @@ -554,13 +548,6 @@ static const char *controller_to_dirname(const char *controller) { * just cuts off the name= prefixed used for named * hierarchies, if it is specified. */ - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { - if (cg_hybrid_unified()) - controller = SYSTEMD_CGROUP_CONTROLLER_HYBRID; - else - controller = SYSTEMD_CGROUP_CONTROLLER_LEGACY; - } - e = startswith(controller, "name="); if (e) return e; @@ -719,7 +706,7 @@ static int trim_cb(const char *path, const struct stat *sb, int typeflag, struct int cg_trim(const char *controller, const char *path, bool delete_root) { _cleanup_free_ char *fs = NULL; - int r = 0, q; + int r = 0; assert(path); @@ -742,12 +729,6 @@ 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_trim(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, delete_root); - if (q < 0) - log_warning_errno(q, "Failed to trim compat systemd cgroup %s: %m", path); - } - return r; } @@ -771,12 +752,6 @@ int cg_create(const char *controller, const char *path) { return -errno; } - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { - r = cg_create(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); - if (r < 0) - log_warning_errno(r, "Failed to create compat systemd cgroup %s: %m", path); - } - return 1; } @@ -814,17 +789,7 @@ int cg_attach(const char *controller, const char *path, pid_t pid) { xsprintf(c, PID_FMT "\n", pid); - r = write_string_file(fs, c, 0); - if (r < 0) - return r; - - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { - 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); - } - - return 0; + return write_string_file(fs, c, 0); } int cg_attach_fallback(const char *controller, const char *path, pid_t pid) { @@ -874,17 +839,7 @@ int cg_set_group_access( if (r < 0) return r; - r = chmod_and_chown(fs, mode, uid, gid); - if (r < 0) - return r; - - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { - 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); - } - - return 0; + return chmod_and_chown(fs, mode, uid, gid); } int cg_set_task_access( @@ -913,18 +868,13 @@ int cg_set_task_access( if (r < 0) return r; - if (!cg_unified(controller)) { - /* 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 (cg_unified(controller)) + return 0; - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER) && cg_hybrid_unified()) { - 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); - } + /* 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); return 0; } @@ -970,7 +920,7 @@ int cg_get_xattr(const char *controller, const char *path, const char *name, voi int cg_pid_get_path(const char *controller, pid_t pid, char **path) { _cleanup_fclose_ FILE *f = NULL; char line[LINE_MAX]; - const char *fs, *controller_str; + const char *fs; size_t cs = 0; bool unified; @@ -984,14 +934,8 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { controller = SYSTEMD_CGROUP_CONTROLLER; unified = cg_unified(controller); - if (!unified) { - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) - controller_str = SYSTEMD_CGROUP_CONTROLLER_LEGACY; - else - controller_str = controller; - - cs = strlen(controller_str); - } + if (!unified) + cs = strlen(controller); fs = procfs_file_alloca(pid, "cgroup"); log_debug_elogind("Searching for PID %u in \"%s\" (controller \"%s\")", @@ -1030,7 +974,7 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { *e = 0; FOREACH_WORD_SEPARATOR(word, k, l, ",", state) { - if (k == cs && memcmp(word, controller_str, cs) == 0) { + if (k == cs && memcmp(word, controller, cs) == 0) { found = true; break; } @@ -1916,9 +1860,6 @@ bool cg_controller_is_valid(const char *p) { if (!p) return false; - if (streq(p, SYSTEMD_CGROUP_CONTROLLER)) - return true; - s = startswith(p, "name="); if (s) p = s; @@ -2358,16 +2299,11 @@ static int cg_update_unified(void) { if (F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) unified_cache = CGROUP_UNIFIED_ALL; else if (F_TYPE_EQUAL(fs.f_type, TMPFS_MAGIC)) { - if (statfs("/sys/fs/cgroup/unified/", &fs) == 0 && - F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC)) - unified_cache = CGROUP_UNIFIED_SYSTEMD; - else { - if (statfs("/sys/fs/cgroup/systemd/", &fs) < 0) - return -errno; - if (!F_TYPE_EQUAL(fs.f_type, CGROUP_SUPER_MAGIC)) - return -ENOMEDIUM; - unified_cache = CGROUP_UNIFIED_NONE; - } + if (statfs("/sys/fs/cgroup/systemd/", &fs) < 0) + return -errno; + + unified_cache = F_TYPE_EQUAL(fs.f_type, CGROUP2_SUPER_MAGIC) ? + CGROUP_UNIFIED_SYSTEMD : CGROUP_UNIFIED_NONE; } else return -ENOMEDIUM; #else @@ -2398,13 +2334,6 @@ bool cg_all_unified(void) { } #if 0 /// UNNEEDED by elogind -bool cg_hybrid_unified(void) { - - assert(cg_update_unified() >= 0); - - return unified_cache == CGROUP_UNIFIED_SYSTEMD; -} - int cg_unified_flush(void) { unified_cache = CGROUP_UNIFIED_UNKNOWN; @@ -2509,6 +2438,11 @@ bool cg_is_legacy_wanted(void) { /* The meaning of the kernel option is reversed wrt. to the return value * of this function, hence the negation. */ return (wanted = r > 0 ? !b : false); + return (wanted = r > 0 ? b : false); +} + +bool cg_is_legacy_systemd_controller_wanted(void) { + return cg_is_legacy_wanted() && !cg_is_unified_systemd_controller_wanted(); } #endif // 0 diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index f78f2355b..5fbea23d0 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -255,7 +255,6 @@ bool cg_ns_supported(void); #if 0 /// UNNEEDED by elogind bool cg_all_unified(void); -bool cg_hybrid_unified(void); bool cg_unified(const char *controller); int cg_unified_flush(void); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 5ee5bd795..e306b8bce 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -288,14 +288,24 @@ static int lookup_block_device(const char *p, dev_t *dev) { static int whitelist_device(const char *path, const char *node, const char *acc) { char buf[2+DECIMAL_STR_MAX(dev_t)*2+2+4]; struct stat st; + bool ignore_notfound; int r; assert(path); assert(acc); + if (node[0] == '-') { + /* Non-existent paths starting with "-" must be silently ignored */ + node++; + ignore_notfound = true; + } else + ignore_notfound = false; + if (stat(node, &st) < 0) { - log_warning("Couldn't stat device %s", node); - return -errno; + if (errno == ENOENT && ignore_notfound) + return 0; + + return log_warning_errno(errno, "Couldn't stat device %s: %m", node); } if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) { @@ -669,7 +679,7 @@ static void cgroup_context_apply(Unit *u, CGroupMask mask, ManagerState state) { bool has_weight = cgroup_context_has_cpu_weight(c); bool has_shares = cgroup_context_has_cpu_shares(c); - if (cg_all_unified() > 0) { + if (cg_all_unified()) { uint64_t weight; if (has_weight) @@ -849,7 +859,7 @@ static void cgroup_context_apply(Unit *u, CGroupMask mask, ManagerState state) { } if ((mask & CGROUP_MASK_MEMORY) && !is_root) { - if (cg_all_unified() > 0) { + if (cg_all_unified()) { uint64_t max; uint64_t swap_max = CGROUP_LIMIT_MAX; @@ -916,8 +926,8 @@ static void cgroup_context_apply(Unit *u, CGroupMask mask, ManagerState state) { "/dev/pts/ptmx\0" "rw\0" /* /dev/pts/ptmx may not be duplicated, but accessed */ /* Allow /run/elogind/inaccessible/{chr,blk} devices for mapping InaccessiblePaths */ /* Allow /run/systemd/inaccessible/{chr,blk} devices for mapping InaccessiblePaths */ - "/run/systemd/inaccessible/chr\0" "rwm\0" - "/run/systemd/inaccessible/blk\0" "rwm\0"; + "-/run/systemd/inaccessible/chr\0" "rwm\0" + "-/run/systemd/inaccessible/blk\0" "rwm\0"; const char *x, *y; @@ -1025,7 +1035,7 @@ CGroupMask unit_get_own_mask(Unit *u) { e = unit_get_exec_context(u); if (!e || exec_context_maintains_privileges(e) || - cg_all_unified() > 0) + cg_all_unified()) return _CGROUP_MASK_ALL; } @@ -1252,10 +1262,7 @@ int unit_watch_cgroup(Unit *u) { return 0; /* 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"); - if (r == 0) + if (!cg_unified(SYSTEMD_CGROUP_CONTROLLER)) return 0; /* Don't watch the root slice, it's pointless. */ @@ -1675,7 +1682,7 @@ 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 */ + if (cg_unified(SYSTEMD_CGROUP_CONTROLLER)) /* On unified we can use proper notifications */ return 0; return unit_watch_pids_in_path(u, u->cgroup_path); @@ -1749,7 +1756,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; char *e; assert(m); @@ -1790,16 +1797,13 @@ 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) + if (cg_all_unified()) log_debug("Unified cgroup hierarchy is located at %s.", path); - else if (systemd_unified > 0) + else if (cg_unified(SYSTEMD_CGROUP_CONTROLLER)) 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); @@ -1808,7 +1812,7 @@ int manager_setup_cgroup(Manager *m) { const char *scope_path; /* 3. Install agent */ - if (systemd_unified) { + if (cg_unified(SYSTEMD_CGROUP_CONTROLLER)) { /* In the unified hierarchy we can get * cgroup empty notifications via inotify. */ @@ -1886,7 +1890,7 @@ int manager_setup_cgroup(Manager *m) { return log_error_errno(errno, "Failed to open pin file: %m"); /* 6. Always enable hierarchical support if it exists... */ - if (!all_unified) + if (!cg_all_unified()) (void) cg_set_attribute("memory", "/", "memory.use_hierarchy", "1"); } @@ -2037,7 +2041,7 @@ 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) + if (!cg_all_unified()) r = cg_get_attribute("memory", u->cgroup_path, "memory.usage_in_bytes", &v); else r = cg_get_attribute("memory", u->cgroup_path, "memory.current", &v); @@ -2082,7 +2086,7 @@ static int unit_get_cpu_usage_raw(Unit *u, nsec_t *ret) { if (!u->cgroup_path) return -ENODATA; - if (cg_all_unified() > 0) { + if (cg_all_unified()) { const char *keys[] = { "usage_usec", NULL }; _cleanup_free_ char *val = NULL; uint64_t us; -- 2.30.2