Hi Teo,
Thanks for the efforts in solving that cgroup nightmare!
On Thu, Jan 12, 2023 at 10:20:25AM +0000, Teo Couprie Diaz wrote:
In our debian-based distribution, the two files used in lib/tst_pid are not available, but cgroups still imposes a task limit far lesser 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 fixed msgstress04, but it appeared that msgstress03 didn't account for all of its PIDs, as it expects that tasks will terminate quickly and not reach the PID limit, so it still hit it. Change its limit to account for all possible tasks running simultaneously.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
lib/tst_pid.c | 43 +++++++++++++++++++ .../syscalls/ipc/msgstress/msgstress03.c | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/lib/tst_pid.c b/lib/tst_pid.c index 21cadef2a..1fc2f11f1 100644 --- a/lib/tst_pid.c +++ b/lib/tst_pid.c @@ -103,6 +103,49 @@ static int get_session_pids_limit(void (*cleanup_fn) (void)) if (max_pids < 0) max_pids = read_session_pids_limit(CGROUPS_V1_SLICE_FMT, uid, cleanup_fn);
As we are trying to determine the proper path for pid controller below, do we really need the checks above with fixed paths ?
- /* Check for generic cgroup v1 pid.max */
- if (max_pids < 0) {
char path[PATH_MAX];char cgroup_pids[PATH_MAX];char catchall;int cgroup_ok = 0;cgroup_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", "%*d :pids: %s \n", &cgroup_pids);
We could actually drop early here - not much point reading the mountinfo proc entry if we don't have or couldn't find pids cgroup controller.
/** 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* 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.*/cgroup_ok |= FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", "%*s %*s %*s %*s %s %*s shared:16 - cgroup %c", &path, &catchall);
It makes sense as we need to make sure the whole format has been matched. It could be solved by modifying relevant file_lines_scanf function and using "%n" directive but that might be too much of a hassle so we can stick to this approach instead. I'm not entirely sure whether assuming shared mounts is a good idea, but the group identifier should definitely not be fixed and we should not assume '16' here. I think we could skip those with the '*' and appropriate formats. (same applies below). Also I am wondering if we should not consider mount's root, as mount point is supposed to be relative to root , so in some weird cases this will not provide an actual full path. I guess we can ignore that for now, but slapping a comment here would be good.
if (!cgroup_ok) {strncat(path, cgroup_pids, PATH_MAX);strncat(path, "/pids.max", PATH_MAX);max_pids = read_session_pids_limit(path, uid,cleanup_fn);}- }
- /* Check for generic cgroup v2 pid.max */
- if (max_pids < 0) {
char path[PATH_MAX];char cgroup_pids[PATH_MAX];char catchall;int cgroup2_ok = 0;cgroup2_ok = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup", "%*d :: %s \n", &cgroup_pids);cgroup2_ok |= FILE_LINES_SCANF(cleanup_fn, "/proc/self/mountinfo", "%*s %*s %*s %*s %s %*s shared:9 - cgroup2 %c", &path, &catchall);if (!cgroup2_ok) {strncat(path, cgroup_pids, PATH_MAX);strncat(path, "/pids.max", PATH_MAX);max_pids = read_session_pids_limit(path, uid,cleanup_fn);}- }
My initial tought was to combine the two and decide which cgroup version we are dealing with based on parsing the meminfo with:
"%*s %*s %*s %*s %s %*s shared:9 - cgroup%[ 2]", &path, &cgroup_ver
but that might not work entirely with the unified hierarchy, where we would catch v2 but with no pids.max entry. It would also fail with mixed support. Looking also at the v1 support,this needs 'pids' name controller in the format string for sscanf otherwise we will get wrong path (it will match first entry with cgroup mount type, which might not be for pid controller and the final path will be the wrong one). So I guess the simplest approach is to check for each cgroup separately as you did. I would just suggest doing that in one block keeping with single declaration for needed variables.
if (max_pids < 0) return -1; diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c index 3cb70ab18..0c46927b8 100644 --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c @@ -109,7 +109,7 @@ int main(int argc, char **argv) } }
- free_pids = tst_get_free_pids(cleanup);
- free_pids = tst_get_free_pids(cleanup) / 2;
Could we move that to a separate patch as this is dealing with a different issue?
--- BR B.
if (nprocs >= free_pids) { tst_resm(TINFO, "Requested number of processes higher than limit (%d > %d), " -- 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