From 38f3b737dc3bfc62005bd6340ef9c0d0b6c5deef Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 19:22:11 +0200 Subject: [PATCH 01/12] core/cgroup: correct macro alignment --- src/core/cgroup.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 1581a88cb7..d258bcb3ae 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -496,10 +496,10 @@ void unit_reset_io_accounting_last(Unit *u); int unit_reset_io_accounting(Unit *u); int unit_reset_accounting(Unit *u); -#define UNIT_CGROUP_BOOL(u, name) \ - ({ \ - CGroupContext *cc = unit_get_cgroup_context(u); \ - cc ? cc->name : false; \ +#define UNIT_CGROUP_BOOL(u, name) \ + ({ \ + CGroupContext *cc = unit_get_cgroup_context(u); \ + cc ? cc->name : false; \ }) bool manager_owns_host_root_cgroup(Manager *m); From 8153be97c46dec725fdfaa3358d518ce350bb2ec Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 2 Jun 2024 06:37:08 +0800 Subject: [PATCH 02/12] core/cgroup: use > 0 comparison rather than == 1 --- src/core/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 19028b810b..bb78ded3eb 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3549,7 +3549,7 @@ bool unit_maybe_release_cgroup(Unit *u) { * failed) we need the cgroup paths to continue to be tracked by the manager so they can be looked up * and cleaned up later. */ r = unit_cgroup_is_empty(u); - if (r == 1) { + if (r > 0) { unit_release_cgroup(u); return true; } From c97c79aded8d313237e0952062a2cce71fb88eb5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 19:23:40 +0200 Subject: [PATCH 03/12] core/cgroup: check CGroupRuntime.cgroup_path rather than _realized for freezer The same check is used everywhere else. --- src/core/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index bb78ded3eb..9296a5e4ac 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -5134,7 +5134,7 @@ int unit_cgroup_freezer_action(Unit *u, FreezerAction action) { unit_next_freezer_state(u, action, &next, &target); CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_realized) { + if (!crt || !crt->cgroup_path) { /* No realized cgroup = nothing to freeze */ u->freezer_state = freezer_state_finish(next); return 0; From 4442aef08e0fe8ba381b580455f7eb281c5a28a1 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 15 Jun 2024 13:15:08 +0200 Subject: [PATCH 04/12] core/cgroup: make unit_has_host_root_cgroup take const Unit* --- src/core/cgroup.c | 3 ++- src/core/cgroup.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 9296a5e4ac..41d7dd5ada 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -102,8 +102,9 @@ bool unit_has_startup_cgroup_constraints(Unit *u) { c->startup_memory_low_set; } -bool unit_has_host_root_cgroup(Unit *u) { +bool unit_has_host_root_cgroup(const Unit *u) { assert(u); + assert(u->manager); /* Returns whether this unit manages the root cgroup. This will return true if this unit is the root slice and * the manager manages the root cgroup. */ diff --git a/src/core/cgroup.h b/src/core/cgroup.h index d258bcb3ae..08fccf9612 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -503,7 +503,7 @@ int unit_reset_accounting(Unit *u); }) bool manager_owns_host_root_cgroup(Manager *m); -bool unit_has_host_root_cgroup(Unit *u); +bool unit_has_host_root_cgroup(const Unit *u); bool unit_has_startup_cgroup_constraints(Unit *u); From 5a8c2c95598f17fb79cca07365e70e54a7abcc88 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 19 Jun 2024 21:07:07 +0200 Subject: [PATCH 05/12] core/cgroup: call bpf_firewall_close in cgroup_runtime_free No functional change, just deduplicate default values in cgroup_runtime_free() and remove pointless call in unit_free() (at the time it's called the CGRuntime has been destroyed already). --- src/core/bpf-firewall.c | 8 ++------ src/core/bpf-firewall.h | 3 ++- src/core/cgroup.c | 15 +-------------- src/core/unit.c | 2 -- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/core/bpf-firewall.c b/src/core/bpf-firewall.c index 29f62693ee..26f50500ce 100644 --- a/src/core/bpf-firewall.c +++ b/src/core/bpf-firewall.c @@ -970,12 +970,8 @@ void emit_bpf_firewall_warning(Unit *u) { warned = true; } -void bpf_firewall_close(Unit *u) { - assert(u); - - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt) - return; +void bpf_firewall_close(CGroupRuntime *crt) { + assert(crt); crt->ip_accounting_ingress_map_fd = safe_close(crt->ip_accounting_ingress_map_fd); crt->ip_accounting_egress_map_fd = safe_close(crt->ip_accounting_egress_map_fd); diff --git a/src/core/bpf-firewall.h b/src/core/bpf-firewall.h index 58b401f834..f7b72d0ea1 100644 --- a/src/core/bpf-firewall.h +++ b/src/core/bpf-firewall.h @@ -3,6 +3,7 @@ #include +#include "cgroup.h" #include "unit.h" enum { @@ -22,4 +23,4 @@ int bpf_firewall_reset_accounting(int map_fd); void emit_bpf_firewall_warning(Unit *u); -void bpf_firewall_close(Unit *u); +void bpf_firewall_close(CGroupRuntime *crt); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 41d7dd5ada..dec47e14f7 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -5266,20 +5266,7 @@ CGroupRuntime *cgroup_runtime_free(CGroupRuntime *crt) { #endif fdset_free(crt->initial_restrict_ifaces_link_fds); - safe_close(crt->ipv4_allow_map_fd); - safe_close(crt->ipv6_allow_map_fd); - safe_close(crt->ipv4_deny_map_fd); - safe_close(crt->ipv6_deny_map_fd); - - bpf_program_free(crt->ip_bpf_ingress); - bpf_program_free(crt->ip_bpf_ingress_installed); - bpf_program_free(crt->ip_bpf_egress); - bpf_program_free(crt->ip_bpf_egress_installed); - - set_free(crt->ip_bpf_custom_ingress); - set_free(crt->ip_bpf_custom_ingress_installed); - set_free(crt->ip_bpf_custom_egress); - set_free(crt->ip_bpf_custom_egress_installed); + bpf_firewall_close(crt); free(crt->cgroup_path); diff --git a/src/core/unit.c b/src/core/unit.c index 0e931be484..9d64103aef 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -844,8 +844,6 @@ Unit* unit_free(Unit *u) { if (u->in_release_resources_queue) LIST_REMOVE(release_resources_queue, u->manager->release_resources_queue, u); - bpf_firewall_close(u); - condition_free_list(u->conditions); condition_free_list(u->asserts); From bc347edfe075f36bada0e1e7aefd62b10fcad4a7 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 2 Jun 2024 07:50:09 +0800 Subject: [PATCH 06/12] core: unify reset_accounting handling Since the introduction of CGroupRuntime, there's no need to call *_reset_accounting in unit_new(), hence make those static. While at it, refrain from hardcoding default values in cgroup_runtime_new(), but call the corresponding funcs. This also corrects the default value of io_accounting_base. Fixes #33482 --- src/core/cgroup.c | 167 ++++++++++++++++++++-------------------------- src/core/cgroup.h | 9 +-- src/core/unit.c | 3 - 3 files changed, 74 insertions(+), 105 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index dec47e14f7..47413e6bc3 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -4593,15 +4593,14 @@ int unit_get_tasks_current(Unit *u, uint64_t *ret) { return cg_get_attribute_as_uint64("pids", crt->cgroup_path, "pids.current", ret); } -static int unit_get_cpu_usage_raw(Unit *u, nsec_t *ret) { - uint64_t ns; +static int unit_get_cpu_usage_raw(const Unit *u, const CGroupRuntime *crt, nsec_t *ret) { int r; assert(u); + assert(crt); assert(ret); - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) + if (!crt->cgroup_path) return -ENODATA; /* The root cgroup doesn't expose this information, let's get it from /proc instead */ @@ -4615,25 +4614,24 @@ static int unit_get_cpu_usage_raw(Unit *u, nsec_t *ret) { r = cg_all_unified(); if (r < 0) return r; - if (r > 0) { - _cleanup_free_ char *val = NULL; - uint64_t us; - - r = cg_get_keyed_attribute("cpu", crt->cgroup_path, "cpu.stat", STRV_MAKE("usage_usec"), &val); - if (IN_SET(r, -ENOENT, -ENXIO)) - return -ENODATA; - if (r < 0) - return r; - - r = safe_atou64(val, &us); - if (r < 0) - return r; - - ns = us * NSEC_PER_USEC; - } else + if (r == 0) return cg_get_attribute_as_uint64("cpuacct", crt->cgroup_path, "cpuacct.usage", ret); - *ret = ns; + _cleanup_free_ char *val = NULL; + uint64_t us; + + r = cg_get_keyed_attribute("cpu", crt->cgroup_path, "cpu.stat", STRV_MAKE("usage_usec"), &val); + if (IN_SET(r, -ENOENT, -ENXIO)) + return -ENODATA; + if (r < 0) + return r; + + r = safe_atou64(val, &us); + if (r < 0) + return r; + + *ret = us * NSEC_PER_USEC; + return 0; } @@ -4654,7 +4652,7 @@ int unit_get_cpu_usage(Unit *u, nsec_t *ret) { if (!UNIT_CGROUP_BOOL(u, cpu_accounting)) return -ENODATA; - r = unit_get_cpu_usage_raw(u, &ns); + r = unit_get_cpu_usage_raw(u, crt, &ns); if (r == -ENODATA && crt->cpu_usage_last != NSEC_INFINITY) { /* If we can't get the CPU usage anymore (because the cgroup was already removed, for example), use our * cached value. */ @@ -4771,22 +4769,27 @@ int unit_get_effective_limit(Unit *u, CGroupLimitType type, uint64_t *ret) { return 0; } -static int unit_get_io_accounting_raw(Unit *u, uint64_t ret[static _CGROUP_IO_ACCOUNTING_METRIC_MAX]) { - static const char *const field_names[_CGROUP_IO_ACCOUNTING_METRIC_MAX] = { +static int unit_get_io_accounting_raw( + const Unit *u, + const CGroupRuntime *crt, + uint64_t ret[static _CGROUP_IO_ACCOUNTING_METRIC_MAX]) { + + static const char* const field_names[_CGROUP_IO_ACCOUNTING_METRIC_MAX] = { [CGROUP_IO_READ_BYTES] = "rbytes=", [CGROUP_IO_WRITE_BYTES] = "wbytes=", [CGROUP_IO_READ_OPERATIONS] = "rios=", [CGROUP_IO_WRITE_OPERATIONS] = "wios=", }; + uint64_t acc[_CGROUP_IO_ACCOUNTING_METRIC_MAX] = {}; _cleanup_free_ char *path = NULL; _cleanup_fclose_ FILE *f = NULL; int r; assert(u); + assert(crt); - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) + if (!crt->cgroup_path) return -ENODATA; if (unit_has_host_root_cgroup(u)) @@ -4876,7 +4879,7 @@ int unit_get_io_accounting( if (allow_cache && crt->io_accounting_last[metric] != UINT64_MAX) goto done; - r = unit_get_io_accounting_raw(u, raw); + r = unit_get_io_accounting_raw(u, crt, raw); if (r == -ENODATA && crt->io_accounting_last[metric] != UINT64_MAX) goto done; if (r < 0) @@ -4897,45 +4900,52 @@ done: return 0; } -int unit_reset_cpu_accounting(Unit *u) { +static int unit_reset_cpu_accounting(Unit *unit, CGroupRuntime *crt) { int r; - assert(u); - - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return 0; + assert(crt); + crt->cpu_usage_base = 0; crt->cpu_usage_last = NSEC_INFINITY; - r = unit_get_cpu_usage_raw(u, &crt->cpu_usage_base); - if (r < 0) { - crt->cpu_usage_base = 0; - return r; + if (unit) { + r = unit_get_cpu_usage_raw(unit, crt, &crt->cpu_usage_base); + if (r < 0 && r != -ENODATA) + return r; } return 0; } -void unit_reset_memory_accounting_last(Unit *u) { - assert(u); +static int unit_reset_io_accounting(Unit *unit, CGroupRuntime *crt) { + int r; - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return; + assert(crt); + + zero(crt->io_accounting_base); + FOREACH_ELEMENT(i, crt->io_accounting_last) + *i = UINT64_MAX; + + if (unit) { + r = unit_get_io_accounting_raw(unit, crt, crt->io_accounting_base); + if (r < 0 && r != -ENODATA) + return r; + } + + return 0; +} + +static void cgroup_runtime_reset_memory_accounting_last(CGroupRuntime *crt) { + assert(crt); FOREACH_ELEMENT(i, crt->memory_accounting_last) *i = UINT64_MAX; } -int unit_reset_ip_accounting(Unit *u) { +static int cgroup_runtime_reset_ip_accounting(CGroupRuntime *crt) { int r = 0; - assert(u); - - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return 0; + assert(crt); if (crt->ip_accounting_ingress_map_fd >= 0) RET_GATHER(r, bpf_firewall_reset_accounting(crt->ip_accounting_ingress_map_fd)); @@ -4948,46 +4958,19 @@ int unit_reset_ip_accounting(Unit *u) { return r; } -void unit_reset_io_accounting_last(Unit *u) { - assert(u); - - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return; - - FOREACH_ARRAY(i, crt->io_accounting_last, _CGROUP_IO_ACCOUNTING_METRIC_MAX) - *i = UINT64_MAX; -} - -int unit_reset_io_accounting(Unit *u) { - int r; - - assert(u); - - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return 0; - - unit_reset_io_accounting_last(u); - - r = unit_get_io_accounting_raw(u, crt->io_accounting_base); - if (r < 0) { - zero(crt->io_accounting_base); - return r; - } - - return 0; -} - int unit_reset_accounting(Unit *u) { int r = 0; assert(u); - RET_GATHER(r, unit_reset_cpu_accounting(u)); - RET_GATHER(r, unit_reset_io_accounting(u)); - RET_GATHER(r, unit_reset_ip_accounting(u)); - unit_reset_memory_accounting_last(u); + CGroupRuntime *crt = unit_get_cgroup_runtime(u); + if (!crt) + return 0; + + cgroup_runtime_reset_memory_accounting_last(crt); + RET_GATHER(r, unit_reset_cpu_accounting(u, crt)); + RET_GATHER(r, unit_reset_io_accounting(u, crt)); + RET_GATHER(r, cgroup_runtime_reset_ip_accounting(crt)); return r; } @@ -5211,7 +5194,7 @@ int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name) { return parse_cpu_set_full(v, cpus, false, NULL, NULL, 0, NULL); } -CGroupRuntime *cgroup_runtime_new(void) { +CGroupRuntime* cgroup_runtime_new(void) { _cleanup_(cgroup_runtime_freep) CGroupRuntime *crt = NULL; crt = new(CGroupRuntime, 1); @@ -5219,8 +5202,6 @@ CGroupRuntime *cgroup_runtime_new(void) { return NULL; *crt = (CGroupRuntime) { - .cpu_usage_last = NSEC_INFINITY, - .cgroup_control_inotify_wd = -1, .cgroup_memory_inotify_wd = -1, @@ -5235,19 +5216,15 @@ CGroupRuntime *cgroup_runtime_new(void) { .cgroup_invalidated_mask = _CGROUP_MASK_ALL, }; - FOREACH_ELEMENT(i, crt->memory_accounting_last) - *i = UINT64_MAX; - FOREACH_ELEMENT(i, crt->io_accounting_base) - *i = UINT64_MAX; - FOREACH_ELEMENT(i, crt->io_accounting_last) - *i = UINT64_MAX; - FOREACH_ELEMENT(i, crt->ip_accounting_extra) - *i = UINT64_MAX; + unit_reset_cpu_accounting(/* unit = */ NULL, crt); + unit_reset_io_accounting(/* unit = */ NULL, crt); + cgroup_runtime_reset_memory_accounting_last(crt); + assert_se(cgroup_runtime_reset_ip_accounting(crt) >= 0); return TAKE_PTR(crt); } -CGroupRuntime *cgroup_runtime_free(CGroupRuntime *crt) { +CGroupRuntime* cgroup_runtime_free(CGroupRuntime *crt) { if (!crt) return NULL; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 08fccf9612..7bc849c542 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -489,11 +489,6 @@ int unit_get_io_accounting(Unit *u, CGroupIOAccountingMetric metric, bool allow_ int unit_get_ip_accounting(Unit *u, CGroupIPAccountingMetric metric, uint64_t *ret); int unit_get_effective_limit(Unit *u, CGroupLimitType type, uint64_t *ret); -int unit_reset_cpu_accounting(Unit *u); -void unit_reset_memory_accounting_last(Unit *u); -int unit_reset_ip_accounting(Unit *u); -void unit_reset_io_accounting_last(Unit *u); -int unit_reset_io_accounting(Unit *u); int unit_reset_accounting(Unit *u); #define UNIT_CGROUP_BOOL(u, name) \ @@ -527,8 +522,8 @@ int unit_cgroup_freezer_action(Unit *u, FreezerAction action); const char* freezer_action_to_string(FreezerAction a) _const_; FreezerAction freezer_action_from_string(const char *s) _pure_; -CGroupRuntime *cgroup_runtime_new(void); -CGroupRuntime *cgroup_runtime_free(CGroupRuntime *crt); +CGroupRuntime* cgroup_runtime_new(void); +CGroupRuntime* cgroup_runtime_free(CGroupRuntime *crt); DEFINE_TRIVIAL_CLEANUP_FUNC(CGroupRuntime*, cgroup_runtime_free); int cgroup_runtime_serialize(Unit *u, FILE *f, FDSet *fds); diff --git a/src/core/unit.c b/src/core/unit.c index 9d64103aef..f5563c1fe9 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -129,9 +129,6 @@ Unit* unit_new(Manager *m, size_t size) { .burst = 16 }; - unit_reset_memory_accounting_last(u); - unit_reset_io_accounting_last(u); - return u; } From d3d035395eeced715c9609aeb8bfac7157dd0aa9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 13 Jun 2024 11:00:26 +0200 Subject: [PATCH 07/12] core/cgroup: check root cgroup earlier for unit_get_memory_accounting --- src/core/cgroup.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 47413e6bc3..dfbe716e8b 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -4527,6 +4527,10 @@ int unit_get_memory_accounting(Unit *u, CGroupMemoryAccountingMetric metric, uin if (!UNIT_CGROUP_BOOL(u, memory_accounting)) return -ENODATA; + /* The root cgroup doesn't expose this information. */ + if (unit_has_host_root_cgroup(u)) + return -ENODATA; + CGroupRuntime *crt = unit_get_cgroup_runtime(u); if (!crt) return -ENODATA; @@ -4534,10 +4538,6 @@ int unit_get_memory_accounting(Unit *u, CGroupMemoryAccountingMetric metric, uin /* If the cgroup is already gone, we try to find the last cached value. */ goto finish; - /* The root cgroup doesn't expose this information. */ - if (unit_has_host_root_cgroup(u)) - return -ENODATA; - if (!FLAGS_SET(crt->cgroup_realized_mask, CGROUP_MASK_MEMORY)) return -ENODATA; From 3849d1f56b7ff9db94656ca84156eb9fe1574ecb Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 13 Jun 2024 11:01:03 +0200 Subject: [PATCH 08/12] core/cgroup: actually make use of the cached accounting values If cgroup is already gone, i.e. CGRuntime.cgroup_path is NULL, do not return -ENODATA prematurely, but check for cached values first. For #33149 --- src/core/cgroup.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index dfbe716e8b..cfcf8ab117 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -4645,11 +4645,11 @@ int unit_get_cpu_usage(Unit *u, nsec_t *ret) { * started. If the cgroup has been removed already, returns the last cached value. To cache the value, simply * call this function with a NULL return value. */ - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) + if (!UNIT_CGROUP_BOOL(u, cpu_accounting)) return -ENODATA; - if (!UNIT_CGROUP_BOOL(u, cpu_accounting)) + CGroupRuntime *crt = unit_get_cgroup_runtime(u); + if (!crt) return -ENODATA; r = unit_get_cpu_usage_raw(u, crt, &ns); @@ -4693,7 +4693,7 @@ int unit_get_ip_accounting( return -ENODATA; CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) + if (!crt) return -ENODATA; fd = IN_SET(metric, CGROUP_IP_INGRESS_BYTES, CGROUP_IP_INGRESS_PACKETS) ? @@ -4873,7 +4873,7 @@ int unit_get_io_accounting( return -ENODATA; CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) + if (!crt) return -ENODATA; if (allow_cache && crt->io_accounting_last[metric] != UINT64_MAX) From 4c1fc52d2ae09036cda98bf7ad589595a5f6923f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 20:06:13 +0200 Subject: [PATCH 09/12] core/cgroup: drop spurious ", ignoring" for unit_cgroup_is_empty --- src/core/cgroup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index cfcf8ab117..52243067c8 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3531,8 +3531,7 @@ int unit_cgroup_is_empty(Unit *u) { r = cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, crt->cgroup_path); if (r < 0) - return log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty, ignoring: %m", empty_to_root(crt->cgroup_path)); - + log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty: %m", empty_to_root(crt->cgroup_path)); return r; } From 4918f14adabba71913cf3abb8b5d50a8ca0fe844 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 20:06:46 +0200 Subject: [PATCH 10/12] core: do not drop CGroupRuntime when unit stops, but only on GC Fixes #33149 Replaces #33145 --- src/core/cgroup.c | 28 +++++++++++++++------------- src/core/cgroup.h | 5 +---- src/core/unit.c | 6 +++--- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 52243067c8..9620436f68 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2686,7 +2686,7 @@ int unit_set_cgroup_path(Unit *u, const char *path) { if (crt && streq_ptr(crt->cgroup_path, path)) return 0; - unit_release_cgroup(u); + unit_release_cgroup(u, /* drop_cgroup_runtime = */ true); crt = unit_setup_cgroup_runtime(u); if (!crt) @@ -3484,7 +3484,7 @@ int unit_realize_cgroup(Unit *u) { return unit_realize_cgroup_now(u, manager_state(u->manager)); } -void unit_release_cgroup(Unit *u) { +void unit_release_cgroup(Unit *u, bool drop_cgroup_runtime) { assert(u); /* Forgets all cgroup details for this cgroup — but does *not* destroy the cgroup. This is hence OK to call @@ -3515,7 +3515,8 @@ void unit_release_cgroup(Unit *u) { crt->cgroup_memory_inotify_wd = -1; } - *(CGroupRuntime**) ((uint8_t*) u + UNIT_VTABLE(u)->cgroup_runtime_offset) = cgroup_runtime_free(crt); + if (drop_cgroup_runtime) + *(CGroupRuntime**) ((uint8_t*) u + UNIT_VTABLE(u)->cgroup_runtime_offset) = cgroup_runtime_free(crt); } int unit_cgroup_is_empty(Unit *u) { @@ -3535,14 +3536,13 @@ int unit_cgroup_is_empty(Unit *u) { return r; } -bool unit_maybe_release_cgroup(Unit *u) { +static bool unit_maybe_release_cgroup(Unit *u) { int r; - assert(u); + /* Releases the cgroup only if it is recursively empty. + * Returns true if the cgroup was released, false otherwise. */ - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return true; + assert(u); /* Don't release the cgroup if there are still processes under it. If we get notified later when all * the processes exit (e.g. the processes were in D-state and exited after the unit was marked as @@ -3550,7 +3550,10 @@ bool unit_maybe_release_cgroup(Unit *u) { * and cleaned up later. */ r = unit_cgroup_is_empty(u); if (r > 0) { - unit_release_cgroup(u); + /* Do not free CGroupRuntime when called from unit_prune_cgroup. Various accounting data + * we should keep, especially CPU usage and *_peak ones which would be shown even after + * the unit stops. */ + unit_release_cgroup(u, /* drop_cgroup_runtime = */ false); return true; } @@ -3558,8 +3561,8 @@ bool unit_maybe_release_cgroup(Unit *u) { } void unit_prune_cgroup(Unit *u) { - int r; bool is_root_slice; + int r; assert(u); @@ -3597,9 +3600,8 @@ void unit_prune_cgroup(Unit *u) { if (!unit_maybe_release_cgroup(u)) /* Returns true if the cgroup was released */ return; - crt = unit_get_cgroup_runtime(u); /* The above might have destroyed the runtime object, let's see if it's still there */ - if (!crt) - return; + assert(crt == unit_get_cgroup_runtime(u)); + assert(!crt->cgroup_path); crt->cgroup_realized = false; crt->cgroup_realized_mask = 0; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 7bc849c542..e31e69364e 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -449,10 +449,7 @@ int unit_watch_cgroup_memory(Unit *u); void unit_add_to_cgroup_realize_queue(Unit *u); int unit_cgroup_is_empty(Unit *u); -void unit_release_cgroup(Unit *u); -/* Releases the cgroup only if it is recursively empty. - * Returns true if the cgroup was released, false otherwise. */ -bool unit_maybe_release_cgroup(Unit *u); +void unit_release_cgroup(Unit *u, bool drop_cgroup_runtime); void unit_add_to_cgroup_empty_queue(Unit *u); int unit_check_oomd_kill(Unit *u); diff --git a/src/core/unit.c b/src/core/unit.c index f5563c1fe9..23772f143f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -481,8 +481,8 @@ bool unit_may_gc(Unit *u) { /* If the unit has a cgroup, then check whether there's anything in it. If so, we should stay * around. Units with active processes should never be collected. */ r = unit_cgroup_is_empty(u); - if (r <= 0 && r != -ENXIO) - return false; /* ENXIO means: currently not realized */ + if (r <= 0 && !IN_SET(r, -ENXIO, -EOWNERDEAD)) + return false; /* ENXIO/EOWNERDEAD means: currently not realized */ if (!UNIT_VTABLE(u)->may_gc) return true; @@ -787,7 +787,7 @@ Unit* unit_free(Unit *u) { if (u->on_console) manager_unref_console(u->manager); - unit_release_cgroup(u); + unit_release_cgroup(u, /* drop_cgroup_runtime = */ true); if (!MANAGER_IS_RELOADING(u->manager)) unit_unlink_state_files(u); From 8d178f702d7c3b632233299b98158e34ef171c74 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 20:41:27 +0200 Subject: [PATCH 11/12] core: cast ignored retval of unit_realize_cgroup to void --- src/core/dbus-mount.c | 2 +- src/core/dbus-scope.c | 2 +- src/core/dbus-service.c | 2 +- src/core/dbus-slice.c | 2 +- src/core/dbus-socket.c | 2 +- src/core/dbus-swap.c | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/dbus-mount.c b/src/core/dbus-mount.c index f6a9ea97b7..d59aa06a11 100644 --- a/src/core/dbus-mount.c +++ b/src/core/dbus-mount.c @@ -193,7 +193,7 @@ int bus_mount_set_property( int bus_mount_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index 165aa65a7b..15c3595a42 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -249,7 +249,7 @@ int bus_scope_set_property( int bus_scope_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index cf7e2fe0eb..4d2a7bb47e 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -785,7 +785,7 @@ int bus_service_set_property( int bus_service_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } diff --git a/src/core/dbus-slice.c b/src/core/dbus-slice.c index de41d651da..6e3fcb0f88 100644 --- a/src/core/dbus-slice.c +++ b/src/core/dbus-slice.c @@ -28,7 +28,7 @@ int bus_slice_set_property( int bus_slice_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c index 320a93e981..2ab4961b2b 100644 --- a/src/core/dbus-socket.c +++ b/src/core/dbus-socket.c @@ -468,7 +468,7 @@ int bus_socket_set_property( int bus_socket_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } diff --git a/src/core/dbus-swap.c b/src/core/dbus-swap.c index 7230352e9c..dbcc5a3621 100644 --- a/src/core/dbus-swap.c +++ b/src/core/dbus-swap.c @@ -49,7 +49,7 @@ int bus_swap_set_property( int bus_swap_commit_properties(Unit *u) { assert(u); - unit_realize_cgroup(u); + (void) unit_realize_cgroup(u); return 0; } From f26b2ec46118a4493608618da2253bb9dfc6b517 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 5 Jun 2024 20:45:12 +0200 Subject: [PATCH 12/12] core: do not set up cgroup runtime on coldplug Currently, unit_setup_cgroup_runtime() is called in various _coldplug() functions if the unit is not inactive. That seems unnecessary though, and kinda defeats the purpose of CGroupRuntime. If we need to fork off a process for the unit or got something during deserialization, the CGroupRuntime would be automatically set up by unit_prepare_exec() / cgroup_runtime_deserialize_one(). Otherwise it would mean the cgroup doesn't exist and we don't need to allocate that in the first place. Plus, note that socket units might also carry a cgroup with ExecStartPre=/ExecStartPost=/... Hence the existing code is really inconsistent. --- src/core/mount.c | 4 +--- src/core/service.c | 1 - src/core/swap.c | 4 +--- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index c6100b0b0a..e040f151f4 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -764,10 +764,8 @@ static int mount_coldplug(Unit *u) { return r; } - if (!IN_SET(m->deserialized_state, MOUNT_DEAD, MOUNT_FAILED)) { + if (!IN_SET(m->deserialized_state, MOUNT_DEAD, MOUNT_FAILED)) (void) unit_setup_exec_runtime(u); - (void) unit_setup_cgroup_runtime(u); - } mount_set_state(m, m->deserialized_state); return 0; diff --git a/src/core/service.c b/src/core/service.c index 937729cd06..9af10df0cd 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1366,7 +1366,6 @@ static int service_coldplug(Unit *u) { SERVICE_DEAD_RESOURCES_PINNED)) { (void) unit_enqueue_rewatch_pids(u); (void) unit_setup_exec_runtime(u); - (void) unit_setup_cgroup_runtime(u); } if (IN_SET(s->deserialized_state, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD, SERVICE_RELOAD_SIGNAL, SERVICE_RELOAD_NOTIFY)) diff --git a/src/core/swap.c b/src/core/swap.c index c4d2ba8f3a..e6f6f78dc9 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -548,10 +548,8 @@ static int swap_coldplug(Unit *u) { return r; } - if (!IN_SET(new_state, SWAP_DEAD, SWAP_FAILED)) { + if (!IN_SET(new_state, SWAP_DEAD, SWAP_FAILED)) (void) unit_setup_exec_runtime(u); - (void) unit_setup_cgroup_runtime(u); - } swap_set_state(s, new_state); return 0;