On 03/02/2023 10:49, Beata Michalska wrote:
On Thu, Feb 02, 2023 at 05:13:43PM +0000, Teo Couprie Diaz wrote:
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.
THe 'uid' will not be needed anyway, and I think this patch is actually missing the part of getting rid of it in get_session_pids_limit (?)
You are correct, I forgot to remove it in this patch. I noticed it when fixing the warnings you highlighted in the other patch.
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 ?
Yes, that should be enough.
Will do that then, thanks !
{
- 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);}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 ?
That is not a big deal really, was just suggesting potential improvements, although indeed they might be bit pointless.
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
So you did try to avoid too many string manipulating routines for a reason: what was that if it's not for performance ?
Mainly a not-totally warranted avoidance for them in C, as I'm not really comfortable using them in a way that I'm sure is as safe as it could be. Knowing they can easily be the source of safety issues, I tend to avoid them altogether when I can which is _probably_ not the best either !
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 ?
So if I understood that correctly, you would like to create full path once, and then on each iteration append "go-up" as of : "/../" ? I guess that would work.
That is correct, which is basically what the current patch does already.
Alternatively, smth along the lines of:
char *next_level, *guard;
ret = snprintf(path, sizeof(path), "%s%s", cgroup_mount, cgroup_path); guard = path + strlen(cgroup_mount);
while ( next_level = strrchr(path, '/')) { if (next_level <= guard) break; *(path + (next_level - path)) = '\0'; }
There are numbers of ways this can be done. But I agree not much point in optimizing things here too much, though if we could avoid building a path each iteration, that would be great.
Agreed, I wouldn't want to spend too much time optimizing but it would be best if the solution is as clear and logical as possible !
BR B.
Thanks for the comments ! Téo
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);@@ -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);-- 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