netdev - Re: [PATCH net] selftests: net: cope with slow env in gro.sh test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 07 Feb 2024 15:35:57 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>,  Jakub Kicinski <kuba@...nel.org>, Shuah Khan
 <shuah@...nel.org>, Willem de Bruijn <willemb@...gle.com>, Coco Li
 <lixiaoyan@...gle.com>,  linux-kselftest@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH net] selftests: net: cope with slow env in gro.sh test

On Wed, 2024-02-07 at 12:16 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 06/02/2024 16:27, Paolo Abeni wrote:
> > The gro self-tests sends the packets to be aggregated with
> > multiple write operations.
> > 
> > When running is slow environment, it's hard to guarantee that
> > the GRO engine will wait for the last packet in an intended
> > train.
> > 
> > The above causes almost deterministic failures in our CI for
> > the 'large' test-case.
> > 
> > Address the issue explicitly ignoring failures for such case
> > in slow environments (KSFT_MACHINE_SLOW==true).
> 
> To what value is KSFT_MACHINE_SLOW set in the CI?

AFAIK, the CI initialize KSFT_MACHINE_SLOW (to true) only on slow env.

> Is it set to a different value if the machine is not slow? e.g.
> 
>   KSFT_MACHINE_SLOW == false
> 
> (please see below)
> 
> > Fixes: 7d1575014a63 ("selftests/net: GRO coalesce test")
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> > Note that the fixes tag is there mainly to justify targeting the net
> > tree, and this is aiming at net to hopefully make the test more stable
> > ASAP for both trees.
> > 
> > I experimented with a largish refactory replacing the multiple writes
> > with a single GSO packet, but exhausted by time budget before reaching
> > any good result.
> > ---
> >  tools/testing/selftests/net/gro.sh | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
> > index 19352f106c1d..114b5281a3f5 100755
> > --- a/tools/testing/selftests/net/gro.sh
> > +++ b/tools/testing/selftests/net/gro.sh
> > @@ -31,6 +31,10 @@ run_test() {
> >        1>>log.txt
> >      wait "${server_pid}"
> >      exit_code=$?
> > +    if [ ${test} == "large" -a -n "${KSFT_MACHINE_SLOW}" ]; then
> 
> Maybe best to avoid using:
> 
>   -n "${KSFT_MACHINE_SLOW}"
> 
> Otherwise, we have the same behaviour if KSFT_MACHINE_SLOW is set to
> 1/yes/true or 0/no/false.

For consistency, I followed the logic already in place in commit
c41dfb0dfbec ("selftests/net: ignore timing errors in so_txtime if
KSFT_MACHINE_SLOW").

> But maybe it is fine like that, and what is just missing is adding
> somewhere how KSFT_MACHINE_SLOW is supposed to be set/used? :)
> 
> 
> Not linked to that, but a small detail about the new line, just in case
> you need to send a v2: it looks like it is better to avoid using '-a':
> 
>   https://www.shellcheck.net/wiki/SC2166

Thank for the pointer, I was not aware of that. 

I guess a v2 dropping '-a' would be better.

Thanks,

Paolo