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 aCould 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