From 62678ec1aa02b53cb116b6f7dd72a54bf61153b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Nov 2021 18:18:21 +0100 Subject: [PATCH] procfs-util: fix confusion wrt. quantity limit and maximum value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From packit/rawhide-arm64 logs: Assertion 'limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH' failed at src/test/test-process-util.c:855, function test_get_process_ppid(). Aborting. ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― The kernel has a few different limits. In particular kernel.threads-max can be set to some lower value, and kernel.pid_max can be set to a higher value. This is nice because it reduces PID reuse, even if the number of threads that is allowed is limited. But the tests assumed that we cannot have a thread with PID above MIN(kernel.threads-max, kernel.pid_max-1), which is not valid. So let's rework the whole thing: let's expose the helpers to read kernel.threads-max and kernel.pid_max, and print what they return in tests. procfs_tasks_get_limit() was something that is only used in tests, and wasn't very well defined, so let's drop it. Fixes #21193. (cherry picked from commit c3dead53d50e334f2d072a2248256983d6dc9f8c) Related: #1977569 --- src/basic/procfs-util.c | 53 +++++++++--------------------------- src/basic/procfs-util.h | 4 ++- src/basic/util.c | 49 +++++++++++++++++++++++---------- src/test/test-process-util.c | 10 +++++-- src/test/test-procfs-util.c | 37 +++++++++++++++++++------ 5 files changed, 88 insertions(+), 65 deletions(-) diff --git a/src/basic/procfs-util.c b/src/basic/procfs-util.c index 7aaf95bfce..fa5671dd72 100644 --- a/src/basic/procfs-util.c +++ b/src/basic/procfs-util.c @@ -12,54 +12,34 @@ #include "stdio-util.h" #include "string-util.h" -int procfs_tasks_get_limit(uint64_t *ret) { +int procfs_get_pid_max(uint64_t *ret) { _cleanup_free_ char *value = NULL; - uint64_t pid_max, threads_max; int r; assert(ret); - /* So there are two sysctl files that control the system limit of processes: - * - * 1. kernel.threads-max: this is probably the sysctl that makes more sense, as it directly puts a limit on - * concurrent tasks. - * - * 2. kernel.pid_max: this limits the numeric range PIDs can take, and thus indirectly also limits the number - * of concurrent threads. AFAICS it's primarily a compatibility concept: some crappy old code used a signed - * 16bit type for PIDs, hence the kernel provides a way to ensure the PIDs never go beyond INT16_MAX by - * default. - * - * By default #2 is set to much lower values than #1, hence the limit people come into contact with first, as - * it's the lowest boundary they need to bump when they want higher number of processes. - * - * Also note the weird definition of #2: PIDs assigned will be kept below this value, which means the number of - * tasks that can be created is one lower, as PID 0 is not a valid process ID. */ - r = read_one_line_file("/proc/sys/kernel/pid_max", &value); if (r < 0) return r; - r = safe_atou64(value, &pid_max); - if (r < 0) - return r; + return safe_atou64(value, ret); +} - value = mfree(value); - r = read_one_line_file("/proc/sys/kernel/threads-max", &value); - if (r < 0) - return r; +int procfs_get_threads_max(uint64_t *ret) { + _cleanup_free_ char *value = NULL; + int r; - r = safe_atou64(value, &threads_max); + assert(ret); + + r = read_one_line_file("/proc/sys/kernel/threads-max", &value); if (r < 0) return r; - /* Subtract one from pid_max, since PID 0 is not a valid PID */ - *ret = MIN(pid_max-1, threads_max); - return 0; + return safe_atou64(value, ret); } int procfs_tasks_set_limit(uint64_t limit) { char buffer[DECIMAL_STR_MAX(uint64_t)+1]; - _cleanup_free_ char *value = NULL; uint64_t pid_max; int r; @@ -74,10 +54,7 @@ int procfs_tasks_set_limit(uint64_t limit) { * set it to the maximum. */ limit = CLAMP(limit, 20U, TASKS_MAX); - r = read_one_line_file("/proc/sys/kernel/pid_max", &value); - if (r < 0) - return r; - r = safe_atou64(value, &pid_max); + r = procfs_get_pid_max(&pid_max); if (r < 0) return r; @@ -98,14 +75,10 @@ int procfs_tasks_set_limit(uint64_t limit) { /* Hmm, we couldn't write this? If so, maybe it was already set properly? In that case let's not * generate an error */ - value = mfree(value); - if (read_one_line_file("/proc/sys/kernel/threads-max", &value) < 0) - return r; /* return original error */ - - if (safe_atou64(value, &threads_max) < 0) + if (procfs_get_threads_max(&threads_max) < 0) return r; /* return original error */ - if (MIN(pid_max-1, threads_max) != limit) + if (MIN(pid_max - 1, threads_max) != limit) return r; /* return original error */ /* Yay! Value set already matches what we were trying to set, hence consider this a success. */ diff --git a/src/basic/procfs-util.h b/src/basic/procfs-util.h index 5a44e9eff7..caaee8b0b6 100644 --- a/src/basic/procfs-util.h +++ b/src/basic/procfs-util.h @@ -5,7 +5,9 @@ #include "time-util.h" -int procfs_tasks_get_limit(uint64_t *ret); +int procfs_get_pid_max(uint64_t *ret); +int procfs_get_threads_max(uint64_t *ret); + int procfs_tasks_set_limit(uint64_t limit); int procfs_tasks_get_current(uint64_t *ret); diff --git a/src/basic/util.c b/src/basic/util.c index 609f8c2f33..548e3652cc 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -527,23 +527,46 @@ uint64_t physical_memory_scale(uint64_t v, uint64_t max) { } uint64_t system_tasks_max(void) { - - uint64_t a = TASKS_MAX, b = TASKS_MAX; + uint64_t a = TASKS_MAX, b = TASKS_MAX, c = TASKS_MAX; _cleanup_free_ char *root = NULL; int r; - /* Determine the maximum number of tasks that may run on this system. We check three sources to determine this - * limit: + /* Determine the maximum number of tasks that may run on this system. We check three sources to + * determine this limit: + * + * a) kernel.threads-max sysctl: the maximum number of tasks (threads) the kernel allows. + * + * This puts a direct limit on the number of concurrent tasks. + * + * b) kernel.pid_max sysctl: the maximum PID value. + * + * This limits the numeric range PIDs can take, and thus indirectly also limits the number of + * concurrent threads. It's primarily a compatibility concept: some crappy old code used a signed + * 16bit type for PIDs, hence the kernel provides a way to ensure the PIDs never go beyond + * INT16_MAX by default. * - * a) the maximum tasks value the kernel allows on this architecture - * b) the cgroups pids_max attribute for the system - * c) the kernel's configured maximum PID value + * Also note the weird definition: PIDs assigned will be kept below this value, which means + * the number of tasks that can be created is one lower, as PID 0 is not a valid process ID. * - * And then pick the smallest of the three */ + * c) pids.max on the root cgroup: the kernel's configured maximum number of tasks. + * + * and then pick the smallest of the three. + * + * By default pid_max is set to much lower values than threads-max, hence the limit people come into + * contact with first, as it's the lowest boundary they need to bump when they want higher number of + * processes. + */ + + r = procfs_get_threads_max(&a); + if (r < 0) + log_debug_errno(r, "Failed to read kernel.threads-max, ignoring: %m"); - r = procfs_tasks_get_limit(&a); + r = procfs_get_pid_max(&b); if (r < 0) - log_debug_errno(r, "Failed to read maximum number of tasks from /proc, ignoring: %m"); + log_debug_errno(r, "Failed to read kernel.pid_max, ignoring: %m"); + else if (b > 0) + /* Subtract one from pid_max, since PID 0 is not a valid PID */ + b--; r = cg_get_root_path(&root); if (r < 0) @@ -555,15 +578,13 @@ uint64_t system_tasks_max(void) { if (r < 0) log_debug_errno(r, "Failed to read pids.max attribute of cgroup root, ignoring: %m"); else if (!streq(value, "max")) { - r = safe_atou64(value, &b); + r = safe_atou64(value, &c); if (r < 0) log_debug_errno(r, "Failed to parse pids.max attribute of cgroup root, ignoring: %m"); } } - return MIN3(TASKS_MAX, - a <= 0 ? TASKS_MAX : a, - b <= 0 ? TASKS_MAX : b); + return MIN3(a, b, c); } uint64_t system_tasks_max_scale(uint64_t v, uint64_t max) { diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 6b14ff592b..6295889b47 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -598,8 +598,14 @@ static void test_get_process_ppid(void) { assert_se(get_process_ppid(1, NULL) == -EADDRNOTAVAIL); /* the process with the PID above the global limit definitely doesn't exist. Verify that */ - assert_se(procfs_tasks_get_limit(&limit) >= 0); - assert_se(limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH); + assert_se(procfs_get_pid_max(&limit) >= 0); + log_debug("kernel.pid_max = %"PRIu64, limit); + + if (limit < INT_MAX) { + r = get_process_ppid(limit + 1, NULL); + log_debug_errno(r, "get_process_limit(%"PRIu64") → %d/%m", limit + 1, r); + assert(r == -ESRCH); + } for (pid_t pid = 0;;) { _cleanup_free_ char *c1 = NULL, *c2 = NULL; diff --git a/src/test/test-procfs-util.c b/src/test/test-procfs-util.c index 1d0612985b..bb6943fed0 100644 --- a/src/test/test-procfs-util.c +++ b/src/test/test-procfs-util.c @@ -5,11 +5,13 @@ #include "log.h" #include "parse-util.h" #include "procfs-util.h" +#include "process-util.h" +#include "util.h" int main(int argc, char *argv[]) { char buf[CONST_MAX(FORMAT_TIMESPAN_MAX, FORMAT_BYTES_MAX)]; nsec_t nsec; - uint64_t v; + uint64_t v, w; int r; log_parse_environment(); @@ -24,22 +26,41 @@ int main(int argc, char *argv[]) { assert_se(procfs_tasks_get_current(&v) >= 0); log_info("Current number of tasks: %" PRIu64, v); - assert_se(procfs_tasks_get_limit(&v) >= 0); + v = TASKS_MAX; + r = procfs_get_pid_max(&v); + assert(r >= 0 || r == -ENOENT || ERRNO_IS_PRIVILEGE(r)); + log_info("kernel.pid_max: %"PRIu64, v); + + w = TASKS_MAX; + r = procfs_get_threads_max(&w); + assert(r >= 0 || r == -ENOENT || ERRNO_IS_PRIVILEGE(r)); + log_info("kernel.threads-max: %"PRIu64, w); + + v = MIN(v - (v > 0), w); + + assert_se(r >= 0); log_info("Limit of tasks: %" PRIu64, v); assert_se(v > 0); - assert_se(procfs_tasks_set_limit(v) >= 0); + r = procfs_tasks_set_limit(v); + if (r == -ENOENT || ERRNO_IS_PRIVILEGE(r)) { + log_notice_errno(r, "Skipping test: can't set task limits"); + return EXIT_TEST_SKIP; + } + assert(r >= 0); if (v > 100) { - uint64_t w; + log_info("Reducing limit by one to %"PRIu64"…", v-1); + r = procfs_tasks_set_limit(v-1); - assert_se(IN_SET(r, 0, -EPERM, -EACCES, -EROFS)); + log_info_errno(r, "procfs_tasks_set_limit: %m"); + assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r)); - assert_se(procfs_tasks_get_limit(&w) >= 0); - assert_se((r == 0 && w == v - 1) || (r < 0 && w == v)); + assert_se(procfs_get_threads_max(&w) >= 0); + assert_se(r >= 0 ? w == v - 1 : w == v); assert_se(procfs_tasks_set_limit(v) >= 0); - assert_se(procfs_tasks_get_limit(&w) >= 0); + assert_se(procfs_get_threads_max(&w) >= 0); assert_se(v == w); }