controllers/cpuacct: rewrote testcases
PATCH v2 1/1 controllers/cpuacct: rewrote testcases
Rewrote cpuacct testcases since they were outdated. Removed old files
Added 6 tests for cpuacct.
--- runtest/controllers | 7 + testcases/kernel/controllers/cpuacct/.gitignore | 2 +- testcases/kernel/controllers/cpuacct/README | 51 ---- testcases/kernel/controllers/cpuacct/cpuacct.sh | 173 +++++++++++ .../kernel/controllers/cpuacct/cpuacct_setup.sh | 122 -------- .../kernel/controllers/cpuacct/cpuacct_task.c | 42 +++ .../kernel/controllers/cpuacct/cpuacct_task01.c | 17 -- .../controllers/cpuacct/cpuacct_testplan.txt | 44 --- .../kernel/controllers/cpuacct/run_cpuacct_test.sh | 340 --------------------- 9 files changed, 223 insertions(+), 575 deletions(-) delete mode 100644 testcases/kernel/controllers/cpuacct/README create mode 100755 testcases/kernel/controllers/cpuacct/cpuacct.sh delete mode 100755 testcases/kernel/controllers/cpuacct/cpuacct_setup.sh create mode 100644 testcases/kernel/controllers/cpuacct/cpuacct_task.c delete mode 100644 testcases/kernel/controllers/cpuacct/cpuacct_task01.c delete mode 100644 testcases/kernel/controllers/cpuacct/cpuacct_testplan.txt delete mode 100755 testcases/kernel/controllers/cpuacct/run_cpuacct_test.sh
runtest/controllers +7 -0
@@ -197,6 +197,13 @@ cgroup_fj_stress_memory_1_100_100_2 run_cgroup_test_fj.sh stress memory 1 100 10 controllers test_controllers.sh +cpuacct_1_1 cpuacct.sh 1 1 +cpuacct_1_10 cpuacct.sh 1 10 +cpuacct_10_10 cpuacct.sh 10 10 +cpuacct_1_100 cpuacct.sh 1 100 +cpuacct_100_1 cpuacct.sh 100 1 +cpuacct_100_100 cpuacct.sh 100 100 + cpuset_base_ops cpuset_base_ops_testset.sh cpuset_inherit cpuset_inherit_testset.sh cpuset_exclusive cpuset_exclusive_test.sh
testcases/kernel/controllers/cpuacct/.gitignore +1 -1
@@ -1 +1 @@ -/cpuacct_task01 +/cpuacct_task
testcases/kernel/controllers/cpuacct/README deleted +0 -51
51 lines not on this page · raw message
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +173 -0
173 lines not on this page · raw message
testcases/kernel/controllers/cpuacct/cpuacct_setup.sh deleted +0 -122
122 lines not on this page · raw message
testcases/kernel/controllers/cpuacct/cpuacct_task.c new file +42 -0
42 lines not on this page · raw message
testcases/kernel/controllers/cpuacct/cpuacct_task01.c deleted +0 -17
@@ -1,17 +0,0 @@ -#include <stdio.h> -#include <stdlib.h> - -int main (void ) -{ -FILE *fp; - -while (1 ) { - fp =fopen ("txt.x" ,"w" ); -if (fp ==NULL ) { -perror ("fopen failed" ); -exit (1 ); - } -fclose (fp); - } -return 0 ; -}
testcases/kernel/controllers/cpuacct/cpuacct_testplan.txt deleted +0 -44
44 lines not on this page · raw message
testcases/kernel/controllers/cpuacct/run_cpuacct_test.sh deleted +0 -340
340 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH v2 1/1 controllers/cpuacct: rewrote testcases
Hi!
Quoted 1 line
+#!/bin/bash
LTP aims to work on embedded hardware as well (where bash may not be installed at all), so this should rather be pointing to #!/bin/sh.
Quoted 5 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +5
-0+# Usage +# ./cpuacct.sh nbsubgroup nbprocess +# +# 1) nbsubgroup : number of subgroup to create +# 2) nbprocess : number of process to attach to each subgroup
testcases/kernel/controllers/cpuacct/cpuacct.sh line 40
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +10
-0+# +# Description +# +# 1) Find if cpuacct is mounted, if not mounted, cpuacct will be mounted +# 2) Create a subgroup ltp_test in cpuacct +# 3) Create nbsubgroup subgroup in ltp_test and attach them nbprocess process +# 4) Check that ltp_test/subgroup*/cpuacct.usage != 0 (test1) +# 5) Check that sum ltp_test/subgroup*/cpuacct.usage = ltp_test/cpuacct.usage +# (test2) +# 6) cleanup
No need to comment the setup and cleanup here. Keep just the part that is not obvious, i.e. points 2 - 5.
testcases/kernel/controllers/cpuacct/cpuacct.sh line 47
Quoted 6 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +6
-0+mounted=1 +mount_point="" +max=$1 +nbprocess=$2 + +cd $LTPROOT/testcases/bin
What is this needed for?
I do not see that you are working with files in this directory.
(I've missed this in the first review)
testcases/kernel/controllers/cpuacct/cpuacct.sh line 50
Quoted 2 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +2
-0+export TCID="cpuacct_$1_$2" +export TESTROOT=`pwd`
What is this used for?
Quoted 1 line
+export TST_TOTAL=3;
^ Useless semicolon
Quoted 1 line
+export TST_COUNT=1;
The TST_COUNT is handled by test.sh library, you shouldn't define it here.
Quoted 1 line
+status=0
The status is not used now, please remove it.
Quoted 7 lines
+.
test.shtestcases/kernel/controllers/cpuacct/cpuacct.sh new file +6
-0+ +verify_result() +{ + result=$1 + expected=$2 + message=$3
testcases/kernel/controllers/cpuacct/cpuacct.sh line 71
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +10
-0+ if [ "$result" -ne "$expected" ]; then + tst_resm TINFO "expected $expected, got $result" + cleanup; + tst_brkm TBROK NULL $message + fi +} + +mount_cpuacct() +{ + ROD mount -t cgroup -o cpuacct none $mount_point
Now that it's just single line it does not deserve to be in separate funciton, doesn't it?
testcases/kernel/controllers/cpuacct/cpuacct.sh line 78
Quoted 7 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +7
-0+} + +umount_cpuacct() +{ + tst_resm TINFO "Umounting cpuacct" + umount $mount_point + verify_result $? 0 "Error occured while umounting cgroup"
Now this can be done using ROD as well. If you have a look at our documentation at:
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#233-cleanup
You can setup TST_CLEANUP to point to a cleanup function and have it executed on tst_brkm (which is called by ROD) and on tst_exit.
So all you need to do is to set TST_CLEANUP in the setup() right after you mount the cpuacct and create the directory and you have guaranteed that the cleanup will be called if you call library function that causes test exit.
testcases/kernel/controllers/cpuacct/cpuacct.sh line 91
Quoted 13 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +13
-0+} + +do_mkdir() +{ + mkdir -p $1 + verify_result $? 0 "Error occured with mkdir" +} + +do_rmdir() +{ + rmdir $1 + verify_result $? 0 "Error occured with rmdir" +}
These as well. You can just do ROD rmdir foo in the code instead of defining new functions...
Quoted 32 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +32
-0+setup() +{ + mount_point=`grep -w cpuacct /proc/mounts | cut -f 2 | cut -d " " -f2` + tst_resm TINFO "cpuacct: $mount_point" + if [ "$mount_point" = "" ]; then + mounted=0 + mount_point=/dev/cgroup + fi + + testpath=$mount_point/ltp_$TCID + + if [ "$mounted" -eq "0" ]; then + do_mkdir $mount_point + mount_cpuacct + fi + do_mkdir $testpath +} + +cleanup() +{ + tst_resm TINFO "removing created directories" + rmdir $testpath/subgroup_*; + rmdir $testpath; + if [ "$mounted" -ne 1 ] ; then + umount_cpuacct + do_rmdir $mount_point + fi +} + +setup; + +# create subgroups
testcases/kernel/controllers/cpuacct/cpuacct.sh line 134
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +10
-0+for i in `seq 1 $max` +do + do_mkdir $testpath/subgroup_$i +done + +# create and attach process to subgroups +for i in `seq 1 $max` +do + for j in `seq 1 $nbprocess` + do
The prefered style for bash for loops is:
for foo; do
done
Quoted 7 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +7
-0+ cpuacct_task $testpath/subgroup_$i/tasks & + done +done + +for job in `jobs -p` +do + wait $job
testcases/kernel/controllers/cpuacct/cpuacct.sh line 151
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +10
-0+done + +acc=0 +error=0 +for i in `seq 1 $max` +do + tmp=`cat $testpath/subgroup_$i/cpuacct.usage` + if [ "$tmp" -eq "0" ]; then + error=1 + status=1
Now the status is not used at all, right?
Also the error should rather be named as fail.
And maybe we can do better naming it fails and incrementing for each zero value and then print how many of them were zero in the FAIL message below.
Review
Quoted 2 lines
(quoted diff) +2
-0+ fi + ((acc = acc + tmp))
This is bashism (does not work with portable shell, i.e. on debian dash shell)
Portable way is acc=$((acc + tmp))
Quoted 4 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +4
-0+done + +## check that cpuacct.usage != 0 for every subgroup +if [ "$error" -eq "1" ]; then
testcases/kernel/controllers/cpuacct/cpuacct.sh line 167
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +10
-0+ tst_resm TFAIL "cpuacct.usage should not be equal to 0" +else + tst_resm TPASS "cpuacct.usage is not equal to 0 for every subgroup" +fi + +## check that ltp_subgroup/cpuacct.usage == sum ltp_subgroup/subgroup*/cpuacct.usage +ref=`cat $testpath/cpuacct.usage` +if [ "$ref" != "$acc" ]; then + tst_resm TFAIL "ltp_test/cpuacct.usage not equal to ltp_test/subgroup*/cpuacct.usage" + status=1
Here status is unused as well.
testcases/kernel/controllers/cpuacct/cpuacct.sh line 172
Quoted 5 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +5
-0+else + tst_resm TPASS "ltp_test/cpuacct.usage equal to ltp_test/subgroup*/cpuacct.usage" +fi + +cleanup;
^
There is no need for semicolon here.
Also if you set the TST_CLEANUP there si no need to call it here.
Quoted 35 lines
testcases/kernel/controllers/cpuacct/cpuacct.sh new file +1
-0+tst_exittestcases/kernel/controllers/cpuacct/cpuacct_task.c new file +26
-0@@ -0,0 +1,42 @@ +/* +* Copyright (c) 2015 Cedric Hnyda <[email protected]> +* +* 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 would 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 the Free Software Foundation, +* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +/* +* Description: +* Attach the current process to argv[1] +*/ + +#include <stdio.h> +#include <sys/time.h> +#include <sys/types.h>
testcases/kernel/controllers/cpuacct/cpuacct_task.c line 36
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c new file +10
-0+#include <unistd.h> +#include <stdlib.h> + +int main (int argc,char **argv) +{ +char cmd[512 ]; +long pid =getpid (); + +sprintf (cmd,"echo %lu >> %s" , pid, argv[1 ]); +system (cmd);
This is kind of ugly way to write to a file from C.
You can either open the file and then print to it as:
FILE *f; f = fopen(argv[1], "w"); if (f) ... handle error fprintf(f, "%i\n", getpid()); fclose(f);
Or use FILE_PRINTF(argv[1], "%i\n", getpid()) from the LTP test library.
Quoted 6 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c new file +6
-0+struct itimerval it = {.it_value = {.tv_sec =0 , .tv_usec =10000 }}; + +setitimer (ITIMER_VIRTUAL , &it,NULL ); +for (;;); +return 0 ; +}
Signature · 2 lines
Cyril Hrubis [email protected]