| Message ID | 20211025160134.9283-7-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce a concept of test max_runtime | expand |
On Tue, Oct 26, 2021 at 12:02 AM Cyril Hrubis <chrubis@suse.cz> wrote: > Rarely there is a need to set the test runtime dynamically, the only > tests in LTP that does this are the timer tests that can get two > parameters, number of iterations and sleep time, and the test runtime is > close to the multiplication of these two. > > It's still cleaner to set the runtime and let the test library figure > out the timeout in this case. > If so, should we consider to hinden the .timeout in struct tst_test to prevent users from changing it? IIRC, we currently have ".timeout == -1" to disable test timed out in unsure situation, e.g some OOM tests. But in this patch, I saw you remove that, but not handle it in tst_set_runtime. > > Also when no parameters are passed to these tests the runtime is a sum > of multiplications from the tst_timer_test.c source so we define a > constant in the tst_timer_test.h header and set the max_runtime in the > testcases accordingly. With this we get correct estimate for the test > runtime and tighter, but still forgiving enough, timeout as well. >
Hi! > > Rarely there is a need to set the test runtime dynamically, the only > > tests in LTP that does this are the timer tests that can get two > > parameters, number of iterations and sleep time, and the test runtime is > > close to the multiplication of these two. > > > > It's still cleaner to set the runtime and let the test library figure > > out the timeout in this case. > > > > If so, should we consider to hinden the .timeout in struct tst_test > to prevent users from changing it? If we decide to apply this patchset that would be logical end result. There are only a few .timeout = foo left in the codebase after this patchset that either disable timeout for the few unpredictable cases or shorten it to make the test timeout faster if it gets stuck. We can deal with these by making the .max_runtime accept -1 and by shortening the default timeout considerably. > IIRC, we currently have ".timeout == -1" to disable test timed > out in unsure situation, e.g some OOM tests. But in this patch, > I saw you remove that, but not handle it in tst_set_runtime. Ah, right, I've removed the timeout == -1 handling by mistake. I wanted to keep it working after this patchset as well until a follow up patchset deals with the rest of the tests that set the .timeout.
On Tue, Oct 26, 2021 at 3:13 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > Rarely there is a need to set the test runtime dynamically, the only > > > tests in LTP that does this are the timer tests that can get two > > > parameters, number of iterations and sleep time, and the test runtime > is > > > close to the multiplication of these two. > > > > > > It's still cleaner to set the runtime and let the test library figure > > > out the timeout in this case. > > > > > > > If so, should we consider to hinden the .timeout in struct tst_test > > to prevent users from changing it? > > If we decide to apply this patchset that would be logical end result. > There are only a few .timeout = foo left in the codebase after this > patchset that either disable timeout for the few unpredictable cases or > shorten it to make the test timeout faster if it gets stuck. We can deal > with these by making the .max_runtime accept -1 and by shortening the > default timeout considerably. > Yes, that should be great. After a quick reviewing the whole patchset, I feel that .timeout is redundant since .max_runtime can do more thing to totally replace it by the end. ---------------- Btw, it looks weird to simply double the runtime by plus MAX(10u, runtime) in the runtime_to_timeout, I guess you probably just wanna another 10sec for some reclaiming work. And the .max_runtime is also maximal time per test iteration, but from the output below misleading me to think it is for the whole test time. See: # LTP_TIMEOUT_MUL=1 ./pty03 tst_test.c:1376: TINFO: Test max runtime 360s tst_test.c:1371: TINFO: Timeout per run is 0h 12m 00s .... > > > IIRC, we currently have ".timeout == -1" to disable test timed > > out in unsure situation, e.g some OOM tests. But in this patch, > > I saw you remove that, but not handle it in tst_set_runtime. > > Ah, right, I've removed the timeout == -1 handling by mistake. I wanted > to keep it working after this patchset as well until a follow up > patchset deals with the rest of the tests that set the .timeout. > Sound good.
Hi! > Yes, that should be great. > > After a quick reviewing the whole patchset, I feel that .timeout is > redundant since .max_runtime can do more thing to totally replace > it by the end. Agreed. > ---------------- > > Btw, it looks weird to simply double the runtime by plus MAX(10u, runtime) > in the runtime_to_timeout, I guess you probably just wanna another > 10sec for some reclaiming work. The exact formula is up for a discussion, but I do not think that we should make it too tight, it's just a safety that is not going to be triggered unless there is a real bug. > And the .max_runtime is also maximal time per test iteration, > but from the output below misleading me to think it is for the > whole test time. > > See: > > # LTP_TIMEOUT_MUL=1 ./pty03 > tst_test.c:1376: TINFO: Test max runtime 360s > tst_test.c:1371: TINFO: Timeout per run is 0h 12m 00s > .... Ah, right, that should be fixed. Anything else that should be fixed? Also once we get this merged as well as the preprocessor support for the metadata extractor it's going to be fairly easy to write a short script that parses the metadata and then prints overall test runtime given a test name, which is what I'm going to send once these two patchsets are merged.
> > Ah, right, that should be fixed. Anything else that should be fixed? > No more from my side, the rest looks quite good.
diff --git a/include/tst_test.h b/include/tst_test.h index a9746b440..3348eda96 100644 --- a/include/tst_test.h +++ b/include/tst_test.h @@ -309,8 +309,14 @@ const char *tst_strstatus(int status); */ unsigned int tst_remaining_runtime(void); +/* + * Sets the test runtime dymamically, this is supposed to be used from a test + * setup() in cases where the test runtime depends on parameters passed to the + * test. + */ +void tst_set_runtime(unsigned int runtime); + unsigned int tst_multiply_timeout(unsigned int timeout); -void tst_set_timeout(int timeout); /* * Returns path to the test temporary directory in a newly allocated buffer. diff --git a/include/tst_timer_test.h b/include/tst_timer_test.h index b825a4d1a..2a863391a 100644 --- a/include/tst_timer_test.h +++ b/include/tst_timer_test.h @@ -39,6 +39,9 @@ void tst_timer_sample(void); +/* This is a bit more than the sum of all the iterations the test does */ +#define TST_TIMER_TEST_MAX_RUNTIME 10 + # ifdef TST_NO_DEFAULT_MAIN struct tst_test *tst_timer_test_setup(struct tst_test *test); # endif /* TST_NO_DEFAULT_MAIN */ diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore index 403076ba5..1a33d8967 100644 --- a/lib/newlib_tests/.gitignore +++ b/lib/newlib_tests/.gitignore @@ -48,3 +48,4 @@ tst_fuzzy_sync03 test_zero_hugepage test_max_runtime01 test_max_runtime02 +test_max_runtime03 diff --git a/lib/newlib_tests/test_max_runtime03.c b/lib/newlib_tests/test_max_runtime03.c new file mode 100644 index 000000000..5d1c1099c --- /dev/null +++ b/lib/newlib_tests/test_max_runtime03.c @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2021, Linux Test Project + */ + +#include <stdlib.h> +#include <unistd.h> +#include "tst_test.h" + +#define MAX_RUNTIME 3 + +static void run(void) +{ + unsigned int ms = 0; + + do { + usleep(10000); + ms++; + } while (tst_remaining_runtime()); + + if (ms > MAX_RUNTIME * 100) + tst_res(TFAIL, "Slept for longer than 1s %u", tst_remaining_runtime()); + else + tst_res(TPASS, "Timeout remaining: %d, Slept for %ums", tst_remaining_runtime(), 10 * ms); +} + +static void setup(void) +{ + tst_set_runtime(MAX_RUNTIME); +} + +static struct tst_test test = { + .setup = setup, + .test_all = run, +}; diff --git a/lib/tst_test.c b/lib/tst_test.c index 23f55b306..9a1c23dce 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -62,6 +62,7 @@ struct results { int warnings; int broken; unsigned int timeout; + unsigned int max_runtime; }; static struct results *results; @@ -975,8 +976,6 @@ static void do_setup(int argc, char *argv[]) if (tst_test->max_runtime) { if (tst_test->timeout) tst_brk(TBROK, "Only one of timeout and max_iter_runtime can be set!"); - - tst_test->timeout = tst_test->max_runtime + MAX(10u, tst_test->max_runtime); } if (tst_test->needs_root && geteuid() != 0) @@ -1326,8 +1325,8 @@ unsigned int tst_remaining_runtime(void) tst_res(TWARN | TERRNO, "tst_clock_gettime() failed"); elapsed = tst_timespec_diff_ms(now, tst_start_time)/1000; - if (tst_test->max_runtime > elapsed) - return tst_test->max_runtime - elapsed; + if (results->max_runtime > elapsed) + return results->max_runtime - elapsed; return 0; } @@ -1358,16 +1357,13 @@ unsigned int tst_multiply_timeout(unsigned int timeout) return timeout * timeout_mul; } -static void set_timeout(int timeout) +static int runtime_to_timeout(unsigned int runtime) { - if (timeout == -1) { - tst_res(TINFO, "Timeout per run is disabled"); - return; - } - - if (timeout < 1) - tst_brk(TBROK, "timeout must to be >= 1! (%d)", timeout); + return runtime + MAX(10u, runtime); +} +static void set_timeout(int timeout) +{ results->timeout = tst_multiply_timeout(timeout); tst_res(TINFO, "Timeout per run is %uh %02um %02us", @@ -1375,9 +1371,16 @@ static void set_timeout(int timeout) results->timeout % 60); } -void tst_set_timeout(int timeout) +static void set_runtime(unsigned int max_runtime) +{ + tst_res(TINFO, "Test max runtime %us", max_runtime); + results->max_runtime = max_runtime; + set_timeout(runtime_to_timeout(max_runtime)); +} + +void tst_set_runtime(unsigned int max_runtime) { - set_timeout(timeout); + set_runtime(max_runtime); heartbeat(); } @@ -1475,7 +1478,9 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self) SAFE_SIGNAL(SIGALRM, alarm_handler); SAFE_SIGNAL(SIGUSR1, heartbeat_handler); - if (tst_test->timeout) + if (tst_test->max_runtime) + set_runtime(tst_test->max_runtime); + else if (tst_test->timeout) set_timeout(tst_test->timeout); else set_timeout(300); diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c index 3cd52fc9d..19de1ff97 100644 --- a/lib/tst_timer_test.c +++ b/lib/tst_timer_test.c @@ -441,9 +441,9 @@ static void parse_timer_opts(void) if (!sample_cnt) sample_cnt = 500; - long long timeout = sleep_time * sample_cnt / 1000000; + long long runtime = sleep_time * sample_cnt / 1000000; - tst_set_timeout(timeout + timeout/10); + tst_set_runtime(runtime); test->test_all = single_timer_test; test->test = NULL; diff --git a/testcases/kernel/syscalls/clock_nanosleep/clock_nanosleep02.c b/testcases/kernel/syscalls/clock_nanosleep/clock_nanosleep02.c index feb3e4791..656555652 100644 --- a/testcases/kernel/syscalls/clock_nanosleep/clock_nanosleep02.c +++ b/testcases/kernel/syscalls/clock_nanosleep/clock_nanosleep02.c @@ -32,5 +32,6 @@ int sample_fn(int clk_id, long long usec) static struct tst_test test = { .scall = "clock_nanosleep()", + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, .sample = sample_fn, }; diff --git a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait03.c b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait03.c index 2ad1a6abc..19a5ea664 100644 --- a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait03.c +++ b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait03.c @@ -70,5 +70,6 @@ static struct tst_test test = { .sample = sample_fn, .setup = setup, .cleanup = cleanup, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, .test_variants = TEST_VARIANTS, }; diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait02.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait02.c index d2c0b6ef4..5e9986905 100644 --- a/testcases/kernel/syscalls/epoll_wait/epoll_wait02.c +++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait02.c @@ -69,5 +69,6 @@ static struct tst_test test = { .scall = "epoll_wait()", .sample = sample_fn, .setup = setup, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, .cleanup = cleanup, }; diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait04.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait04.c index dc62e9202..623c044d2 100644 --- a/testcases/kernel/syscalls/epoll_wait/epoll_wait04.c +++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait04.c @@ -13,7 +13,7 @@ #include <sys/epoll.h> #include "tst_test.h" -#include "tst_timer_test.h" +#include "tst_timer.h" #define USEC_PRECISION 1000 /* Error margin allowed in usec */ diff --git a/testcases/kernel/syscalls/futex/futex_cmp_requeue01.c b/testcases/kernel/syscalls/futex/futex_cmp_requeue01.c index 13e67c758..03cc544ab 100644 --- a/testcases/kernel/syscalls/futex/futex_cmp_requeue01.c +++ b/testcases/kernel/syscalls/futex/futex_cmp_requeue01.c @@ -15,7 +15,7 @@ #include <linux/futex.h> #include <sys/time.h> -#include "tst_timer_test.h" +#include "tst_timer.h" #include "tst_test.h" #include "futextest.h" diff --git a/testcases/kernel/syscalls/futex/futex_wait05.c b/testcases/kernel/syscalls/futex/futex_wait05.c index 8fad5d858..f7d66133c 100644 --- a/testcases/kernel/syscalls/futex/futex_wait05.c +++ b/testcases/kernel/syscalls/futex/futex_wait05.c @@ -41,4 +41,5 @@ int sample_fn(int clk_id, long long usec) static struct tst_test test = { .scall = "futex_wait()", .sample = sample_fn, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, }; diff --git a/testcases/kernel/syscalls/nanosleep/nanosleep01.c b/testcases/kernel/syscalls/nanosleep/nanosleep01.c index eaacb89fa..9dec2276e 100644 --- a/testcases/kernel/syscalls/nanosleep/nanosleep01.c +++ b/testcases/kernel/syscalls/nanosleep/nanosleep01.c @@ -35,4 +35,5 @@ int sample_fn(int clk_id, long long usec) static struct tst_test test = { .scall = "nanosleep()", .sample = sample_fn, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, }; diff --git a/testcases/kernel/syscalls/poll/poll02.c b/testcases/kernel/syscalls/poll/poll02.c index c0665927b..7e2ae91f4 100644 --- a/testcases/kernel/syscalls/poll/poll02.c +++ b/testcases/kernel/syscalls/poll/poll02.c @@ -55,4 +55,5 @@ static struct tst_test test = { .sample = sample_fn, .setup = setup, .cleanup = cleanup, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, }; diff --git a/testcases/kernel/syscalls/prctl/prctl09.c b/testcases/kernel/syscalls/prctl/prctl09.c index 07ce57063..be5583f98 100644 --- a/testcases/kernel/syscalls/prctl/prctl09.c +++ b/testcases/kernel/syscalls/prctl/prctl09.c @@ -44,4 +44,5 @@ static struct tst_test test = { .setup = setup, .scall = "prctl()", .sample = sample_fn, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, }; diff --git a/testcases/kernel/syscalls/pselect/pselect01.c b/testcases/kernel/syscalls/pselect/pselect01.c index 5b2b8b3ba..5c97c386c 100644 --- a/testcases/kernel/syscalls/pselect/pselect01.c +++ b/testcases/kernel/syscalls/pselect/pselect01.c @@ -34,4 +34,5 @@ int sample_fn(int clk_id, long long usec) static struct tst_test test = { .scall = "pselect()", .sample = sample_fn, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, }; diff --git a/testcases/kernel/syscalls/select/select02.c b/testcases/kernel/syscalls/select/select02.c index 784ec9211..f3f2b0e9d 100644 --- a/testcases/kernel/syscalls/select/select02.c +++ b/testcases/kernel/syscalls/select/select02.c @@ -62,4 +62,5 @@ static struct tst_test test = { .setup = setup, .test_variants = TEST_VARIANTS, .cleanup = cleanup, + .max_runtime = TST_TIMER_TEST_MAX_RUNTIME, };
Rarely there is a need to set the test runtime dynamically, the only tests in LTP that does this are the timer tests that can get two parameters, number of iterations and sleep time, and the test runtime is close to the multiplication of these two. It's still cleaner to set the runtime and let the test library figure out the timeout in this case. Also when no parameters are passed to these tests the runtime is a sum of multiplications from the tst_timer_test.c source so we define a constant in the tst_timer_test.h header and set the max_runtime in the testcases accordingly. With this we get correct estimate for the test runtime and tighter, but still forgiving enough, timeout as well. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- include/tst_test.h | 8 ++++- include/tst_timer_test.h | 3 ++ lib/newlib_tests/.gitignore | 1 + lib/newlib_tests/test_max_runtime03.c | 35 +++++++++++++++++++ lib/tst_test.c | 35 +++++++++++-------- lib/tst_timer_test.c | 4 +-- .../clock_nanosleep/clock_nanosleep02.c | 1 + .../syscalls/epoll_pwait/epoll_pwait03.c | 1 + .../kernel/syscalls/epoll_wait/epoll_wait02.c | 1 + .../kernel/syscalls/epoll_wait/epoll_wait04.c | 2 +- .../syscalls/futex/futex_cmp_requeue01.c | 2 +- .../kernel/syscalls/futex/futex_wait05.c | 1 + .../kernel/syscalls/nanosleep/nanosleep01.c | 1 + testcases/kernel/syscalls/poll/poll02.c | 1 + testcases/kernel/syscalls/prctl/prctl09.c | 1 + testcases/kernel/syscalls/pselect/pselect01.c | 1 + testcases/kernel/syscalls/select/select02.c | 1 + 17 files changed, 79 insertions(+), 20 deletions(-) create mode 100644 lib/newlib_tests/test_max_runtime03.c