From 65ba2ff4a7b126d9f5359ae08d7a1cd98eeff9b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 15 Aug 2026 21:16:34 +0200 Subject: [PATCH] report-basic: add idle/iowait/steal as CPUWait family As discussed in the PR, those fields can go down, so they cannot be advertised as type COUNTER. But the type is shared for the whole family, so we need to create a new family to keep the other ones as COUNTER and add the new ones as GAUGE. kernel/sched/cputime.c says above get_cpu_idle_time_us() and get_cpu_iowait_time_us(): > Note that this is partially broken due to the counter of iowait tasks that > can be remotely updated without any synchronization. Therefore it is possible > to observe backward values within two consecutive reads. steal is apparently also unreliable because it is reported by the hyperviser. So let's be cautious and report it as gauge too. --- src/report/report-basic.c | 39 ++++++++++++++++++++------ test/units/TEST-74-AUX-UTILS.report.sh | 14 ++++++++- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/report/report-basic.c b/src/report/report-basic.c index 7cb36d6885..4b8d9ce024 100644 --- a/src/report/report-basic.c +++ b/src/report/report-basic.c @@ -237,10 +237,11 @@ static int cpus_online_generate(const MetricFamily *mf, sd_varlink *link, void * /* fields= */ NULL); } -static int cpu_usage_generate(const MetricFamily *mf, sd_varlink *link, void *userdata) { +static int cpu_usage_generate(const MetricFamily mf[static 2], sd_varlink *link, void *userdata) { int r; - assert(mf && mf->name); + assert(mf && mf[0].name && mf[1].name); + assert(!mf[1].generate); assert(link); /* The aggregate "cpu" line of /proc/stat, converted from USER_HZ ticks to nanoseconds, matching @@ -254,24 +255,36 @@ static int cpu_usage_generate(const MetricFamily *mf, sd_varlink *link, void *us return 0; } - static const char* const types[] = { "user", "system" }; - const uint64_t values[] = { + static const struct { + const char *name; + size_t family_offset; + } attributes[] = { + { "user", 0 }, + { "system", 0 }, + { "idle", 1 }, + { "iowait", 1 }, + { "steal", 1 }, + }; + uint64_t values[] = { ticks.user + ticks.nice, ticks.system + ticks.irq + ticks.softirq, + ticks.idle, + ticks.iowait, + ticks.steal, }; - assert_cc(ELEMENTSOF(types) == ELEMENTSOF(values)); + assert_cc(ELEMENTSOF(attributes) == ELEMENTSOF(values)); - for (size_t i = 0; i < ELEMENTSOF(values); i++) { + for (size_t i = 0; i < ELEMENTSOF(attributes); i++) { _cleanup_(sd_json_variant_unrefp) sd_json_variant *fields = NULL; - r = sd_json_buildo(&fields, SD_JSON_BUILD_PAIR_STRING("type", types[i])); + r = sd_json_buildo(&fields, SD_JSON_BUILD_PAIR_STRING("type", attributes[i].name)); if (r < 0) return r; uint64_t nsec = procfs_ticks_to_nsec(values[i]); r = metric_build_send_unsigned( - mf, + &mf[attributes[i].family_offset], link, /* object= */ NULL, nsec, @@ -719,7 +732,7 @@ static int virtualization_generate(const MetricFamily *mf, sd_varlink *link, voi } static const MetricFamily metric_family_table[] = { - /* Keep entries ordered alphabetically */ + /* Keep entries (with .generate) ordered alphabetically */ { METRIC_IO_SYSTEMD_BASIC_PREFIX "Architecture", "CPU architecture", @@ -744,12 +757,20 @@ static const MetricFamily metric_family_table[] = { METRIC_FAMILY_TYPE_GAUGE, .generate = cpus_online_generate, }, + /* METRIC_FAMILY_TYPE_COUNTER must be monotonic, but steal and iowait can go down, + * so CPUUsage is split into two families of different types. */ { METRIC_IO_SYSTEMD_BASIC_PREFIX "CPUUsage", "Aggregate CPU usage across all CPUs in nanoseconds (type=user|system)", METRIC_FAMILY_TYPE_COUNTER, .generate = cpu_usage_generate, }, + { + METRIC_IO_SYSTEMD_BASIC_PREFIX "CPUWait", + "Aggregate CPU wait across all CPUs in nanoseconds (type=idle|iowait|steal)", + METRIC_FAMILY_TYPE_GAUGE, + }, + /* Keep those ↑ in sync with cpu_usage_generate(). */ { METRIC_IO_SYSTEMD_BASIC_PREFIX "DiskReadBytes", "Per block device metric: cumulative number of bytes read " diff --git a/test/units/TEST-74-AUX-UTILS.report.sh b/test/units/TEST-74-AUX-UTILS.report.sh index 625095c32e..5cf8cb7ac8 100755 --- a/test/units/TEST-74-AUX-UTILS.report.sh +++ b/test/units/TEST-74-AUX-UTILS.report.sh @@ -104,6 +104,18 @@ for type in user system; do test -n "$(basic_number CPUUsage ".fields.type == \"$type\"")" done +# The values are USER_HZ ticks converted to nanoseconds. The metrics were sampled before the +# current /proc/stat state, and the idle counter mostly grows, so the reported idle time +# should be positive and no larger than what /proc/stat shows now with some margin. +cpu_idle_reported="$(basic_number CPUWait '.fields.type == "idle"')" +awk -v hz="$(getconf CLK_TCK)" -v sampled="$cpu_idle_reported" \ + '/^cpu / { exit !(sampled > 0 && sampled <= ($5 + 1) / hz * 1e9 ) }' /proc/stat + +# iowait/idle/steal must be valid nanoseconds values, but can go up and down. +for type in idle iowait steal; do + test -n "$(basic_number CPUWait ".fields.type == \"$type\"")" +done + # MemoryUsedBytes is MemTotal − MemAvailable, so it cannot exceed the MemTotal reported by the # same file. (PhysicalMemoryBytes is not a valid bound here: it is capped by the root cgroup's # memory limit, so it can legitimately be smaller than the value computed from /proc/meminfo.) @@ -155,7 +167,7 @@ done # Every new metric family must be described, even those whose values depend on the environment. basic_describe="$(varlinkctl call --more /run/systemd/report/io.systemd.Basic io.systemd.Metrics.Describe {})" -for name in CPUUsage DiskReadBytes DiskWriteBytes MemoryUsedBytes PressureAvg10 PressureStallSeconds SwapUsedBytes; do +for name in CPUUsage CPUWait DiskReadBytes DiskWriteBytes MemoryUsedBytes PressureAvg10 PressureStallSeconds SwapUsedBytes; do echo "$basic_describe" | grep -F "io.systemd.Basic.$name" >/dev/null done