Skip to contentSkip to conversation

controllers/cgroup_fj: fix

PATCH New Cedric Hnyda 2 participants 2 messages Oct 20, 2015 – Oct 27, 2015

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 controllers/cgroup_fj: fix

Modified the structure of the tests to make them
compatible with newer kernels

---
 .../controllers/cgroup_fj/cgroup_fj_function.sh    | 38 ++++-----
 .../controllers/cgroup_fj/cgroup_fj_stress.sh      | 46 ++++++-----
 .../controllers/cgroup_fj/cgroup_fj_utility.sh     | 90 +++++++++++++++-------
 3 files changed, 110 insertions(+), 64 deletions(-)
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +20 -18
index 3167fab..4f32cc7 100755
@@ -125,6 +125,8 @@ export TMPFILE=$TESTROOT/tmp_tasks
 
 . $TESTROOT/cgroup_fj_utility.sh
 
+mount_point="/dev/cgroup"
+get_mount_point;
 ##########################  main   #######################
 if [ "$#" -ne "9" ]; then
 	echo "ERROR: Wrong input parameters... Exiting test";
@@ -148,15 +150,15 @@ mkdir_subgroup;
 
 # cpuset.cpus and cpuset.mems should be specified with suitable value
 # before attaching operation if subsystem is cpuset
-if [ "$subsystem" == "cpuset" ] || [ "$subsystem" == "all" ] || [ $subsystem == "none" ] ; then
+if [ "$subsystem" == "cpuset" ] || [ "$subsystem" == "all" ] || [ "$subsystem" == "none" ] ; then
 	exist=`grep -w cpuset /proc/cgroups | cut -f1`;
 	if [ "$exist" != "" ]; then
 		if [ "$noprefix_use" == "no" ]; then
-			do_echo 1 1 `cat /dev/cgroup/cpus` /dev/cgroup/subgroup_1/cpus;
-			do_echo 1 1 `cat /dev/cgroup/mems` /dev/cgroup/subgroup_1/mems;
+			do_echo 1 1 `cat $mount_point/cpus` $mount_point/subgroup_1/cpus;
+			do_echo 1 1 `cat $mount_point/mems` $mount_point/subgroup_1/mems;
 		else
-			do_echo 1 1 `cat /dev/cgroup/cpuset.cpus` /dev/cgroup/subgroup_1/cpuset.cpus;
-			do_echo 1 1 `cat /dev/cgroup/cpuset.mems` /dev/cgroup/subgroup_1/cpuset.mems;
+			do_echo 1 1 `cat $mount_point/cpuset.cpus` $mount_point/subgroup_1/cpuset.cpus;
+			do_echo 1 1 `cat $mount_point/cpuset.mems` $mount_point/subgroup_1/cpuset.mems;
 		fi
 	fi
 fi
@@ -166,12 +168,12 @@ case $attach_operation in
 "1" )
 	;;
 "2" )
-	do_echo 1 1 $pid /dev/cgroup/subgroup_1/tasks;
+	do_echo 1 1 $pid $mount_point/subgroup_1/tasks;
 	;;
 "3" )
 	$TESTROOT/cgroup_fj_proc &
 	pid2=$!
-	cat /dev/cgroup/tasks > $TMPFILE
+	cat $mount_point/tasks > $TMPFILE
 	nlines=`cat $TMPFILE | wc -l`
 	for i in `seq 1 $nlines`
 	do
@@ -190,12 +192,12 @@ case $attach_operation in
 					continue
 				fi
 			fi
-			do_echo 1 1 "$cur_pid" /dev/cgroup/subgroup_1/tasks
+			do_echo 1 1 "$cur_pid" $mount_point/subgroup_1/tasks
 		fi
 	done
 	;;
 "4" )
-	do_echo 1 1 $pid /dev/cgroup/subgroup_1/tasks;
+	do_echo 1 1 $pid $mount_point/subgroup_1/tasks;
 	sleep 1
 	do_kill 1 1 10 $pid
 	;;
@@ -214,18 +216,18 @@ esac
 #if [ $notify_on_release -ne 0 ] && [ $notify_on_release -ne 1 ] && [ $notify_on_release -ne 2 ];then
 #	expected=0
 #fi
-do_echo 1 $expected $notify_on_release_str /dev/cgroup/subgroup_1/notify_on_release;
+do_echo 1 $expected $notify_on_release_str $mount_point/subgroup_1/notify_on_release;
 
 # echo release_agent that analysed from parameter
 if [ $release_agent_echo -ne 1 ]; then
-	do_echo 1 1 $release_agent_str /dev/cgroup/release_agent;
+	do_echo 1 1 $release_agent_str $mount_point/release_agent;
 fi
 
 sleep 1
 
 # pid could not be echoed from subgroup if subsystem is ( or include ) ns,
 # so we kill them here
-if [ "$subsystem" == "ns" ] || [ "$subsystem" == "all" ] || [ $subsystem == "none" ] ; then
+if [ "$subsystem" == "ns" ] || [ "$subsystem" == "all" ] || [ "$subsystem" == "none" ] ; then
 	do_kill 1 1 9 $pid
 	do_kill 1 1 9 $pid2
 # removing operation
@@ -234,20 +236,20 @@ else
 	"1" )
 		;;
 	"2" )
-		do_echo 1 1 $pid /dev/cgroup/tasks
+		do_echo 1 1 $pid $mount_point/tasks
 		if [ $pid2 -ne 0 ]  ; then
-			do_echo 1 1 $pid2 /dev/cgroup/tasks
+			do_echo 1 1 $pid2 $mount_point/tasks
 		fi
 		;;
 	"3" )
-		cat /dev/cgroup/subgroup_1/tasks > $TMPFILE
+		cat $mount_point/subgroup_1/tasks > $TMPFILE
 		nlines=`cat $TMPFILE | wc -l`
 		if [ $nlines -ne 0 ]; then
 			for i in `seq 1 $nlines`
 			do
 				cur_pid=`sed -n "$i""p" $TMPFILE`
 				if [ -e /proc/$cur_pid/ ];then
-					do_echo 1 1 "$cur_pid" /dev/cgroup/tasks
+					do_echo 1 1 "$cur_pid" $mount_point/tasks
 				fi
 			done
 		fi
@@ -262,9 +264,9 @@ else
 	esac
 fi
 
-sleep 1
+sleep 5
 
-do_rmdir 0 1 /dev/cgroup/subgroup_*
+do_rmdir 0 1 $mount_point/subgroup_*
 
 cleanup;
 do_kill 1 1 9 $pid
testcases/kernel/controllers/cgroup_fj/cgroup_fj_stress.sh +27 -19

134 lines not on this page · raw message

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +63 -27

186 lines not on this page · raw message

Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/1 controllers/cgroup_fj: fix

Hi!

Quoted 2 lines

Modified the structure of the tests to make them
compatible with newer kernels

You should be more verbose and describe why these changes are needed.

I.e. that the cgroups are by default mounted and newer kernels does not allow to mount them more than once...

Imagine yourself to be the one reviewing the patch and wondering why exactly has changes and how these changes fixes that.

Quoted 12 lines
---
 .../controllers/cgroup_fj/cgroup_fj_function.sh    | 38 ++++-----
 .../controllers/cgroup_fj/cgroup_fj_stress.sh      | 46 ++++++-----
 .../controllers/cgroup_fj/cgroup_fj_utility.sh     | 90 +++++++++++++++-------
 3 files changed, 110 insertions(+), 64 deletions(-)
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +0 -0
index 3167fab..4f32cc7 100755

testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh export TMPFILE=$TESTROOT/tmp_tasks · line 129

Quoted 5 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +2 -0
@@ -125,6 +125,8 @@ export TMPFILE=$TESTROOT/tmp_tasks
 
 . $TESTROOT/cgroup_fj_utility.sh
 
+mount_point="/dev/cgroup"
+get_mount_point;

It would be a bit cleaner to do an echo in the get_mount_point (instead of assigning global variable) and call this here as:

mount_point=$(get_mount_point)

Also setting the default value here is not clear way to do things. The right path should be returned by the get_mount_point function, otherwise we would have to fix all occurences when need to change the default arrives.

testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh else · line 267

Quoted 6 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +1 -1
@@ -262,9 +264,9 @@ else
 	esac
 fi
 
-sleep 1
+sleep 5

Why do we have to increase the sleep here? I'm certainly against this unless there is no other solution. Adding sleep statements to testcases increases the test runtime and makes continuous testing hard if not impossible.

Quoted 13 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +1 -1
 pid=0;
 release_agent_para=1;
 release_agent_echo=1;
@@ -108,7 +111,7 @@ get_subgroup_path1()
 		return;
 	fi
 
-	cur_subgroup_path1="/dev/cgroup/subgroup_$1/"
+	cur_subgroup_path1="$mount_point/subgroup_$1/"
 }
 
 

testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh esac · line 156

Quoted 7 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +3 -1
@@ -148,8 +151,11 @@ esac
 
 ##########################  main   #######################
 
-setup;
+if ! [ "$subsystem" == "all" ] && ! [ "$subsystem" == "none" ] ; then
+	exist_subsystem;
+fi

This adds a check that subsystem exists before we run the tests. This should be mentioned in the commit changelog as well.

testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh esac

Quoted 8 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +3 -1
 		fi
 		setup;
 		$TESTROOT/cgroup_fj_proc &
 		pid=$!
-		mount_cgroup;
+		if ! [ "$mount_point" == "/dev/cgroup" ] ; then
+			mount_cgroup;
+		fi

Hmm, this is a bit hacky. Maybe we should do "if cgroup is not mounted -> mount it". Or set a some variable in get_mount_point() that would be used to decide if we need to mount/umount it or not.

Quoted 11 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +2 -2
 		mkdir_subgroup;
 		if [ "$subsystem" == "cpuset" ] || [ "$subsystem" == "all" ] ; then
 			if [ "$exist_cpuset" != "" ]; then
-				do_echo 1 1 "$cpus" /dev/cgroup/subgroup_1/cpuset.cpus;
-				do_echo 1 1 "$mems" /dev/cgroup/subgroup_1/cpuset.mems;
+				do_echo 1 1 "$cpus" $mount_point/subgroup_1/cpuset.cpus;
+				do_echo 1 1 "$mems" $mount_point/subgroup_1/cpuset.mems;
 			fi
 		fi
 		let "count = $count + 1"

testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh else · line 247

Quoted 5 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +1 -1
@@ -236,7 +244,7 @@ else
 	done
 	echo "...mkdired $count times"
 
-	sleep 1
+	sleep 5

Here (the sleep) as well.

Quoted 43 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_function.sh +6 -6
 	case $attach_operation in
 	"1" )
@@ -244,12 +252,12 @@ else
 		do
 			do_echo 1 1 $pid "${pathes[$i]}""tasks"
 		done
-		do_echo 1 1 $pid /dev/cgroup/tasks
+		do_echo 1 1 $pid $mount_point/tasks
 		;;
 	"2" )
-		pathes2[0]="/dev/cgroup/"
+		pathes2[0]="$mount_point"
 		pathes2[1]="${pathes[$count]}"
-		pathes2[3]="/dev/cgroup/"
+		pathes2[3]="$mount_point/"
 		for i in `seq 1 $nlines`
 		do
 			j=$i
@@ -271,8 +279,8 @@ else
 	"3" )
 		count2=$count
 		let "count2 = $count2 + 1"
-		pathes[0]="/dev/cgroup/"
-		pathes[$count2]="/dev/cgroup/"
+		pathes[0]="$mount_point/"
+		pathes[$count2]="$mount_point/"
 		for i in `seq 0 $count`
 		do
 			j=$i
@@ -303,7 +311,7 @@ else
 	done
 fi
 
-do_rmdir 0 1 /dev/cgroup/subgroup_*
+do_rmdir 0 1 $mount_point/subgroup_*
 
 sleep 1
 
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +0 -0
index 9782f45..1228c96 100755

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh exist_subsystem() · line 55

Quoted 6 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +3 -0
@@ -50,6 +50,25 @@ exist_subsystem()
 	fi
 }
 
+get_mount_point()
+{
+	mount_point=`grep -w $subsystem /proc/mounts | cut -f 2 | cut -d " " -f2`;

^
No need for semicolon

So the mount_point is initialized to the /dev/cgroup/ in the test script but first thing this function does is to change it whatever we found in /proc/mounts (which may be empty string).

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh exist_subsystem() · line 69

Quoted 13 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +13 -0
+	if [ "$subsystem" = "none" ] && [ -e /sys/fs/cgroup/ ]; then
+		mount_point="/sys/fs/cgroup"
+	fi
+
+	if [ "$subsystem" = "all" ] && [ -e /sys/fs/cgroup/ ];  then
+		mount_point="/sys/fs/cgroup"
+	fi
+
+	if [ "$mount_point" = "" ]; then
+		mount_point=/dev/cgroup
+	fi
+
+	echo "mount_point is: $mount_point" 
                                           ^
			             trailing whitespace
Quoted 29 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +4 -4
+}
 
 
 get_subsystem()
 {
 	case $subsystem in
@@ -234,18 +253,17 @@ do_exit()
 	fi
 
 	exit_here=$1
-	expectted=$2
+	expected=$2
 	exit_status=$3
-
 	if [ $exit_status -eq 0 ] ;then
-		if [ $expectted -lt 1 ]; then
+		if [ $expected -lt 1 ]; then
 			if [ $exit_here -ge 1 ]; then
 				cleanup;
 				exit -1
 			fi
 		fi
 	else
-		if [ $expectted -ge 1 ]; then
+		if [ $expected -ge 1 ]; then
 			if [ $exit_here -ge 1 ]; then
 				cleanup;
 				exit -1

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh do_mount() · line 389

Quoted 3 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +0 -0
@@ -369,7 +387,7 @@ do_mount()
 			echo "\"mount -t cgroup $para_o $something $target\" (expected: fail)"
 		fi
 	fi

It's better to keep formatting changes separate from functional ones. Not that this is significant enough to be in separate patch, but you should at least add a line like this in the commit message:

* Small cleanup

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh do_kill() · line 472

Quoted 10 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +2 -1
+	echo mount -t cgroup $para_o $something $target
 	mount -t cgroup $para_o $something $target
 	do_exit $exit_here $expected $?;
 }
@@ -451,10 +469,22 @@ do_kill()
 
 setup()
 {
-	if [ -e /dev/cgroup ]; then
+	echo $mount_point ----

We shouldn't add another debug print here, the mount point is printed in the get_mount_point function allready, isn't it?

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh do_kill() · line 481

Quoted 8 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +7 -0
+	# Current test will fail if the previous one failed to rmdir
+	# so try to remove all subgroups
+	rm -rf $mount_point/subgroup_*
+
+	if [ -e $mount_point ] && [ "$mount_point" == "/dev/cgroup" ]; then
+		rm -rf /dev/cgroup
 		cleanup;
+		do_mkdir 1 1 $mount_point

Here as well, we should reason about the deletion/creation depending on some global flag not on the mount_point being set to /dev/cgroup

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh do_kill() · line 486

Quoted 6 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +4 -1
 	fi
-	do_mkdir 1 1 /dev/cgroup
+
+	if [ "$mount_point" == "/dev/cgroup" ]; then
+		do_mkdir 1 1 $mount_point
+	fi

Here we create the mount point, so we should remove the do_mkdir from the previous if.

Quoted 34 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +4 -11
 	if [ -e $TESTROOT/cgroup_fj_release_agent ]
 	then
@@ -493,47 +523,49 @@ cleanup()
 
 	killall -9 cgroup_fj_proc 1>/dev/null 2>&1;
 
-	if [ -e /dev/cgroup/subgroup_1 ]; then
-		cat /dev/cgroup/subgroup_1/tasks > $TMPFILE
+	if [ -e $mount_point/subgroup_1 ]; then
+		cat $mount_point/subgroup_1/tasks > $TMPFILE
 		nlines=`cat $TMPFILE | wc -l`
 		for i in `seq 1 $nlines`
 		do
 			cur_pid=`sed -n "$i""p" $TMPFILE`
 			if [ -e /proc/$cur_pid/ ];then
-				do_echo 0 1 "$cur_pid" /dev/cgroup/tasks
+				do_echo 0 1 "$cur_pid" $mount_point/tasks
 			fi
 		done
-		do_rmdir 0 1 /dev/cgroup/subgroup_*
+		do_rmdir 0 1 $mount_point/subgroup_*
 	fi
 
 	if [ -e $TMPFILE ]; then
 		rm -f $TMPFILE 2>/dev/null
 	fi
 
-	mount_str="`mount -l | grep /dev/cgroup`"
-	if [ "$mount_str" != "" ]; then
-		do_umount 0 1 /dev/cgroup
-	fi
-
-	if [ -e /dev/cgroup ]; then
-		do_rmdir 0 1 /dev/cgroup

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh cleanup() · line 552

Quoted 10 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +9 -0
+	if [ "$mount_point" = "/dev/cgroup" ] ; then
+		mount_str="`mount -l | grep $mount_point`"
+		if [ "$mount_str" != "" ]; then
+			do_umount 0 1 $mount_point
+		fi
+		if [ -e $mount_point ]; then
+			echo "about to rm $mount_point"
+			do_rmdir 0 1 $mount_point
+		fi
 	fi

And here same as in the setup.

Quoted 25 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +3 -3
 }
 
 reclaim_foundling()
 {
-	if ! [ -e /dev/cgroup/subgroup_1 ]; then
+	if ! [ -e $mount_point/subgroup_1 ]; then
 		return
 	fi
 	foundlings=0
-	cat `find /dev/cgroup/subgroup_* -name "tasks"` > $TMPFILE
+	cat `find $mount_point/subgroup_* -name "tasks"` > $TMPFILE
 	nlines=`cat "$TMPFILE" | wc -l`
 	for k in `seq 1 $nlines`
 	do
 		cur_pid=`sed -n "$k""p" $TMPFILE`
 		if [ -e /proc/$cur_pid/ ];then
 			echo "ERROR: pid $cur_pid reclaimed"
-			do_echo 0 1 "$cur_pid" "/dev/cgroup/tasks"
+			do_echo 0 1 "$cur_pid" "$mount_point/tasks"
 			: $((foundlings += 1))
 		fi
 	done
@@ -545,18 +577,22 @@ reclaim_foundling()
 
 mkdir_subgroup()

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh reclaim_foundling() · line 585

Quoted 10 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +3 -3
 {
-	if ! [ -e /dev/cgroup ]; then
-		echo "ERROR: /dev/cgroup doesn't exist... Exiting test"
+	if ! [ -e $mount_point ]; then
+		echo "ERROR: $mount_point doesn't exist... Exiting test"
 		exit -1;
 	fi
 
-	do_mkdir 1 1 /dev/cgroup/subgroup_1
+	do_mkdir 1 1 $mount_point/subgroup_1

Since we are touching the code it would be better to rename the subgroup_1 to start with 'ltp_' prefix to make sure that we will not collide with anything else on the system. But that should be done in followup patch.

testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh reclaim_foundling() · line 595

Quoted 10 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +4 -0
 }
 
 mount_cgroup ()
 {
 	expected=1
 	PARAMETER_O="";
+
+	if ! [ "$mount_point" == "/dev/cgroup" ] ; then
+		return 0
+	fi

So we actually do not mount the mount_point when it's mounted. I guess that the ifs in the tests that does not call this function when it's mounted are leftovers then.

But still we should reason here given something different than the mount_point == /dev/cgroup. I guess that the best would be global variable set in get_mount_point.

Or we can cram it all into mount_cgroup() function and make it initialize the mount_point variable, mount the cgroup, if needed, and set up needs_umount variable that would be used in umount_cgroup() function that would be called from test cleanup in order to clean things.

Quoted 31 lines
testcases/kernel/controllers/cgroup_fj/cgroup_fj_utility.sh +3 -3
 	if [ "$subsystem" == "abc" ]; then
 		expected=0
 	fi
@@ -579,10 +615,10 @@ mount_cgroup ()
 	fi
 	if [ "$remount_use_str" != "" ]; then
 		if [ "$PARAMETER_O" != "" ]; then
-			do_mount 1 1 "-o$PARAMETER_O" /dev/cgroup
+			do_mount 1 1 "-o$PARAMETER_O" $mount_point
 			PARAMETER_O="$PARAMETER_O"",""$remount_use_str"
 		else
-			do_mount 1 1 "" /dev/cgroup
+			do_mount 1 1 "" $mount_point
 			PARAMETER_O="$remount_use_str"
 		fi
 		sleep 1
@@ -592,7 +628,7 @@ mount_cgroup ()
 		PARAMETER_O="-o""$PARAMETER_O"
 	fi
 
-	do_mount 1 $expected "$PARAMETER_O" /dev/cgroup
+	do_mount 1 $expected "$PARAMETER_O" $mount_point
 }
 
 check_para()
Signature · 5 lines
2.1.4


-- 
Mailing list info: http://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis
[email protected]