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

[Petr Vorel <pvorel@suse.cz>]

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