[LTP] [PATCH] memcg/functional: check several times if the process is killed

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

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 <stanislav.kholmanskikh@oracle.com>
---
 .../controllers/memcg/functional/memcg_lib.sh      |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
index 9b9b0fd..93c61a1 100755
--- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
+++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
@@ -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"
-- 
1.7.1

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

Hi!
> 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 <stanislav.kholmanskikh@oracle.com>
> ---
>  .../controllers/memcg/functional/memcg_lib.sh      |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> index 9b9b0fd..93c61a1 100755
> --- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> +++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> @@ -220,14 +220,20 @@ test_proc_kill()
>  
>  	$TEST_PATH/memcg_process $2 -s $3 &
>  	pid=$!
> -	sleep 1

This sleep sure is useless.

>  	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

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?

> +	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
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

On 05/23/2016 07:02 PM, Cyril Hrubis wrote:
> Hi!
>> 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 <stanislav.kholmanskikh@oracle.com>
>> ---
>>   .../controllers/memcg/functional/memcg_lib.sh      |   14 ++++++++++----
>>   1 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
>> index 9b9b0fd..93c61a1 100755
>> --- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
>> +++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
>> @@ -220,14 +220,20 @@ test_proc_kill()
>>
>>   	$TEST_PATH/memcg_process $2 -s $3 &
>>   	pid=$!
>> -	sleep 1
>
> This sleep sure is useless.
>
>>   	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
>
> 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?

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.

>
>> +	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
>

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

On 05/23/2016 07:39 PM, Cyril Hrubis wrote:
> Hi!
>>> 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?
>>
>> 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.
 >

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.

[LTP] [PATCH V2] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

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 <stanislav.kholmanskikh@oracle.com>
---
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(-)

diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
index 9b9b0fd..5662312 100755
--- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
+++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
@@ -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"
-- 
1.7.1

[LTP] [PATCH V2] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

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

> +		sleep 1

And I would change this sleep to at least 1/4s so that we do not waste
too much time here.

> +	done
> +
> +	if [ $tpk_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
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Stanislav Kholmanskikh" <stanislav.kholmanskikh@oracle.com>
> Cc: "vasily isaenko" <vasily.isaenko@oracle.com>, ltp@lists.linux.it
> Sent: Monday, 23 May, 2016 6:02:57 PM
> Subject: Re: [LTP] [PATCH] memcg/functional: check several times if the process is killed
> 
> Hi!
> > 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 <stanislav.kholmanskikh@oracle.com>
> > ---
> >  .../controllers/memcg/functional/memcg_lib.sh      |   14 ++++++++++----
> >  1 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> > b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> > index 9b9b0fd..93c61a1 100755
> > --- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> > +++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> > @@ -220,14 +220,20 @@ test_proc_kill()
> >  
> >  	$TEST_PATH/memcg_process $2 -s $3 &
> >  	pid=$!
> > -	sleep 1
> 
> This 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)

> 
> >  	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
> 
> 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?
> 
> > +	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
> chrubis@suse.cz
> 
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
> 

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

Hi!
> > >  	$TEST_PATH/memcg_process $2 -s $3 &
> > >  	pid=$!
> > > -	sleep 1
> > 
> > This 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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

On 05/24/2016 12:18 PM, Cyril Hrubis wrote:
> Hi!
>>>>   	$TEST_PATH/memcg_process $2 -s $3 &
>>>>   	pid=$!
>>>> -	sleep 1
>>>
>>> This 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!


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

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?

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

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

> So maybe keep this sleep() as is?

I'm Ok with that one as well. But we should rethink and fix it later.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Stanislav Kholmanskikh" <stanislav.kholmanskikh@oracle.com>
> Cc: "Jan Stancek" <jstancek@redhat.com>, "vasily isaenko" <vasily.isaenko@oracle.com>, ltp@lists.linux.it
> Sent: Tuesday, 24 May, 2016 12:15:28 PM
> Subject: Re: [LTP] [PATCH] memcg/functional: check several times if the process is killed
> 
> Hi!
> > > 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.
> > >
> > 
> > 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.

There's also some ld setup going on before that. Opening/mapping/reading
various config and .so files.

> 
> > 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

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Cyril Hrubis]

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

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

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] memcg/functional: check several times if the process is killed

[Stanislav Kholmanskikh]

On 05/24/2016 01:48 PM, Cyril Hrubis wrote:
> Hi!
>>>>> 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.
>>>>>
>>>>
>>>> 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.
>>
>> 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.
>
>>>> 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.
>
> 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.