[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[shuang.qiu]

From: Shuang Qiu <shuang.qiu@oracle.com>

* 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 <shuang.qiu@oracle.com>
---
 .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
index 1d15872..668e74d 100644
--- a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
+++ b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
@@ -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
-- 
1.7.7

[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[Cyril Hrubis]

Hi!
> * 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 <shuang.qiu@oracle.com>
> ---
>  .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
> index 1d15872..668e74d 100644
> --- a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
> +++ b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
> @@ -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

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.

> @@ -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 
               ^
	       trailing whitespace
>  	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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[Shuang Qiu]

Hi Cyril,
Thanks for review.
On 11/03/2016 07:00 PM, Cyril Hrubis wrote:
> Hi!
>> * 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 <shuang.qiu@oracle.com>
>> ---
>>   .../memcg/control/memcg_control_test.sh            |   15 +++++++++++----
>>   1 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>> index 1d15872..668e74d 100644
>> --- a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>> +++ b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>> @@ -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
> 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.
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.
>
>> @@ -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
>                 ^
> 	       trailing whitespace
>>   	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
>

[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[Shuang Qiu]

On 11/03/2016 10:08 PM, Shuang Qiu wrote:
> Hi Cyril,
> Thanks for review.
> On 11/03/2016 07:00 PM, Cyril Hrubis wrote:
>> Hi!
>>> * 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 <shuang.qiu@oracle.com>
>>> ---
>>>   .../memcg/control/memcg_control_test.sh            |   15 
>>> +++++++++++----
>>>   1 files changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git 
>>> a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh 
>>> b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>>> index 1d15872..668e74d 100644
>>> --- a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>>> +++ b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh
>>> @@ -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
>> 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.
> 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.
>>
>>> @@ -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
>>                 ^
>>            trailing whitespace
>>>       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.
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
>
> Thanks
> Shuang
>>
>
>

[LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue

[Cyril Hrubis]

Hi!
> >> 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.

-- 
Cyril Hrubis
chrubis@suse.cz