From a0020ad84bb092fc72cde7dca5784a0a4e613fd7 Mon Sep 17 00:00:00 2001 From: Ivan Shapovalov Date: Fri, 20 Sep 2024 17:02:13 +0200 Subject: [PATCH 1/2] core/cgroup: drop `allow_cache` parameter in `unit_get_io_accounting()` The name of the parameter is misleading and it does not save us much work because it is not used during regular unit property queries. It is only used during unit_log_resources(), and the cgroup is already dead by that point so it won't be read anyway. --- src/core/cgroup.c | 4 ---- src/core/cgroup.h | 2 +- src/core/dbus-unit.c | 2 +- src/core/unit.c | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 3f96f64dad..61814efa9c 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -4858,7 +4858,6 @@ static int unit_get_io_accounting_raw( int unit_get_io_accounting( Unit *u, CGroupIOAccountingMetric metric, - bool allow_cache, uint64_t *ret) { uint64_t raw[_CGROUP_IO_ACCOUNTING_METRIC_MAX]; @@ -4873,9 +4872,6 @@ int unit_get_io_accounting( if (!crt) return -ENODATA; - if (allow_cache && crt->io_accounting_last[metric] != UINT64_MAX) - goto done; - r = unit_get_io_accounting_raw(u, crt, raw); if (r == -ENODATA && crt->io_accounting_last[metric] != UINT64_MAX) goto done; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 1c5c1f75ab..7525da728e 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -481,7 +481,7 @@ int unit_get_memory_current(Unit *u, uint64_t *ret); int unit_get_memory_accounting(Unit *u, CGroupMemoryAccountingMetric metric, uint64_t *ret); int unit_get_tasks_current(Unit *u, uint64_t *ret); int unit_get_cpu_usage(Unit *u, nsec_t *ret); -int unit_get_io_accounting(Unit *u, CGroupIOAccountingMetric metric, bool allow_cache, uint64_t *ret); +int unit_get_io_accounting(Unit *u, CGroupIOAccountingMetric metric, uint64_t *ret); int unit_get_ip_accounting(Unit *u, CGroupIPAccountingMetric metric, uint64_t *ret); int unit_get_effective_limit(Unit *u, CGroupLimitType type, uint64_t *ret); diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index d7869f115a..2db473fbed 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1456,7 +1456,7 @@ static int property_get_io_counter( assert(property); assert_se((metric = cgroup_io_accounting_metric_from_string(property)) >= 0); - (void) unit_get_io_accounting(u, metric, /* allow_cache= */ false, &value); + (void) unit_get_io_accounting(u, metric, &value); return sd_bus_message_append(reply, "t", value); } diff --git a/src/core/unit.c b/src/core/unit.c index cebae4d53e..ae3cefcdde 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2385,7 +2385,7 @@ static int unit_log_resources(Unit *u) { assert(io_fields[k].journal_field); - (void) unit_get_io_accounting(u, k, k > 0, &value); + (void) unit_get_io_accounting(u, k, &value); if (value == UINT64_MAX) continue; From 17bbdefd8c49617d7596bbf708c818a9773a9b44 Mon Sep 17 00:00:00 2001 From: Ivan Shapovalov Date: Fri, 20 Sep 2024 13:01:51 +0200 Subject: [PATCH 2/2] core/cgroup: cache IO accounting data when pruning a cgroup When removing a cgroup in unit_prune_cgroup(), read IO metrics to cache them similar to the existing treatment of the CPU and memory usage data. Note that we do not do this for the IP metrics as the firewall objects are only destroyed in unit_free() and thus stay alive long enough to be read out directly by all interested parties. Fixes #26988. --- src/core/cgroup.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 61814efa9c..fb89a22d2e 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3571,12 +3571,17 @@ void unit_prune_cgroup(Unit *u) { if (!crt || !crt->cgroup_path) return; - /* Cache the last CPU and memory usage values before we destroy the cgroup */ + /* Cache the last resource usage values before we destroy the cgroup */ (void) unit_get_cpu_usage(u, /* ret = */ NULL); for (CGroupMemoryAccountingMetric metric = 0; metric <= _CGROUP_MEMORY_ACCOUNTING_METRIC_CACHED_LAST; metric++) (void) unit_get_memory_accounting(u, metric, /* ret = */ NULL); + /* All IO metrics are read at once from the underlying cgroup, so issue just a single call */ + (void) unit_get_io_accounting(u, _CGROUP_IO_ACCOUNTING_METRIC_INVALID, /* ret = */ NULL); + + /* We do not cache IP metrics here because the firewall objects are not freed with cgroups */ + #if BPF_FRAMEWORK (void) bpf_restrict_fs_cleanup(u); /* Remove cgroup from the global LSM BPF map */ #endif @@ -4863,7 +4868,14 @@ int unit_get_io_accounting( uint64_t raw[_CGROUP_IO_ACCOUNTING_METRIC_MAX]; int r; - /* Retrieve an IO account parameter. This will subtract the counter when the unit was started. */ + /* + * Retrieve an IO counter, subtracting the value of the counter value at the time the unit was started. + * If ret == NULL and metric == _<...>_INVALID, no return value is expected (refresh the caches only). + */ + + assert(u); + assert(metric >= 0 || (!ret && metric == _CGROUP_IO_ACCOUNTING_METRIC_INVALID)); + assert(metric < _CGROUP_IO_ACCOUNTING_METRIC_MAX); if (!UNIT_CGROUP_BOOL(u, io_accounting)) return -ENODATA; @@ -4873,7 +4885,7 @@ int unit_get_io_accounting( return -ENODATA; r = unit_get_io_accounting_raw(u, crt, raw); - if (r == -ENODATA && crt->io_accounting_last[metric] != UINT64_MAX) + if (r == -ENODATA && metric >= 0 && crt->io_accounting_last[metric] != UINT64_MAX) goto done; if (r < 0) return r;