* [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue
@ 2016-11-03 9:42 shuang.qiu
2016-11-03 11:00 ` Cyril Hrubis
0 siblings, 1 reply; 5+ messages in thread
From: shuang.qiu @ 2016-11-03 9:42 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue
2016-11-03 9:42 [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue shuang.qiu
@ 2016-11-03 11:00 ` Cyril Hrubis
2016-11-03 14:08 ` Shuang Qiu
0 siblings, 1 reply; 5+ messages in thread
From: Cyril Hrubis @ 2016-11-03 11:00 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue
2016-11-03 11:00 ` Cyril Hrubis
@ 2016-11-03 14:08 ` Shuang Qiu
2016-11-17 9:04 ` Shuang Qiu
0 siblings, 1 reply; 5+ messages in thread
From: Shuang Qiu @ 2016-11-03 14:08 UTC (permalink / raw)
To: ltp
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue
2016-11-03 14:08 ` Shuang Qiu
@ 2016-11-17 9:04 ` Shuang Qiu
2016-11-21 11:39 ` Cyril Hrubis
0 siblings, 1 reply; 5+ messages in thread
From: Shuang Qiu @ 2016-11-17 9:04 UTC (permalink / raw)
To: ltp
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
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue
2016-11-17 9:04 ` Shuang Qiu
@ 2016-11-21 11:39 ` Cyril Hrubis
0 siblings, 0 replies; 5+ messages in thread
From: Cyril Hrubis @ 2016-11-21 11:39 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-21 11:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-03 9:42 [LTP] [PATCH] memcg_control_test.sh:incorrect test result and a pipe issue shuang.qiu
2016-11-03 11:00 ` Cyril Hrubis
2016-11-03 14:08 ` Shuang Qiu
2016-11-17 9:04 ` Shuang Qiu
2016-11-21 11:39 ` Cyril Hrubis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox