[LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[Petr Vorel]

Performance failures in tests which use tst_netload_compare() (tests in
runtest/net.features) can hide a real error (e.g. test fails due missing
required kernel module). Best solution would be to have feature tests
(likely written in C API) and performance tests (the current ones).

But until it happens, just disable performance failure by default,
print TINFO message instead unless TST_NET_FEATURES_TEST_PERFORMANCE=1
environment variable is set.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_net.sh    | 12 +++++++++---
 testcases/lib/tst_test.sh   |  5 +++--
 testcases/network/README.md |  4 ++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index e47ee9676..46d49c266 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -869,16 +869,22 @@ tst_netload_compare()
 	local new_time=$2
 	local threshold_low=$3
 	local threshold_hi=$4
+	local ttype='TFAIL'
+	local msg res
 
 	if [ -z "$base_time" -o -z "$new_time" -o -z "$threshold_low" ]; then
 		tst_brk_ TBROK "tst_netload_compare: invalid argument(s)"
 	fi
 
-	local res=$(((base_time - new_time) * 100 / base_time))
-	local msg="performance result is ${res}%"
+	res=$(((base_time - new_time) * 100 / base_time))
+	msg="performance result is ${res}%"
 
 	if [ "$res" -lt "$threshold_low" ]; then
-		tst_res_ TFAIL "$msg < threshold ${threshold_low}%"
+		if [ -z "$TST_NET_FEATURES_TEST_PERFORMANCE" ]; then
+			ttype='TINFO';
+			tst_res_ TINFO "WARNING: slow performance is not treated as error, change it with TST_NET_FEATURES_TEST_PERFORMANCE=1"
+		fi
+		tst_res_ $ttype "$msg < threshold ${threshold_low}%"
 		return
 	fi
 
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 5f178a1be..06ee6005a 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
-# Copyright (c) Linux Test Project, 2014-2022
+# Copyright (c) Linux Test Project, 2014-2023
 # Author: Cyril Hrubis <chrubis@suse.cz>
 #
 # LTP test library for shell.
@@ -681,7 +681,8 @@ tst_run()
 			NEEDS_KCONFIGS|NEEDS_KCONFIGS_IFS);;
 			IPV6|IPV6_FLAG|IPVER|TEST_DATA|TEST_DATA_IFS);;
 			RETRY_FUNC|RETRY_FN_EXP_BACKOFF|TIMEOUT);;
-			NET_DATAROOT|NET_MAX_PKT|NET_RHOST_RUN_DEBUG|NETLOAD_CLN_NUMBER);;
+			NETLOAD_CLN_NUMBER|NET_DATAROOT|NET_FEATURES_TEST_PERFORMANCE);;
+			NET_MAX_PKT|NET_RHOST_RUN_DEBUG);;
 			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
 			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
 			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
diff --git a/testcases/network/README.md b/testcases/network/README.md
index a0a1d3d95..0547c3f9f 100644
--- a/testcases/network/README.md
+++ b/testcases/network/README.md
@@ -84,6 +84,10 @@ Where
 Default values for all LTP network parameters are set in `testcases/lib/tst_net.sh`.
 Network stress parameters are documented in `testcases/network/stress/README`.
 
+Tests which use `tst_netload_compare()` test just basic functionality,
+performance failure is just printed with 'TINFO'. To enable also performance
+testing, set `TST_NET_FEATURES_TEST_PERFORMANCE=1` environment variable.
+
 ## Debugging
 Both single and two host configurations support debugging via
 `TST_NET_RHOST_RUN_DEBUG=1` environment variable.
-- 
2.43.0


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

Re: [LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[Petr Vorel]

Hi all,

> Performance failures in tests which use tst_netload_compare() (tests in
> runtest/net.features) can hide a real error (e.g. test fails due missing
> required kernel module). Best solution would be to have feature tests
> (likely written in C API) and performance tests (the current ones).

> But until it happens, just disable performance failure by default,
> print TINFO message instead unless TST_NET_FEATURES_TEST_PERFORMANCE=1
> environment variable is set.

NOTE: why is this enabled by default? IMHO it's more important to have feature
tests than performance tests often failing and thus hiding feature test.
Even introducing some LTP_TIMEOUT_MUL like variable would not help in case SUT
performance differs (sometimes SUTs are ok, other time slow due QEMU host which
runs SUTs is overbooked).

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[Petr Vorel]

> Hi all,

[ Cc Alexey, see
https://lore.kernel.org/ltp/20231219123709.339435-1-pvorel@suse.cz/
https://patchwork.ozlabs.org/project/ltp/patch/20231219123709.339435-1-pvorel@suse.cz/
]

> > Performance failures in tests which use tst_netload_compare() (tests in
> > runtest/net.features) can hide a real error (e.g. test fails due missing
> > required kernel module). Best solution would be to have feature tests
> > (likely written in C API) and performance tests (the current ones).

> > But until it happens, just disable performance failure by default,
> > print TINFO message instead unless TST_NET_FEATURES_TEST_PERFORMANCE=1
> > environment variable is set.

> NOTE: why is this enabled by default? IMHO it's more important to have feature
> tests than performance tests often failing and thus hiding feature test.
> Even introducing some LTP_TIMEOUT_MUL like variable would not help in case SUT
> performance differs (sometimes SUTs are ok, other time slow due QEMU host which
> runs SUTs is overbooked).

I knew there is some variable for performance tuning of network feature
tests: VIRT_PERF_THRESHOLD_MIN. But again, if the performance of the QEMU host
varies, the value must be set too high, which can hide the failure on broken
system which is idle. IMHO these tests should be run 2x - just feature tests
then with properly VIRT_PERF_THRESHOLD_MIN.

And, also VIRT_PERF_THRESHOLD_MIN is used only in virt_lib.sh, but
tst_netload_compare() which can fail due performance is used also in other
tests, e.g. sctp01.sh. That's why I think it's better to disable performance by
default.

Kind regards,
Petr

> Kind regards,
> Petr

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

Re: [LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[Martin Doucha]

Hi,

On 19. 12. 23 13:37, Petr Vorel wrote:
> Performance failures in tests which use tst_netload_compare() (tests in
> runtest/net.features) can hide a real error (e.g. test fails due missing
> required kernel module). Best solution would be to have feature tests
> (likely written in C API) and performance tests (the current ones).
> 
> But until it happens, just disable performance failure by default,
> print TINFO message instead unless TST_NET_FEATURES_TEST_PERFORMANCE=1
> environment variable is set.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>   testcases/lib/tst_net.sh    | 12 +++++++++---
>   testcases/lib/tst_test.sh   |  5 +++--
>   testcases/network/README.md |  4 ++++
>   3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index e47ee9676..46d49c266 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -869,16 +869,22 @@ tst_netload_compare()
>   	local new_time=$2
>   	local threshold_low=$3
>   	local threshold_hi=$4
> +	local ttype='TFAIL'
> +	local msg res
>   
>   	if [ -z "$base_time" -o -z "$new_time" -o -z "$threshold_low" ]; then
>   		tst_brk_ TBROK "tst_netload_compare: invalid argument(s)"
>   	fi
>   
> -	local res=$(((base_time - new_time) * 100 / base_time))
> -	local msg="performance result is ${res}%"
> +	res=$(((base_time - new_time) * 100 / base_time))
> +	msg="performance result is ${res}%"
>   
>   	if [ "$res" -lt "$threshold_low" ]; then
> -		tst_res_ TFAIL "$msg < threshold ${threshold_low}%"
> +		if [ -z "$TST_NET_FEATURES_TEST_PERFORMANCE" ]; then
> +			ttype='TINFO';
> +			tst_res_ TINFO "WARNING: slow performance is not treated as error, change it with TST_NET_FEATURES_TEST_PERFORMANCE=1"

This TINFO message should probably be moved to tst_net_setup(). 
Otherwise some tests will print it multiple times.

I'd also slightly prefer to keep the default as is and use a variable to 
disable perf checks instead.

> +		fi
> +		tst_res_ $ttype "$msg < threshold ${threshold_low}%"
>   		return
>   	fi
>   
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 5f178a1be..06ee6005a 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -1,6 +1,6 @@
>   #!/bin/sh
>   # SPDX-License-Identifier: GPL-2.0-or-later
> -# Copyright (c) Linux Test Project, 2014-2022
> +# Copyright (c) Linux Test Project, 2014-2023
>   # Author: Cyril Hrubis <chrubis@suse.cz>
>   #
>   # LTP test library for shell.
> @@ -681,7 +681,8 @@ tst_run()
>   			NEEDS_KCONFIGS|NEEDS_KCONFIGS_IFS);;
>   			IPV6|IPV6_FLAG|IPVER|TEST_DATA|TEST_DATA_IFS);;
>   			RETRY_FUNC|RETRY_FN_EXP_BACKOFF|TIMEOUT);;
> -			NET_DATAROOT|NET_MAX_PKT|NET_RHOST_RUN_DEBUG|NETLOAD_CLN_NUMBER);;
> +			NETLOAD_CLN_NUMBER|NET_DATAROOT|NET_FEATURES_TEST_PERFORMANCE);;
> +			NET_MAX_PKT|NET_RHOST_RUN_DEBUG);;
>   			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
>   			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
>   			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
> diff --git a/testcases/network/README.md b/testcases/network/README.md
> index a0a1d3d95..0547c3f9f 100644
> --- a/testcases/network/README.md
> +++ b/testcases/network/README.md
> @@ -84,6 +84,10 @@ Where
>   Default values for all LTP network parameters are set in `testcases/lib/tst_net.sh`.
>   Network stress parameters are documented in `testcases/network/stress/README`.
>   
> +Tests which use `tst_netload_compare()` test just basic functionality,
> +performance failure is just printed with 'TINFO'. To enable also performance
> +testing, set `TST_NET_FEATURES_TEST_PERFORMANCE=1` environment variable.
> +
>   ## Debugging
>   Both single and two host configurations support debugging via
>   `TST_NET_RHOST_RUN_DEBUG=1` environment variable.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH 1/1] net: tst_netload_compare(): Ignore performance failures

[Petr Vorel]

Hi Martin,

> Hi,

> On 19. 12. 23 13:37, Petr Vorel wrote:
> > Performance failures in tests which use tst_netload_compare() (tests in
> > runtest/net.features) can hide a real error (e.g. test fails due missing
> > required kernel module). Best solution would be to have feature tests
> > (likely written in C API) and performance tests (the current ones).

> > But until it happens, just disable performance failure by default,
> > print TINFO message instead unless TST_NET_FEATURES_TEST_PERFORMANCE=1
> > environment variable is set.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >   testcases/lib/tst_net.sh    | 12 +++++++++---
> >   testcases/lib/tst_test.sh   |  5 +++--
> >   testcases/network/README.md |  4 ++++
> >   3 files changed, 16 insertions(+), 5 deletions(-)

> > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> > index e47ee9676..46d49c266 100644
> > --- a/testcases/lib/tst_net.sh
> > +++ b/testcases/lib/tst_net.sh
> > @@ -869,16 +869,22 @@ tst_netload_compare()
> >   	local new_time=$2
> >   	local threshold_low=$3
> >   	local threshold_hi=$4
> > +	local ttype='TFAIL'
> > +	local msg res
> >   	if [ -z "$base_time" -o -z "$new_time" -o -z "$threshold_low" ]; then
> >   		tst_brk_ TBROK "tst_netload_compare: invalid argument(s)"
> >   	fi
> > -	local res=$(((base_time - new_time) * 100 / base_time))
> > -	local msg="performance result is ${res}%"
> > +	res=$(((base_time - new_time) * 100 / base_time))
> > +	msg="performance result is ${res}%"
> >   	if [ "$res" -lt "$threshold_low" ]; then
> > -		tst_res_ TFAIL "$msg < threshold ${threshold_low}%"
> > +		if [ -z "$TST_NET_FEATURES_TEST_PERFORMANCE" ]; then
> > +			ttype='TINFO';
> > +			tst_res_ TINFO "WARNING: slow performance is not treated as error, change it with TST_NET_FEATURES_TEST_PERFORMANCE=1"

> This TINFO message should probably be moved to tst_net_setup(). Otherwise
> some tests will print it multiple times.

I wanted to print it only when relevant. When added to tst_net_setup() it will
print also for tests which does not use tst_netload_compare(), which is IMHO
worth than printing multiple times the info. I could avoid it with guarding
with a variable (e.g. _tst_net_test_performance_warn_printed, ugly solution but
solves the problem). WDYT?

> I'd also slightly prefer to keep the default as is and use a variable to
> disable perf checks instead.

That's obviously more friendly as it's backward compatible, so I'm ok to send v2
which implements that. The reason why I dared to change the default is that I
haven't seen any report on these tests thus I suspect not many people (if any
outside SUSE) use them. I also wonted to promote them as working tests to the
testing community, but I would recommend to use them only with disabled checks.

Kind regards,
Petr

> > +		fi
> > +		tst_res_ $ttype "$msg < threshold ${threshold_low}%"
> >   		return
> >   	fi
> > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> > index 5f178a1be..06ee6005a 100644
> > --- a/testcases/lib/tst_test.sh
> > +++ b/testcases/lib/tst_test.sh
> > @@ -1,6 +1,6 @@
> >   #!/bin/sh
> >   # SPDX-License-Identifier: GPL-2.0-or-later
> > -# Copyright (c) Linux Test Project, 2014-2022
> > +# Copyright (c) Linux Test Project, 2014-2023
> >   # Author: Cyril Hrubis <chrubis@suse.cz>

> >   # LTP test library for shell.
> > @@ -681,7 +681,8 @@ tst_run()
> >   			NEEDS_KCONFIGS|NEEDS_KCONFIGS_IFS);;
> >   			IPV6|IPV6_FLAG|IPVER|TEST_DATA|TEST_DATA_IFS);;
> >   			RETRY_FUNC|RETRY_FN_EXP_BACKOFF|TIMEOUT);;
> > -			NET_DATAROOT|NET_MAX_PKT|NET_RHOST_RUN_DEBUG|NETLOAD_CLN_NUMBER);;
> > +			NETLOAD_CLN_NUMBER|NET_DATAROOT|NET_FEATURES_TEST_PERFORMANCE);;
> > +			NET_MAX_PKT|NET_RHOST_RUN_DEBUG);;
> >   			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
> >   			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
> >   			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
> > diff --git a/testcases/network/README.md b/testcases/network/README.md
> > index a0a1d3d95..0547c3f9f 100644
> > --- a/testcases/network/README.md
> > +++ b/testcases/network/README.md
> > @@ -84,6 +84,10 @@ Where
> >   Default values for all LTP network parameters are set in `testcases/lib/tst_net.sh`.
> >   Network stress parameters are documented in `testcases/network/stress/README`.
> > +Tests which use `tst_netload_compare()` test just basic functionality,
> > +performance failure is just printed with 'TINFO'. To enable also performance
> > +testing, set `TST_NET_FEATURES_TEST_PERFORMANCE=1` environment variable.
> > +
> >   ## Debugging
> >   Both single and two host configurations support debugging via
> >   `TST_NET_RHOST_RUN_DEBUG=1` environment variable.

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