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

[Richard Palethorpe]

Hello,

Teo Couprie Diaz <teo.coupriediaz@arm.com> writes:

> 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 [...]

OK, thanks

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

I haven't used pids.max, IIRC the memory controller had max added in
V2. The controllers have become more standard over time, but they still
all have their quirks.

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

I think this would be a useful addition to the cgroups API. Most likely
there are a lot of cgroup limits which we would like to detect if
present.

At the moment we probably circumvent a lot of problems by putting tests
in new CGroups which are in the LTP cgroup. We create the LTP group in
root by default, but it can be created by the system in advance with
limits applied.

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


-- 
Thank you,
Richard.