For a cgroup resource limitation, where a numerical value is expected "max" is a valid limitation. 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 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)) { - 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; @@ -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; }

On 01/02/2023 09:53, Teo Couprie Diaz wrote:

For a cgroup resource limitation, where a numerical value is expected "max" is a valid limitation. 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 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))
  

I feel that the name is still accurate, maybe even more so, but I could change it if needed. Would some comment explaining what it does be useful here ?

{

• 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;

Not really happy with the amount of duplicated code here, but it's necessary to handle the case where the pid mount is the root of the cgroup hierarchy, to prevent going up a level. Even though I guess I could make use of a do_while ? Very open to any suggestions on this as I don't like the structure very much.

• 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.",
  

Regarding this and the one long message later, the checkpatch complains that it's too long, but it complains when I split the string literal on multiple lines. What would be the expected fix there ?

• 			 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; @@ -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; }

Hi Teo,

Within the subject: s/programatically/programmatically On Wed, Feb 01, 2023 at 09:53:23AM +0000, Teo Couprie Diaz wrote:

In our debian-based distribution, the two files used in lib/tst_pid.c

Could we switch 'our debian-based' to smth like: 'on some distributions' ?

are not available, but cgroups still imposes a task limit far smaller than the kernel pid_max. If the cgroup sysfs is mounted, we can use it to retrieve the current task limit imposed to the process. Implement the retrieval of this limit.

This can be done by first checking /proc/self/cgroup to get the cgroup the process is in, which will be a path under the cgroup sysfs. To get the path to the cgroup sysfs, check /proc/self/mountinfo. Finally, concatenate those two paths with pid.max to get the full path to the file containing the limit.

This fixes failures for msgstress04.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

Review branch as the changes are quite large: https://git.morello-project.org/Teo-CD/morello-linux-ltp/-/commits/review/te... lib/tst_pid.c | 55 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/lib/tst_pid.c b/lib/tst_pid.c index 21cadef2a..ee3b98771 100644 --- a/lib/tst_pid.c +++ b/lib/tst_pid.c @@ -31,8 +31,6 @@ #include "tst_safe_macros.h" #define PID_MAX_PATH "/proc/sys/kernel/pid_max" -#define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max" -#define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max" /* Leave some available processes for the OS */ #define PIDS_RESERVE 50 @@ -96,18 +94,53 @@ static int read_session_pids_limit(const char *path_fmt, int uid, static int get_session_pids_limit(void (*cleanup_fn) (void)) {

• int max_pids, uid;

• char path[PATH_MAX + 1];
• char cgroup_pids[PATH_MAX + 1];
• char catchall;
• int uid, cgroup_ok = 0;

The empty line should be preserved here.

uid = get_session_uid();

• max_pids = read_session_pids_limit(CGROUPS_V2_SLICE_FMT, uid, cleanup_fn);
• if (max_pids < 0)

• max_pids = read_session_pids_limit(CGROUPS_V1_SLICE_FMT, uid,
  

• 				   cleanup_fn);
  

• if (max_pids < 0)

• return -1;
  

• /* Check for generic cgroup v1 pid.max */
• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",

• 						 "%*d :pids: %s\n", &cgroup_pids);
  

cgroup_ok and it's use across the function seems bit counterintuitive (at least for me): the name suggest that expected value should be non-zero (true), whereas thie condition below relates succesful parsing the file with !cgroup_ok, so it gest bit misleading. Maybe renaming it to simply result/ret which does not imply (by it's name) what's the expected value ?

The format string does seem to contain an extra whitespace before ":pids:". sscanf does seem to not care about that too much and it does produce the valid result in the end. Assuming I got things right, one whitespace in a format string is supposed to match any combination of those within the input string, where any means none included. So it seems to be fine to have that one I guess.

Also cgroup_pids does not need the '&' (address-of) operator. I'd actually would expect a warning there complaining about type mismatch ... (same applies below)

• /*

• * This is a bit of hack of scanf format strings. It only works forward
  

• * which means that if a variable has been matched, but not a later
  

• * part of the format it still counts as a match. Thus, we need a
  

Could we potentially rephrase that a bit saying that the search is being terminated once all convertion specifications are matched, disregarding outstanding literal characters ? The 'only works forward' statement is slightly inaccurate in this case.

• * catch-all match later in the string that will _not_ be counted if
  

• * the static part of the format doesn't match.
  

• * This way, the macro will go on the next line rather than returning
  

• * on a line matching the %s but not the static part later.
  

• *
  

• * Assume that the root of the mount is /. It can technically be anything,
  

• * but it should be / on any normal system.
  

Minor: Could we put quotes around '/' ?

--- BR B.

• */
  

• if (!cgroup_ok)

• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo",
  

• 							 "%*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);
  

• }

• return max_pids;

• /* Check for generic cgroup v2 pid.max */
• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",

• 						 "%*d :: %s\n", &cgroup_pids);
  

• if (!cgroup_ok)

• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo",
  

• 							 "%*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);
  

• }
• return -1;

} int tst_get_free_pids_(void (*cleanup_fn) (void)) -- 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

On 02/02/2023 14:31, Beata Michalska wrote:

Hi Teo,

Within the subject: s/programatically/programmatically

Thanks for spotting that !

On Wed, Feb 01, 2023 at 09:53:23AM +0000, Teo Couprie Diaz wrote:

...

In our debian-based distribution, the two files used in lib/tst_pid.c

Could we switch 'our debian-based' to smth like: 'on some distributions' ?

That makes sense. Maybe "on some minimalist distributions" ? Not necessarily needed.

...

are not available, but cgroups still imposes a task limit far smaller than the kernel pid_max. If the cgroup sysfs is mounted, we can use it to retrieve the current task limit imposed to the process. Implement the retrieval of this limit.

This can be done by first checking /proc/self/cgroup to get the cgroup the process is in, which will be a path under the cgroup sysfs. To get the path to the cgroup sysfs, check /proc/self/mountinfo. Finally, concatenate those two paths with pid.max to get the full path to the file containing the limit.

This fixes failures for msgstress04.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

Review branch as the changes are quite large: https://git.morello-project.org/Teo-CD/morello-linux-ltp/-/commits/review/te... lib/tst_pid.c | 55 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/lib/tst_pid.c b/lib/tst_pid.c index 21cadef2a..ee3b98771 100644 --- a/lib/tst_pid.c +++ b/lib/tst_pid.c @@ -31,8 +31,6 @@ #include "tst_safe_macros.h" #define PID_MAX_PATH "/proc/sys/kernel/pid_max" -#define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max" -#define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max" /* Leave some available processes for the OS */ #define PIDS_RESERVE 50 @@ -96,18 +94,53 @@ static int read_session_pids_limit(const char *path_fmt, int uid, static int get_session_pids_limit(void (*cleanup_fn) (void)) {

• int max_pids, uid;

• char path[PATH_MAX + 1];
• char cgroup_pids[PATH_MAX + 1];
• char catchall;
• int uid, cgroup_ok = 0;

The empty line should be preserved here.

...

uid = get_session_uid();

• max_pids = read_session_pids_limit(CGROUPS_V2_SLICE_FMT, uid, cleanup_fn);
• if (max_pids < 0)

• max_pids = read_session_pids_limit(CGROUPS_V1_SLICE_FMT, uid,
  

• 				   cleanup_fn);
  

• if (max_pids < 0)

• return -1;
  

• /* Check for generic cgroup v1 pid.max */
• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",

• 						 "%*d :pids: %s\n", &cgroup_pids);
  

cgroup_ok and it's use across the function seems bit counterintuitive (at least for me): the name suggest that expected value should be non-zero (true), whereas thie condition below relates succesful parsing the file with !cgroup_ok, so it gest bit misleading. Maybe renaming it to simply result/ret which does not imply (by it's name) what's the expected value ?

That's fair, I wasn't too happy with it either as it's used to "ok" after failures as well. Just "ret" feels better. I thought of putting some of the function calls in the if conditions directly, but that would be worse to read I think.

The format string does seem to contain an extra whitespace before ":pids:". sscanf does seem to not care about that too much and it does produce the valid result in the end. Assuming I got things right, one whitespace in a format string is supposed to match any combination of those within the input string, where any means none included. So it seems to be fine to have that one I guess.

That was my understanding of the white spaces in the scanf format strings, I added them here for readability more than anything else. Happy to remove them if you feel the goal of the format would be clearer without.

Also cgroup_pids does not need the '&' (address-of) operator. I'd actually would expect a warning there complaining about type mismatch ... (same applies below)

Ah you're right indeed, I missed that somehow. Fixed for path as well.

...

• /*

• * This is a bit of hack of scanf format strings. It only works forward
  

• * which means that if a variable has been matched, but not a later
  

• * part of the format it still counts as a match. Thus, we need a
  

Could we potentially rephrase that a bit saying that the search is being terminated once all convertion specifications are matched, disregarding outstanding literal characters ? The 'only works forward' statement is slightly inaccurate in this case.

Mh, we might be thinking about two different things here, but that does mean the comment could be better

I'll be honest I wrote that a while ago and I'm not sure the "works only forwards" is clear to me either now !

What do you think of something along those lines ?

"This is a bit of a hack of scanf format strings. Indeed, if all conversion specifications have been matched the return of scanf will be the same whether any outstanding literal characters match or not. As we want to match the literal part, we can add a catchall after it so that it won't be counted if the literal part doesn't match. This makes the macro go to the next line until the catchall, thus the literal parts, is matched."

...

• * catch-all match later in the string that will _not_ be counted if
  

• * the static part of the format doesn't match.
  

• * This way, the macro will go on the next line rather than returning
  

• * on a line matching the %s but not the static part later.
  

• *
  

• * Assume that the root of the mount is /. It can technically be anything,
  

• * but it should be / on any normal system.
  

Minor: Could we put quotes around '/' ?

Sure !

---

BR B.

Thanks for the review, Téo

...

• */
  

• if (!cgroup_ok)

• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo",
  

• 							 "%*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);
  

• }

• return max_pids;

• /* Check for generic cgroup v2 pid.max */
• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",

• 						 "%*d :: %s\n", &cgroup_pids);
  

• if (!cgroup_ok)

• cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo",
  

• 							 "%*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);
  

• }
• return -1; }

int tst_get_free_pids_(void (*cleanup_fn) (void)) -- 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

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

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);
  

• }
  

  } 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 ?

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);
  

  /* 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