Re: [LTP] [PATCH V3] lib: redefine the overall timeout logic of test

[Petr Vorel <pvorel@suse.cz>]

Hi Li, Cyril,

We have some warnings due this change:
-	int max_runtime;
+	unsigned int runtime;

tst_test.c: In function ‘tst_remaining_runtime’:
tst_test.c:1691:30: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
 1691 |         if (results->runtime > elapsed)
      |                              ^
tst_test.c: In function ‘set_overall_timeout’:
tst_test.c:1720:30: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
 1720 |         if (results->runtime < 0) {
      |                              ^

I suppose we don't need this check any more:

	if (results->runtime < 0) {
		tst_brk(TBROK, "runtime must to be > 0! (%d)",
			results->runtime);
	}

because TST_UNLIMITED_TIMEOUT is now tested against tst_test->timeout.

Could you please fix it before merge?

> >  timeout: Defines the maximum time allowed for the entire test, including
> >           setup, execution, and cleanup, when no explicit runtime is set.
> > 	  But if a runtime is explicitly defined and tst_remaining_runtime()
> > 	  is used, the timeout applies only to the setup and cleanup phases,
> > 	  as the runtime controls the test execution duration.

> >  runtime: The maximum runtime of the test's main execution loop, used
> >           in tests that call tst_remaining_runtime(). It ensures the
> > 	  main execution runs for a fixed duration, regardless of kernel
> > 	  configuration (e.g., debug kernel).

If I look correctly atm we have none tests with both .timeout and .runtime.

IMHO only tests with tst_remaining_runtime() or fuzzy sync should use .runtime.

When I check:

$ git grep -l -e tst_remaining_runtime -e tst_fuzzy_sync.h $(git log --oneline --pretty="format:" --name-only -1) |wc -l
60

$ git grep  '\.runtime' $(git log --oneline --pretty="format:" --name-only -1) |wc -l
60

testcases/kernel/syscalls/readahead/readahead02.c uses .runtime but (as Cyril
noted) you wanted to add it .timeout (I also think it should use .timeout).

But testcases/kernel/sched/cfs-scheduler/starvation.c calls
tst_remaining_runtime() but it's without .runtime. Don't we want to set it?

Also it got TCONF previously, I might try to put it back after this is merged.

Or better for whole tree:
$ git grep -l -e tst_remaining_runtime -e tst_fuzzy_sync.h testcases/ |wc -l
54

$ git grep -l '\.runtime' testcases/ |wc -l
53

The difference is with testcases/kernel/io/ltp-aiodio/common.h and all tests
which use it use .runtime.

=> everything looks ok.

> > Overall timeout is structured as follows:

> > | -- (default_30s + timeout) * timeout_mul -- | -- runtime * runtime_mul -- |

I like the diagram :). Later maybe it could be added to tst_remaining_runtime(),
which does not use sphinx doc but it's used by many tests. Also, overall timeout
would deserve moving from doc/old/C-Test-API.asciidoc to some specific page in
sphinx (maybe part of lib/README.md conversion).

> + *           set to a sufficientlylarge value or TST_UNLIMITED_TIMEOUT to remove
missing space:
    *           set to a sufficiently large value or TST_UNLIMITED_TIMEOUT to remove

Could you please fix it before merge?

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > * Move .runtime to .timeout for some tests (fsplough, ksm02, ksm04)
FYI fsplough uses tst_set_timeout() only.

> > * Drop the changes on shell test test_timeout.sh

I wonder what to do with shell timeout in the future. Not needed to be changed?

> > -	if (results->max_runtime == TST_UNLIMITED_RUNTIME) {
> > +	if (tst_test->timeout == TST_UNLIMITED_TIMEOUT) {
> >  		tst_res(TINFO, "Timeout per run is disabled");

> +additional safety margin. If test without an explicit runtime, the 'timeout'
nit: I would format also 'runtime'
  +additional safety margin. If test without an explicit 'runtime', the 'timeout'

Maybe we could unify also this commit message to:

	if (tst_test->timeout == TST_UNLIMITED_TIMEOUT) {
		tst_res(TINFO, "Test timeout is not limited");
		return;
	}


> To patch reviewer:

> Looks like this patch fell behind the last one commit f162bff0af19ce7d7f3,
> please drop that one manually if you want to apply it without conflicts
> locally.

> I will do code rebase once getting enough Reviewed-by tags.

I'm sorry for merging f162bff0af19ce7d7f3. I suppose rebase master should be ok.

For me worked without the need to resolve the conflict:
git checkout f162bff0af19ce7d7f3~
git am YOUR_PATCH.mbox
git rebase master

Kind regards,
Petr

> Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp