On Wed, Feb 01, 2023 at 09:53:24AM +0000, Teo Couprie Diaz wrote:
For a cgroup resource limitation, where a numerical value is expected "max" is a valid limitation.
The actual limits are within a range [0, max], with 'max' being apparently default and is treaded as noop; implying that 'numerical value is expected' might be too strong of a statement. (as per https://docs.kernel.org/admin-guide/cgroup-v2.html)
It means that the cgroup will not be limited _more_ than it already is. This can mean that it will use the kernel limit for the resource, if it exists, or the limit of a parent cgroup.
This patch reworks "read_session_pids_limit" to go up the cgroup hierarchy if it encounters a "max" value rather than a numerical one, using the kernel limit in the event where it doesn't find any. If it doesn't find either "max" or a numerical value, or errors, it will
/or errors/or errors occur ?
return -1.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
lib/tst_pid.c | 73 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 28 deletions(-)
diff --git a/lib/tst_pid.c b/lib/tst_pid.c index ee3b98771..baceb328f 100644 --- a/lib/tst_pid.c +++ b/lib/tst_pid.c @@ -64,29 +64,52 @@ static unsigned int get_session_uid(void) return getuid(); } -static int read_session_pids_limit(const char *path_fmt, int uid,
void (*cleanup_fn) (void))+static int read_session_pids_limit(const char *cgroup_mount,
- const char *cgroup_path, void (*cleanup_fn) (void))
Although the changes introduced in: [PATCH v3 1/3] lib/tst_pid: Find cgroup pid.max programatically do not alter the overall behavior of this function, the do change the nature of the first argument (plain string vs format string) so it would be preferable to either align the read_session_pids_limit in that very patch or mention that in it's commit message.
{
- int max_pids, ret;
- int ret, max_pids = -1; char max_pid_value[100];
- char path[PATH_MAX];
- char path[PATH_MAX + 1], file_path[PATH_MAX + 1];
- struct stat stat;
- ino_t cgroup_root_inode, current_dir_inode;
- ret = snprintf(path, sizeof(path), path_fmt, uid);
- ret = snprintf(path, sizeof(path), "%s%s", cgroup_mount, cgroup_path); if (ret < 0 || (size_t)ret >= sizeof(path)) return -1;
- if (access(path, R_OK) != 0) {
tst_resm(TINFO, "Cannot read session user limits from '%s'", path);return -1;- }
- SAFE_FILE_SCANF(cleanup_fn, path, "%s", max_pid_value);
- if (!strcmp(max_pid_value, "max")) {
SAFE_FILE_SCANF(cleanup_fn, PID_MAX_PATH, "%d", &max_pids);tst_resm(TINFO, "Found limit of processes %d (from %s=max)", max_pids, path);- } else {
max_pids = SAFE_STRTOL(max_pid_value, 0, INT_MAX);tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, path);
- ret = snprintf(file_path, sizeof(file_path), "%s/pids.max", path);
if (ret < 0 || (size_t)ret >= sizeof(path))return -1;- SAFE_STAT(cgroup_mount, &stat);
- cgroup_root_inode = stat.st_ino;
- SAFE_STAT(file_path, &stat);
- current_dir_inode = stat.st_ino;
- while (cgroup_root_inode != current_dir_inode) {
ret = snprintf(file_path, sizeof(file_path), "%s/pids.max", path);if (ret < 0 || (size_t)ret >= sizeof(path))return -1;strncat(path, "/..", PATH_MAX);SAFE_STAT(path, &stat);current_dir_inode = stat.st_ino;if (access(file_path, R_OK) != 0) {tst_resm(TINFO, "Cannot read session user limits from '%s', going up the cgroup hierarchy hoping to find one.",file_path);continue;}SAFE_FILE_SCANF(cleanup_fn, file_path, "%s", max_pid_value);if (strcmp(max_pid_value, "max")) {max_pids = SAFE_STRTOL(max_pid_value, 0, INT_MAX);tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, file_path);return max_pids;} else if (max_pids < 0) {SAFE_FILE_SCANF(cleanup_fn, PID_MAX_PATH, "%d", &max_pids);tst_resm(TINFO, "Using kernel processes limit of %d (from %s=max) unless we find a value while going back up the cgroup hierarchy",max_pids, file_path);}return max_pids;
So this could be probably optimised a bit, smth along the lines of (completely untested! ):
SAFE_STAT(cgroup_mount, &stat); cgroup__root_inode = stat.st_ino;
ret = snprintf(path, sizeof(path), "%s%s", cgroup_mount, cgroup_path); if (ret < 0 ...
do { /* Create a path to read from */ ret = snprintf(file_path, sizeof(file_path), "%s/pids.max", path); if (ret < 0 || (size_t)ret >= sizeof(file_path)) return -1;
max_pids = __read_pids_limit(file_path); if (max_pids >= 0) return max_pids;
strncat(path, "/..", PATH_MAX); SAFE_STAT(path, &stat); current_dir_inode = stat.st_ino; } while (cgroup_root_inode != current_dir_inode);
if (max_pids < 0) { /* Read kernel imposed limits */ SAFE_FILE_SCANF(cleanup_fn, PID_MAX_PATH, "%d", &max_pids); tst_resm(TINFO, "Using kernel processes limit of %d", max_pids); }
return max_pids;
with:
static int __read_pids_limit(const char *path) { char max_pids_value[100]; int max_pids;
if (access(path, R_OK) != 0) { tst_resm(TINFO, "Cannot read session user limits from '%s'", path); return -1; }
SAFE_FILE_SCANF(cleanup_fn, file_path, "%s", max_pid_value); if (strcmp(max_pid_value, "max")) { max_pids = SAFE_STRTOL(max_pid_value, 0, INT_MAX); tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, path); } else { max_pids = -1; }
return max_pids; }
Also, it might be better (performance-wise) to drop the SAFE_STAT calls (which will trigger context switches for the syscall) and keep operating on the srings directly with using strrchr to find the upper-level directory and having the do {} while loop condition being based on the length of both strings ?
--- BR B.
@@ -120,11 +143,8 @@ static int get_session_pids_limit(void (*cleanup_fn) (void)) "%*s %*s %*s %*s %s %*s %*s - %*s cgroup %*[rw],pid%c", &path, &catchall);
- if (!cgroup_ok) {
strncat(path, cgroup_pids, PATH_MAX);strncat(path, "/pids.max", PATH_MAX);return read_session_pids_limit(path, uid, cleanup_fn);- }
- if (!cgroup_ok)
return read_session_pids_limit(path, cgroup_pids, cleanup_fn);/* Check for generic cgroup v2 pid.max */ cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", @@ -134,11 +154,8 @@ static int get_session_pids_limit(void (*cleanup_fn) (void)) "%*s %*s %*s %*s %s %*s %*s - cgroup2 %c", &path, &catchall);
- if (!cgroup_ok) {
strncat(path, cgroup_pids, PATH_MAX);strncat(path, "/pids.max", PATH_MAX);return read_session_pids_limit(path, uid, cleanup_fn);- }
- if (!cgroup_ok)
return read_session_pids_limit(path, cgroup_pids, cleanup_fn);return -1; } -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org