On 02/02/2023 14:31, Beata Michalska wrote:
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)
Mh, yeah that's fair. Will just say that it can be a number or 'max' then.
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 ?
Would make sense to reword indeed, will do.
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.
Mh... It could make sense to remove the string formatting part in the first patch indeed. That would mean removing the uid part as well and so changing the signature already.
I'm not against doing it in the first patch, but I feel that it would make the changes less clear on their purpose. I agree that it does change the way the function is used, but it doesn't affect the expected results of the function itself. (A format string without format specifiers is still a format string after all). So I would lean more towards mentioning it in the commit message and keep all changes relative to that in the second patch. What do you think ?
{
- 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! ):
Totally agree as I mentioned in my self-comment. Thanks a lot for taking the time to lay that out, splitting in another function and the do while make a lot of sense.
I will make some changes accordingly and do some more testing for the next revision.
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 ?
The repeated syscalls are clearly not the best for performance. However this is just run during set-up of some tests (basically just msgstress0{3,4} at the moment), so it should be mostly ok ?
The reason I did it that way is because it reduced the need for string manipulations. Thinking about it now, I guess there's a simpler way which would be counting the number of slashes in the cgroup sysfs and the cgroup group path, subtracting the two to get the number of sub-groups and just counting the loop iterations.
I'm quite happy with that idea, and it would work without having to make absolute paths each time, thus keeping the move up the directory structure the simple "add /.." it is now. What do you think ?
BR B.
Thanks again for the review, Téo
@@ -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