[LTP] [PATCH 2/3] lib/tst_pid: Go to parent cgroups for max value

[Teo Couprie Diaz]

Hello,

On 28/02/2023 14:07, Richard Palethorpe wrote:
> Hello,
>
> Teo Couprie Diaz <teo.coupriediaz@arm.com> writes:
>
>> A cgroup resource limitation can be either a number or "max".
> IIRC this is not true on V1, at least not historically. On some tests we
> have to simulate "max" when V1 is detected.
>
> Perhaps for pids.max specifically it is the case though?
You would have more experience than me on that front ! However, from my 
understanding, for `pids.max` specifically it should be correct.
The kernel documentation[0] of the cgroup-v1 pid controller states :

 > To set a cgroup to have no limit, set pids.max to “max”. This is the 
default for all new cgroups [...]

And from my testing I was not able to generate a cgroup *without* 
pids.max: it was always present and set to "max".

However, there is no `pids.max` for the root cgroup at all; it cannot be 
enabled. Thus it could make sense that if the process is in the root 
cgroup you had to simulate "max", because there wouldn't be `pids.max` 
to read.


But as discussed on the first patch I will need to come back to this 
series anyway and rework it to make use of tst_cgroup, and extend _that_ 
if needed, rather than slap some more functionality on top of tst_pid.
If I do so and still need to add a patch similar to this one I will keep 
in mind that this might be only true for the pid controller, for cgroups 
other than the root cgroup.

Thanks again for the review,
Best regards
Téo

[0]: 
https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v1/pids.html#usage

>
>> 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.
>>
>> Clean up uid related code as it is not used anymore.
>>
>> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
>> Co-developed-by: Beata Michalska <beata.michalska@arm.com>
>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
>> ---
>>   lib/tst_pid.c | 96 +++++++++++++++++++++++++++++----------------------
>>   1 file changed, 54 insertions(+), 42 deletions(-)
>>
>> diff --git a/lib/tst_pid.c b/lib/tst_pid.c
>> index 3d0be6dcd..ad1893290 100644
>> --- a/lib/tst_pid.c
>> +++ b/lib/tst_pid.c
>> @@ -44,50 +44,69 @@ pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
>>   	return pid;
>>   }
>>   
>> -/*
>> - * Get the effective session UID - either one invoking current test via sudo
>> - * or the real UID.
>> - */
>> -static unsigned int get_session_uid(void)
>> +static int __read_pids_limit(const char *path, void (*cleanup_fn) (void))
>>   {
>> -	const char *sudo_uid;
>> +	char max_pids_value[100];
>> +	int max_pids;
>>   
>> -	sudo_uid = getenv("SUDO_UID");
>> -	if (sudo_uid) {
>> -		unsigned int real_uid;
>> -		int ret;
>> +	if (access(path, R_OK) != 0) {
>> +		tst_resm(TINFO, "Cannot read session user limits from '%s'", path);
>> +		return -1;
>> +	}
>>   
>> -		ret = sscanf(sudo_uid, "%u", &real_uid);
>> -		if (ret == 1)
>> -			return real_uid;
>> +	SAFE_FILE_SCANF(cleanup_fn, path, "%s", max_pids_value);
>> +	if (strcmp(max_pids_value, "max")) {
>> +		max_pids =  SAFE_STRTOL(max_pids_value, 0, INT_MAX);
>> +		tst_resm(TINFO, "Found limit of processes %d (from %s)",
>> +				max_pids, path);
>> +	} else {
>> +		max_pids = -1;
>>   	}
>>   
>> -	return getuid();
>> +	return max_pids;
>>   }
>>   
>> -static int read_session_pids_limit(const char *path_fmt, int uid,
>> -				   void (*cleanup_fn) (void))
>> +/*
>> + * Take the path to the cgroup mount and to the current cgroup pid controller
>> + * and try to find the PID limit imposed by cgroup.
>> + * Go up the cgroup hierarchy if needed, otherwise use the kernel PID limit.
>> + */
>> +static int read_session_pids_limit(const char *cgroup_mount,
>> +				   const char *cgroup_path, void (*cleanup_fn) (void))
>>   {
>> -	int max_pids, ret;
>> -	char max_pid_value[100];
>> -	char path[PATH_MAX];
>> -
>> -	ret = snprintf(path, sizeof(path), path_fmt, uid);
>> +	int ret, cgroup_depth = 0, max_pids = -1;
>> +	char path[PATH_MAX + 1], file_path[PATH_MAX + 1];
>> +	const char *sub_path = cgroup_path;
>> +
>> +	/* Find the number of groups we can go up. */
>> +	do {
>> +		cgroup_depth += 1;
>> +		sub_path++;
>> +		sub_path = strchr(sub_path, '/');
>> +	} while (sub_path);
>> +
>> +	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;
>> +	for (int i = 0 ; i < cgroup_depth ; i++) {
>> +		/* 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, cleanup_fn);
>> +		if (max_pids >= 0)
>> +			return max_pids;
>> +
>> +		strncat(path, "/..", PATH_MAX);
>>   	}
>>   
>> -	SAFE_FILE_SCANF(cleanup_fn, path, "%s", max_pid_value);
>> -	if (!strcmp(max_pid_value, "max")) {
>> +	if (max_pids < 0) {
>> +		/* Read kernel imposed limits */
>>   		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);
>> +		tst_resm(TINFO, "Using kernel processes limit of %d",
>> +			 max_pids);
>>   	}
>>   
>>   	return max_pids;
>> @@ -98,9 +117,8 @@ static int get_session_pids_limit(void (*cleanup_fn) (void))
>>   	char path[PATH_MAX + 1];
>>   	char cgroup_pids[PATH_MAX + 1];
>>   	char catchall;
>> -	int uid, ret = 0;
>> +	int ret = 0;
>>   
>> -	uid = get_session_uid();
>>   	/* Check for generic cgroup v1 pid.max */
>>   	ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",
>>   						   "%*d:pids:%s\n", cgroup_pids);
>> @@ -121,11 +139,8 @@ static int get_session_pids_limit(void (*cleanup_fn) (void))
>>   							   "%*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 (!ret)
>> +		return read_session_pids_limit(path, cgroup_pids, cleanup_fn);
>>   
>>   	/* Check for generic cgroup v2 pid.max */
>>   	ret = FILE_LINES_SCANF(cleanup_fn, "/proc/self/cgroup",
>> @@ -135,11 +150,8 @@ static int get_session_pids_limit(void (*cleanup_fn) (void))
>>   							   "%*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);
>> -	}
>> +	if (!ret)
>> +		return read_session_pids_limit(path, cgroup_pids, cleanup_fn);
>>   
>>   	return -1;
>>   }
>> -- 
>> 2.25.1
>