[v3] memcg_stress_test.sh: ported to newlib

Message ID 20190104105407.48123-2-cristian.marussi@arm.com
State Accepted
Delegated to: Petr Vorel
Headers show
Series
  • [v3] memcg_stress_test.sh: ported to newlib
Related show

Commit Message

Cristian Marussi Jan. 4, 2019, 10:54 a.m.
Ported to newlib framework and general cleanup.
Moved an helper function out into cgroup_lib.sh.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 testcases/kernel/controllers/cgroup_lib.sh    |  16 +++
 .../kernel/controllers/memcg/stress/Makefile  |  21 +--
 .../memcg/stress/memcg_process_stress.c       |  34 ++---
 .../memcg/stress/memcg_stress_test.sh         | 135 ++++++++----------
 4 files changed, 92 insertions(+), 114 deletions(-)

Comments

Petr Vorel Jan. 28, 2019, 4:32 p.m. | #1
Hi Cristian,

thanks for your patch. I was thinking just to fix minor formatting issues, but
eval in run_stress() deserve to be rewritten.

> Ported to newlib framework and general cleanup.
> Moved an helper function out into cgroup_lib.sh.

..
> +	[ "x$val" = "x1" ] && return 0
drop x, it's portable enough like this::
[ "$val" = "1" ] && return 0

...
> +++ b/testcases/kernel/controllers/memcg/stress/Makefile
> @@ -1,24 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2009, Cisco Systems Inc.
> +# Author: Ngie Cooper, September 2009

>  #    kernel/controllers/memcg/stress testcase suite Makefile.
Delete this as well.

> +++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
...
> +	if ! is_cgroup_subsystem_available_and_enabled "memory";then
> +		tst_res TWARN "Either Kernel does not support MEMORY resource controller or feature not enabled"
> +		tst_brk TCONF ignored "Skipping all memory cgroup testcases...."
TWARN followed by TCONF does not make sense. Also skip is redundant. Please,
avoid using dots (even in comments).
This is enough:

    if ! is_cgroup_subsystem_available_and_enabled "memory"; then
        tst_res TCONF "Either Kernel does not support MEMORY resource controller or feature not enabled"
    fi

...
> +
> +	echo 3 > /proc/sys/vm/drop_caches
> +	sleep 2
> +	local mem_free=`cat /proc/meminfo | grep MemFree | awk '{ print $2 }'`
> +	local swap_free=`cat /proc/meminfo | grep SwapFree | awk '{ print $2 }'`
Test started using awk, which might not be everywhere (even it's quite basic.
TST_NEEDS_CMDS="awk" would be good for tests.

And also TST_NEEDS_CMDS="$TST_NEEDS_CMDS awk" would be good for cgroup_lib.sh.

> +
> +	MEM=$(( $mem_free + $swap_free / 2 ))
> +	MEM=$(( $MEM / 1024 ))
> +	RUN_TIME=$(( 15 * 60 ))
> +
> +	tst_res TINFO "Calculated available memory $MEM MB"
>  }

> +do_cleanup()
> +{
> +	if [ -e /dev/memcg ]; then
> +		umount /dev/memcg 2> /dev/null
> +		rmdir /dev/memcg 2> /dev/null
> +	fi
> +}

>  do_mount()
>  {
> -	cleanup;
> +	do_cleanup

>  	mkdir /dev/memcg 2> /dev/null
>  	mount -t cgroup -omemory memcg /dev/memcg
> @@ -65,62 +65,53 @@ do_mount()
>  # $4 - How long does this test run ? in second
>  run_stress()
>  {
> -	do_mount;
> +	local i
> +
> +	do_mount

>  	for i in $(seq 0 $(($1-1)))
>  	do
Please keep do on the same line (in memcg_stress_test.sh on several times):
for i in $(seq 0 $(($1-1))); do

>  		mkdir /dev/memcg/$i 2> /dev/null
> -		./memcg_process_stress $2 $3 &
> -		eval pid$i=$!
Eval is evil. And eval with local is even more evil. Even if it works on dash,
using eval in whole function is just bad. Could you please rewrite it into
single variable?  You can just separate pids with space.
+ maybe describe variables instead of using positional variables, e.g.:
local cgroups="$1"
local mem="$2"
...

> +		memcg_process_stress $2 $3 &
> +		eval local pid$i=$!

>  		eval echo \$pid$i > /dev/memcg/$i/tasks
>  	done

>  	for i in $(seq 0 $(($1-1)))
>  	do
> -		eval /bin/kill -s SIGUSR1 \$pid$i 2> /dev/null
> +		eval kill -USR1 \$pid$i 2> /dev/null
>  	done

>  	sleep $4

>  	for i in $(seq 0 $(($1-1)))
>  	do
> -		eval /bin/kill -s SIGKILL \$pid$i 2> /dev/null
> +		eval kill -KILL \$pid$i 2> /dev/null
>  		eval wait \$pid$i

>  		rmdir /dev/memcg/$i 2> /dev/null
>  	done

> -	cleanup;
> +	do_cleanup
>  }

>  testcase_1()
I'd rename it to test1.

>  {
> -	run_stress 150 $(( ($mem-150) / 150 )) 5 $RUN_TIME
> +	tst_res TINFO "testcase 1 started...it will run for $RUN_TIME secs"
Can you avoid dots, will?
Maybe describe it a bit.
I'd print test info in run_stress(), something like this:
tst_res TINFO "Testing $cgroups cgroups, using $mem, run for $RUN_TIME secs"

Kind regards,
Petr
Petr Vorel Jan. 29, 2019, 5:50 p.m. | #2
Hi Cristian,

I suggest following cleanup of your patch (see bellow).

This change, with my other cleanup patch [1] + your patch [2] can be seen on [3].

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/patch/1032923/
[2] https://patchwork.ozlabs.org/patch/1020887/
[3] https://github.com/pevik/ltp/commits/christian/memcg_stress_test.sh.v3.fixes

diff --git testcases/kernel/controllers/cgroup_lib.sh testcases/kernel/controllers/cgroup_lib.sh
index c164932fa..7918b5636 100644
--- testcases/kernel/controllers/cgroup_lib.sh
+++ testcases/kernel/controllers/cgroup_lib.sh
@@ -33,7 +33,7 @@ is_cgroup_subsystem_available_and_enabled()
 	[ $# -eq 0 ] && tst_brk TBROK "is_cgroup_subsystem_available_and_enabled: subsystem not defined"
 
 	val=$(grep -w $subsystem /proc/cgroups | awk '{ print $4 }')
-	[ "x$val" = "x1" ] && return 0
+	[ "$val" = "1" ] && return 0
 
 	return 1
 }
diff --git testcases/kernel/controllers/memcg/stress/Makefile testcases/kernel/controllers/memcg/stress/Makefile
index 773363cfc..a9678bf3b 100644
--- testcases/kernel/controllers/memcg/stress/Makefile
+++ testcases/kernel/controllers/memcg/stress/Makefile
@@ -1,9 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (C) 2009, Cisco Systems Inc.
 # Author: Ngie Cooper, September 2009
-#
-#    kernel/controllers/memcg/stress testcase suite Makefile.
-#
 
 top_srcdir		?= ../../../../..
 
diff --git testcases/kernel/controllers/memcg/stress/memcg_process_stress.c testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
index 6af550012..422deaeee 100644
--- testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
+++ testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2009 FUJITSU LIMITED
- *
  * Author: Li Zefan <lizf@cn.fujitsu.com>
  */
 
diff --git testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
index 652d99e55..9972b6c45 100755
--- testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
+++ testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
@@ -7,25 +7,23 @@
 # Restructure for LTP: Shi Weihua <shiwh@cn.fujitsu.com>
 # Added memcg enable/disable functionality: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com
 
-TST_TESTFUNC=testcase_
-TST_SETUP=do_setup
-TST_CLEANUP=do_cleanup
+TST_TESTFUNC=test
+TST_SETUP=setup
+TST_CLEANUP=cleanup
 TST_CNT=2
 TST_NEEDS_ROOT=1
 TST_NEEDS_CMDS="mount umount cat kill mkdir rmdir grep awk cut"
 
-# Each test case runs for 900 secs when everything fine...
-# ...so the default 5mins timeout is not enough.
+# Each test case runs for 900 secs when everything fine
+# therefore the default 5 mins timeout is not enough.
 LTP_TIMEOUT_MUL=7
 
-. tst_test.sh
 . cgroup_lib.sh
 
-do_setup()
+setup()
 {
-	if ! is_cgroup_subsystem_available_and_enabled "memory";then
-		tst_res TWARN "Either Kernel does not support MEMORY resource controller or feature not enabled"
-		tst_brk TCONF ignored "Skipping all memory cgroup testcases...."
+	if ! is_cgroup_subsystem_available_and_enabled "memory"; then
+		tst_brk TCONF "Either kernel does not support Memory Resource Controller or feature not enabled"
 	fi
 
 	echo 3 > /proc/sys/vm/drop_caches
@@ -40,7 +38,7 @@ do_setup()
 	tst_res TINFO "Calculated available memory $MEM MB"
 }
 
-do_cleanup()
+cleanup()
 {
 	if [ -e /dev/memcg ]; then
 		umount /dev/memcg 2> /dev/null
@@ -50,15 +48,12 @@ do_cleanup()
 
 do_mount()
 {
-	do_cleanup
+	cleanup
 
 	mkdir /dev/memcg 2> /dev/null
 	mount -t cgroup -omemory memcg /dev/memcg
 }
 
-
-# Run the stress test
-#
 # $1 - Number of cgroups
 # $2 - Allocated how much memory in one process? in MB
 # $3 - The interval to touch memory in a process
@@ -69,34 +64,31 @@ run_stress()
 
 	do_mount
 
-	for i in $(seq 0 $(($1-1)))
-	do
+	for i in $(seq 0 $(($1-1))); do
 		mkdir /dev/memcg/$i 2> /dev/null
 		memcg_process_stress $2 $3 &
-		eval local pid$i=$!
+		eval pid$i=$!
 
 		eval echo \$pid$i > /dev/memcg/$i/tasks
 	done
 
-	for i in $(seq 0 $(($1-1)))
-	do
+	for i in $(seq 0 $(($1-1))); do
 		eval kill -USR1 \$pid$i 2> /dev/null
 	done
 
 	sleep $4
 
-	for i in $(seq 0 $(($1-1)))
-	do
+	for i in $(seq 0 $(($1-1))); do
 		eval kill -KILL \$pid$i 2> /dev/null
 		eval wait \$pid$i
 
 		rmdir /dev/memcg/$i 2> /dev/null
 	done
 
-	do_cleanup
+	cleanup
 }
 
-testcase_1()
+test1()
 {
 	tst_res TINFO "testcase 1 started...it will run for $RUN_TIME secs"
 
@@ -105,7 +97,7 @@ testcase_1()
 	tst_res TPASS "stress test 1 passed"
 }
 
-testcase_2()
+test2()
 {
 	tst_res TINFO "testcase 2 started...it will run for $RUN_TIME secs"
Cristian Marussi Jan. 29, 2019, 7:25 p.m. | #3
Hi

On 29/01/2019 17:50, Petr Vorel wrote:> Hi Cristian,
>
> I suggest following cleanup of your patch (see bellow).
>
> This change, with my other cleanup patch [1] + your patch [2] can be seen on [3].
>
> Kind regards,
> Petr

Thanks for the review and the cleanup. (next I was going to rework following your today's advises, but you've been faster...)

I'll test asap.

Thanks

Regards

Cristian
Petr Vorel Feb. 1, 2019, 1:45 p.m. | #4
Hi Cristian,

> Thanks for the review and the cleanup. (next I was going to rework following your today's advises, but you've been faster...)
I'm sorry, I'll be patient next time :).
I was thinking to add these changes to your commit and merge patchset,
but as I introduced 2 regressions in your patchset last time (I test it of
course), I was careful.

> I'll test asap.
Thanks a lot!


Kind regards,
Petr
Cristian Marussi Feb. 4, 2019, 11:56 a.m. | #5
Hi

On 01/02/2019 13:45, Petr Vorel wrote:
> Hi Cristian,
> 
>> Thanks for the review and the cleanup. (next I was going to rework following your today's advises, but you've been faster...)
> I'm sorry, I'll be patient next time :).
> I was thinking to add these changes to your commit and merge patchset,
> but as I introduced 2 regressions in your patchset last time (I test it of
> course), I was careful.
> 
>> I'll test asap.
> Thanks a lot!

Tested from you branch with ToT as:

10df48aa7 (HEAD -> pevik_ltp/christian/memcg_stress_test.sh.v3.fixes)
memcg_stress_test.sh: fix memory usage
ebd6efaa6 memcg_stress_test.sh: Further cleanup
02c520928 memcg_stress_test.sh: ported to newlib
fc544f842 controllers/mem_process.c: Fix comparison warning
...

on 4k/64k pages defocnfig....looks fine to me.

Thanks a lot for the cleanup.

Regards

Cristian

> 
> 
> Kind regards,
> Petr
>
Petr Vorel Feb. 4, 2019, 11:34 p.m. | #6
Hi Cristian,

> Tested from you branch with ToT as:

> 10df48aa7 (HEAD -> pevik_ltp/christian/memcg_stress_test.sh.v3.fixes)
> memcg_stress_test.sh: fix memory usage
> ebd6efaa6 memcg_stress_test.sh: Further cleanup
> 02c520928 memcg_stress_test.sh: ported to newlib
> fc544f842 controllers/mem_process.c: Fix comparison warning
> ...

> on 4k/64k pages defocnfig....looks fine to me.

> Thanks a lot for the cleanup.
Thanks a lot for testing!
Pushed (with your Tested-by on my commit).

I did few cleanup changes in my commit (TINFO messages, fix missing local pid, redirect
stderr for wait to make it quiet, move TPASS message into run_stress()).

I wonder how to suppress kill message for the first cgroup pid:
/opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 14750 User defined signal 1   memcg_process_stress $mem_size $interval

This pid is the only one killed by -USR1, but line 65 is not in fist kill loop.
Therefore it cannot be killed by -KILL (second kill loop).

Kind regards,
Petr
Cristian Marussi Feb. 5, 2019, 5:46 p.m. | #7
Hi Petr

On 04/02/2019 23:34, Petr Vorel wrote:
> Hi Cristian,
> 
>> Tested from you branch with ToT as:
> 
>> 10df48aa7 (HEAD -> pevik_ltp/christian/memcg_stress_test.sh.v3.fixes)
>> memcg_stress_test.sh: fix memory usage
>> ebd6efaa6 memcg_stress_test.sh: Further cleanup
>> 02c520928 memcg_stress_test.sh: ported to newlib
>> fc544f842 controllers/mem_process.c: Fix comparison warning
>> ...
> 
>> on 4k/64k pages defocnfig....looks fine to me.
> 
>> Thanks a lot for the cleanup.
> Thanks a lot for testing!
> Pushed (with your Tested-by on my commit).
> 
> I did few cleanup changes in my commit (TINFO messages, fix missing local pid, redirect
> stderr for wait to make it quiet, move TPASS message into run_stress()).
> 
> I wonder how to suppress kill message for the first cgroup pid:
> /opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 14750 User defined signal 1   memcg_process_stress $mem_size $interval
> 
> This pid is the only one killed by -USR1, but line 65 is not in fist kill loop.
> Therefore it cannot be killed by -KILL (second kill loop).
> 

? ... I saw no error messages in my testing :D

Regards

Cristian
Xiao Yang Feb. 14, 2019, 8:32 a.m. | #8
On 2019/02/05 7:34, Petr Vorel wrote:
> Hi Cristian,
>
>> Tested from you branch with ToT as:
>> 10df48aa7 (HEAD ->  pevik_ltp/christian/memcg_stress_test.sh.v3.fixes)
>> memcg_stress_test.sh: fix memory usage
>> ebd6efaa6 memcg_stress_test.sh: Further cleanup
>> 02c520928 memcg_stress_test.sh: ported to newlib
>> fc544f842 controllers/mem_process.c: Fix comparison warning
>> ...
>> on 4k/64k pages defocnfig....looks fine to me.
>> Thanks a lot for the cleanup.
> Thanks a lot for testing!
> Pushed (with your Tested-by on my commit).
>
> I did few cleanup changes in my commit (TINFO messages, fix missing local pid, redirect
> stderr for wait to make it quiet, move TPASS message into run_stress()).
>
> I wonder how to suppress kill message for the first cgroup pid:
> /opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 14750 User defined signal 1   memcg_process_stress $mem_size $interval

Hi Petr, Cristian

I also saw the same message occasionally when running memcg_stress_test, 
as below:
-----------------------------------------------------------------------
...
/opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11262 User defined 
signal 1   memcg_process_stress $mem_size $interval
/opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11264 User defined 
signal 1   memcg_process_stress $mem_size $interval
/opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11267 User defined 
signal 1   memcg_process_stress $mem_size $interval
...
-----------------------------------------------------------------------

It is possible for memcg_process_stress to be killed by SIGUSR1(default 
action), because it may
have received SIGUSR1 before completing the specified initialization of 
SIGUSR1 signal action.

Perpahs, we should sleep a few seconds to wait for the completion of 
initializing SIGUSR1 signal action. :-)

Please see my patch for details:
-----------------------------------------------------------------------
diff --git 
a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh 
b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
index 5b19cc2..697a973 100755
--- a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
+++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
@@ -81,6 +81,13 @@ run_stress()
                 pids="$! $pids"
         done

+       # If memcg_process_stress has received SIGUSR1 before completing the
+       # specified initialization of SIGUSR1 signal action, 
memcg_process_stress
+       # will be killed by SIGUSR1(i.e. default action).  So we should 
wait for
+       # the completion of initializing SIGUSR1 signal action by 
sleeping a few
+       # seconds.
+       sleep 2
+
         for pid in $pids; do
                 kill -USR1 $pid 2> /dev/null
         done
-----------------------------------------------------------------------

Best Regards,
Xiao Yang

> This pid is the only one killed by -USR1, but line 65 is not in fist kill loop.
> Therefore it cannot be killed by -KILL (second kill loop).
>
> Kind regards,
> Petr
>
>
Cristian Marussi Feb. 15, 2019, 10:09 a.m. | #9
Hi Xiao

On 14/02/2019 08:32, Xiao Yang wrote:
> On 2019/02/05 7:34, Petr Vorel wrote:
>> Hi Cristian,

> Hi Petr, Cristian
> 
> I also saw the same message occasionally when running memcg_stress_test, 
> as below:
> -----------------------------------------------------------------------
> ...
> /opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11262 User defined 
> signal 1   memcg_process_stress $mem_size $interval
> /opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11264 User defined 
> signal 1   memcg_process_stress $mem_size $interval
> /opt/ltp/testcases/bin/memcg_stress_test.sh: line 65: 11267 User defined 
> signal 1   memcg_process_stress $mem_size $interval
> ...
> -----------------------------------------------------------------------
> 
> It is possible for memcg_process_stress to be killed by SIGUSR1(default 
> action), because it may
> have received SIGUSR1 before completing the specified initialization of 
> SIGUSR1 signal action
> 
> Perpahs, we should sleep a few seconds to wait for the completion of 
> initializing SIGUSR1 signal action. :-)

Mmm...yes that's a possibility....but I was noticing that we spawn a lot of bg
processes in a loop before starting signaling all of them in the following loop
(i.e. after all the forks)...but I just realized than then we signal them
starting from the most recently spawned...not the oldest (in reverse
order)....so maybe it could be enough to start signalling from the oldest.

Signaling in the natural-born order...

diff --git a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
index 5b19cc292..aa91f77b2 100755
--- a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
+++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
@@ -78,7 +78,7 @@ run_stress()
                mkdir /dev/memcg/$i 2> /dev/null
                memcg_process_stress $mem_size $interval &
                echo $! > /dev/memcg/$i/tasks
-               pids="$! $pids"
+               pids="$pids $!"
        done

        for pid in $pids; do


I understand that this approach is NO more deterministic than your sleeping
solution, anyway.
IMHO the only real deterministic solution would be to somehow explicitly "sync"
at the end of process-spawn loop, with the test script waiting for all the
spawned child to be ready....but it is maybe a bit overkill for the issue we face.
Maybe it is worth trying signaling in the correct order and then sleeping a bit
and see if it solves...I haven't seen this issue at all on my local setup on ARM.

Regards

Thanks

Cristian


> 
> Please see my patch for details:
> -----------------------------------------------------------------------
> diff --git 
> a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh 
> b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
> index 5b19cc2..697a973 100755
> --- a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
> +++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
> @@ -81,6 +81,13 @@ run_stress()
>                  pids="$! $pids"
>          done
> 
> +       # If memcg_process_stress has received SIGUSR1 before completing the
> +       # specified initialization of SIGUSR1 signal action, 
> memcg_process_stress
> +       # will be killed by SIGUSR1(i.e. default action).  So we should 
> wait for
> +       # the completion of initializing SIGUSR1 signal action by 
> sleeping a few
> +       # seconds.
> +       sleep 2
> +
>          for pid in $pids; do
>                  kill -USR1 $pid 2> /dev/null
>          done
> -----------------------------------------------------------------------
> 
> Best Regards,
> Xiao Yang
> 
>> This pid is the only one killed by -USR1, but line 65 is not in fist kill loop.
>> Therefore it cannot be killed by -KILL (second kill loop).
>>
>> Kind regards,
>> Petr
>>
>>
> 
> 
>

Patch

diff --git a/testcases/kernel/controllers/cgroup_lib.sh b/testcases/kernel/controllers/cgroup_lib.sh
index 8c19c6c3e..dbf5199e6 100644
--- a/testcases/kernel/controllers/cgroup_lib.sh
+++ b/testcases/kernel/controllers/cgroup_lib.sh
@@ -21,3 +21,19 @@  get_cgroup_mountpoint()
 	echo $mntpoint
 	return 0
 }
+
+# Check if given subsystem is supported and enabled
+# is_cgroup_subsystem_available_and_enabled SUBSYSTEM
+# RETURN: 0 if subsystem supported and enabled, otherwise 1
+is_cgroup_subsystem_available_and_enabled()
+{
+	local val
+	local subsystem=$1
+
+	[ $# -eq 0 ] && tst_brk TBROK "is_cgroup_subsystem_available_and_enabled: subsystem not defined"
+
+	val=$(grep -w $subsystem /proc/cgroups | awk '{ print $4 }')
+	[ "x$val" = "x1" ] && return 0
+
+	return 1
+}
diff --git a/testcases/kernel/controllers/memcg/stress/Makefile b/testcases/kernel/controllers/memcg/stress/Makefile
index a0456ac5d..773363cfc 100644
--- a/testcases/kernel/controllers/memcg/stress/Makefile
+++ b/testcases/kernel/controllers/memcg/stress/Makefile
@@ -1,24 +1,9 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2009, Cisco Systems Inc.
+# Author: Ngie Cooper, September 2009
 #
 #    kernel/controllers/memcg/stress testcase suite Makefile.
 #
-#    Copyright (C) 2009, Cisco Systems Inc.
-#
-#    This program is free software; you can redistribute it and/or modify
-#    it under the terms of the GNU General Public License as published by
-#    the Free Software Foundation; either version 2 of the License, or
-#    (at your option) any later version.
-#
-#    This program is distributed in the hope that it will be useful,
-#    but WITHOUT ANY WARRANTY; without even the implied warranty of
-#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-#    GNU General Public License for more details.
-#
-#    You should have received a copy of the GNU General Public License along
-#    with this program; if not, write to the Free Software Foundation, Inc.,
-#    51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
-#
-# Ngie Cooper, September 2009
-#
 
 top_srcdir		?= ../../../../..
 
diff --git a/testcases/kernel/controllers/memcg/stress/memcg_process_stress.c b/testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
index 870575c26..6af550012 100644
--- a/testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
+++ b/testcases/kernel/controllers/memcg/stress/memcg_process_stress.c
@@ -1,24 +1,9 @@ 
-/******************************************************************************/
-/*                                                                            */
-/* Copyright (c) 2009 FUJITSU LIMITED                                         */
-/*                                                                            */
-/* This program is free software;  you can redistribute it and/or modify      */
-/* it under the terms of the GNU General Public License as published by       */
-/* the Free Software Foundation; either version 2 of the License, or          */
-/* (at your option) any later version.                                        */
-/*                                                                            */
-/* This program is distributed in the hope that it will be useful,            */
-/* but WITHOUT ANY WARRANTY;  without even the implied warranty of            */
-/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See                  */
-/* the GNU General Public License for more details.                           */
-/*                                                                            */
-/* You should have received a copy of the GNU General Public License          */
-/* along with this program;  if not, write to the Free Software               */
-/* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    */
-/*                                                                            */
-/* Author: Li Zefan <lizf@cn.fujitsu.com>                                     */
-/*                                                                            */
-/******************************************************************************/
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2009 FUJITSU LIMITED
+ *
+ * Author: Li Zefan <lizf@cn.fujitsu.com>
+ */
 
 #include <sys/mman.h>
 #include <err.h>
@@ -91,14 +76,15 @@  int main(int argc, char *argv[])
 	if (interval <= 0)
 		interval = 1;
 
-	/* TODO (garrcoop): add error handling. */
 	memset(&sigint_action, 0, sizeof(sigint_action));
 	sigint_action.sa_handler = &sigint_handler;
-	sigaction(SIGINT, &sigint_action, NULL);
+	if (sigaction(SIGINT, &sigint_action, NULL))
+		err(1, "sigaction(%s) failed", "SIGINT");
 
 	memset(&sigusr_action, 0, sizeof(sigusr_action));
 	sigusr_action.sa_handler = &sigusr_handler;
-	sigaction(SIGUSR1, &sigusr_action, NULL);
+	if (sigaction(SIGUSR1, &sigusr_action, NULL))
+		err(1, "sigaction(%s) failed", "SIGUSR1");
 
 	while (!flag_exit) {
 		sleep(interval);
diff --git a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
index af1a708a8..652d99e55 100755
--- a/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
+++ b/testcases/kernel/controllers/memcg/stress/memcg_stress_test.sh
@@ -1,56 +1,56 @@ 
-#! /bin/sh
-
-################################################################################
-##                                                                            ##
-## Copyright (c) 2009 FUJITSU LIMITED                                         ##
-##                                                                            ##
-## This program is free software;  you can redistribute it and#or modify      ##
-## it under the terms of the GNU General Public License as published by       ##
-## the Free Software Foundation; either version 2 of the License, or          ##
-## (at your option) any later version.                                        ##
-##                                                                            ##
-## This program is distributed in the hope that it will be useful, but        ##
-## WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY ##
-## or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License   ##
-## for more details.                                                          ##
-##                                                                            ##
-## You should have received a copy of the GNU General Public License          ##
-## along with this program;  if not, write to the Free Software               ##
-## Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    ##
-##                                                                            ##
-## Author: Li Zefan <lizf@cn.fujitsu.com>                                     ##
-## Restructure for LTP: Shi Weihua <shiwh@cn.fujitsu.com>                     ##
-## Added memcg enable/disable functinality: Rishikesh K Rajak                 ##
-##                                              <risrajak@linux.vnet.ibm.com  ##
-##                                                                            ##
-################################################################################
-
-cd $LTPROOT/testcases/bin
-export TCID="memcg_stress_test"
-export TST_TOTAL=2
-export TST_COUNT=0
-
-if [ "x$(grep -w memory /proc/cgroups | cut -f4)" != "x1" ]; then
-        echo "WARNING:";
-        echo "Either Kernel does not support for memory resource controller or feature not enabled";
-        echo "Skipping all memcgroup testcases....";
-        exit 0
-fi
-
-RUN_TIME=$(( 15 * 60 ))
-
-cleanup()
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2009 FUJITSU LIMITED
+# Copyright (c) 2018-2019 ARM Ltd. All Rights Reserved.
+#
+# Author: Li Zefan <lizf@cn.fujitsu.com>
+# Restructure for LTP: Shi Weihua <shiwh@cn.fujitsu.com>
+# Added memcg enable/disable functionality: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com
+
+TST_TESTFUNC=testcase_
+TST_SETUP=do_setup
+TST_CLEANUP=do_cleanup
+TST_CNT=2
+TST_NEEDS_ROOT=1
+TST_NEEDS_CMDS="mount umount cat kill mkdir rmdir grep awk cut"
+
+# Each test case runs for 900 secs when everything fine...
+# ...so the default 5mins timeout is not enough.
+LTP_TIMEOUT_MUL=7
+
+. tst_test.sh
+. cgroup_lib.sh
+
+do_setup()
 {
-	if [ -e /dev/memcg ]; then
-		umount /dev/memcg 2>/dev/null
-		rmdir /dev/memcg 2>/dev/null
+	if ! is_cgroup_subsystem_available_and_enabled "memory";then
+		tst_res TWARN "Either Kernel does not support MEMORY resource controller or feature not enabled"
+		tst_brk TCONF ignored "Skipping all memory cgroup testcases...."
 	fi
+
+	echo 3 > /proc/sys/vm/drop_caches
+	sleep 2
+	local mem_free=`cat /proc/meminfo | grep MemFree | awk '{ print $2 }'`
+	local swap_free=`cat /proc/meminfo | grep SwapFree | awk '{ print $2 }'`
+
+	MEM=$(( $mem_free + $swap_free / 2 ))
+	MEM=$(( $MEM / 1024 ))
+	RUN_TIME=$(( 15 * 60 ))
+
+	tst_res TINFO "Calculated available memory $MEM MB"
 }
 
+do_cleanup()
+{
+	if [ -e /dev/memcg ]; then
+		umount /dev/memcg 2> /dev/null
+		rmdir /dev/memcg 2> /dev/null
+	fi
+}
 
 do_mount()
 {
-	cleanup;
+	do_cleanup
 
 	mkdir /dev/memcg 2> /dev/null
 	mount -t cgroup -omemory memcg /dev/memcg
@@ -65,62 +65,53 @@  do_mount()
 # $4 - How long does this test run ? in second
 run_stress()
 {
-	do_mount;
+	local i
+
+	do_mount
 
 	for i in $(seq 0 $(($1-1)))
 	do
 		mkdir /dev/memcg/$i 2> /dev/null
-		./memcg_process_stress $2 $3 &
-		eval pid$i=$!
+		memcg_process_stress $2 $3 &
+		eval local pid$i=$!
 
 		eval echo \$pid$i > /dev/memcg/$i/tasks
 	done
 
 	for i in $(seq 0 $(($1-1)))
 	do
-		eval /bin/kill -s SIGUSR1 \$pid$i 2> /dev/null
+		eval kill -USR1 \$pid$i 2> /dev/null
 	done
 
 	sleep $4
 
 	for i in $(seq 0 $(($1-1)))
 	do
-		eval /bin/kill -s SIGKILL \$pid$i 2> /dev/null
+		eval kill -KILL \$pid$i 2> /dev/null
 		eval wait \$pid$i
 
 		rmdir /dev/memcg/$i 2> /dev/null
 	done
 
-	cleanup;
+	do_cleanup
 }
 
 testcase_1()
 {
-	run_stress 150 $(( ($mem-150) / 150 )) 5 $RUN_TIME
+	tst_res TINFO "testcase 1 started...it will run for $RUN_TIME secs"
+
+	run_stress 150 $(( ($MEM - 150) / 150 )) 5 $RUN_TIME
 
-	tst_resm TPASS "stress test 1 passed"
+	tst_res TPASS "stress test 1 passed"
 }
 
 testcase_2()
 {
-	run_stress 1 $mem 5 $RUN_TIME
+	tst_res TINFO "testcase 2 started...it will run for $RUN_TIME secs"
 
-	tst_resm TPASS "stress test 2 passed"
-}
-
-echo 3 > /proc/sys/vm/drop_caches
-sleep 2
-mem_free=`cat /proc/meminfo | grep MemFree | awk '{ print $2 }'`
-swap_free=`cat /proc/meminfo | grep SwapFree | awk '{ print $2 }'`
+	run_stress 1 $MEM 5 $RUN_TIME
 
-mem=$(( $mem_free + $swap_free / 2 ))
-mem=$(( mem / 1024 ))
-
-date
-export TST_COUNT=$(( $TST_COUNT + 1 ))
-testcase_1
-export TST_COUNT=$(( $TST_COUNT + 1 ))
-testcase_2
-date
+	tst_res TPASS "stress test 2 passed"
+}
 
-exit 0
+tst_run