[LTP] [PATCH 1/3] lib/tst_pid: Find cgroup pid.max programmatically

[Teo Couprie Diaz]

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
>