* [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions
@ 2019-07-18 8:39 Petr Vorel
2019-07-18 8:39 ` [LTP] [PATCH 2/2] c: " Petr Vorel
2019-07-19 4:56 ` [LTP] [PATCH 1/2] shell: " Li Wang
0 siblings, 2 replies; 6+ messages in thread
From: Petr Vorel @ 2019-07-18 8:39 UTC (permalink / raw)
To: ltp
in TST_RETRY_FN_EXP_BACKOFF() and thus in TST_RETRY_FUNC() in new shell API.
This function should also address possibility of slow machine.
Because using on 2 places moved the default definition to the beginning
of the script.
+ use $((...)) instead of expr.
+ move expression using $3 after check whether we have enough parameters
+ document LTP_TIMEOUT_MUL environment variable in wiki.
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi,
I can squash these 2 commits.
General question about Test Writing Guidelines in wiki [1]:
1) we have separate section for C and shell API, which is probably a good
choice, but we're trying to sync them. That leads to things like retry
function documented under chapter for shell API (2.3).
+ That page is too long and without table of contents (not easy to
navigate in it; I tried to add TOC, but without success). Maybe adding
TOC or split the page to 3: 1) general info 2) C API 3) shell API would
help. WDYT?
Kind regards,
Petr
[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
[2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#retry-a-function-in-limited-time
doc/test-writing-guidelines.txt | 2 ++
testcases/lib/tst_test.sh | 9 +++++----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 869e6ed35..782e14f32 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1881,6 +1881,8 @@ simply by setting right '$TST_NEEDS_FOO'.
the test (see below).
| 'TST_NEEDS_MODULE' | Test module name needed for the test (see below).
| 'TST_NEEDS_DRIVERS'| Checks kernel drivers support for the test.
+| 'LTP_TIMEOUT_MUL' | Multiply timeout (> 1 is useful for slow machines to
+ avoid unexpected timeout).
|=============================================================================
Checking for presence of commands
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 31b3a3951..60d765424 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -17,6 +17,8 @@ export TST_ITERATIONS=1
export TST_TMPDIR_RHOST=0
export TST_LIB_LOADED=1
+export LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
+
. tst_ansi_color.sh
. tst_security.sh
@@ -164,12 +166,13 @@ TST_RETRY_FN_EXP_BACKOFF()
{
local tst_fun="$1"
local tst_exp=$2
- local tst_sec=$(expr $3 \* 1000000)
local tst_delay=1
+ local tst_sec
if [ $# -ne 3 ]; then
tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF expects 3 parameters"
fi
+ tst_sec=$(($3 * LTP_TIMEOUT_MUL * 1000000))
if ! tst_is_int "$tst_sec"; then
tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF: tst_sec must be integer ('$tst_sec')"
@@ -185,7 +188,7 @@ TST_RETRY_FN_EXP_BACKOFF()
tst_sleep ${tst_delay}us
tst_delay=$((tst_delay*2))
else
- tst_brk TBROK "\"$tst_fun\" timed out"
+ tst_brk TBROK "\"$tst_fun\" timed out! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
fi
done
@@ -374,8 +377,6 @@ _tst_rescmp()
_tst_setup_timer()
{
- LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
-
local sec=$((300 * LTP_TIMEOUT_MUL))
local h=$((sec / 3600))
local m=$((sec / 60 % 60))
--
2.22.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [LTP] [PATCH 2/2] c: Use $LTP_TIMEOUT_MUL also in retry functions
2019-07-18 8:39 [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions Petr Vorel
@ 2019-07-18 8:39 ` Petr Vorel
2019-07-19 5:02 ` Li Wang
2019-07-19 4:56 ` [LTP] [PATCH 1/2] shell: " Li Wang
1 sibling, 1 reply; 6+ messages in thread
From: Petr Vorel @ 2019-07-18 8:39 UTC (permalink / raw)
To: ltp
in TST_RETRY_FN_EXP_BACKOFF() and thus in TST_RETRY_FUNC() in new C API.
This sync C API with shell API (added in previous commit).
+ mention LTP_TIMEOUT_MUL use in wiki.
+ use SPDX, add copyright
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
doc/test-writing-guidelines.txt | 2 ++
include/tst_common.h | 26 +++++++++++---------------
2 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 782e14f32..3548d6043 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2064,6 +2064,8 @@ Sometimes LTP test needs retrying a function for many times to get success.
This achievement makes that possible via keeping it retrying if the return
value of the function is NOT as we expected. After exceeding a limited time,
test will break from the retries immediately.
+NOTE: both 'TST_RETRY_FUNC()' and 'TST_RETRY_FN_EXP_BACKOFF()' use 'LTP_TIMEOUT_MUL'
+environment variable.
[source,c]
-------------------------------------------------------------------------------
diff --git a/include/tst_common.h b/include/tst_common.h
index c21505450..454a166e8 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -1,21 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
/*
+ * Copyright (c) Linux Test Project, 2017-2019
* Copyright (c) 2016 Cyril Hrubis <chrubis@suse.cz>
* Copyright (c) 2013 Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
* Copyright (c) 2010 Ngie Cooper <yaneurabeya@gmail.com>
* Copyright (c) 2008 Mike Frysinger <vapier@gmail.com>
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#ifndef TST_COMMON_H__
@@ -51,15 +40,22 @@
#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY) \
({ int tst_delay_ = 1; \
+ float m = 1; \
+ char *mul = getenv("LTP_TIMEOUT_MUL"); \
+ if (mul) { \
+ m = atof(mul); \
+ if (m < 1) \
+ tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \
+ } \
for (;;) { \
typeof(FUNC) tst_ret_ = FUNC; \
if (tst_ret_ == ERET) \
break; \
- if (tst_delay_ < MAX_DELAY * 1000000) { \
+ if (tst_delay_ < MAX_DELAY * m * 1000000) { \
usleep(tst_delay_); \
tst_delay_ *= 2; \
} else { \
- tst_brk(TBROK, #FUNC" timed out"); \
+ tst_brk(TBROK, #FUNC" timed out! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"); \
} \
} \
ERET; \
--
2.22.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [LTP] [PATCH 2/2] c: Use $LTP_TIMEOUT_MUL also in retry functions
2019-07-18 8:39 ` [LTP] [PATCH 2/2] c: " Petr Vorel
@ 2019-07-19 5:02 ` Li Wang
2019-07-19 9:15 ` Petr Vorel
0 siblings, 1 reply; 6+ messages in thread
From: Li Wang @ 2019-07-19 5:02 UTC (permalink / raw)
To: ltp
On Thu, Jul 18, 2019 at 4:40 PM Petr Vorel <pvorel@suse.cz> wrote:
>...
> #ifndef TST_COMMON_H__
> @@ -51,15 +40,22 @@
>
> #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY) \
> ({ int tst_delay_ = 1; \
> + float m = 1; \
> + char *mul = getenv("LTP_TIMEOUT_MUL"); \
We also need a prefix/suffix in the variable definition to make sure
that it will
not alias with anything that has been passed to the FUNC, just like what we do
for the tst_delay_.
e.g. if the FUNC is defined as foo_func(m); the m variable will be aliased and
the function will do something very unexpected.
> + if (mul) { \
> + m = atof(mul); \
> + if (m < 1) \
> + tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \
If we reverse some code order in tst_set_timeout() function, then here
we have no need to check if m < 1 again, since the LTP_TIMEOUT_MUL
valid check will be finished in setup() early phase.
(This comment is just FYI, and I also think it's OK to check twice.)
--------------------------------------
void tst_set_timeout(int timeout)
{
float m = 1;
char *mul = getenv("LTP_TIMEOUT_MUL");
if (mul) {
m = atof(mul);
if (m < 1)
tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
}
if (timeout == -1) {
tst_res(TINFO, "Timeout per run is disabled");
return;
}
results->timeout = timeout * m + 0.5;
tst_res(TINFO, "Timeout per run is %uh %02um %02us",
results->timeout/3600, (results->timeout%3600)/60,
results->timeout % 60);
if (getpid() == lib_pid)
alarm(results->timeout);
else
heartbeat();
}
--
Regards,
Li Wang
^ permalink raw reply [flat|nested] 6+ messages in thread* [LTP] [PATCH 2/2] c: Use $LTP_TIMEOUT_MUL also in retry functions
2019-07-19 5:02 ` Li Wang
@ 2019-07-19 9:15 ` Petr Vorel
0 siblings, 0 replies; 6+ messages in thread
From: Petr Vorel @ 2019-07-19 9:15 UTC (permalink / raw)
To: ltp
Hi Li,
thanks for your review!
> On Thu, Jul 18, 2019 at 4:40 PM Petr Vorel <pvorel@suse.cz> wrote:
> >...
> > #ifndef TST_COMMON_H__
> > @@ -51,15 +40,22 @@
> > #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY) \
> > ({ int tst_delay_ = 1; \
> > + float m = 1; \
> > + char *mul = getenv("LTP_TIMEOUT_MUL"); \
> We also need a prefix/suffix in the variable definition to make sure
> that it will
> not alias with anything that has been passed to the FUNC, just like what we do
> for the tst_delay_.
> e.g. if the FUNC is defined as foo_func(m); the m variable will be aliased and
> the function will do something very unexpected.
Good point, I'll fix it in v3.
> > + if (mul) { \
> > + m = atof(mul); \
> > + if (m < 1) \
> > + tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \
> If we reverse some code order in tst_set_timeout() function, then here
> we have no need to check if m < 1 again, since the LTP_TIMEOUT_MUL
> valid check will be finished in setup() early phase.
> (This comment is just FYI, and I also think it's OK to check twice.)
Good point. I'd probably check twice in case the logic changes one day.
> --------------------------------------
> void tst_set_timeout(int timeout)
> {
> float m = 1;
> char *mul = getenv("LTP_TIMEOUT_MUL");
> if (mul) {
> m = atof(mul);
> if (m < 1)
> tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
> }
> if (timeout == -1) {
> tst_res(TINFO, "Timeout per run is disabled");
> return;
> }
> results->timeout = timeout * m + 0.5;
> tst_res(TINFO, "Timeout per run is %uh %02um %02us",
> results->timeout/3600, (results->timeout%3600)/60,
> results->timeout % 60);
> if (getpid() == lib_pid)
> alarm(results->timeout);
> else
> heartbeat();
> }
Kind regards,
Petr
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions
2019-07-18 8:39 [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions Petr Vorel
2019-07-18 8:39 ` [LTP] [PATCH 2/2] c: " Petr Vorel
@ 2019-07-19 4:56 ` Li Wang
2019-07-19 9:01 ` Petr Vorel
1 sibling, 1 reply; 6+ messages in thread
From: Li Wang @ 2019-07-19 4:56 UTC (permalink / raw)
To: ltp
On Thu, Jul 18, 2019 at 4:39 PM Petr Vorel <pvorel@suse.cz> wrote:
> in TST_RETRY_FN_EXP_BACKOFF() and thus in TST_RETRY_FUNC() in new shell
> API.
>
> This function should also address possibility of slow machine.
>
> Because using on 2 places moved the default definition to the beginning
> of the script.
> + use $((...)) instead of expr.
> + move expression using $3 after check whether we have enough parameters
>
> + document LTP_TIMEOUT_MUL environment variable in wiki.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi,
>
> I can squash these 2 commits.
>
> General question about Test Writing Guidelines in wiki [1]:
> 1) we have separate section for C and shell API, which is probably a good
> choice, but we're trying to sync them. That leads to things like retry
> function documented under chapter for shell API (2.3).
>
Thanks for point out this.
>
> + That page is too long and without table of contents (not easy to
> navigate in it; I tried to add TOC, but without success). Maybe adding
> TOC or split the page to 3: 1) general info 2) C API 3) shell API would
> help. WDYT?
>
Personally, I think adding TOC may be a better choice, since we don't have
the whole picture for the guidelines and usually hard to navigate in it.
To split the guideline that probably leads to the new LTP-user to read more
document pages. That's an annoying thing for me at least :).
>
> Kind regards,
> Petr
>
> [1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
> [2]
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#retry-a-function-in-limited-time
>
> doc/test-writing-guidelines.txt | 2 ++
> testcases/lib/tst_test.sh | 9 +++++----
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/doc/test-writing-guidelines.txt
> b/doc/test-writing-guidelines.txt
> index 869e6ed35..782e14f32 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -1881,6 +1881,8 @@ simply by setting right '$TST_NEEDS_FOO'.
> the test (see below).
> | 'TST_NEEDS_MODULE' | Test module name needed for the test (see below).
> | 'TST_NEEDS_DRIVERS'| Checks kernel drivers support for the test.
> +| 'LTP_TIMEOUT_MUL' | Multiply timeout (> 1 is useful for slow machines
> to
> + avoid unexpected timeout).
>
> |=============================================================================
>
> Checking for presence of commands
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 31b3a3951..60d765424 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -17,6 +17,8 @@ export TST_ITERATIONS=1
> export TST_TMPDIR_RHOST=0
> export TST_LIB_LOADED=1
>
> +export LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
> +
> . tst_ansi_color.sh
> . tst_security.sh
>
> @@ -164,12 +166,13 @@ TST_RETRY_FN_EXP_BACKOFF()
> {
> local tst_fun="$1"
> local tst_exp=$2
> - local tst_sec=$(expr $3 \* 1000000)
> local tst_delay=1
> + local tst_sec
>
> if [ $# -ne 3 ]; then
> tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF expects 3
> parameters"
> fi
> + tst_sec=$(($3 * LTP_TIMEOUT_MUL * 1000000))
> if ! tst_is_int "$tst_sec"; then
> tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF: tst_sec must be
> integer ('$tst_sec')"
> @@ -185,7 +188,7 @@ TST_RETRY_FN_EXP_BACKOFF()
> tst_sleep ${tst_delay}us
> tst_delay=$((tst_delay*2))
> else
> - tst_brk TBROK "\"$tst_fun\" timed out"
> + tst_brk TBROK "\"$tst_fun\" timed out! If you are
> running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
> fi
> done
>
> @@ -374,8 +377,6 @@ _tst_rescmp()
>
> _tst_setup_timer()
> {
> - LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
> -
> local sec=$((300 * LTP_TIMEOUT_MUL))
>
Maybe we need to check if the LTP_TIMEOUT_MUL is valid(>1) as what we do
for C API?
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190719/ef9fca12/attachment-0001.htm>
^ permalink raw reply [flat|nested] 6+ messages in thread* [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions
2019-07-19 4:56 ` [LTP] [PATCH 1/2] shell: " Li Wang
@ 2019-07-19 9:01 ` Petr Vorel
0 siblings, 0 replies; 6+ messages in thread
From: Petr Vorel @ 2019-07-19 9:01 UTC (permalink / raw)
To: ltp
Hi Li,
> > + That page is too long and without table of contents (not easy to
> > navigate in it; I tried to add TOC, but without success). Maybe adding
> > TOC or split the page to 3: 1) general info 2) C API 3) shell API would
> > help. WDYT?
> Personally, I think adding TOC may be a better choice, since we don't have
> the whole picture for the guidelines and usually hard to navigate in it.
> To split the guideline that probably leads to the new LTP-user to read more
> document pages. That's an annoying thing for me at least :).
Agree. I'll give TOC next try sooner or later :).
...
> > _tst_setup_timer()
> > {
> > - LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
> > -
> > local sec=$((300 * LTP_TIMEOUT_MUL))
> Maybe we need to check if the LTP_TIMEOUT_MUL is valid(>1) as what we do
> for C API?
Correct, sorry for omitting that => v3 needed.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-19 9:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-18 8:39 [LTP] [PATCH 1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions Petr Vorel
2019-07-18 8:39 ` [LTP] [PATCH 2/2] c: " Petr Vorel
2019-07-19 5:02 ` Li Wang
2019-07-19 9:15 ` Petr Vorel
2019-07-19 4:56 ` [LTP] [PATCH 1/2] shell: " Li Wang
2019-07-19 9:01 ` Petr Vorel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox