memcg/functional: check several times if the process is killed
PATCH 1/1 memcg/functional: check several times if the process is killed
On some systems it may take slightly more than one second to kill the memcg_process. So let's check several times if the process is alive.
Also removed sleep() before moving the process to the memory cgroup, since this looks reduntant.
--- .../controllers/memcg/functional/memcg_lib.sh | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +10 -4
@@ -220,14 +220,20 @@ test_proc_kill() $TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1 echo $pid > tasks kill -s USR1 $pid 2> /dev/null - sleep 1 - ps -p $pid > /dev/null 2> /dev/null - if [ $? -ne 0 ]; then + pid_exists=1 + for tpk_iter in $(seq 5); do + if ! kill $pid 2> /dev/null; then + pid_exists=0 + break + fi + sleep 1 + done + + if [ $pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error"
Signature · 1 line
1.7.1
PATCH 1/1 memcg/functional: check several times if the process is killed
Hi!
Quoted 17 lines
On some systems it may take slightly more than one second to kill the
memcg_process. So let's check several times if the process is alive.Also removed
sleep()before moving the process to the memory cgroup, since this looks reduntant.Signed-off-by Stanislav Kholmanskikh <[email protected]>--- .../controllers/memcg/functional/memcg_lib.sh | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-0
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 223
Quoted 4 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-1@@ -220,14 +220,20 @@ test_proc_kill() $TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1
This sleep sure is useless.
Quoted 5 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-2echo $pid > tasks kill -s USR1 $pid 2> /dev/null - sleep 1 -
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 234
Quoted 10 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +8
-2- ps -p $pid > /dev/null 2> /dev/null - if [ $? -ne 0 ]; then + pid_exists=1 + for tpk_iter in $(seq 5); do + if ! kill $pid 2> /dev/null; then + pid_exists=0 + break + fi + sleep 1 + done
This does no seem right to me. The original code send a SIGUSR1 signal to the memcg_process which caused it to allocate memory which supposedly provokes OOM to kill it. Hence the sleep 1 after the kill -s USR $pid.
Now this code hammers the memcg_process with SIGKILL instead.
As far as I can tell the right thing to do here is to wait with reasonable timeout for the memcg_process to become zombie and only kill it if that hasn't happened. Or did I miss something?
Quoted 10 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +1
-0+ if [ $pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error"Signature · 5 lines
1.7.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
Quoted 45 lines · Cyril Hrubis
Hi!
Quoted 17 lines
On some systems it may take slightly more than one second to kill the
memcg_process. So let's check several times if the process is alive.Also removed
sleep()before moving the process to the memory cgroup, since this looks reduntant.Signed-off-by Stanislav Kholmanskikh <[email protected]>--- .../controllers/memcg/functional/memcg_lib.sh | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-0testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 223
Quoted 4 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-1@@ -220,14 +220,20 @@ test_proc_kill() $TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1This sleep sure is useless.
Quoted 5 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-2echo $pid > tasks kill -s USR1 $pid 2> /dev/null - sleep 1 -testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 234
Quoted 10 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +8
-2- ps -p $pid > /dev/null 2> /dev/null - if [ $? -ne 0 ]; then + pid_exists=1 + for tpk_iter in $(seq 5); do + if ! kill $pid 2> /dev/null; then + pid_exists=0 + break + fi + sleep 1 + doneThis does no seem right to me. The original code send a SIGUSR1 signal to the
memcg_processwhich caused it to allocate memory which supposedly provokes OOM to kill it. Hence the sleep 1 after the kill -s USR $pid.
Quoted 5 lines
Now this code hammers the
memcg_processwith SIGKILL instead.As far as I can tell the right thing to do here is to wait with reasonable timeout for the
memcg_processto become zombie and only kill it if that hasn't happened. Or did I miss something?
No, you didn't miss anything. I was planning to use 'kill' to check whether the pid is alive or not. But I should have used 'kill -s 0' instead of plain 'kill'.
Thank you.
Quoted 12 lines
Quoted 11 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +1
-1+ if [ $pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error" --1.7.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
PATCH 1/1 memcg/functional: check several times if the process is killed
Hi!
Quoted 6 lines
Quoted 6 lines
This does no seem right to me. The original code send a SIGUSR1 signal to the
memcg_processwhich caused it to allocate memory which supposedly provokes OOM to kill it. Hence the sleep 1 after the kill -s USR $pid.Now this code hammers the
memcg_processwith SIGKILL instead.
Quoted 7 lines
Quoted 3 lines
As far as I can tell the right thing to do here is to wait with reasonable timeout for the
memcg_processto become zombie and only kill it if that hasn't happened. Or did I miss something?No, you didn't miss anything. I was planning to use 'kill' to check whether the pid is alive or not. But I should have used 'kill -s 0' instead of plain 'kill'.
Would that even work? Technically till you wait the process the pid still exists albeit in a zombie state.
And looking into POSIX there were some systems that returned ESRCH in this case but it looks this behavior is strongly discouraged.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
Quoted 15 lines · Cyril Hrubis
Hi!
Quoted 6 lines
Quoted 6 lines
This does no seem right to me. The original code send a SIGUSR1 signal to the
memcg_processwhich caused it to allocate memory which supposedly provokes OOM to kill it. Hence the sleep 1 after the kill -s USR $pid.Now this code hammers the
memcg_processwith SIGKILL instead.Quoted 7 lines
Quoted 3 lines
As far as I can tell the right thing to do here is to wait with reasonable timeout for the
memcg_processto become zombie and only kill it if that hasn't happened. Or did I miss something?No, you didn't miss anything. I was planning to use 'kill' to check whether the pid is alive or not. But I should have used 'kill -s 0' instead of plain 'kill'.
Quoted 6 lines
Would that even work? Technically till you wait the process the pid still exists albeit in a zombie state.
And looking into POSIX there were some systems that returned ESRCH in this case but it looks this behavior is strongly discouraged.
I see that if I issue 'command &' in bash, it setups a handler for SIGCHLD signal, and executes a wait() in this handler. I.e. leaves no zombies on the system.
All I found in POSIX is that "APPLICATION USAGE" from
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/wait.html
states that "This volume of POSIX.1-2008 requires the implementation to keep the status of terminated jobs available until the status is requested,...". The shell can get the status of terminated jobs only after it calls wait() on them. However, it is still not clear whether wait() must be called in a SIGCHLD handler, or when the 'wait' built-in is executed.
So, indeed, the approach with 'kill -s 0' may not be standard.
I'll switch to using 'ps' then, i.e. if there is memcg_process_stress listed as a zombie (Z) or there is no such process, it means that it was killed by the OOM handler, i.e. everything is fine.
Thanks.
PATCH v2 1/1 memcg/functional: check several times if the process is killed
On some systems it may take slightly more than one second to kill the memcg_process. So let's check several times if the process is alive.
Also removed sleep() before moving the process to the memory cgroup, since this looks reduntant.
---
Changes since V1:
* now we verify wheter the process was killed by checking the output from 'ps'. If it reports no pid or the pid is a zombie, we treat it as the fact that the process was killed by OOM-killer.
* pid_exists -> tpk_pid_exists
.../controllers/memcg/functional/memcg_lib.sh | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-)
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +11 -4
@@ -220,14 +220,21 @@ test_proc_kill() $TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1 echo $pid > tasks kill -s USR1 $pid 2> /dev/null - sleep 1 - ps -p $pid > /dev/null 2> /dev/null - if [ $? -ne 0 ]; then + tpk_pid_exists=1 + for tpk_iter in $(seq 5); do + tpk_state=$(ps -o state= -p $pid) + if [ -z "$tpk_state" ] || echo "$tpk_state" | grep -q Z; then + tpk_pid_exists=0 + break + fi + sleep 1 + done + + if [ $tpk_pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error"
Signature · 1 line
1.7.1
PATCH v2 1/1 memcg/functional: check several times if the process is killed
Hi!
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 233
Quoted 7 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +7
-0+ tpk_pid_exists=1 + for tpk_iter in $(seq 5); do + tpk_state=$(ps -o state= -p $pid) + if [ -z "$tpk_state" ] || echo "$tpk_state" | grep -q Z; then + tpk_pid_exists=0 + break + fi
We may also poke directly the /proc/$PID/ and /proc/$PID/status something as (beware untested):
if [ ! -d /proc/$PID/ ] || grep -q "Z (zombie)" /proc/$PID/status; then ... fi
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 234
Quoted 1 line
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +1
-0+ sleep 1
And I would change this sleep to at least 1/4s so that we do not waste too much time here.
Quoted 12 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +3
-0+ done + + if [ $tpk_pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error"Signature · 5 lines
1.7.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
In reply to Cyril Hrubis----- Original Message -----
Quoted 26 lines
From: "Cyril Hrubis" <[email protected]>
To: "Stanislav Kholmanskikh" <[email protected]>
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Monday, 23 May, 2016 6:02:57 PM
Subject: Re: [LTP] [PATCH] memcg/functional: check several times if the process is killedHi!
Quoted 19 lines
On some systems it may take slightly more than one second to kill the
memcg_process. So let's check several times if the process is alive.Also removed
sleep()before moving the process to the memory cgroup, since this looks reduntant.Signed-off-by Stanislav Kholmanskikh <[email protected]>--- .../controllers/memcg/functional/memcg_lib.sh | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-0@@ -220,14 +220,20 @@ test_proc_kill()
Quoted 5 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 223
Quoted 3 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-1$TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1This sleep sure is useless.
Isn't it there to make sure, that SIGUSR1 handler had time to set up? (for example with single CPU and sched_child_runs_first == 0)
Quoted 45 lines
Quoted 6 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-2echo $pid > tasks kill -s USR1 $pid 2> /dev/null - sleep 1 -testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 234
Quoted 10 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +8
-2- ps -p $pid > /dev/null 2> /dev/null - if [ $? -ne 0 ]; then + pid_exists=1 + for tpk_iter in $(seq 5); do + if ! kill $pid 2> /dev/null; then + pid_exists=0 + break + fi + sleep 1 + doneThis does no seem right to me. The original code send a SIGUSR1 signal to the
memcg_processwhich caused it to allocate memory which supposedly provokes OOM to kill it. Hence the sleep 1 after the kill -s USR $pid.Now this code hammers the
memcg_processwith SIGKILL instead.As far as I can tell the right thing to do here is to wait with reasonable timeout for the
memcg_processto become zombie and only kill it if that hasn't happened. Or did I miss something?Quoted 10 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +1
-1+ if [ $pid_exists -eq 0 ]; then wait $pid if [ $? -eq 1 ]; then result $FAIL "process $pid is killed by error" --1.7.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp--
Cyril Hrubis
[email protected]--
Mailing list info: https://lists.linux.it/listinfo/ltp
PATCH 1/1 memcg/functional: check several times if the process is killed
Hi!
Quoted 8 lines
Quoted 5 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh test_proc_kill() · line 223
Quoted 3 lines
testcases/kernel/controllers/memcg/functional/memcg_lib.sh +0
-1$TEST_PATH/memcg_process $2 -s $3 & pid=$! - sleep 1This sleep sure is useless.
Isn't it there to make sure, that SIGUSR1 handler had time to set up? (for example with single CPU and
sched_child_runs_first== 0)
Looking at the code, you are right.
We should better change this for something more robust. Given that the memgc_progcess does while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
Quoted 11 lines · Cyril Hrubis
Hi!
Quoted 8 lines
Quoted 5 lines
Review
Quoted 3 lines
$TEST_PATH/memcg_process $2 -s $3 &
pid=$!(quoted diff) +0
-1- sleep 1This sleep sure is useless.
Isn't it there to make sure, that SIGUSR1 handler had time to set up? (for example with single CPU and
sched_child_runs_first== 0)Looking at the code, you are right.
Indeed. Thanks!
Quoted 5 lines
We should better change this for something more robust. Given that the
memgc_progcessdoes while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.
I don't understand why it's more robust. For example, a process can get into the sleep state (interruptible sleep) due to a call to a syscall, no?
So maybe keep this sleep() as is?
PATCH 1/1 memcg/functional: check several times if the process is killed
Hi!
Quoted 7 lines
Quoted 4 lines
We should better change this for something more robust. Given that the
memgc_progcessdoes while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.I don't understand why it's more robust. For example, a process can get into the sleep state (interruptible sleep) due to a call to a syscall, no?
Generally yes, but in this case it's unlikely that opening /dev/zero or calling sigaction() would block.
Quoted 1 line
So maybe keep this
sleep()as is?
I'm Ok with that one as well. But we should rethink and fix it later.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
----- Original Message -----
Quoted 12 lines
From: "Cyril Hrubis" <[email protected]>
To: "Stanislav Kholmanskikh" <[email protected]>
Cc: "Jan Stancek" <[email protected]>, "vasily isaenko" <[email protected]>, [email protected]
Sent: Tuesday, 24 May, 2016 12:15:28 PM
Subject: Re: [LTP] [PATCH] memcg/functional: check several times if the process is killedHi!
Quoted 5 lines
Quoted 4 lines
We should better change this for something more robust. Given that the
memgc_progcessdoes while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.
Quoted 5 lines
Quoted 2 lines
I don't understand why it's more robust. For example, a process can get into the sleep state (interruptible sleep) due to a call to a syscall, no?
Generally yes, but in this case it's unlikely that opening
/dev/zeroor callingsigaction()would block.
There's also some ld setup going on before that. Opening/mapping/reading various config and .so files.
Quoted 4 lines
Quoted 2 lines
So maybe keep this
sleep()as is?I'm Ok with that one as well. But we should rethink and fix it later.
My immediate idea was to let memcg_process open "/tmp/memcg_process_ready" and shell script would monitor /proc/$pid/fd/ until sees that or hits timeout.
Regards,
Jan
PATCH 1/1 memcg/functional: check several times if the process is killed
Hi!
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
We should better change this for something more robust. Given that the
memgc_progcessdoes while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
I don't understand why it's more robust. For example, a process can get into the sleep state (interruptible sleep) due to a call to a syscall, no?
Generally yes, but in this case it's unlikely that opening
/dev/zeroor callingsigaction()would block.There's also some ld setup going on before that. Opening/mapping/reading various config and .so files.
Ah right, I tend to forgot that part.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
So maybe keep this
sleep()as is?I'm Ok with that one as well. But we should rethink and fix it later.
My immediate idea was to let
memcg_processopen "/tmp/memcg_process_ready" and shell script would monitor/proc/$pid/fd/until sees that or hits timeout.
Or we can reuse the checkpoint library, given that the memory is now backed by tmpfs and path to the file si passed in environment it should be relatively easy to write a helper binary so that we can use it from the shell as well.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 memcg/functional: check several times if the process is killed
Quoted 21 lines · Cyril Hrubis
Hi!
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
We should better change this for something more robust. Given that the
memgc_progcessdoes while (!flag_exit) sleep(1); in the main loop we may as well wait till the process gets into the sleep state.Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
I don't understand why it's more robust. For example, a process can get into the sleep state (interruptible sleep) due to a call to a syscall, no?
Generally yes, but in this case it's unlikely that opening
/dev/zeroor callingsigaction()would block.There's also some ld setup going on before that. Opening/mapping/reading various config and .so files.
Ah right, I tend to forgot that part.
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
So maybe keep this
sleep()as is?I'm Ok with that one as well. But we should rethink and fix it later.
Quoted 8 lines
Quoted 2 lines
My immediate idea was to let
memcg_processopen "/tmp/memcg_process_ready" and shell script would monitor/proc/$pid/fd/until sees that or hits timeout.Or we can reuse the checkpoint library, given that the memory is now backed by tmpfs and path to the file si passed in environment it should be relatively easy to write a helper binary so that we can use it from the shell as well.
memcg_lib.sh has 22 instances of 'sleep 1', which are used in scenarios:
* it starts memcg_process_stress and 'sleep 1' (presumably) until the
process finishes setting up signal signal handlers
* it does 'sleep 1' after it sends USR1 to the memcg_process_stress. I
believe it's just to make sure that the sigusr handler finished its job
It seems that both the scenarios could modified to use a different form of communication not involving 'sleep'.
Let my try the proposed checkpoint library here. I think I'll come up with something later this week.
Thanks.