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]

Message-ID: <be5e4878-18a7-4de9-b03e-55298a84ab0f@kernel.org>
Date: Wed, 7 Feb 2024 12:16:27 +0100
From: Matthieu Baerts <matttbe@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
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

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?

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.

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

(but here, it looks like the usage is fine)

> +        echo "Ignoring errors due to slow environment" 1>&2
> +        exit_code=0
> +    fi
>      if [[ "${exit_code}" -eq 0 ]]; then
>          break;
>      fi

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.