From 75b86b564ab89d146eeed8e7df6fdbcc363a8150 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 14:34:01 +0100 Subject: [PATCH 01/10] limits-util: tweak overflow checks for (physical_memory|system_tasks)_max_scale() Also, shortcut two special cases for passing through values as-is, so that we are not needlessly subjected to overflow issues for them. --- src/basic/limits-util.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/basic/limits-util.c b/src/basic/limits-util.c index 259c311a67..9f8e26d46a 100644 --- a/src/basic/limits-util.c +++ b/src/basic/limits-util.c @@ -77,7 +77,13 @@ uint64_t physical_memory(void) { } uint64_t physical_memory_scale(uint64_t v, uint64_t max) { - uint64_t p, m, ps, r; + uint64_t p, m, ps; + + /* Shortcut two special cases */ + if (v == 0) + return 0; + if (v == max) + return physical_memory(); assert(max > 0); @@ -90,17 +96,16 @@ uint64_t physical_memory_scale(uint64_t v, uint64_t max) { p = physical_memory() / ps; assert(p > 0); - m = p * v; - if (m / p != v) + if (v > UINT64_MAX / p) return UINT64_MAX; + m = p * v; m /= max; - r = m * ps; - if (r / ps != m) + if (m > UINT64_MAX / ps) return UINT64_MAX; - return r; + return m * ps; } uint64_t system_tasks_max(void) { @@ -138,6 +143,12 @@ uint64_t system_tasks_max(void) { uint64_t system_tasks_max_scale(uint64_t v, uint64_t max) { uint64_t t, m; + /* Shortcut two special cases */ + if (v == 0) + return 0; + if (v == max) + return system_tasks_max(); + assert(max > 0); /* Multiply the system's task value by the fraction v/max. Hence, if max==100 this calculates percentages @@ -146,9 +157,9 @@ uint64_t system_tasks_max_scale(uint64_t v, uint64_t max) { t = system_tasks_max(); assert(t > 0); - m = t * v; - if (m / t != v) /* overflow? */ + if (v > UINT64_MAX / t) /* overflow? */ return UINT64_MAX; + m = t * v; return m / max; } From fe845b5e7653677f922070ab3c92abfd49bbb4e5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 14:37:08 +0100 Subject: [PATCH 02/10] tree-wide: parse permyriads wherever we can MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Given that we now have a parser for permyriads, let's use it everywhere for greater accuracy. This means wherever we previously supported % and ‰, we now also support ‱. --- src/core/load-fragment.c | 12 ++++++------ src/home/homectl.c | 6 +++--- src/login/logind-user.c | 6 +++--- src/login/pam_systemd.c | 4 ++-- src/network/tc/tc-util.c | 8 ++++---- src/shared/bus-unit-util.c | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 11836c0938..a1ab5f70eb 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3593,13 +3593,13 @@ int config_parse_cpu_quota( return 0; } - r = parse_permille_unbounded(rvalue); + r = parse_permyriad_unbounded(rvalue); if (r <= 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Invalid CPU quota '%s', ignoring.", rvalue); return 0; } - c->cpu_quota_per_sec_usec = ((usec_t) r * USEC_PER_SEC) / 1000U; + c->cpu_quota_per_sec_usec = ((usec_t) r * USEC_PER_SEC) / 10000U; return 0; } @@ -3664,7 +3664,7 @@ int config_parse_memory_limit( bytes = CGROUP_LIMIT_MIN; else if (!isempty(rvalue) && !streq(rvalue, "infinity")) { - r = parse_permille(rvalue); + r = parse_permyriad(rvalue); if (r < 0) { r = parse_size(rvalue, 1024, &bytes); if (r < 0) { @@ -3672,7 +3672,7 @@ int config_parse_memory_limit( return 0; } } else - bytes = physical_memory_scale(r, 1000U); + bytes = physical_memory_scale(r, 10000U); if (bytes >= UINT64_MAX || (bytes <= 0 && !STR_IN_SET(lvalue, "MemorySwapMax", "MemoryLow", "MemoryMin", "DefaultMemoryLow", "DefaultMemoryMin"))) { @@ -3734,9 +3734,9 @@ int config_parse_tasks_max( return 0; } - r = parse_permille(rvalue); + r = parse_permyriad(rvalue); if (r >= 0) - *tasks_max = (TasksMax) { r, 1000U }; /* r‰ */ + *tasks_max = (TasksMax) { r, 10000U }; /* r‱ */ else { r = safe_atou64(rvalue, &v); if (r < 0) { diff --git a/src/home/homectl.c b/src/home/homectl.c index fa9d11e69d..98835327cd 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -1567,7 +1567,7 @@ static int resize_home(int argc, char *argv[], void *userdata) { (void) polkit_agent_open_if_enabled(arg_transport, arg_ask_password); if (arg_disk_size_relative != UINT64_MAX || - (argc > 2 && parse_percent(argv[2]) >= 0)) + (argc > 2 && parse_permyriad(argv[2]) >= 0)) return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "Relative disk size specification currently not supported when resizing."); @@ -2653,7 +2653,7 @@ static int parse_argv(int argc, char *argv[]) { break; } - r = parse_permille(optarg); + r = parse_permyriad(optarg); if (r < 0) { r = parse_size(optarg, 1024, &arg_disk_size); if (r < 0) @@ -2670,7 +2670,7 @@ static int parse_argv(int argc, char *argv[]) { arg_disk_size_relative = UINT64_MAX; } else { /* Normalize to UINT32_MAX == 100% */ - arg_disk_size_relative = (uint64_t) r * UINT32_MAX / 1000U; + arg_disk_size_relative = (uint64_t) r * UINT32_MAX / 10000U; r = drop_from_identity("diskSize"); if (r < 0) diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 9b3ec07906..4406f36614 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -907,9 +907,9 @@ int config_parse_tmpfs_size( assert(data); /* First, try to parse as percentage */ - r = parse_permille(rvalue); - if (r > 0 && r < 1000) - *sz = physical_memory_scale(r, 1000U); + r = parse_permyriad(rvalue); + if (r > 0) + *sz = physical_memory_scale(r, 10000U); else { uint64_t k; diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 8e7a94db55..fce27262f1 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -334,9 +334,9 @@ static int append_session_memory_max(pam_handle_t *handle, sd_bus_message *m, co return PAM_SUCCESS; } - r = parse_permille(limit); + r = parse_permyriad(limit); if (r >= 0) { - r = sd_bus_message_append(m, "(sv)", "MemoryMaxScale", "u", (uint32_t) (((uint64_t) r * UINT32_MAX) / 1000U)); + r = sd_bus_message_append(m, "(sv)", "MemoryMaxScale", "u", (uint32_t) ((uint64_t) r * UINT32_MAX) / 10000U); if (r < 0) return pam_bus_log_create_error(handle, r); diff --git a/src/network/tc/tc-util.c b/src/network/tc/tc-util.c index 3e10b50c96..1890e96164 100644 --- a/src/network/tc/tc-util.c +++ b/src/network/tc/tc-util.c @@ -57,17 +57,17 @@ int tc_time_to_tick(usec_t t, uint32_t *ret) { return 0; } -int parse_tc_percent(const char *s, uint32_t *percent) { +int parse_tc_percent(const char *s, uint32_t *ret_fraction) { int r; assert(s); - assert(percent); + assert(ret_fraction); - r = parse_permille(s); + r = parse_permyriad(s); if (r < 0) return r; - *percent = (double) r / 1000 * UINT32_MAX; + *ret_fraction = (double) r / 10000 * UINT32_MAX; return 0; } diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 548dc57e3c..a1a253e801 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -539,7 +539,7 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons return 1; } - r = parse_permille(eq); + r = parse_permyriad(eq); if (r >= 0) { char *n; @@ -548,7 +548,7 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons * size can be determined server-side. */ n = strjoina(field, "Scale"); - r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) (((uint64_t) r * UINT32_MAX) / 1000U)); + r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) (((uint64_t) r * UINT32_MAX) / 10000U)); if (r < 0) return bus_log_create_error(r); @@ -565,14 +565,14 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons if (isempty(eq)) r = sd_bus_message_append(m, "(sv)", "CPUQuotaPerSecUSec", "t", USEC_INFINITY); else { - r = parse_permille_unbounded(eq); + r = parse_permyriad_unbounded(eq); if (r == 0) return log_error_errno(SYNTHETIC_ERRNO(ERANGE), "CPU quota too small."); if (r < 0) return log_error_errno(r, "CPU quota '%s' invalid.", eq); - r = sd_bus_message_append(m, "(sv)", "CPUQuotaPerSecUSec", "t", (((uint64_t) r * USEC_PER_SEC) / 1000U)); + r = sd_bus_message_append(m, "(sv)", "CPUQuotaPerSecUSec", "t", (((uint64_t) r * USEC_PER_SEC) / 10000U)); } if (r < 0) From 60dcf3dc1b5b4c63e2c35ad7eba1d6f7ab5e086c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 14:40:13 +0100 Subject: [PATCH 03/10] main: let's use physical_memory_scale() where appropriate This way we can take benefit of the fact that physical_memory_scale() aligns on page sizes. --- src/core/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index f29449d691..67189fef6d 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1256,8 +1256,8 @@ static int bump_rlimit_memlock(struct rlimit *saved_rlimit) { * must be unsigned, hence this is a given, but let's make this clear here. */ assert_cc(RLIM_INFINITY > 0); - mm = physical_memory() / 8; /* Let's scale how much we allow to be locked by the amount of physical - * RAM. We allow an eighth to be locked by us, just to pick a value. */ + mm = physical_memory_scale(1, 8); /* Let's scale how much we allow to be locked by the amount of physical + * RAM. We allow an eighth to be locked by us, just to pick a value. */ new_rlimit = (struct rlimit) { .rlim_cur = MAX3(HIGH_RLIMIT_MEMLOCK, saved_rlimit->rlim_cur, mm), From ed5033fd6c5e73a1ee874608338cbfd28b9083b4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 15:23:15 +0100 Subject: [PATCH 04/10] util: move percent/permille/permyriad parser into percent-util.[ch] A good chunk of parse-util.[ch] has been about parsing parts per hundred/thousand/ten-thousand. Let's split that out into its own file. No code changes, just some shuffling around. --- src/basic/meson.build | 2 + src/basic/parse-util.c | 140 ------------------------------- src/basic/parse-util.h | 9 -- src/basic/percent-util.c | 145 ++++++++++++++++++++++++++++++++ src/basic/percent-util.h | 11 +++ src/core/load-fragment.c | 4 + src/home/homectl.c | 1 + src/import/importd.c | 1 + src/login/logind-user.c | 3 +- src/login/pam_systemd.c | 1 + src/network/tc/tc-util.c | 1 + src/shared/bus-unit-util.c | 1 + src/shared/conf-parser.c | 1 + src/test/meson.build | 2 + src/test/test-parse-util.c | 144 -------------------------------- src/test/test-percent-util.c | 155 +++++++++++++++++++++++++++++++++++ 16 files changed, 327 insertions(+), 294 deletions(-) create mode 100644 src/basic/percent-util.c create mode 100644 src/basic/percent-util.h create mode 100644 src/test/test-percent-util.c diff --git a/src/basic/meson.build b/src/basic/meson.build index 0b0982d543..88639bc9b5 100644 --- a/src/basic/meson.build +++ b/src/basic/meson.build @@ -176,6 +176,8 @@ basic_sources = files(''' path-lookup.h path-util.c path-util.h + percent-util.c + percent-util.h prioq.c prioq.h proc-cmdline.c diff --git a/src/basic/parse-util.c b/src/basic/parse-util.c index 97d224f165..b79c885dfd 100644 --- a/src/basic/parse-util.c +++ b/src/basic/parse-util.c @@ -627,146 +627,6 @@ int parse_fractional_part_u(const char **p, size_t digits, unsigned *res) { return 0; } -static int parse_parts_value_whole(const char *p, const char *symbol) { - const char *pc, *n; - int r, v; - - pc = endswith(p, symbol); - if (!pc) - return -EINVAL; - - n = strndupa(p, pc - p); - r = safe_atoi(n, &v); - if (r < 0) - return r; - if (v < 0) - return -ERANGE; - - return v; -} - -static int parse_parts_value_with_tenths_place(const char *p, const char *symbol) { - const char *pc, *dot, *n; - int r, q, v; - - pc = endswith(p, symbol); - if (!pc) - return -EINVAL; - - dot = memchr(p, '.', pc - p); - if (dot) { - if (dot + 2 != pc) - return -EINVAL; - if (dot[1] < '0' || dot[1] > '9') - return -EINVAL; - q = dot[1] - '0'; - n = strndupa(p, dot - p); - } else { - q = 0; - n = strndupa(p, pc - p); - } - r = safe_atoi(n, &v); - if (r < 0) - return r; - if (v < 0) - return -ERANGE; - if (v > (INT_MAX - q) / 10) - return -ERANGE; - - v = v * 10 + q; - return v; -} - -static int parse_parts_value_with_hundredths_place(const char *p, const char *symbol) { - const char *pc, *dot, *n; - int r, q, v; - - pc = endswith(p, symbol); - if (!pc) - return -EINVAL; - - dot = memchr(p, '.', pc - p); - if (dot) { - if (dot + 3 != pc) - return -EINVAL; - if (dot[1] < '0' || dot[1] > '9' || dot[2] < '0' || dot[2] > '9') - return -EINVAL; - q = (dot[1] - '0') * 10 + (dot[2] - '0'); - n = strndupa(p, dot - p); - } else { - q = 0; - n = strndupa(p, pc - p); - } - r = safe_atoi(n, &v); - if (r < 0) - return r; - if (v < 0) - return -ERANGE; - if (v > (INT_MAX - q) / 100) - return -ERANGE; - - v = v * 100 + q; - return v; -} - -int parse_percent_unbounded(const char *p) { - return parse_parts_value_whole(p, "%"); -} - -int parse_percent(const char *p) { - int v; - - v = parse_percent_unbounded(p); - if (v > 100) - return -ERANGE; - - return v; -} - -int parse_permille_unbounded(const char *p) { - const char *pm; - - pm = endswith(p, "‰"); - if (pm) - return parse_parts_value_whole(p, "‰"); - - return parse_parts_value_with_tenths_place(p, "%"); -} - -int parse_permille(const char *p) { - int v; - - v = parse_permille_unbounded(p); - if (v > 1000) - return -ERANGE; - - return v; -} - -int parse_permyriad_unbounded(const char *p) { - const char *pm; - - pm = endswith(p, "‱"); - if (pm) - return parse_parts_value_whole(p, "‱"); - - pm = endswith(p, "‰"); - if (pm) - return parse_parts_value_with_tenths_place(p, "‰"); - - return parse_parts_value_with_hundredths_place(p, "%"); -} - -int parse_permyriad(const char *p) { - int v; - - v = parse_permyriad_unbounded(p); - if (v > 10000) - return -ERANGE; - - return v; -} - int parse_nice(const char *p, int *ret) { int n, r; diff --git a/src/basic/parse-util.h b/src/basic/parse-util.h index 29e04cf562..908202dafd 100644 --- a/src/basic/parse-util.h +++ b/src/basic/parse-util.h @@ -127,15 +127,6 @@ int safe_atod(const char *s, double *ret_d); int parse_fractional_part_u(const char **s, size_t digits, unsigned *res); -int parse_percent_unbounded(const char *p); -int parse_percent(const char *p); - -int parse_permille_unbounded(const char *p); -int parse_permille(const char *p); - -int parse_permyriad_unbounded(const char *p); -int parse_permyriad(const char *p); - int parse_nice(const char *p, int *ret); int parse_ip_port(const char *s, uint16_t *ret); diff --git a/src/basic/percent-util.c b/src/basic/percent-util.c new file mode 100644 index 0000000000..f58a51dcf9 --- /dev/null +++ b/src/basic/percent-util.c @@ -0,0 +1,145 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "percent-util.h" +#include "string-util.h" +#include "parse-util.h" + +static int parse_parts_value_whole(const char *p, const char *symbol) { + const char *pc, *n; + int r, v; + + pc = endswith(p, symbol); + if (!pc) + return -EINVAL; + + n = strndupa(p, pc - p); + r = safe_atoi(n, &v); + if (r < 0) + return r; + if (v < 0) + return -ERANGE; + + return v; +} + +static int parse_parts_value_with_tenths_place(const char *p, const char *symbol) { + const char *pc, *dot, *n; + int r, q, v; + + pc = endswith(p, symbol); + if (!pc) + return -EINVAL; + + dot = memchr(p, '.', pc - p); + if (dot) { + if (dot + 2 != pc) + return -EINVAL; + if (dot[1] < '0' || dot[1] > '9') + return -EINVAL; + q = dot[1] - '0'; + n = strndupa(p, dot - p); + } else { + q = 0; + n = strndupa(p, pc - p); + } + r = safe_atoi(n, &v); + if (r < 0) + return r; + if (v < 0) + return -ERANGE; + if (v > (INT_MAX - q) / 10) + return -ERANGE; + + v = v * 10 + q; + return v; +} + +static int parse_parts_value_with_hundredths_place(const char *p, const char *symbol) { + const char *pc, *dot, *n; + int r, q, v; + + pc = endswith(p, symbol); + if (!pc) + return -EINVAL; + + dot = memchr(p, '.', pc - p); + if (dot) { + if (dot + 3 != pc) + return -EINVAL; + if (dot[1] < '0' || dot[1] > '9' || dot[2] < '0' || dot[2] > '9') + return -EINVAL; + q = (dot[1] - '0') * 10 + (dot[2] - '0'); + n = strndupa(p, dot - p); + } else { + q = 0; + n = strndupa(p, pc - p); + } + r = safe_atoi(n, &v); + if (r < 0) + return r; + if (v < 0) + return -ERANGE; + if (v > (INT_MAX - q) / 100) + return -ERANGE; + + v = v * 100 + q; + return v; +} + +int parse_percent_unbounded(const char *p) { + return parse_parts_value_whole(p, "%"); +} + +int parse_percent(const char *p) { + int v; + + v = parse_percent_unbounded(p); + if (v > 100) + return -ERANGE; + + return v; +} + +int parse_permille_unbounded(const char *p) { + const char *pm; + + pm = endswith(p, "‰"); + if (pm) + return parse_parts_value_whole(p, "‰"); + + return parse_parts_value_with_tenths_place(p, "%"); +} + +int parse_permille(const char *p) { + int v; + + v = parse_permille_unbounded(p); + if (v > 1000) + return -ERANGE; + + return v; +} + +int parse_permyriad_unbounded(const char *p) { + const char *pm; + + pm = endswith(p, "‱"); + if (pm) + return parse_parts_value_whole(p, "‱"); + + pm = endswith(p, "‰"); + if (pm) + return parse_parts_value_with_tenths_place(p, "‰"); + + return parse_parts_value_with_hundredths_place(p, "%"); +} + +int parse_permyriad(const char *p) { + int v; + + v = parse_permyriad_unbounded(p); + if (v > 10000) + return -ERANGE; + + return v; +} diff --git a/src/basic/percent-util.h b/src/basic/percent-util.h new file mode 100644 index 0000000000..26707e8703 --- /dev/null +++ b/src/basic/percent-util.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +int parse_percent_unbounded(const char *p); +int parse_percent(const char *p); + +int parse_permille_unbounded(const char *p); +int parse_permille(const char *p); + +int parse_permyriad_unbounded(const char *p); +int parse_permyriad(const char *p); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index a1ab5f70eb..c56c2ffc61 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -46,6 +46,7 @@ #include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #if HAVE_SECCOMP #include "seccomp-util.h" @@ -3842,6 +3843,7 @@ int config_parse_managed_oom_mode( const char *rvalue, void *data, void *userdata) { + ManagedOOMMode *mode = data, m; UnitType t; @@ -3861,6 +3863,7 @@ int config_parse_managed_oom_mode( log_syntax(unit, LOG_WARNING, filename, line, m, "Invalid syntax, ignoring: %s", rvalue); return 0; } + *mode = m; return 0; } @@ -3876,6 +3879,7 @@ int config_parse_managed_oom_mem_pressure_limit( const char *rvalue, void *data, void *userdata) { + uint32_t *limit = data; UnitType t; int r; diff --git a/src/home/homectl.c b/src/home/homectl.c index 98835327cd..dbe5c3af20 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -26,6 +26,7 @@ #include "parse-argument.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "pkcs11-util.h" #include "pretty-print.h" #include "process-util.h" diff --git a/src/import/importd.c b/src/import/importd.c index 9ac2f8dcfe..f9e8481a6d 100644 --- a/src/import/importd.c +++ b/src/import/importd.c @@ -22,6 +22,7 @@ #include "mkdir.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #include "service-util.h" #include "signal-util.h" diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 4406f36614..a2c468e8dd 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -19,11 +19,12 @@ #include "label.h" #include "limits-util.h" #include "logind-dbus.h" -#include "logind-user.h" #include "logind-user-dbus.h" +#include "logind-user.h" #include "mkdir.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "rm-rf.h" #include "serialize.h" #include "special.h" diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index fce27262f1..3fc4f43d28 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -34,6 +34,7 @@ #include "pam-util.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #include "rlimit-util.h" #include "socket-util.h" diff --git a/src/network/tc/tc-util.c b/src/network/tc/tc-util.c index 1890e96164..3781182946 100644 --- a/src/network/tc/tc-util.c +++ b/src/network/tc/tc-util.c @@ -5,6 +5,7 @@ #include "extract-word.h" #include "fileio.h" #include "parse-util.h" +#include "percent-util.h" #include "tc-util.h" #include "time-util.h" diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index a1a253e801..a6e5e74132 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -28,6 +28,7 @@ #include "numa-util.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #include "rlimit-util.h" #if HAVE_SECCOMP diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index c3d7e536ec..df6472d4be 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -22,6 +22,7 @@ #include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #include "rlimit-util.h" #include "sd-id128.h" diff --git a/src/test/meson.build b/src/test/meson.build index 5bf1ca3490..c567a4cfcb 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -547,6 +547,8 @@ tests += [ [['src/test/test-bus-util.c']], + [['src/test/test-percent-util.c']], + [['src/test/test-sd-hwdb.c']], [['src/test/test-sd-path.c']], diff --git a/src/test/test-parse-util.c b/src/test/test-parse-util.c index 6e23efe134..756934acad 100644 --- a/src/test/test-parse-util.c +++ b/src/test/test-parse-util.c @@ -718,144 +718,6 @@ static void test_safe_atod(void) { assert_se(r == -EINVAL); } -static void test_parse_percent(void) { - assert_se(parse_percent("") == -EINVAL); - assert_se(parse_percent("foo") == -EINVAL); - assert_se(parse_percent("0") == -EINVAL); - assert_se(parse_percent("50") == -EINVAL); - assert_se(parse_percent("100") == -EINVAL); - assert_se(parse_percent("-1") == -EINVAL); - assert_se(parse_percent("0%") == 0); - assert_se(parse_percent("55%") == 55); - assert_se(parse_percent("100%") == 100); - assert_se(parse_percent("-7%") == -ERANGE); - assert_se(parse_percent("107%") == -ERANGE); - assert_se(parse_percent("%") == -EINVAL); - assert_se(parse_percent("%%") == -EINVAL); - assert_se(parse_percent("%1") == -EINVAL); - assert_se(parse_percent("1%%") == -EINVAL); - assert_se(parse_percent("3.2%") == -EINVAL); -} - -static void test_parse_percent_unbounded(void) { - assert_se(parse_percent_unbounded("101%") == 101); - assert_se(parse_percent_unbounded("400%") == 400); -} - -static void test_parse_permille(void) { - assert_se(parse_permille("") == -EINVAL); - assert_se(parse_permille("foo") == -EINVAL); - assert_se(parse_permille("0") == -EINVAL); - assert_se(parse_permille("50") == -EINVAL); - assert_se(parse_permille("100") == -EINVAL); - assert_se(parse_permille("-1") == -EINVAL); - - assert_se(parse_permille("0‰") == 0); - assert_se(parse_permille("555‰") == 555); - assert_se(parse_permille("1000‰") == 1000); - assert_se(parse_permille("-7‰") == -ERANGE); - assert_se(parse_permille("1007‰") == -ERANGE); - assert_se(parse_permille("‰") == -EINVAL); - assert_se(parse_permille("‰‰") == -EINVAL); - assert_se(parse_permille("‰1") == -EINVAL); - assert_se(parse_permille("1‰‰") == -EINVAL); - assert_se(parse_permille("3.2‰") == -EINVAL); - - assert_se(parse_permille("0%") == 0); - assert_se(parse_permille("55%") == 550); - assert_se(parse_permille("55.5%") == 555); - assert_se(parse_permille("100%") == 1000); - assert_se(parse_permille("-7%") == -ERANGE); - assert_se(parse_permille("107%") == -ERANGE); - assert_se(parse_permille("%") == -EINVAL); - assert_se(parse_permille("%%") == -EINVAL); - assert_se(parse_permille("%1") == -EINVAL); - assert_se(parse_permille("1%%") == -EINVAL); - assert_se(parse_permille("3.21%") == -EINVAL); -} - -static void test_parse_permille_unbounded(void) { - assert_se(parse_permille_unbounded("1001‰") == 1001); - assert_se(parse_permille_unbounded("4000‰") == 4000); - assert_se(parse_permille_unbounded("2147483647‰") == 2147483647); - assert_se(parse_permille_unbounded("2147483648‰") == -ERANGE); - assert_se(parse_permille_unbounded("4294967295‰") == -ERANGE); - assert_se(parse_permille_unbounded("4294967296‰") == -ERANGE); - - assert_se(parse_permille_unbounded("101%") == 1010); - assert_se(parse_permille_unbounded("400%") == 4000); - assert_se(parse_permille_unbounded("214748364.7%") == 2147483647); - assert_se(parse_permille_unbounded("214748364.8%") == -ERANGE); - assert_se(parse_permille_unbounded("429496729.5%") == -ERANGE); - assert_se(parse_permille_unbounded("429496729.6%") == -ERANGE); -} - -static void test_parse_permyriad(void) { - assert_se(parse_permyriad("") == -EINVAL); - assert_se(parse_permyriad("foo") == -EINVAL); - assert_se(parse_permyriad("0") == -EINVAL); - assert_se(parse_permyriad("50") == -EINVAL); - assert_se(parse_permyriad("100") == -EINVAL); - assert_se(parse_permyriad("-1") == -EINVAL); - - assert_se(parse_permyriad("0‱") == 0); - assert_se(parse_permyriad("555‱") == 555); - assert_se(parse_permyriad("1000‱") == 1000); - assert_se(parse_permyriad("-7‱") == -ERANGE); - assert_se(parse_permyriad("10007‱") == -ERANGE); - assert_se(parse_permyriad("‱") == -EINVAL); - assert_se(parse_permyriad("‱‱") == -EINVAL); - assert_se(parse_permyriad("‱1") == -EINVAL); - assert_se(parse_permyriad("1‱‱") == -EINVAL); - assert_se(parse_permyriad("3.2‱") == -EINVAL); - - assert_se(parse_permyriad("0‰") == 0); - assert_se(parse_permyriad("555.5‰") == 5555); - assert_se(parse_permyriad("1000.0‰") == 10000); - assert_se(parse_permyriad("-7‰") == -ERANGE); - assert_se(parse_permyriad("1007‰") == -ERANGE); - assert_se(parse_permyriad("‰") == -EINVAL); - assert_se(parse_permyriad("‰‰") == -EINVAL); - assert_se(parse_permyriad("‰1") == -EINVAL); - assert_se(parse_permyriad("1‰‰") == -EINVAL); - assert_se(parse_permyriad("3.22‰") == -EINVAL); - - assert_se(parse_permyriad("0%") == 0); - assert_se(parse_permyriad("55%") == 5500); - assert_se(parse_permyriad("55.53%") == 5553); - assert_se(parse_permyriad("100%") == 10000); - assert_se(parse_permyriad("-7%") == -ERANGE); - assert_se(parse_permyriad("107%") == -ERANGE); - assert_se(parse_permyriad("%") == -EINVAL); - assert_se(parse_permyriad("%%") == -EINVAL); - assert_se(parse_permyriad("%1") == -EINVAL); - assert_se(parse_permyriad("1%%") == -EINVAL); - assert_se(parse_permyriad("3.212%") == -EINVAL); -} - -static void test_parse_permyriad_unbounded(void) { - assert_se(parse_permyriad_unbounded("1001‱") == 1001); - assert_se(parse_permyriad_unbounded("4000‱") == 4000); - assert_se(parse_permyriad_unbounded("2147483647‱") == 2147483647); - assert_se(parse_permyriad_unbounded("2147483648‱") == -ERANGE); - assert_se(parse_permyriad_unbounded("4294967295‱") == -ERANGE); - assert_se(parse_permyriad_unbounded("4294967296‱") == -ERANGE); - - assert_se(parse_permyriad_unbounded("101‰") == 1010); - assert_se(parse_permyriad_unbounded("400‰") == 4000); - assert_se(parse_permyriad_unbounded("214748364.7‰") == 2147483647); - assert_se(parse_permyriad_unbounded("214748364.8‰") == -ERANGE); - assert_se(parse_permyriad_unbounded("429496729.5‰") == -ERANGE); - assert_se(parse_permyriad_unbounded("429496729.6‰") == -ERANGE); - - assert_se(parse_permyriad_unbounded("99%") == 9900); - assert_se(parse_permyriad_unbounded("40%") == 4000); - assert_se(parse_permyriad_unbounded("21474836.47%") == 2147483647); - assert_se(parse_permyriad_unbounded("21474836.48%") == -ERANGE); - assert_se(parse_permyriad_unbounded("42949672.95%") == -ERANGE); - assert_se(parse_permyriad_unbounded("42949672.96%") == -ERANGE); -} - static void test_parse_nice(void) { int n; @@ -1049,12 +911,6 @@ int main(int argc, char *argv[]) { test_safe_atoi64(); test_safe_atoux64(); test_safe_atod(); - test_parse_percent(); - test_parse_percent_unbounded(); - test_parse_permille(); - test_parse_permille_unbounded(); - test_parse_permyriad(); - test_parse_permyriad_unbounded(); test_parse_nice(); test_parse_dev(); test_parse_errno(); diff --git a/src/test/test-percent-util.c b/src/test/test-percent-util.c new file mode 100644 index 0000000000..9d0ad90de1 --- /dev/null +++ b/src/test/test-percent-util.c @@ -0,0 +1,155 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "percent-util.h" +#include "tests.h" + +static void test_parse_percent(void) { + assert_se(parse_percent("") == -EINVAL); + assert_se(parse_percent("foo") == -EINVAL); + assert_se(parse_percent("0") == -EINVAL); + assert_se(parse_percent("50") == -EINVAL); + assert_se(parse_percent("100") == -EINVAL); + assert_se(parse_percent("-1") == -EINVAL); + assert_se(parse_percent("0%") == 0); + assert_se(parse_percent("55%") == 55); + assert_se(parse_percent("100%") == 100); + assert_se(parse_percent("-7%") == -ERANGE); + assert_se(parse_percent("107%") == -ERANGE); + assert_se(parse_percent("%") == -EINVAL); + assert_se(parse_percent("%%") == -EINVAL); + assert_se(parse_percent("%1") == -EINVAL); + assert_se(parse_percent("1%%") == -EINVAL); + assert_se(parse_percent("3.2%") == -EINVAL); +} + +static void test_parse_percent_unbounded(void) { + assert_se(parse_percent_unbounded("101%") == 101); + assert_se(parse_percent_unbounded("400%") == 400); +} + +static void test_parse_permille(void) { + assert_se(parse_permille("") == -EINVAL); + assert_se(parse_permille("foo") == -EINVAL); + assert_se(parse_permille("0") == -EINVAL); + assert_se(parse_permille("50") == -EINVAL); + assert_se(parse_permille("100") == -EINVAL); + assert_se(parse_permille("-1") == -EINVAL); + + assert_se(parse_permille("0‰") == 0); + assert_se(parse_permille("555‰") == 555); + assert_se(parse_permille("1000‰") == 1000); + assert_se(parse_permille("-7‰") == -ERANGE); + assert_se(parse_permille("1007‰") == -ERANGE); + assert_se(parse_permille("‰") == -EINVAL); + assert_se(parse_permille("‰‰") == -EINVAL); + assert_se(parse_permille("‰1") == -EINVAL); + assert_se(parse_permille("1‰‰") == -EINVAL); + assert_se(parse_permille("3.2‰") == -EINVAL); + + assert_se(parse_permille("0%") == 0); + assert_se(parse_permille("55%") == 550); + assert_se(parse_permille("55.5%") == 555); + assert_se(parse_permille("100%") == 1000); + assert_se(parse_permille("-7%") == -ERANGE); + assert_se(parse_permille("107%") == -ERANGE); + assert_se(parse_permille("%") == -EINVAL); + assert_se(parse_permille("%%") == -EINVAL); + assert_se(parse_permille("%1") == -EINVAL); + assert_se(parse_permille("1%%") == -EINVAL); + assert_se(parse_permille("3.21%") == -EINVAL); +} + +static void test_parse_permille_unbounded(void) { + assert_se(parse_permille_unbounded("1001‰") == 1001); + assert_se(parse_permille_unbounded("4000‰") == 4000); + assert_se(parse_permille_unbounded("2147483647‰") == 2147483647); + assert_se(parse_permille_unbounded("2147483648‰") == -ERANGE); + assert_se(parse_permille_unbounded("4294967295‰") == -ERANGE); + assert_se(parse_permille_unbounded("4294967296‰") == -ERANGE); + + assert_se(parse_permille_unbounded("101%") == 1010); + assert_se(parse_permille_unbounded("400%") == 4000); + assert_se(parse_permille_unbounded("214748364.7%") == 2147483647); + assert_se(parse_permille_unbounded("214748364.8%") == -ERANGE); + assert_se(parse_permille_unbounded("429496729.5%") == -ERANGE); + assert_se(parse_permille_unbounded("429496729.6%") == -ERANGE); +} + +static void test_parse_permyriad(void) { + assert_se(parse_permyriad("") == -EINVAL); + assert_se(parse_permyriad("foo") == -EINVAL); + assert_se(parse_permyriad("0") == -EINVAL); + assert_se(parse_permyriad("50") == -EINVAL); + assert_se(parse_permyriad("100") == -EINVAL); + assert_se(parse_permyriad("-1") == -EINVAL); + + assert_se(parse_permyriad("0‱") == 0); + assert_se(parse_permyriad("555‱") == 555); + assert_se(parse_permyriad("1000‱") == 1000); + assert_se(parse_permyriad("-7‱") == -ERANGE); + assert_se(parse_permyriad("10007‱") == -ERANGE); + assert_se(parse_permyriad("‱") == -EINVAL); + assert_se(parse_permyriad("‱‱") == -EINVAL); + assert_se(parse_permyriad("‱1") == -EINVAL); + assert_se(parse_permyriad("1‱‱") == -EINVAL); + assert_se(parse_permyriad("3.2‱") == -EINVAL); + + assert_se(parse_permyriad("0‰") == 0); + assert_se(parse_permyriad("555.5‰") == 5555); + assert_se(parse_permyriad("1000.0‰") == 10000); + assert_se(parse_permyriad("-7‰") == -ERANGE); + assert_se(parse_permyriad("1007‰") == -ERANGE); + assert_se(parse_permyriad("‰") == -EINVAL); + assert_se(parse_permyriad("‰‰") == -EINVAL); + assert_se(parse_permyriad("‰1") == -EINVAL); + assert_se(parse_permyriad("1‰‰") == -EINVAL); + assert_se(parse_permyriad("3.22‰") == -EINVAL); + + assert_se(parse_permyriad("0%") == 0); + assert_se(parse_permyriad("55%") == 5500); + assert_se(parse_permyriad("55.53%") == 5553); + assert_se(parse_permyriad("100%") == 10000); + assert_se(parse_permyriad("-7%") == -ERANGE); + assert_se(parse_permyriad("107%") == -ERANGE); + assert_se(parse_permyriad("%") == -EINVAL); + assert_se(parse_permyriad("%%") == -EINVAL); + assert_se(parse_permyriad("%1") == -EINVAL); + assert_se(parse_permyriad("1%%") == -EINVAL); + assert_se(parse_permyriad("3.212%") == -EINVAL); +} + +static void test_parse_permyriad_unbounded(void) { + assert_se(parse_permyriad_unbounded("1001‱") == 1001); + assert_se(parse_permyriad_unbounded("4000‱") == 4000); + assert_se(parse_permyriad_unbounded("2147483647‱") == 2147483647); + assert_se(parse_permyriad_unbounded("2147483648‱") == -ERANGE); + assert_se(parse_permyriad_unbounded("4294967295‱") == -ERANGE); + assert_se(parse_permyriad_unbounded("4294967296‱") == -ERANGE); + + assert_se(parse_permyriad_unbounded("101‰") == 1010); + assert_se(parse_permyriad_unbounded("400‰") == 4000); + assert_se(parse_permyriad_unbounded("214748364.7‰") == 2147483647); + assert_se(parse_permyriad_unbounded("214748364.8‰") == -ERANGE); + assert_se(parse_permyriad_unbounded("429496729.5‰") == -ERANGE); + assert_se(parse_permyriad_unbounded("429496729.6‰") == -ERANGE); + + assert_se(parse_permyriad_unbounded("99%") == 9900); + assert_se(parse_permyriad_unbounded("40%") == 4000); + assert_se(parse_permyriad_unbounded("21474836.47%") == 2147483647); + assert_se(parse_permyriad_unbounded("21474836.48%") == -ERANGE); + assert_se(parse_permyriad_unbounded("42949672.95%") == -ERANGE); + assert_se(parse_permyriad_unbounded("42949672.96%") == -ERANGE); +} + +int main(int argc, char *argv[]) { + test_setup_logging(LOG_DEBUG); + + test_parse_percent(); + test_parse_percent_unbounded(); + test_parse_permille(); + test_parse_permille_unbounded(); + test_parse_permyriad(); + test_parse_permyriad_unbounded(); + + return 0; +} From 38d0c270063c9d7379b45b80b20b179a713edc6e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 15:33:05 +0100 Subject: [PATCH 05/10] percent-util: when parsing permyriads, permit percents too with 1 place after the dot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when parsing myriads, we'd support: x% → percent, no places after the dot x.yz% → percent, two places after the dot x‰ → permille, no places after the dot x.y‰ → permille, one place after the dot x‱ → permyriad, no places after the dot What's missing is: x.y% → percent, one place after the dot Let's add it in. --- src/basic/percent-util.c | 20 ++++++++++++++++---- src/test/test-percent-util.c | 9 +++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/basic/percent-util.c b/src/basic/percent-util.c index f58a51dcf9..06f20fd61e 100644 --- a/src/basic/percent-util.c +++ b/src/basic/percent-util.c @@ -64,11 +64,23 @@ static int parse_parts_value_with_hundredths_place(const char *p, const char *sy dot = memchr(p, '.', pc - p); if (dot) { - if (dot + 3 != pc) + if (dot + 3 == pc) { + /* Support two places after the dot */ + + if (dot[1] < '0' || dot[1] > '9' || dot[2] < '0' || dot[2] > '9') + return -EINVAL; + q = (dot[1] - '0') * 10 + (dot[2] - '0'); + + } else if (dot + 2 == pc) { + /* Support one place after the dot */ + + if (dot[1] < '0' || dot[1] > '9') + return -EINVAL; + q = (dot[1] - '0') * 10; + } else + /* We do not support zero or more than two places */ return -EINVAL; - if (dot[1] < '0' || dot[1] > '9' || dot[2] < '0' || dot[2] > '9') - return -EINVAL; - q = (dot[1] - '0') * 10 + (dot[2] - '0'); + n = strndupa(p, dot - p); } else { q = 0; diff --git a/src/test/test-percent-util.c b/src/test/test-percent-util.c index 9d0ad90de1..75f8a6c83d 100644 --- a/src/test/test-percent-util.c +++ b/src/test/test-percent-util.c @@ -7,6 +7,7 @@ static void test_parse_percent(void) { assert_se(parse_percent("") == -EINVAL); assert_se(parse_percent("foo") == -EINVAL); assert_se(parse_percent("0") == -EINVAL); + assert_se(parse_percent("0.1") == -EINVAL); assert_se(parse_percent("50") == -EINVAL); assert_se(parse_percent("100") == -EINVAL); assert_se(parse_percent("-1") == -EINVAL); @@ -34,6 +35,10 @@ static void test_parse_permille(void) { assert_se(parse_permille("50") == -EINVAL); assert_se(parse_permille("100") == -EINVAL); assert_se(parse_permille("-1") == -EINVAL); + assert_se(parse_permille("0.1") == -EINVAL); + assert_se(parse_permille("5%") == 50); + assert_se(parse_permille("5.5%") == 55); + assert_se(parse_permille("5.12%") == -EINVAL); assert_se(parse_permille("0‰") == 0); assert_se(parse_permille("555‰") == 555); @@ -45,6 +50,7 @@ static void test_parse_permille(void) { assert_se(parse_permille("‰1") == -EINVAL); assert_se(parse_permille("1‰‰") == -EINVAL); assert_se(parse_permille("3.2‰") == -EINVAL); + assert_se(parse_permille("0.1‰") == -EINVAL); assert_se(parse_permille("0%") == 0); assert_se(parse_permille("55%") == 550); @@ -57,6 +63,7 @@ static void test_parse_permille(void) { assert_se(parse_permille("%1") == -EINVAL); assert_se(parse_permille("1%%") == -EINVAL); assert_se(parse_permille("3.21%") == -EINVAL); + assert_se(parse_permille("0.1%") == 1); } static void test_parse_permille_unbounded(void) { @@ -107,6 +114,8 @@ static void test_parse_permyriad(void) { assert_se(parse_permyriad("0%") == 0); assert_se(parse_permyriad("55%") == 5500); + assert_se(parse_permyriad("55.5%") == 5550); + assert_se(parse_permyriad("55.50%") == 5550); assert_se(parse_permyriad("55.53%") == 5553); assert_se(parse_permyriad("100%") == 10000); assert_se(parse_permyriad("-7%") == -ERANGE); From 3b6e71ad0351c5ed2dd8382a63ab3581fa2d6174 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 16:53:11 +0100 Subject: [PATCH 06/10] util: add some helpers for converting percent/permille/permyriad to parts of 2^32-1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At various places we accept values scaled to the range 0…2^32-1 which are exposed to the user as percentages/permille/permyriad. Let's add some helper macros (actually: typesafe macro-like functions) that help with converting our internal encoding to the external encodings. benefits: some of the previous code rounded up, some down. let's always round to nearest, to ensure that our conversions are reversible. Also, check for overflows correctly. This also adds a test that makes sure that for the full percent/permille/permyriad ranges we can convert forth and back without loss of accuracy. --- src/basic/percent-util.h | 50 ++++++++++++++++++++++++++++++++++++ src/test/test-percent-util.c | 47 +++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/src/basic/percent-util.h b/src/basic/percent-util.h index 26707e8703..d806ec007a 100644 --- a/src/basic/percent-util.h +++ b/src/basic/percent-util.h @@ -1,6 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include +#include + +#include "macro.h" + int parse_percent_unbounded(const char *p); int parse_percent(const char *p); @@ -9,3 +14,48 @@ int parse_permille(const char *p); int parse_permyriad_unbounded(const char *p); int parse_permyriad(const char *p); + +/* Some macro-like helpers that convert a percent/permille/permyriad value (as parsed by parse_percent()) to + * a value relative to 100% == 2^32-1. Rounds to closest. */ +static inline uint32_t UINT32_SCALE_FROM_PERCENT(int percent) { + assert_cc(INT_MAX <= UINT32_MAX); + return (uint32_t) (((uint64_t) percent * UINT32_MAX + 50) / 100U); +} + +static inline uint32_t UINT32_SCALE_FROM_PERMILLE(int permille) { + return (uint32_t) (((uint64_t) permille * UINT32_MAX + 500) / 1000U); +} + +static inline uint32_t UINT32_SCALE_FROM_PERMYRIAD(int permyriad) { + return (uint32_t) (((uint64_t) permyriad * UINT32_MAX + 5000) / 10000U); +} + +static inline int UINT32_SCALE_TO_PERCENT(uint32_t scale) { + uint32_t u; + + u = (uint32_t) ((((uint64_t) scale) * 100U + UINT32_MAX/2) / UINT32_MAX); + if (u > INT_MAX) + return -ERANGE; + + return (int) u; +} + +static inline int UINT32_SCALE_TO_PERMILLE(uint32_t scale) { + uint32_t u; + + u = (uint32_t) ((((uint64_t) scale) * 1000U + UINT32_MAX/2) / UINT32_MAX); + if (u > INT_MAX) + return -ERANGE; + + return (int) u; +} + +static inline int UINT32_SCALE_TO_PERMYRIAD(uint32_t scale) { + uint32_t u; + + u = (uint32_t) ((((uint64_t) scale) * 10000U + UINT32_MAX/2) / UINT32_MAX); + if (u > INT_MAX) + return -ERANGE; + + return (int) u; +} diff --git a/src/test/test-percent-util.c b/src/test/test-percent-util.c index 75f8a6c83d..b8801438a7 100644 --- a/src/test/test-percent-util.c +++ b/src/test/test-percent-util.c @@ -2,6 +2,7 @@ #include "percent-util.h" #include "tests.h" +#include "time-util.h" static void test_parse_percent(void) { assert_se(parse_percent("") == -EINVAL); @@ -150,6 +151,51 @@ static void test_parse_permyriad_unbounded(void) { assert_se(parse_permyriad_unbounded("42949672.96%") == -ERANGE); } +static void test_scale(void) { + /* Check some fixed values */ + assert_se(UINT32_SCALE_FROM_PERCENT(0) == 0); + assert_se(UINT32_SCALE_FROM_PERCENT(50) == UINT32_MAX/2+1); + assert_se(UINT32_SCALE_FROM_PERCENT(100) == UINT32_MAX); + + assert_se(UINT32_SCALE_FROM_PERMILLE(0) == 0); + assert_se(UINT32_SCALE_FROM_PERMILLE(500) == UINT32_MAX/2+1); + assert_se(UINT32_SCALE_FROM_PERMILLE(1000) == UINT32_MAX); + + assert_se(UINT32_SCALE_FROM_PERMYRIAD(0) == 0); + assert_se(UINT32_SCALE_FROM_PERMYRIAD(5000) == UINT32_MAX/2+1); + assert_se(UINT32_SCALE_FROM_PERMYRIAD(10000) == UINT32_MAX); + + /* Make sure there's no numeric noise on the 0%…100% scale when converting from percent and back. */ + for (int percent = 0; percent <= 100; percent++) { + log_debug("%i%% → %" PRIu32 " → %i%%", + percent, + UINT32_SCALE_FROM_PERCENT(percent), + UINT32_SCALE_TO_PERCENT(UINT32_SCALE_FROM_PERCENT(percent))); + + assert_se(UINT32_SCALE_TO_PERCENT(UINT32_SCALE_FROM_PERCENT(percent)) == percent); + } + + /* Make sure there's no numeric noise on the 0‰…1000‰ scale when converting from permille and back. */ + for (int permille = 0; permille <= 1000; permille++) { + log_debug("%i‰ → %" PRIu32 " → %i‰", + permille, + UINT32_SCALE_FROM_PERMILLE(permille), + UINT32_SCALE_TO_PERMILLE(UINT32_SCALE_FROM_PERMILLE(permille))); + + assert_se(UINT32_SCALE_TO_PERMILLE(UINT32_SCALE_FROM_PERMILLE(permille)) == permille); + } + + /* Make sure there's no numeric noise on the 0‱…10000‱ scale when converting from permyriad and back. */ + for (int permyriad = 0; permyriad <= 10000; permyriad++) { + log_debug("%i‱ → %" PRIu32 " → %i‱", + permyriad, + UINT32_SCALE_FROM_PERMYRIAD(permyriad), + UINT32_SCALE_TO_PERMYRIAD(UINT32_SCALE_FROM_PERMYRIAD(permyriad))); + + assert_se(UINT32_SCALE_TO_PERMYRIAD(UINT32_SCALE_FROM_PERMYRIAD(permyriad)) == permyriad); + } +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); @@ -159,6 +205,7 @@ int main(int argc, char *argv[]) { test_parse_permille_unbounded(); test_parse_permyriad(); test_parse_permyriad_unbounded(); + test_scale(); return 0; } From 9cba32bcde31033bffbac6d53c028f8f95212293 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 17:03:52 +0100 Subject: [PATCH 07/10] tree-wide: port various pieces of code over to UINT32_SCALE_FROM_PERMYRIAD() --- src/core/dbus-cgroup.c | 7 ++++--- src/home/homectl.c | 2 +- src/login/pam_systemd.c | 2 +- src/shared/bus-unit-util.c | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 6652c212f1..e22233d6ff 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -16,6 +16,7 @@ #include "fileio.h" #include "limits-util.h" #include "path-util.h" +#include "percent-util.h" BUS_DEFINE_PROPERTY_GET(bus_property_get_tasks_max, "t", TasksMax, tasks_max_resolve); @@ -704,10 +705,10 @@ static int bus_cgroup_set_boolean( /* Prepare to chop off suffix */ \ assert_se(endswith(name, "Scale")); \ \ - uint32_t scaled = DIV_ROUND_UP((uint64_t) raw * 1000, (uint64_t) UINT32_MAX); \ - unit_write_settingf(u, flags, name, "%.*s=%" PRIu32 ".%" PRIu32 "%%", \ + int scaled = UINT32_SCALE_TO_PERMYRIAD(raw); \ + unit_write_settingf(u, flags, name, "%.*s=%i.%02i%%", \ (int)(strlen(name) - strlen("Scale")), name, \ - scaled / 10, scaled % 10); \ + scaled / 100, scaled % 100); \ } \ \ return 1; \ diff --git a/src/home/homectl.c b/src/home/homectl.c index dbe5c3af20..d59dfb1f60 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -2671,7 +2671,7 @@ static int parse_argv(int argc, char *argv[]) { arg_disk_size_relative = UINT64_MAX; } else { /* Normalize to UINT32_MAX == 100% */ - arg_disk_size_relative = (uint64_t) r * UINT32_MAX / 10000U; + arg_disk_size_relative = UINT32_SCALE_FROM_PERMYRIAD(r); r = drop_from_identity("diskSize"); if (r < 0) diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 3fc4f43d28..a23200b3b5 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -337,7 +337,7 @@ static int append_session_memory_max(pam_handle_t *handle, sd_bus_message *m, co r = parse_permyriad(limit); if (r >= 0) { - r = sd_bus_message_append(m, "(sv)", "MemoryMaxScale", "u", (uint32_t) ((uint64_t) r * UINT32_MAX) / 10000U); + r = sd_bus_message_append(m, "(sv)", "MemoryMaxScale", "u", UINT32_SCALE_FROM_PERMYRIAD(r)); if (r < 0) return pam_bus_log_create_error(handle, r); diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index a6e5e74132..27c8eb915f 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -549,7 +549,7 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons * size can be determined server-side. */ n = strjoina(field, "Scale"); - r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) (((uint64_t) r * UINT32_MAX) / 10000U)); + r = sd_bus_message_append(m, "(sv)", n, "u", UINT32_SCALE_FROM_PERMYRIAD(r)); if (r < 0) return bus_log_create_error(r); From 1ead0b2a7973782f4bacaab2d9deed94ac69d867 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 17:29:43 +0100 Subject: [PATCH 08/10] parse-util: add format string macro for outputting permyriad Let's define a set of macros for making output of permyriad values easy. They are printed in pure ASCII, i.e. without the permille/permyriad suffix, using just percent and two places after the dot. --- src/basic/percent-util.h | 3 +++ src/core/cgroup.c | 1 + src/core/dbus-cgroup.c | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/basic/percent-util.h b/src/basic/percent-util.h index d806ec007a..24f4c3bdef 100644 --- a/src/basic/percent-util.h +++ b/src/basic/percent-util.h @@ -59,3 +59,6 @@ static inline int UINT32_SCALE_TO_PERMYRIAD(uint32_t scale) { return (int) u; } + +#define PERMYRIAD_AS_PERCENT_FORMAT_STR "%i.%02i%%" +#define PERMYRIAD_AS_PERCENT_FORMAT_VAL(x) ((x)/100), ((x)%100) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index b0fc67a125..644bf50e7d 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -21,6 +21,7 @@ #include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" +#include "percent-util.h" #include "process-util.h" #include "procfs-util.h" #include "special.h" diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index e22233d6ff..5bf9d7f4a8 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -706,9 +706,9 @@ static int bus_cgroup_set_boolean( assert_se(endswith(name, "Scale")); \ \ int scaled = UINT32_SCALE_TO_PERMYRIAD(raw); \ - unit_write_settingf(u, flags, name, "%.*s=%i.%02i%%", \ + unit_write_settingf(u, flags, name, "%.*s=" PERMYRIAD_AS_PERCENT_FORMAT_STR, \ (int)(strlen(name) - strlen("Scale")), name, \ - scaled / 100, scaled % 100); \ + PERMYRIAD_AS_PERCENT_FORMAT_VAL(scaled)); \ } \ \ return 1; \ From d9d3f05def0057cc064a05f8cbfb5f398779cd8c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 17:51:27 +0100 Subject: [PATCH 09/10] core: use our usual UINT32_MAX scaling for OOMD limits So far OOMD limits used permyriads, as an upgrade from the original percent. The rest of our codebase typically scales stuff relative to UINT32_MAX. Let's clean this up, an make sure this happens here too. This is particularly relevant, as this is exposed in unit files and API, and before we mark this stable we should get the APIs right. --- man/org.freedesktop.systemd1.xml | 36 +++++++++++++-------------- src/core/cgroup.c | 4 +-- src/core/cgroup.h | 2 +- src/core/core-varlink.c | 2 +- src/core/dbus-cgroup.c | 13 +++++----- src/core/load-fragment-gperf.gperf.m4 | 2 +- src/core/load-fragment.c | 3 ++- src/oom/oomd-manager.c | 12 ++++++--- src/shared/bus-unit-util.c | 7 ++---- 9 files changed, 42 insertions(+), 39 deletions(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 2da0ff0579..8c370ba9a4 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -2469,7 +2469,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -2994,7 +2994,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { - + @@ -3560,7 +3560,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { - + @@ -4229,7 +4229,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2esocket { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -4782,7 +4782,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2esocket { - + @@ -5346,7 +5346,7 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2esocket { - + @@ -5928,7 +5928,7 @@ node /org/freedesktop/systemd1/unit/home_2emount { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -6409,7 +6409,7 @@ node /org/freedesktop/systemd1/unit/home_2emount { - + @@ -6891,7 +6891,7 @@ node /org/freedesktop/systemd1/unit/home_2emount { - + @@ -7594,7 +7594,7 @@ node /org/freedesktop/systemd1/unit/dev_2dsda3_2eswap { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -8061,7 +8061,7 @@ node /org/freedesktop/systemd1/unit/dev_2dsda3_2eswap { - + @@ -8529,7 +8529,7 @@ node /org/freedesktop/systemd1/unit/dev_2dsda3_2eswap { - + @@ -9085,7 +9085,7 @@ node /org/freedesktop/systemd1/unit/system_2eslice { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; }; @@ -9222,7 +9222,7 @@ node /org/freedesktop/systemd1/unit/system_2eslice { - + @@ -9364,7 +9364,7 @@ node /org/freedesktop/systemd1/unit/system_2eslice { - + @@ -9526,7 +9526,7 @@ node /org/freedesktop/systemd1/unit/session_2d1_2escope { @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMMemoryPressure = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") - readonly u ManagedOOMMemoryPressureLimitPermyriad = ...; + readonly u ManagedOOMMemoryPressureLimit = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s ManagedOOMPreference = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") @@ -9679,7 +9679,7 @@ node /org/freedesktop/systemd1/unit/session_2d1_2escope { - + @@ -9847,7 +9847,7 @@ node /org/freedesktop/systemd1/unit/session_2d1_2escope { - + diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 644bf50e7d..1f74cb393f 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -419,7 +419,7 @@ void cgroup_context_dump(Unit *u, FILE* f, const char *prefix) { "%sDelegate: %s\n" "%sManagedOOMSwap: %s\n" "%sManagedOOMMemoryPressure: %s\n" - "%sManagedOOMMemoryPressureLimit: %" PRIu32 ".%02" PRIu32 "%%\n" + "%sManagedOOMMemoryPressureLimit: " PERMYRIAD_AS_PERCENT_FORMAT_STR "\n" "%sManagedOOMPreference: %s%%\n", prefix, yes_no(c->cpu_accounting), prefix, yes_no(c->io_accounting), @@ -453,7 +453,7 @@ void cgroup_context_dump(Unit *u, FILE* f, const char *prefix) { prefix, yes_no(c->delegate), prefix, managed_oom_mode_to_string(c->moom_swap), prefix, managed_oom_mode_to_string(c->moom_mem_pressure), - prefix, c->moom_mem_pressure_limit_permyriad / 100, c->moom_mem_pressure_limit_permyriad % 100, + prefix, PERMYRIAD_AS_PERCENT_FORMAT_VAL(UINT32_SCALE_TO_PERMYRIAD(c->moom_mem_pressure_limit)), prefix, managed_oom_preference_to_string(c->moom_preference)); if (c->delegate) { diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 8183551420..fa79ba1523 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -163,7 +163,7 @@ struct CGroupContext { /* Settings for systemd-oomd */ ManagedOOMMode moom_swap; ManagedOOMMode moom_mem_pressure; - uint32_t moom_mem_pressure_limit_permyriad; + uint32_t moom_mem_pressure_limit; /* Normalized to 2^32-1 == 100% */ ManagedOOMPreference moom_preference; }; diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index df542e82d1..d695106658 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -83,7 +83,7 @@ static int build_managed_oom_json_array_element(Unit *u, const char *property, J JSON_BUILD_PAIR("mode", JSON_BUILD_STRING(mode)), JSON_BUILD_PAIR("path", JSON_BUILD_STRING(u->cgroup_path)), JSON_BUILD_PAIR("property", JSON_BUILD_STRING(property)), - JSON_BUILD_PAIR_CONDITION(use_limit, "limit", JSON_BUILD_UNSIGNED(c->moom_mem_pressure_limit_permyriad)))); + JSON_BUILD_PAIR_CONDITION(use_limit, "limit", JSON_BUILD_UNSIGNED(c->moom_mem_pressure_limit)))); } int manager_varlink_send_managed_oom_update(Unit *u) { diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 5bf9d7f4a8..04d2ba34f3 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -396,7 +396,7 @@ const sd_bus_vtable bus_cgroup_vtable[] = { SD_BUS_PROPERTY("DisableControllers", "as", property_get_cgroup_mask, offsetof(CGroupContext, disable_controllers), 0), SD_BUS_PROPERTY("ManagedOOMSwap", "s", property_get_managed_oom_mode, offsetof(CGroupContext, moom_swap), 0), SD_BUS_PROPERTY("ManagedOOMMemoryPressure", "s", property_get_managed_oom_mode, offsetof(CGroupContext, moom_mem_pressure), 0), - SD_BUS_PROPERTY("ManagedOOMMemoryPressureLimitPermyriad", "u", NULL, offsetof(CGroupContext, moom_mem_pressure_limit_permyriad), 0), + SD_BUS_PROPERTY("ManagedOOMMemoryPressureLimit", "u", NULL, offsetof(CGroupContext, moom_mem_pressure_limit), 0), SD_BUS_PROPERTY("ManagedOOMPreference", "s", property_get_managed_oom_preference, offsetof(CGroupContext, moom_preference), 0), SD_BUS_VTABLE_END }; @@ -1699,7 +1699,7 @@ int bus_cgroup_set_property( return 1; } - if (streq(name, "ManagedOOMMemoryPressureLimitPermyriad")) { + if (streq(name, "ManagedOOMMemoryPressureLimit")) { uint32_t v; if (!UNIT_VTABLE(u)->can_set_managed_oom) @@ -1709,12 +1709,11 @@ int bus_cgroup_set_property( if (r < 0) return r; - if (v > 10000) - return -ERANGE; - if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - c->moom_mem_pressure_limit_permyriad = v; - unit_write_settingf(u, flags, name, "ManagedOOMMemoryPressureLimit=%" PRIu32 ".%02" PRIu32 "%%", v / 100, v % 100); + c->moom_mem_pressure_limit = v; + unit_write_settingf(u, flags, name, + "ManagedOOMMemoryPressureLimit=" PERMYRIAD_AS_PERCENT_FORMAT_STR, + PERMYRIAD_AS_PERCENT_FORMAT_VAL(UINT32_SCALE_TO_PERMYRIAD(v))); } if (c->moom_mem_pressure == MANAGED_OOM_KILL) diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index b0460e2764..6ed6b07db2 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -229,7 +229,7 @@ $1.IPIngressFilterPath, config_parse_ip_filter_bpf_progs, $1.IPEgressFilterPath, config_parse_ip_filter_bpf_progs, 0, offsetof($1, cgroup_context.ip_filters_egress) $1.ManagedOOMSwap, config_parse_managed_oom_mode, 0, offsetof($1, cgroup_context.moom_swap) $1.ManagedOOMMemoryPressure, config_parse_managed_oom_mode, 0, offsetof($1, cgroup_context.moom_mem_pressure) -$1.ManagedOOMMemoryPressureLimit, config_parse_managed_oom_mem_pressure_limit, 0, offsetof($1, cgroup_context.moom_mem_pressure_limit_permyriad) +$1.ManagedOOMMemoryPressureLimit, config_parse_managed_oom_mem_pressure_limit, 0, offsetof($1, cgroup_context.moom_mem_pressure_limit) $1.ManagedOOMPreference, config_parse_managed_oom_preference, 0, offsetof($1, cgroup_context.moom_preference) $1.NetClass, config_parse_warn_compat, DISABLED_LEGACY, 0' )m4_dnl diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index c56c2ffc61..b9fc450ac7 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3901,7 +3901,8 @@ int config_parse_managed_oom_mem_pressure_limit( return 0; } - *limit = r; + /* Normalize to 2^32-1 == 100% */ + *limit = UINT32_SCALE_FROM_PERMYRIAD(r); return 0; } diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index bdf41807b2..65c0bfac61 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -10,6 +10,7 @@ #include "oomd-manager-bus.h" #include "oomd-manager.h" #include "path-util.h" +#include "percent-util.h" typedef struct ManagedOOMReply { ManagedOOMMode mode; @@ -100,10 +101,15 @@ static int process_managed_oom_reply( limit = m->default_mem_pressure_limit; if (streq(reply.property, "ManagedOOMMemoryPressure")) { - if (reply.limit > 10000) + if (reply.limit > UINT32_MAX) /* out of range */ continue; - else if (reply.limit != 0) { - ret = store_loadavg_fixed_point((unsigned long) reply.limit / 100, (unsigned long) reply.limit % 100, &limit); + if (reply.limit != 0) { + int permyriad = UINT32_SCALE_TO_PERMYRIAD(reply.limit); + + ret = store_loadavg_fixed_point( + (unsigned long) permyriad / 100, + (unsigned long) permyriad % 100, + &limit); if (ret < 0) continue; } diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 27c8eb915f..83130db2fa 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -441,15 +441,12 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons return bus_append_string(m, field, eq); if (STR_IN_SET(field, "ManagedOOMMemoryPressureLimit")) { - char *n; - r = parse_permyriad(eq); if (r < 0) return log_error_errno(r, "Failed to parse %s value: %s", field, eq); - n = strjoina(field, "Permyriad"); - - r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) r); + /* Pass around scaled to 2^32-1 == 100% */ + r = sd_bus_message_append(m, "(sv)", field, "u", UINT32_SCALE_FROM_PERMYRIAD(r)); if (r < 0) return bus_log_create_error(r); From d06e7fb53233bac014d1c3c9899ee291dcda1f71 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 17:56:26 +0100 Subject: [PATCH 10/10] oomd: increase accuracy of SwapUsedLimit= to permyriads too oomd.conf has two parameters with fractionals: SwapUsedLimit= and DefaultMemoryPressureLimit=, but one accepts permyriads, the other only percentages, for no apparent reason. One carries the "Percent" in the name, the other doesn't. Let's clean this up: always accept permyriads, and drop the suffix, given that it is misleading. I figure we should internally try to focus on scaling everything relative to UINT32_MAX, and if that isn't in the cards at least 10000, but never permille nor percent unless there's a really really good reason for it (e.g. interface defined by someone else). --- man/oomd.conf.xml | 40 ++++++++++++++-------------- src/oom/oomd-manager.c | 57 +++++++++++++++++++++------------------- src/oom/oomd-manager.h | 6 ++--- src/oom/oomd-util.c | 6 ++--- src/oom/oomd-util.h | 4 +-- src/oom/oomd.c | 11 +++++--- src/oom/oomd.conf | 2 +- src/oom/test-oomd-util.c | 6 ++--- 8 files changed, 71 insertions(+), 61 deletions(-) diff --git a/man/oomd.conf.xml b/man/oomd.conf.xml index 2a12be8cad..6156c98fbd 100644 --- a/man/oomd.conf.xml +++ b/man/oomd.conf.xml @@ -48,36 +48,38 @@ - SwapUsedLimitPercent= + SwapUsedLimit= - Sets the limit for swap usage on the system before systemd-oomd will - take action. If the percentage of swap used on the system is more than what is defined here, - systemd-oomd will act on eligible descendant cgroups, starting from the ones with the - highest swap usage to the lowest swap usage. Which cgroups are monitored and what - action gets taken depends on what the unit has configured for ManagedOOMSwap=. - Takes a percentage value between 0% and 100%, inclusive. Defaults to 90%. + Sets the limit for swap usage on the system before systemd-oomd + will take action. If the fraction of swap used on the system is more than what is defined here, + systemd-oomd will act on eligible descendant control groups, starting from the + ones with the highest swap usage to the lowest swap usage. Which control groups are monitored and + what action gets taken depends on what the unit has configured for + ManagedOOMSwap=. Takes a value specified in percent (when suffixed with "%"), + permille ("‰") or permyriad ("‱"), between 0% and 100%, inclusive. Defaults to 90%. DefaultMemoryPressureLimit= - Sets the limit for memory pressure on the unit's cgroup before systemd-oomd - will take action. A unit can override this value with ManagedOOMMemoryPressureLimit=. - The memory pressure for this property represents the fraction of time in a 10 second window in which all tasks - in the cgroup were delayed. For each monitored cgroup, if the memory pressure on that cgroup exceeds the - limit set for longer than the duration set by DefaultMemoryPressureDurationSec=, - systemd-oomd will act on eligible descendant cgroups, - starting from the ones with the most reclaim activity to the least reclaim activity. Which cgroups are - monitored and what action gets taken depends on what the unit has configured for - ManagedOOMMemoryPressure=. Takes a percentage value between 0% and 100%, inclusive. - Defaults to 60%. + Sets the limit for memory pressure on the unit's control group before + systemd-oomd will take action. A unit can override this value with + ManagedOOMMemoryPressureLimit=. The memory pressure for this property represents + the fraction of time in a 10 second window in which all tasks in the control group were delayed. For + each monitored control group, if the memory pressure on that control group exceeds the limit set for + longer than the duration set by DefaultMemoryPressureDurationSec=, + systemd-oomd will act on eligible descendant control groups, starting from the + ones with the most reclaim activity to the least reclaim activity. Which control groups are monitored + and what action gets taken depends on what the unit has configured for + ManagedOOMMemoryPressure=. Takes a fraction specified in the same way as + SwapUsedLimit= above. Defaults to 60%. DefaultMemoryPressureDurationSec= - Sets the amount of time a unit's cgroup needs to have exceeded memory pressure limits before - systemd-oomd will take action. Memory pressure limits are defined by + Sets the amount of time a unit's control group needs to have exceeded memory pressure + limits before systemd-oomd will take action. Memory pressure limits are defined by DefaultMemoryPressureLimit= and ManagedOOMMemoryPressureLimit=. Defaults to 30 seconds when this property is unset or set to 0. diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 65c0bfac61..fad1fb0d45 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -16,7 +16,7 @@ typedef struct ManagedOOMReply { ManagedOOMMode mode; char *path; char *property; - unsigned limit; + uint32_t limit; } ManagedOOMReply; static void managed_oom_reply_destroy(ManagedOOMReply *reply) { @@ -53,10 +53,10 @@ static int process_managed_oom_reply( assert(m); static const JsonDispatch dispatch_table[] = { - { "mode", JSON_VARIANT_STRING, managed_oom_mode, offsetof(ManagedOOMReply, mode), JSON_MANDATORY }, - { "path", JSON_VARIANT_STRING, json_dispatch_string, offsetof(ManagedOOMReply, path), JSON_MANDATORY }, - { "property", JSON_VARIANT_STRING, json_dispatch_string, offsetof(ManagedOOMReply, property), JSON_MANDATORY }, - { "limit", JSON_VARIANT_UNSIGNED, json_dispatch_unsigned, offsetof(ManagedOOMReply, limit), 0 }, + { "mode", JSON_VARIANT_STRING, managed_oom_mode, offsetof(ManagedOOMReply, mode), JSON_MANDATORY }, + { "path", JSON_VARIANT_STRING, json_dispatch_string, offsetof(ManagedOOMReply, path), JSON_MANDATORY }, + { "property", JSON_VARIANT_STRING, json_dispatch_string, offsetof(ManagedOOMReply, property), JSON_MANDATORY }, + { "limit", JSON_VARIANT_UNSIGNED, json_dispatch_uint32, offsetof(ManagedOOMReply, limit), 0 }, {}, }; @@ -87,7 +87,8 @@ static int process_managed_oom_reply( if (ret == -ENOMEM) { r = ret; goto finish; - } else if (ret < 0) + } + if (ret < 0) continue; monitor_hm = streq(reply.property, "ManagedOOMSwap") ? @@ -100,19 +101,15 @@ static int process_managed_oom_reply( limit = m->default_mem_pressure_limit; - if (streq(reply.property, "ManagedOOMMemoryPressure")) { - if (reply.limit > UINT32_MAX) /* out of range */ - continue; - if (reply.limit != 0) { - int permyriad = UINT32_SCALE_TO_PERMYRIAD(reply.limit); + if (streq(reply.property, "ManagedOOMMemoryPressure") && reply.limit > 0) { + int permyriad = UINT32_SCALE_TO_PERMYRIAD(reply.limit); - ret = store_loadavg_fixed_point( - (unsigned long) permyriad / 100, - (unsigned long) permyriad % 100, - &limit); - if (ret < 0) - continue; - } + ret = store_loadavg_fixed_point( + (unsigned long) permyriad / 100, + (unsigned long) permyriad % 100, + &limit); + if (ret < 0) + continue; } ret = oomd_insert_cgroup_context(NULL, monitor_hm, empty_to_root(reply.path)); @@ -354,11 +351,11 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo } } - if (oomd_swap_free_below(&m->system_context, (100 - m->swap_used_limit))) { + if (oomd_swap_free_below(&m->system_context, 10000 - m->swap_used_limit_permyriad)) { _cleanup_hashmap_free_ Hashmap *candidates = NULL; - log_notice("Swap used (%"PRIu64") / total (%"PRIu64") is more than %u%%", - m->system_context.swap_used, m->system_context.swap_total, m->swap_used_limit); + log_notice("Swap used (%"PRIu64") / total (%"PRIu64") is more than " PERMYRIAD_AS_PERCENT_FORMAT_STR, + m->system_context.swap_used, m->system_context.swap_total, PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad)); r = get_monitored_cgroup_contexts_candidates(m->monitored_swap_cgroup_contexts, &candidates); if (r == -ENOMEM) @@ -484,7 +481,13 @@ static int manager_connect_bus(Manager *m) { return 0; } -int manager_start(Manager *m, bool dry_run, int swap_used_limit, int mem_pressure_limit_permyriad, usec_t mem_pressure_usec) { +int manager_start( + Manager *m, + bool dry_run, + int swap_used_limit_permyriad, + int mem_pressure_limit_permyriad, + usec_t mem_pressure_usec) { + unsigned long l, f; int r; @@ -492,10 +495,10 @@ int manager_start(Manager *m, bool dry_run, int swap_used_limit, int mem_pressur m->dry_run = dry_run; - m->swap_used_limit = swap_used_limit != -1 ? swap_used_limit : DEFAULT_SWAP_USED_LIMIT; - assert(m->swap_used_limit <= 100); + m->swap_used_limit_permyriad = swap_used_limit_permyriad >= 0 ? swap_used_limit_permyriad : DEFAULT_SWAP_USED_LIMIT_PERCENT * 100; + assert(m->swap_used_limit_permyriad <= 10000); - if (mem_pressure_limit_permyriad != -1) { + if (mem_pressure_limit_permyriad >= 0) { assert(mem_pressure_limit_permyriad <= 10000); l = mem_pressure_limit_permyriad / 100; @@ -543,12 +546,12 @@ int manager_get_dump_string(Manager *m, char **ret) { fprintf(f, "Dry Run: %s\n" - "Swap Used Limit: %u%%\n" + "Swap Used Limit: " PERMYRIAD_AS_PERCENT_FORMAT_STR "\n" "Default Memory Pressure Limit: %lu.%02lu%%\n" "Default Memory Pressure Duration: %s\n" "System Context:\n", yes_no(m->dry_run), - m->swap_used_limit, + PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad), LOAD_INT(m->default_mem_pressure_limit), LOAD_FRAC(m->default_mem_pressure_limit), format_timespan(buf, sizeof(buf), m->default_mem_pressure_duration_usec, USEC_PER_SEC)); oomd_dump_system_context(&m->system_context, f, "\t"); diff --git a/src/oom/oomd-manager.h b/src/oom/oomd-manager.h index 50f10021c7..9ab8494c6d 100644 --- a/src/oom/oomd-manager.h +++ b/src/oom/oomd-manager.h @@ -18,7 +18,7 @@ * system.slice are assumed to be less latency sensitive. */ #define DEFAULT_MEM_PRESSURE_DURATION_USEC (30 * USEC_PER_SEC) #define DEFAULT_MEM_PRESSURE_LIMIT_PERCENT 60 -#define DEFAULT_SWAP_USED_LIMIT 90 +#define DEFAULT_SWAP_USED_LIMIT_PERCENT 90 #define RECLAIM_DURATION_USEC (30 * USEC_PER_SEC) #define POST_ACTION_DELAY_USEC (15 * USEC_PER_SEC) @@ -32,7 +32,7 @@ struct Manager { Hashmap *polkit_registry; bool dry_run; - unsigned swap_used_limit; + int swap_used_limit_permyriad; loadavg_t default_mem_pressure_limit; usec_t default_mem_pressure_duration_usec; @@ -56,7 +56,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); int manager_new(Manager **ret); -int manager_start(Manager *m, bool dry_run, int swap_used_limit, int mem_pressure_limit_permyriad, usec_t mem_pressure_usec); +int manager_start(Manager *m, bool dry_run, int swap_used_limit_permyriad, int mem_pressure_limit_permyriad, usec_t mem_pressure_usec); int manager_get_dump_string(Manager *m, char **ret); diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index 9dd9b17c6d..b054ccacc4 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -134,13 +134,13 @@ bool oomd_memory_reclaim(Hashmap *h) { return pgscan_of > last_pgscan_of; } -bool oomd_swap_free_below(const OomdSystemContext *ctx, uint64_t threshold_percent) { +bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad) { uint64_t swap_threshold; assert(ctx); - assert(threshold_percent <= 100); + assert(threshold_permyriad <= 10000); - swap_threshold = ctx->swap_total * threshold_percent / ((uint64_t) 100); + swap_threshold = ctx->swap_total * threshold_permyriad / (uint64_t) 10000; return (ctx->swap_total - ctx->swap_used) < swap_threshold; } diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index bffccf75da..181443ae7a 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -61,8 +61,8 @@ int oomd_pressure_above(Hashmap *h, usec_t duration, Set **ret); * current sum is higher than the last interval's sum (there was some reclaim activity). */ bool oomd_memory_reclaim(Hashmap *h); -/* Returns true if the amount of swap free is below the percentage of swap specified by `threshold_percent`. */ -bool oomd_swap_free_below(const OomdSystemContext *ctx, uint64_t threshold_percent); +/* Returns true if the amount of swap free is below the permyriad of swap specified by `threshold_permyriad`. */ +bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad); /* The compare functions will sort from largest to smallest, putting all the contexts with "avoid" at the end * (after the smallest values). */ diff --git a/src/oom/oomd.c b/src/oom/oomd.c index 2e331e267f..674d53fdcf 100644 --- a/src/oom/oomd.c +++ b/src/oom/oomd.c @@ -17,13 +17,13 @@ #include "signal-util.h" static bool arg_dry_run = false; -static int arg_swap_used_limit = -1; +static int arg_swap_used_limit_permyriad = -1; static int arg_mem_pressure_limit_permyriad = -1; static usec_t arg_mem_pressure_usec = 0; static int parse_config(void) { static const ConfigTableItem items[] = { - { "OOM", "SwapUsedLimitPercent", config_parse_percent, 0, &arg_swap_used_limit }, + { "OOM", "SwapUsedLimit", config_parse_permyriad, 0, &arg_swap_used_limit_permyriad }, { "OOM", "DefaultMemoryPressureLimit", config_parse_permyriad, 0, &arg_mem_pressure_limit_permyriad }, { "OOM", "DefaultMemoryPressureDurationSec", config_parse_sec, 0, &arg_mem_pressure_usec }, {} @@ -159,7 +159,12 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to create manager: %m"); - r = manager_start(m, arg_dry_run, arg_swap_used_limit, arg_mem_pressure_limit_permyriad, arg_mem_pressure_usec); + r = manager_start( + m, + arg_dry_run, + arg_swap_used_limit_permyriad, + arg_mem_pressure_limit_permyriad, + arg_mem_pressure_usec); if (r < 0) return log_error_errno(r, "Failed to start up daemon: %m"); diff --git a/src/oom/oomd.conf b/src/oom/oomd.conf index bd6a9391c6..35ba847457 100644 --- a/src/oom/oomd.conf +++ b/src/oom/oomd.conf @@ -12,6 +12,6 @@ # See oomd.conf(5) for details [OOM] -#SwapUsedLimitPercent=90% +#SwapUsedLimit=90% #DefaultMemoryPressureLimit=60% #DefaultMemoryPressureDurationSec=30s diff --git a/src/oom/test-oomd-util.c b/src/oom/test-oomd-util.c index 49a02f9424..a0e583ac6b 100644 --- a/src/oom/test-oomd-util.c +++ b/src/oom/test-oomd-util.c @@ -302,19 +302,19 @@ static void test_oomd_swap_free_below(void) { .swap_total = 20971512 * 1024U, .swap_used = 20971440 * 1024U, }; - assert_se(oomd_swap_free_below(&ctx, 20) == true); + assert_se(oomd_swap_free_below(&ctx, 2000) == true); ctx = (OomdSystemContext) { .swap_total = 20971512 * 1024U, .swap_used = 3310136 * 1024U, }; - assert_se(oomd_swap_free_below(&ctx, 20) == false); + assert_se(oomd_swap_free_below(&ctx, 2000) == false); ctx = (OomdSystemContext) { .swap_total = 0, .swap_used = 0, }; - assert_se(oomd_swap_free_below(&ctx, 20) == false); + assert_se(oomd_swap_free_below(&ctx, 2000) == false); } static void test_oomd_sort_cgroups(void) {