[LTP] [PATCH v5 1/3] lib: add tst_get_free_pids helper program

[LTP] [PATCH v5 1/3] lib: add tst_get_free_pids helper program

[Krzysztof Kozlowski]

Add a tst_get_free_pids helper program so shell code can get limit of
tasks/PIDs in the system without duplicating that code.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

---

Changes since v4:
1. Add SPDX

Changes since v3:
1. New patch
---
 testcases/lib/.gitignore          |  1 +
 testcases/lib/Makefile            |  2 +-
 testcases/lib/tst_get_free_pids.c | 20 ++++++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 testcases/lib/tst_get_free_pids.c

diff --git a/testcases/lib/.gitignore b/testcases/lib/.gitignore
index a2e9f1ef0c08..5a0e8cba2ee7 100644
--- a/testcases/lib/.gitignore
+++ b/testcases/lib/.gitignore
@@ -2,6 +2,7 @@
 /tst_checkpoint
 /tst_device
 /tst_getconf
+/tst_get_free_pids
 /tst_get_median
 /tst_get_unused_port
 /tst_kvcmp
diff --git a/testcases/lib/Makefile b/testcases/lib/Makefile
index 38813e640ab1..179b474795d3 100644
--- a/testcases/lib/Makefile
+++ b/testcases/lib/Makefile
@@ -11,6 +11,6 @@ INSTALL_TARGETS		:= *.sh
 MAKE_TARGETS		:= tst_sleep tst_random tst_checkpoint tst_rod tst_kvcmp\
 			   tst_device tst_net_iface_prefix tst_net_ip_prefix tst_net_vars\
 			   tst_getconf tst_supported_fs tst_check_drivers tst_get_unused_port\
-			   tst_get_median tst_hexdump
+			   tst_get_median tst_hexdump tst_get_free_pids
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/lib/tst_get_free_pids.c b/testcases/lib/tst_get_free_pids.c
new file mode 100644
index 000000000000..d7b68c620614
--- /dev/null
+++ b/testcases/lib/tst_get_free_pids.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#define TST_NO_DEFAULT_MAIN
+#include <stdio.h>
+#include <tst_test.h>
+
+extern struct tst_test *tst_test;
+
+static struct tst_test test = {
+};
+
+int main(void)
+{
+	/* force messages to be printed from new library */
+	tst_test = &test;
+
+	printf("%i\n", tst_get_free_pids());
+
+	return 0;
+}
-- 
2.30.2

[LTP] [PATCH v5 2/3] controllers/cpuacct: skip cpuacct_100_100 on small memory systems

[Krzysztof Kozlowski]

Running cpuacct_100_100 on a system with less than ~4 GB of RAM fails:

    cpuacct 1 TINFO: timeout per run is 0h 5m 0s
    cpuacct 1 TINFO: cpuacct: /sys/fs/cgroup/cpu,cpuacct
    cpuacct 1 TINFO: Creating 100 subgroups each with 100 processes
    testcases/bin/cpuacct.sh: 0: Cannot fork

In dmesg:

    LTP: starting cpuacct_100_100 (cpuacct.sh 100 100)
    cgroup: fork rejected by pids controller in /user.slice/user-1000.slice/session-1.scope

The reason is cgroups pid limit set by systemd user.slice.  For example
on 2 GB RAM machine it is set as:
    /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207

Add a check for both free memory and maximum number of pids (when using
systemd) to skip the test.  Systems not using systemd might still fail.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

---

Changes since v4:
1. None

Changes since v3:
1. Use tst_get_free_pids helper.

Changes since v2:
1. Correct realuid->real_uid typo.
2. Correct handling dash shell tests for return value.
3. Correct handling numbers - infinite.

Changes since v1:
1. Add checking for pids.
2. Accurately check the memory constraints.
---
 .../kernel/controllers/cpuacct/cpuacct.sh     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/testcases/kernel/controllers/cpuacct/cpuacct.sh b/testcases/kernel/controllers/cpuacct/cpuacct.sh
index 323aa7513bf4..39f880a53996 100755
--- a/testcases/kernel/controllers/cpuacct/cpuacct.sh
+++ b/testcases/kernel/controllers/cpuacct/cpuacct.sh
@@ -38,12 +38,61 @@ OPTIONS
 EOF
 }
 
+check_free_memory()
+{
+	local memneeded
+	local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo`
+
+	if [ $? -ne 0 ]; then
+		local memcached
+
+		memfree=`awk '/MemFree/ {print $2}' /proc/meminfo`
+		test $? -eq 0 || return 0
+
+		memcached=`awk '/MemCached/ {print $2}' /proc/meminfo`
+		test $? -eq 0 || return 0
+
+		memfree=$((memfree + memcached))
+	fi
+
+	# On x86_64, each 100 of processes were using ~16 MB of memory,
+	# so try to estimate the needed free memory based on this.
+	memneeded=$((max * nbprocess * 16384 / 100))
+
+	if [ $memfree -lt $memneeded ]; then
+		tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)"
+	fi
+	tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)"
+
+	return 0
+}
+
+check_limits()
+{
+	local tasksneeded=$((max * nbprocess + 100))
+	local tasksmax=$(tst_get_free_pids)
+	test $? -eq 0 || return 0
+
+	if [ $tasksmax -le $tasksneeded ]; then
+		tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)"
+	fi
+	tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)"
+
+	return 0
+}
+
 setup()
 {
 	if ! grep -q -w cpuacct /proc/cgroups; then
 		tst_brk TCONF "cpuacct not supported on this system"
 	fi
 
+	check_limits
+	# Don't bother with memory limit checks on smaller tests
+	if [ $max -ge 100 ] && [ $nbprocess -ge 100 ]; then
+		check_free_memory
+	fi
+
 	mount_point=`grep -w cpuacct /proc/mounts | cut -f 2 | cut -d " " -f2`
 	tst_res TINFO "cpuacct: $mount_point"
 	if [ "$mount_point" = "" ]; then
-- 
2.30.2

[LTP] [PATCH v5 3/3] controllers/cpuacct: fix rmdir failures on early test abort

[Krzysztof Kozlowski]

The "testpath" variable is assigned at the end of setup(), so if test
exits early, the "rmdir $testpath" is wrong:

    cpuacct 1 TCONF: not enough of memory on this system (less than 3840 MB)
    cpuacct 1 TINFO: removing created directories
    rmdir: missing operand
    Try 'rmdir --help' for more information.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

---

Changes since v1:
1. None.
---
 testcases/kernel/controllers/cpuacct/cpuacct.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/controllers/cpuacct/cpuacct.sh b/testcases/kernel/controllers/cpuacct/cpuacct.sh
index 39f880a53996..e95d4b53f936 100755
--- a/testcases/kernel/controllers/cpuacct/cpuacct.sh
+++ b/testcases/kernel/controllers/cpuacct/cpuacct.sh
@@ -120,12 +120,13 @@ cleanup()
 {
 	tst_res TINFO "removing created directories"
 
-	if [ -d "$testpath/subgroup_1" ]; then
-		rmdir $testpath/subgroup_*
+	if [ "$testpath" ]; then
+		if [ -d "$testpath/subgroup_1" ]; then
+			rmdir $testpath/subgroup_*
+		fi
+		rmdir $testpath
 	fi
 
-	rmdir $testpath
-
 	if [ "$mounted" -ne 1 ]; then
 		tst_res TINFO "Umounting cpuacct"
 		umount $mount_point
-- 
2.30.2

[LTP] [PATCH v5 2/3] controllers/cpuacct: skip cpuacct_100_100 on small memory systems

[Cyril Hrubis]

Hi!
> +check_limits()
> +{
> +	local tasksneeded=$((max * nbprocess + 100))

We allready have some reserve for the system in the tst_get_free_pids()
so shouldn't this be just max * nbprocess ?


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v5 1/3] lib: add tst_get_free_pids helper program

[Petr Vorel]

Hi Krzysztof,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH v5 2/3] controllers/cpuacct: skip cpuacct_100_100 on small memory systems

[Petr Vorel]

> Hi!
> > +check_limits()
> > +{
> > +	local tasksneeded=$((max * nbprocess + 100))

> We allready have some reserve for the system in the tst_get_free_pids()
> so shouldn't this be just max * nbprocess ?

Yes:
lib/tst_pid.c

/* Leave some available processes for the OS */
#define PIDS_RESERVE 50

I suppose 50 is enough, but if 100 is experienced to be needed I'd update PIDS_RESERVE.

Also, because you introduced use of awk, I'd add TST_NEEDS_CMDS="awk". Or is it
that common even on embedded systems that we don't bother?

NOTE: all changes can be done during merge, we just need to agree on it.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH v5 2/3] controllers/cpuacct: skip cpuacct_100_100 on small memory systems

[Krzysztof Kozlowski]

On 01/09/2021 20:38, Petr Vorel wrote:
>> Hi!
>>> +check_limits()
>>> +{
>>> +	local tasksneeded=$((max * nbprocess + 100))
> 
>> We allready have some reserve for the system in the tst_get_free_pids()
>> so shouldn't this be just max * nbprocess ?
> 
> Yes:
> lib/tst_pid.c
> 
> /* Leave some available processes for the OS */
> #define PIDS_RESERVE 50
> 
> I suppose 50 is enough, but if 100 is experienced to be needed I'd update PIDS_RESERVE.

Indeed, it's a duplicated reserve now. Let's dtop 100 here and use
existing PIDS_RESERVE.

> 
> Also, because you introduced use of awk, I'd add TST_NEEDS_CMDS="awk". Or is it
> that common even on embedded systems that we don't bother?

Even though most of Busybox installations have awk, it might be still
missing in some specific setup, so TST_NEEDS_CMDS="awk" seems reasonable.

> 
> NOTE: all changes can be done during merge, we just need to agree on it.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> Kind regards,
> Petr
> 



Best regards,
Krzysztof

[LTP] [PATCH v5 3/3] controllers/cpuacct: fix rmdir failures on early test abort

[Petr Vorel]

Hi Krzysztof,

...
> -	if [ -d "$testpath/subgroup_1" ]; then
> -		rmdir $testpath/subgroup_*
> +	if [ "$testpath" ]; then
> +		if [ -d "$testpath/subgroup_1" ]; then
> +			rmdir $testpath/subgroup_*
> +		fi
> +		rmdir $testpath
>  	fi

> -	rmdir $testpath
> -

LGTM, but how about use `rm -rf' instead?

e.g.

if [ "$testpath" ]; then
	rm -rf $testpath
fi

That could simplify checks. Or is it needed to use `rmdir' to make sure there is
no content in the directory? I suppose check like this is needed in cleanup,
thus no problem to use `rm -rf'.

Kind regards,
Petr

[LTP] [PATCH v5 3/3] controllers/cpuacct: fix rmdir failures on early test abort

[Krzysztof Kozlowski]

On 02/09/2021 10:18, Petr Vorel wrote:
> Hi Krzysztof,
> 
> ...
>> -	if [ -d "$testpath/subgroup_1" ]; then
>> -		rmdir $testpath/subgroup_*
>> +	if [ "$testpath" ]; then
>> +		if [ -d "$testpath/subgroup_1" ]; then
>> +			rmdir $testpath/subgroup_*
>> +		fi
>> +		rmdir $testpath
>>  	fi
> 
>> -	rmdir $testpath
>> -
> 
> LGTM, but how about use `rm -rf' instead?
> 
> e.g.
> 
> if [ "$testpath" ]; then
> 	rm -rf $testpath
> fi
> 
> That could simplify checks. Or is it needed to use `rmdir' to make sure there is
> no content in the directory? I suppose check like this is needed in cleanup,
> thus no problem to use `rm -rf'.

It isn't the point of this patch. I don't add here rmdir - all this code
was here before. The only thing added here is to check whether the
"testpath" variable is set or not.

I think using rm -rf should work, but anyway it's a separate commit :)


Best regards,
Krzysztof

[LTP] [PATCH v5 2/3] controllers/cpuacct: skip cpuacct_100_100 on small memory systems

[Cyril Hrubis]

Hi!
> NOTE: all changes can be done during merge, we just need to agree on it.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

For the whole patchset (and with the two minor fixes):

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v5 3/3] controllers/cpuacct: fix rmdir failures on early test abort

[Petr Vorel]

> On 02/09/2021 10:18, Petr Vorel wrote:
> > Hi Krzysztof,

> > ...
> >> -	if [ -d "$testpath/subgroup_1" ]; then
> >> -		rmdir $testpath/subgroup_*
> >> +	if [ "$testpath" ]; then
> >> +		if [ -d "$testpath/subgroup_1" ]; then
> >> +			rmdir $testpath/subgroup_*
> >> +		fi
> >> +		rmdir $testpath
> >>  	fi

> >> -	rmdir $testpath
> >> -

> > LGTM, but how about use `rm -rf' instead?

> > e.g.

> > if [ "$testpath" ]; then
> > 	rm -rf $testpath
> > fi

> > That could simplify checks. Or is it needed to use `rmdir' to make sure there is
> > no content in the directory? I suppose check like this is needed in cleanup,
> > thus no problem to use `rm -rf'.

> It isn't the point of this patch. I don't add here rmdir - all this code
> was here before. The only thing added here is to check whether the
> "testpath" variable is set or not.
Agree.

> I think using rm -rf should work, but anyway it's a separate commit :)
Agree, I just thought in simple thing like this it could be squashed (you add
code which then will be deleted), but sure, you're right.

I'll handle the cleanup with rm (unless you send follow up patch yourself).

Kind regards,
Petr


> Best regards,
> Krzysztof