| Message ID | 20230215145440.78482-2-teo.coupriediaz@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | lib/tst_pid: Find session PID limit more reliably | expand |
Hello, Teo Couprie Diaz <teo.coupriediaz@arm.com> writes: > In some distributions, the two files used in lib/tst_pid.c 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 patch changes the way read_session_pids_limit is called, not passing > a format string to be completed anymore, but is still used the same way. > A following patch will update this function. > > This fixes failures for msgstress04. > > Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> > --- > lib/tst_pid.c | 53 +++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 43 insertions(+), 10 deletions(-) > > diff --git a/lib/tst_pid.c b/lib/tst_pid.c > index 5595e79bd..3d0be6dcd 100644 > --- a/lib/tst_pid.c > +++ b/lib/tst_pid.c > @@ -32,8 +32,6 @@ > > #define PID_MAX_PATH "/proc/sys/kernel/pid_max" > #define THREADS_MAX_PATH "/proc/sys/kernel/threads-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 > > @@ -97,18 +95,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, ret = 0; > > 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); > + /* Check for generic cgroup v1 pid.max */ > + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", > + "%*d:pids:%s\n", cgroup_pids); > + /* > + * 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. > + * > + * Assume that the root of the mount is '/'. It can be anything, > + * but it should be '/' on any normal system. > + */ > + if (!ret) > + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", > + "%*s %*s %*s %*s %s %*[^-] - cgroup %*s %*[rw],pid%c", > + path, > &catchall); Uhhff, I already implemented this logic in tst_cg_scan in tst_cgroup. In there we scan the current CGroup hierarchy and build a data structure which represents it. I guess you are not aware of tst_cgroup? > + > + if (!ret) { > + strncat(path, cgroup_pids, PATH_MAX); > + strncat(path, "/pids.max", PATH_MAX); > + return read_session_pids_limit(path, uid, cleanup_fn); > + } > > - if (max_pids < 0) > - return -1; > + /* Check for generic cgroup v2 pid.max */ > + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", > + "%*d::%s\n", > cgroup_pids); This has not been added to tst_cgroup because usually tests that need a cgroup feature are moved to a cgroup created by LTP. We also check that any required CGroups are available and mount them if necessary. I suppose in this case we do not care if there is no CGroup hierarchy or the pids controller is absent? In any case I think tst_cgroup should be used or extended. > + if (!ret) > + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", > + "%*s %*s %*s %*s %s %*[^-] - cgroup2 %c", > + path, &catchall); > + > + if (!ret) { > + strncat(path, cgroup_pids, PATH_MAX); > + strncat(path, "/pids.max", PATH_MAX); > + return read_session_pids_limit(path, uid, cleanup_fn); > + } > > - return max_pids; > + return -1; > } > > int tst_get_free_pids_(void (*cleanup_fn) (void)) > -- > 2.25.1
Hello Richard, Thanks for taking the time to have a look ! On 28/02/2023 13:50, Richard Palethorpe wrote: > Hello, > > Teo Couprie Diaz <teo.coupriediaz@arm.com> writes: > >> In some distributions, the two files used in lib/tst_pid.c 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 patch changes the way read_session_pids_limit is called, not passing >> a format string to be completed anymore, but is still used the same way. >> A following patch will update this function. >> >> This fixes failures for msgstress04. >> >> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> >> --- >> lib/tst_pid.c | 53 +++++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 43 insertions(+), 10 deletions(-) >> >> diff --git a/lib/tst_pid.c b/lib/tst_pid.c >> index 5595e79bd..3d0be6dcd 100644 >> --- a/lib/tst_pid.c >> +++ b/lib/tst_pid.c >> @@ -32,8 +32,6 @@ >> >> #define PID_MAX_PATH "/proc/sys/kernel/pid_max" >> #define THREADS_MAX_PATH "/proc/sys/kernel/threads-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 >> >> @@ -97,18 +95,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, ret = 0; >> >> 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); >> + /* Check for generic cgroup v1 pid.max */ >> + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", >> + "%*d:pids:%s\n", cgroup_pids); >> + /* >> + * 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. >> + * >> + * Assume that the root of the mount is '/'. It can be anything, >> + * but it should be '/' on any normal system. >> + */ >> + if (!ret) >> + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", >> + "%*s %*s %*s %*s %s %*[^-] - cgroup %*s %*[rw],pid%c", >> + path, >> &catchall); > Uhhff, I already implemented this logic in tst_cg_scan in tst_cgroup. In > there we scan the current CGroup hierarchy and build a data structure > which represents it. > > I guess you are not aware of tst_cgroup? Indeed, it appears that I either missed it or misunderstood it while researching. That's on me, it is indeed already covered there. > >> + >> + if (!ret) { >> + strncat(path, cgroup_pids, PATH_MAX); >> + strncat(path, "/pids.max", PATH_MAX); >> + return read_session_pids_limit(path, uid, cleanup_fn); >> + } >> >> - if (max_pids < 0) >> - return -1; >> + /* Check for generic cgroup v2 pid.max */ >> + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", >> + "%*d::%s\n", >> cgroup_pids); > This has not been added to tst_cgroup because usually tests that need a > cgroup feature are moved to a cgroup created by LTP. We also check that > any required CGroups are available and mount them if necessary. Interesting, that does make sense when testing cgroups and would make it much easier. As I mentioned in the cover letter this patch series was motivated by msgstress03 and msgstress04 which do not set up a cgroup, they just try to read their current limit. This is why I went to the trouble of looking for the cgroup directly. > > I suppose in this case we do not care if there is no CGroup hierarchy or > the pids controller is absent? That is the case indeed : if there's no cgroup hierarchy or pid controller, it would use the kernel PID limit directly instead. This cannot be done by default because if there is indeed a cgroup hierarchy and pid controller, the forks will probably fail far before the kernel pid limit is hit. > > In any case I think tst_cgroup should be used or extended. I agree, hopefully I can find some time to rework this series to make use of tst_cgroup as much as possible. My apologies for the noise and thanks again for the review. Best regards, Téo > >> + if (!ret) >> + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", >> + "%*s %*s %*s %*s %s %*[^-] - cgroup2 %c", >> + path, &catchall); >> + >> + if (!ret) { >> + strncat(path, cgroup_pids, PATH_MAX); >> + strncat(path, "/pids.max", PATH_MAX); >> + return read_session_pids_limit(path, uid, cleanup_fn); >> + } >> >> - return max_pids; >> + return -1; >> } >> >> int tst_get_free_pids_(void (*cleanup_fn) (void)) >> -- >> 2.25.1 >
diff --git a/lib/tst_pid.c b/lib/tst_pid.c index 5595e79bd..3d0be6dcd 100644 --- a/lib/tst_pid.c +++ b/lib/tst_pid.c @@ -32,8 +32,6 @@ #define PID_MAX_PATH "/proc/sys/kernel/pid_max" #define THREADS_MAX_PATH "/proc/sys/kernel/threads-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 @@ -97,18 +95,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, ret = 0; 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); + /* Check for generic cgroup v1 pid.max */ + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", + "%*d:pids:%s\n", cgroup_pids); + /* + * 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. + * + * Assume that the root of the mount is '/'. It can be anything, + * but it should be '/' on any normal system. + */ + if (!ret) + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", + "%*s %*s %*s %*s %s %*[^-] - cgroup %*s %*[rw],pid%c", + path, &catchall); + + if (!ret) { + strncat(path, cgroup_pids, PATH_MAX); + strncat(path, "/pids.max", PATH_MAX); + return read_session_pids_limit(path, uid, cleanup_fn); + } - if (max_pids < 0) - return -1; + /* Check for generic cgroup v2 pid.max */ + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", + "%*d::%s\n", cgroup_pids); + if (!ret) + ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", + "%*s %*s %*s %*s %s %*[^-] - cgroup2 %c", + path, &catchall); + + if (!ret) { + strncat(path, cgroup_pids, PATH_MAX); + strncat(path, "/pids.max", PATH_MAX); + return read_session_pids_limit(path, uid, cleanup_fn); + } - return max_pids; + return -1; } int tst_get_free_pids_(void (*cleanup_fn) (void))
In some distributions, the two files used in lib/tst_pid.c 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 patch changes the way read_session_pids_limit is called, not passing a format string to be completed anymore, but is still used the same way. A following patch will update this function. This fixes failures for msgstress04. Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> --- lib/tst_pid.c | 53 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 10 deletions(-)