Merge pull request #21217 from keszybz/debug-test-process-util

procfs-util: fix confusion wrt. quantity limit and maximum value

5 files changed, 84 insertions, 67 deletions

diff --git a/src/basic/limits-util.c b/src/basic/limits-util.c
index 78e7cb39ee..b4444443aa 100644
--- a/src/basic/limits-util.c
+++ b/src/basic/limits-util.c
@@ -111,35 +111,57 @@ 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) 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
+         * a) kernel.threads-max sysctl: the maximum number of tasks (threads) the kernel allows.
          *
-         * And then pick the smallest of the three */
+         *    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.
+         *
+         *    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.
+         *
+         * 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)
                 log_debug_errno(r, "Failed to determine cgroup root path, ignoring: %m");
         else {
-                r = cg_get_attribute_as_uint64("pids", root, "pids.max", &b);
+                r = cg_get_attribute_as_uint64("pids", root, "pids.max", &c);
                 if (r < 0)
-                        log_debug_errno(r, "Failed to read pids.max attribute of cgroup root, ignoring: %m");
+                        log_debug_errno(r, "Failed to read pids.max attribute of root cgroup, 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/basic/procfs-util.c b/src/basic/procfs-util.c
index 688c0004ea..65f96abb06 100644
--- a/src/basic/procfs-util.c
+++ b/src/basic/procfs-util.c
@@ -13,54 +13,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;
 
@@ -75,10 +55,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;
 
@@ -99,14 +76,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 61fa71d479..eb8c7738b1 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/test/test-process-util.c b/src/test/test-process-util.c
index bee39d567b..b87a24b342 100644
--- a/src/test/test-process-util.c
+++ b/src/test/test-process-util.c
@@ -851,8 +851,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 a961436b40..b5016ecd2d 100644
--- a/src/test/test-procfs-util.c
+++ b/src/test/test-procfs-util.c
@@ -6,11 +6,12 @@
 #include "format-util.h"
 #include "log.h"
 #include "procfs-util.h"
+#include "process-util.h"
 #include "tests.h"
 
 int main(int argc, char *argv[]) {
         nsec_t nsec;
-        uint64_t v;
+        uint64_t v, w;
         int r;
 
         log_parse_environment();
@@ -25,26 +26,39 @@ int main(int argc, char *argv[]) {
         assert_se(procfs_tasks_get_current(&v) >= 0);
         log_info("Current number of tasks: %" PRIu64, v);
 
-        r = procfs_tasks_get_limit(&v);
-        if (r == -ENOENT || ERRNO_IS_PRIVILEGE(r))
-                return log_tests_skipped("can't read /proc/sys/kernel/pid_max");
+        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))
+                return log_tests_skipped("can't set task limits");
+        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) || r == -EROFS);
 
-                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);
         }