Skip to contentSkip to conversation

memcg_control_test.sh:incorrect test result and a pipe issue

PATCH New [email protected] 2 participants 5 messages Nov 3, 2016 – Nov 21, 2016

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 memcg_control_test.sh:incorrect test result and a pipe issue

From: Shuang Qiu <[email protected]>

* The test result flag FAILED_CNT is handled in a sub-shell,which will make the test result passed even the test actually failed,update the scripts to not running in sub-shell.
* Sometimes the mem_process process is killed after more than 5 seconds,increase the sleep time to 15 sceconds.And if the mem_process process is killed after 5 sceconds,the pipe may blocked while writing it(echo m > $STATUS_PIPE;echo x > $STATUS_PIPE),then the whole LTP testing will hang.Adding a sub-shell to handle to pipe.

Signed-off-by Shuang Qiu <[email protected]>
---
 .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +11 -4
index 1d15872..668e74d 100644
@@ -64,11 +64,15 @@ test_proc_kill()
 
 	#Instruct the test process to start acquiring memory
 	echo m > $STATUS_PIPE
-	sleep 5
+	sleep 15
 
 	#Check if killed
 	ps -p $! > /dev/null 2> /dev/null
 	if [ $? -eq 0 ]; then
+		(sleep 30
+		ps -p $! > /dev/null 2> /dev/null
+		[ $? -ne 0 -a -e $STATUS_PIPE ] && cat $STATUS_PIPE && \
+		rm -f $STATUS_PIPE) &
 		echo m > $STATUS_PIPE
 		echo x > $STATUS_PIPE
 	else

@@ -86,7 +90,8 @@ testcase_1()
 	echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes
 
 	mkdir sub
-	(cd sub
+	basedir=$PWD
+	cd sub
 	KILLED_CNT=0
 	test_proc_kill
 

@@ -94,7 +99,8 @@ testcase_1()
 		result $FAIL "Test #1: failed"
 	else
 		result $PASS "Test #1: passed"
-	fi)
+	fi
+	cd $basedir
 	rmdir sub
 }
 

@@ -145,7 +151,8 @@ FAILED_CNT=0
 TST_NUM=1
 while [ $TST_NUM -le $TST_TOTAL ]; do
 	mkdir $TST_PATH/mnt/$TST_NUM
-	(cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM)
+	cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM
+	cd $TMP 
 	rmdir $TST_PATH/mnt/$TST_NUM
 	: $((TST_NUM += 1))
 done
Signature · 1 line
1.7.7
#1 lore raw

PATCH 1/1 memcg_control_test.sh:incorrect test result and a pipe issue

Hi!

Quoted 25 lines

* The test result flag FAILED_CNT is handled in a sub-shell,which will make the test result passed even the test actually failed,update the scripts to not running in sub-shell.
* Sometimes the mem_process process is killed after more than 5 seconds,increase the sleep time to 15 sceconds.And if the mem_process process is killed after 5 sceconds,the pipe may blocked while writing it(echo m > $STATUS_PIPE;echo x > $STATUS_PIPE),then the whole LTP testing will hang.Adding a sub-shell to handle to pipe.

Signed-off-by Shuang Qiu <[email protected]>
---
 .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +1 -1
index 1d15872..668e74d 100644
@@ -64,11 +64,15 @@ test_proc_kill()
 
 	#Instruct the test process to start acquiring memory
 	echo m > $STATUS_PIPE
-	sleep 5
+	sleep 15

testcases/kernel/controllers/memcg/control/memcg_control_test.sh test_proc_kill() · line 78

Quoted 10 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +4 -0
 	#Check if killed
 	ps -p $! > /dev/null 2> /dev/null
 	if [ $? -eq 0 ]; then
+		(sleep 30
+		ps -p $! > /dev/null 2> /dev/null
+		[ $? -ne 0 -a -e $STATUS_PIPE ] && cat $STATUS_PIPE && \
+		rm -f $STATUS_PIPE) &
 		echo m > $STATUS_PIPE
 		echo x > $STATUS_PIPE
 	else

This is plain wrong. Sparkling code with sleeps never solves anything and only slows down the testrun.

What should have been done here instead is checking if the process was killed in a loop something as twice a second with some reasonable timeout, trying it 60 times would give it max 30 seconds to run.

Quoted 21 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +4 -2
@@ -86,7 +90,8 @@ testcase_1()
 	echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes
 
 	mkdir sub
-	(cd sub
+	basedir=$PWD
+	cd sub
 	KILLED_CNT=0
 	test_proc_kill
 

@@ -94,7 +99,8 @@ testcase_1()
 		result $FAIL "Test #1: failed"
 	else
 		result $PASS "Test #1: passed"
-	fi)
+	fi
+	cd $basedir
 	rmdir sub
 }

testcases/kernel/controllers/memcg/control/memcg_control_test.sh FAILED_CNT=0 · line 155

Quoted 6 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +2 -1
@@ -145,7 +151,8 @@ FAILED_CNT=0
 TST_NUM=1
 while [ $TST_NUM -le $TST_TOTAL ]; do
 	mkdir $TST_PATH/mnt/$TST_NUM
-	(cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM)
+	cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM
+	cd $TMP 

               ^
	       trailing whitespace

testcases/kernel/controllers/memcg/control/memcg_control_test.sh FAILED_CNT=0 · line 158

Quoted 3 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +0 -0
 	rmdir $TST_PATH/mnt/$TST_NUM
 	: $((TST_NUM += 1))
 done

Moreover I was looking at the test recently and I would say that the best solution would be to remove it completely, since the same function should be covered by the memcg functional testcases. So the best bet here would be to check that it's covered by functional tests, if not add missing pieces to functional tests and get rid of this mess.

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#2 lore raw

PATCH 1/1 memcg_control_test.sh:incorrect test result and a pipe issue

Hi Cyril,
Thanks for review.

Quoted 39 lines · Cyril Hrubis

Hi!

Quoted 25 lines

* The test result flag FAILED_CNT is handled in a sub-shell,which will make the test result passed even the test actually failed,update the scripts to not running in sub-shell.
* Sometimes the mem_process process is killed after more than 5 seconds,increase the sleep time to 15 sceconds.And if the mem_process process is killed after 5 sceconds,the pipe may blocked while writing it(echo m > $STATUS_PIPE;echo x > $STATUS_PIPE),then the whole LTP testing will hang.Adding a sub-shell to handle to pipe.

Signed-off-by Shuang Qiu <[email protected]>
---
  .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
  1 files changed, 11 insertions(+), 4 deletions(-)
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +1 -1
index 1d15872..668e74d 100644
@@ -64,11 +64,15 @@ test_proc_kill()
 
  	#Instruct the test process to start acquiring memory
  	echo m > $STATUS_PIPE
-	sleep 5
+	sleep 15

testcases/kernel/controllers/memcg/control/memcg_control_test.sh test_proc_kill() · line 78

Quoted 10 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +4 -0
  	#Check if killed
  	ps -p $! > /dev/null 2> /dev/null
  	if [ $? -eq 0 ]; then
+		(sleep 30
+		ps -p $! > /dev/null 2> /dev/null
+		[ $? -ne 0 -a -e $STATUS_PIPE ] && cat $STATUS_PIPE && \
+		rm -f $STATUS_PIPE) &
  		echo m > $STATUS_PIPE
  		echo x > $STATUS_PIPE
  	else

This is plain wrong. Sparkling code with sleeps never solves anything and only slows down the testrun.

Quoted 3 lines

What should have been done here instead is checking if the process was killed in a loop something as twice a second with some reasonable timeout, trying it 60 times would give it max 30 seconds to run.

The added code here is intent on read the pipe after sleeps to avoid the parent process blocked here when writing the pipe.
I agree that it is better to check if the process was killed in a loop instead of just sleeping.

Quoted 30 lines
Quoted 22 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +4 -2
@@ -86,7 +90,8 @@ testcase_1()
  	echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes
 
  	mkdir sub
-	(cd sub
+	basedir=$PWD
+	cd sub
  	KILLED_CNT=0
  	test_proc_kill
 

@@ -94,7 +99,8 @@ testcase_1()
  		result $FAIL "Test #1: failed"
  	else
  		result $PASS "Test #1: passed"
-	fi)
+	fi
+	cd $basedir
  	rmdir sub
  }

testcases/kernel/controllers/memcg/control/memcg_control_test.sh FAILED_CNT=0 · line 155

Quoted 6 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +2 -1
@@ -145,7 +151,8 @@ FAILED_CNT=0
  TST_NUM=1
  while [ $TST_NUM -le $TST_TOTAL ]; do
  	mkdir $TST_PATH/mnt/$TST_NUM
-	(cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM)
+	cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM
+	cd $TMP

                ^
	       trailing whitespace
Quoted 8 lines

testcases/kernel/controllers/memcg/control/memcg_control_test.sh FAILED_CNT=0 · line 158

Quoted 3 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +0 -0
  	rmdir $TST_PATH/mnt/$TST_NUM
  	: $((TST_NUM += 1))
  done

Moreover I was looking at the test recently and I would say that the best solution would be to remove it completely, since the same function should be covered by the memcg functional testcases. So the best bet here would be to check that it's covered by functional tests, if not add missing pieces to functional tests and get rid of this mess.

Ok,I will also check the functional test.

Thanks
Shuang

Quoted 1 line
author
#3 lore raw

PATCH 1/1 memcg_control_test.sh:incorrect test result and a pipe issue

Quoted 81 lines · Shuang Qiu

Hi Cyril,
Thanks for review.

Quoted 40 lines · Cyril Hrubis

Hi!

Quoted 26 lines

* The test result flag FAILED_CNT is handled in a sub-shell,which will make the test result passed even the test actually failed,update the scripts to not running in sub-shell.
* Sometimes the mem_process process is killed after more than 5 seconds,increase the sleep time to 15 sceconds.And if the mem_process process is killed after 5 sceconds,the pipe may blocked while writing it(echo m > $STATUS_PIPE;echo x > $STATUS_PIPE),then the whole LTP testing will hang.Adding a sub-shell to handle to pipe.

Signed-off-by Shuang Qiu <[email protected]>
---
  .../memcg/control/memcg_control_test.sh            |   15 
+++++++++++----
  1 files changed, 11 insertions(+), 4 deletions(-)
(quoted diff) +0 -0

a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh index 1d15872..668e74d 100644

testcases/kernel/controllers/memcg/control/memcg_control_test.sh +1 -1
@@ -64,11 +64,15 @@ test_proc_kill()
        #Instruct the test process to start acquiring memory
      echo m > $STATUS_PIPE
-    sleep 5
+    sleep 15

testcases/kernel/controllers/memcg/control/memcg_control_test.sh test_proc_kill() · line 76

Quoted 10 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +4 -0
        #Check if killed
      ps -p $! > /dev/null 2> /dev/null
      if [ $? -eq 0 ]; then
+        (sleep 30
+        ps -p $! > /dev/null 2> /dev/null
+        [ $? -ne 0 -a -e $STATUS_PIPE ] && cat $STATUS_PIPE && \
+        rm -f $STATUS_PIPE) &
          echo m > $STATUS_PIPE
          echo x > $STATUS_PIPE
      else

This is plain wrong. Sparkling code with sleeps never solves anything and only slows down the testrun.

Quoted 3 lines

What should have been done here instead is checking if the process was killed in a loop something as twice a second with some reasonable timeout, trying it 60 times would give it max 30 seconds to run.

The added code here is intent on read the pipe after sleeps to avoid the parent process blocked here when writing the pipe.
I agree that it is better to check if the process was killed in a loop instead of just sleeping.

Quoted 31 lines
Quoted 16 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +0 -0
@@ -86,7 +90,8 @@ testcase_1()
      echo "$TOT_MEM_LIMIT" >

$TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes
mkdir sub

testcases/kernel/controllers/memcg/control/memcg_control_test.sh +3 -2
-    (cd sub
+    basedir=$PWD
+    cd sub
      KILLED_CNT=0
      test_proc_kill
  @@ -94,7 +99,8 @@ testcase_1()
          result $FAIL "Test #1: failed"
      else
          result $PASS "Test #1: passed"
-    fi)
+    fi

testcases/kernel/controllers/memcg/control/memcg_control_test.sh

Quoted 10 lines
testcases/kernel/controllers/memcg/control/memcg_control_test.sh +3 -1
+    cd $basedir
      rmdir sub
  }
  @@ -145,7 +151,8 @@ FAILED_CNT=0
  TST_NUM=1
  while [ $TST_NUM -le $TST_TOTAL ]; do
      mkdir $TST_PATH/mnt/$TST_NUM
-    (cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM)
+    cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM
+    cd $TMP

                ^
           trailing whitespace
Quoted 3 lines
      rmdir $TST_PATH/mnt/$TST_NUM
      : $((TST_NUM += 1))

done

Quoted 6 lines
Quoted 5 lines

Moreover I was looking at the test recently and I would say that the best solution would be to remove it completely, since the same function should be covered by the memcg functional testcases. So the best bet here would be to check that it's covered by functional tests, if not add missing pieces to functional tests and get rid of this mess.

Ok,I will also check the functional test.

I think this testcase can be covered by memcg_use_hierarchy in functional testcases.
Maybe we can add a sub case in memcg_use_hierarchy test that check the parent limit for memsw.limit_in_bytes.

Thanks
Shuang

Quoted 6 lines

Thanks
Shuang

Quoted 1 line
#4 lore raw

PATCH 1/1 memcg_control_test.sh:incorrect test result and a pipe issue

Hi!

Quoted 10 lines
Quoted 6 lines
Quoted 5 lines

Moreover I was looking at the test recently and I would say that the best solution would be to remove it completely, since the same function should be covered by the memcg functional testcases. So the best bet here would be to check that it's covered by functional tests, if not add missing pieces to functional tests and get rid of this mess.

Ok,I will also check the functional test.

I think this testcase can be covered by memcg_use_hierarchy in functional testcases.
Maybe we can add a sub case in memcg_use_hierarchy test that check the parent limit for memsw.limit_in_bytes.

Both sounds good to me.

Signature · 2 lines
Cyril Hrubis
[email protected]