diff mbox series

[v2,6/6] zram: Increase timeout according to used devices

Message ID 20210129194144.31299-7-pvorel@suse.cz
State New
Headers show
Series zram cleanup | expand

Commit Message

Petr Vorel Jan. 29, 2021, 7:41 p.m. UTC
to avoid unexpected timeout, which occurred even on just 4 devices.
Default setup is 300, using 200 should be safe.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New in v2.

 testcases/kernel/device-drivers/zram/zram_lib.sh | 2 ++
 1 file changed, 2 insertions(+)

Comments

Petr Vorel Jan. 29, 2021, 8:10 p.m. UTC | #1
Hi,

> +	TST_TIMEOUT=$((dev_num*200))
Actually on heavy loaded machine this is not enough due BTRFS.
I can add something like dev_num*600 or even -1 (then previous commit would not
be needed, but IMHO still useful).

Kind regards,
Petr
Petr Vorel Jan. 29, 2021, 8:15 p.m. UTC | #2
> Hi,

> > +	TST_TIMEOUT=$((dev_num*200))
> Actually on heavy loaded machine this is not enough due BTRFS.
> I can add something like dev_num*600 or even -1 (then previous commit would not
> be needed, but IMHO still useful).
And bad thing is that it breaks other zram tests, because the timer probably
does not allow to run the cleanup:

_tst_setup_timer()
{
...
	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &

I'm not sure if shell allow us to do it better. Maybe sent different signal than
SIGKILL and define 'trap _tst_do_exit' for that signal?

Kind regards,
Petr
Li Wang Jan. 30, 2021, 8:26 a.m. UTC | #3
On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi,
>
> > > +   TST_TIMEOUT=$((dev_num*200))
> > Actually on heavy loaded machine this is not enough due BTRFS.
> > I can add something like dev_num*600 or even -1 (then previous commit
> would not
> > be needed, but IMHO still useful).
>

I personally think -1 is better.



> And bad thing is that it breaks other zram tests, because the timer
> probably
> does not allow to run the cleanup:
>
> _tst_setup_timer()
> {
> ...
>         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> -$pid &
>
> I'm not sure if shell allow us to do it better. Maybe sent different
> signal than
> SIGKILL and define 'trap _tst_do_exit' for that signal?


Sounds practicable. I guess sending SIGINT could make more sense, since
sometimes we use CTRL+C stop test in debugging by manual, test should
do cleanup work for that behavior too.
Petr Vorel Feb. 2, 2021, 7:38 a.m. UTC | #4
Hi Li,

> On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:

> > > Hi,

> > > > +   TST_TIMEOUT=$((dev_num*200))
> > > Actually on heavy loaded machine this is not enough due BTRFS.
> > > I can add something like dev_num*600 or even -1 (then previous commit
> > would not
> > > be needed, but IMHO still useful).


> I personally think -1 is better.
OK. In that case we might avoid now unneeded previous commit.



> > And bad thing is that it breaks other zram tests, because the timer
> > probably
> > does not allow to run the cleanup:

> > _tst_setup_timer()
> > {
> > ...
> >         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> > running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> > -$pid &

> > I'm not sure if shell allow us to do it better. Maybe sent different
> > signal than
> > SIGKILL and define 'trap _tst_do_exit' for that signal?


> Sounds practicable. I guess sending SIGINT could make more sense, since
> sometimes we use CTRL+C stop test in debugging by manual, test should
> do cleanup work for that behavior too.
We have already SIGINT defined for main shell process:
trap "tst_brk TBROK 'test interrupted'" INT

so CTRL+C is covered. So maybe run first SIGINT and then SIGKILL for safety
reasons?

Kind regards,
Petr

diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
index 4fa1674cd..61a6fbcdd 100644
--- testcases/lib/tst_test.sh
+++ testcases/lib/tst_test.sh
@@ -459,7 +459,7 @@ _tst_setup_timer()
 
 	tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
 
-	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &
+	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && { kill -2 -$pid; sleep 5; kill -9 -$pid; } &
 
 	_tst_setup_timer_pid=$!
 }
Li Wang Feb. 2, 2021, 11:25 a.m. UTC | #5
Hi Petr,

On Tue, Feb 2, 2021 at 3:38 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > > Hi,
>
> > > > > +   TST_TIMEOUT=$((dev_num*200))
> > > > Actually on heavy loaded machine this is not enough due BTRFS.
> > > > I can add something like dev_num*600 or even -1 (then previous commit
> > > would not
> > > > be needed, but IMHO still useful).
>
>
> > I personally think -1 is better.
> OK. In that case we might avoid now unneeded previous commit.
>
>
>
> > > And bad thing is that it breaks other zram tests, because the timer
> > > probably
> > > does not allow to run the cleanup:
>
> > > _tst_setup_timer()
> > > {
> > > ...
> > >         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> > > running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> > > -$pid &
>
> > > I'm not sure if shell allow us to do it better. Maybe sent different
> > > signal than
> > > SIGKILL and define 'trap _tst_do_exit' for that signal?
>
>
> > Sounds practicable. I guess sending SIGINT could make more sense, since
> > sometimes we use CTRL+C stop test in debugging by manual, test should
> > do cleanup work for that behavior too.
> We have already SIGINT defined for main shell process:
> trap "tst_brk TBROK 'test interrupted'" INT
>
> so CTRL+C is covered. So maybe run first SIGINT and then SIGKILL for safety
> reasons?
>

I'm fine with using SIGINT + SIGKILL.

Or, maybe another choice is to catch a signal SIGTERM?  Once we
cancel the specified signal in KILL command, it will try with SIGTERM
by default, if the test process can not be terminated, then a SIGKILL
may be sent.

--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -22,6 +22,7 @@ export TST_LIB_LOADED=1

 # default trap function
 trap "tst_brk TBROK 'test interrupted'" INT
+trap "_tst_do_exit" TERM

 _tst_do_exit()
 {
@@ -459,7 +460,7 @@ _tst_setup_timer()

        tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"

-       sleep $sec && tst_res TBROK "test killed, timeout! If you are
running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
-$pid &
+       sleep $sec && tst_res TBROK "test terminated, timeout! If you are
running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill --
-$pid &

        _tst_setup_timer_pid=$!
diff mbox series

Patch

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 26ed1846b..c24b83cfc 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -47,6 +47,8 @@  zram_load()
 		tst_brk TBROK "dev_num must be > 0"
 	fi
 
+	TST_TIMEOUT=$((dev_num*200))
+
 	tst_res TINFO "create '$dev_num' zram device(s)"
 
 	modprobe zram num_devices=$dev_num || \