Message-ID: <f63e7367-c4fb-4cdc-a44c-6accbc309c5a@kernel.org>
Date: Fri, 6 Sep 2024 13:43:10 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, ncardwell@...gle.com, shuah@...nel.org,
 linux-kselftest@...r.kernel.org, fw@...len.de,
 Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 2/2] selftests/net: integrate packetdrill with
 ksft

Hi Willem,

On 06/09/2024 01:15, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
> 
> Lay the groundwork to import into kselftests the over 150 packetdrill
> TCP/IP conformance tests on github.com/google/packetdrill.
> 
> Florian recently added support for packetdrill tests in nf_conntrack,
> in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based
> conntrack tests").
> 
> This patch takes a slightly different approach. It relies on
> ksft_runner.sh to run every *.pkt file in the directory.

Thank you for adding this support! I'm looking forward to seeing more
packetdrill tests being validated by the CI, and I hope that will
encourage people to add more tests, and increase the code coverage!


I have some questions about how the packetdrill should be executed:
should they be isolated in dedicated netns?

There are some other comments, but feel free to ignore them if they are
not relevant, or if they can be fixed later.

> Tested:
> 	make -C tools/testing/selftests \
> 	  TARGETS=net/packetdrill \
> 	  run_tests

Please note that this will not run the packetdrill tests in a dedicated
netns. I don't think that's a good idea. Especially when sysctl knobs
are being changed during the tests, and more.

> 	make -C tools/testing/selftests \
> 	  TARGETS=net/packetdrill \
> 	  install INSTALL_PATH=$KSFT_INSTALL_PATH
> 
> 	# in virtme-ng
> 	./run_kselftest.sh -c net/packetdrill
> 	./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt

I see that ./run_kselftest.sh can be executed with the "-n | --netns"
option to run each test in a dedicated net namespace, but it doesn't
seem to work:

> # ./run_kselftest.sh -n -c net/packetdrill
> [  411.087208] kselftest: Running tests in net/packetdrill
> TAP version 13
> 1..3
> Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory
> setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument
> Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory

But maybe it would be better to create the netns in ksft_runner.sh?
Please see below.

(...)

> diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh
> new file mode 100755
> index 0000000000000..1095a7b22f44d
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/defaults.sh
> @@ -0,0 +1,63 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Set standard production config values that relate to TCP behavior.
> +
> +# Flush old cached data (fastopen cookies).
> +ip tcp_metrics flush all > /dev/null 2>&1

(Why ignoring errors if any?)

> +# TCP min, default, and max receive and send buffer sizes.
> +sysctl -q net.ipv4.tcp_rmem="4096 540000 $((15*1024*1024))"
> +sysctl -q net.ipv4.tcp_wmem="4096 $((256*1024)) 4194304"
> +
> +# TCP timestamps.
> +sysctl -q net.ipv4.tcp_timestamps=1
> +
> +# TCP SYN(ACK) retry thresholds
> +sysctl -q net.ipv4.tcp_syn_retries=5
> +sysctl -q net.ipv4.tcp_synack_retries=5
> +
> +# TCP Forward RTO-Recovery, RFC 5682.
> +sysctl -q net.ipv4.tcp_frto=2
> +
> +# TCP Selective Acknowledgements (SACK)
> +sysctl -q net.ipv4.tcp_sack=1
> +
> +# TCP Duplicate Selective Acknowledgements (DSACK)
> +sysctl -q net.ipv4.tcp_dsack=1
> +
> +# TCP FACK (Forward Acknowldgement)
> +sysctl -q net.ipv4.tcp_fack=0
> +
> +# TCP reordering degree ("dupthresh" threshold for entering Fast Recovery).
> +sysctl -q net.ipv4.tcp_reordering=3
> +
> +# TCP congestion control.
> +sysctl -q net.ipv4.tcp_congestion_control=cubic

(maybe safer to add CONFIG_TCP_CONG_CUBIC=y?)

> +
> +# TCP slow start after idle.
> +sysctl -q net.ipv4.tcp_slow_start_after_idle=0
> +
> +# TCP RACK and TLP.
> +sysctl -q net.ipv4.tcp_early_retrans=4 net.ipv4.tcp_recovery=1
> +
> +# TCP method for deciding when to defer sending to accumulate big TSO packets.
> +sysctl -q net.ipv4.tcp_tso_win_divisor=3
> +
> +# TCP Explicit Congestion Notification (ECN)
> +sysctl -q net.ipv4.tcp_ecn=0
> +
> +sysctl -q net.ipv4.tcp_pacing_ss_ratio=200
> +sysctl -q net.ipv4.tcp_pacing_ca_ratio=120
> +sysctl -q net.ipv4.tcp_notsent_lowat=4294967295 > /dev/null 2>&1
> +
> +sysctl -q net.ipv4.tcp_fastopen=0x70403
> +sysctl -q net.ipv4.tcp_fastopen_key=a1a1a1a1-b2b2b2b2-c3c3c3c3-d4d4d4d4
> +
> +sysctl -q net.ipv4.tcp_syncookies=1

(maybe safer to add CONFIG_SYN_COOKIES=y?)

> +# Override the default qdisc on the tun device.
> +# Many tests fail with timing errors if the default
> +# is FQ and that paces their flows.
> +tc qdisc add dev tun0 root pfifo
> +

(when applying your patches with 'b4 shazam', I got the following error,
I guess it was due to this blank line at the end)

  Applying: selftests: support interpreted scripts with ksft_runner.sh
  Applying: selftests/net: integrate packetdrill with ksft
  .git/rebase-apply/patch:142: new blank line at EOF.


> diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
> new file mode 100755
> index 0000000000000..2f62caccbbbc5
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
> @@ -0,0 +1,41 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +source "$(dirname $(realpath $0))/../../kselftest/ktap_helpers.sh"
> +
> +readonly ipv4_args=('--ip_version=ipv4 '
> +		    '--local_ip=192.168.0.1 '
> +		    '--gateway_ip=192.168.0.1 '
> +		    '--netmask_ip=255.255.0.0 '
> +		    '--remote_ip=192.0.2.1 '
> +		    '-D CMSG_LEVEL_IP=SOL_IP '
> +		    '-D CMSG_TYPE_RECVERR=IP_RECVERR ')
> +
> +readonly ipv6_args=('--ip_version=ipv6 '
> +		    '--mtu=1520 '
> +		    '--local_ip=fd3d:0a0b:17d6::1 '
> +		    '--gateway_ip=fd3d:0a0b:17d6:8888::1 '
> +		    '--remote_ip=fd3d:fa7b:d17d::1 '
> +		    '-D CMSG_LEVEL_IP=SOL_IPV6 '
> +		    '-D CMSG_TYPE_RECVERR=IPV6_RECVERR ')

(nit: that's a strange Bash array with spaces in the strings :)
You can simply remove the quotes, then below, you can use the variable
with double quotes: "${ipv4_args[@]}" and shellcheck will stop reporting
an error for that)

> +
> +if [ $# -ne 1 ]; then
> +	ktap_exit_fail_msg "usage: $0 <script>"
> +	exit "$KSFT_FAIL"
> +fi
> +script="$1"
> +
> +if [ -z "$(which packetdrill)" ]; then
> +	ktap_skip_all "packetdrill not found in PATH"
> +	exit "$KSFT_SKIP"
> +fi
> +
> +ktap_print_header
> +ktap_set_plan 2
> +
> +packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \

(Why muting stdout? I see that the TCP MD5 test output lines from
/proc/net/tcp*, is it why? Is this info useful? Or should it be removed
from the test?)


> +	&& ktap_test_pass "ipv4" || ktap_test_fail "ipv4"
> +packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \
> +	&& ktap_test_pass "ipv6" || ktap_test_fail "ipv6"

Even if "run_kselftest.sh" creates dedicated netns before running this
script (RUN_IN_NETNS=1), it looks like the tests in v4 and in v6 will
share the same netns. Is it OK? It means that if a packetdrill test sets
something that is not reset by 'defaults.sh', it might break the
following v6 test.

Why not having "ksft_runner.sh" creating the netns? It should be easy to
do so, using helpers from the "../lib.sh" file:

  ns_v4=
  ns_v6=

  trap cleanup_all_ns EXIT
  if ! setup_ns ns_v4 ns_v6; then
      (...) # fail + exit
  fi

  ip netns exec "${ns_v4}" packetdrill "${ipv4_args[@]}" \
                                       "$(basename "${script}")"
  (...)


(Note that if these tests are isolated in dedicated netns, and if later
we want to accelerate their execution, it should be easy to run these
two tests in parallel, something like the following)

  ip netns exec "${ns_v4}" (...) &
  pid_v4=$!
  ip netns exec "${ns_v6}" (...) &
  pid_v6=$!

  wait ${pid_v4} && tap_test_pass "ipv4" || ktap_test_fail "ipv4"
  wait ${pid_v6} && tap_test_pass "ipv6" || ktap_test_fail "ipv6"

> +
> +ktap_finished
> diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt
> new file mode 100644
> index 0000000000000..df49c67645ac8
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Test TCP_INQ and TCP_CM_INQ on the client side.
> +`./defaults.sh
> +`
(I guess you prefer not to modify these tests, and keep them
self-contained, but just in case it is easier for you, this line could
be removed, and have ksft_runner.sh sourcing this file before executing
the packetdrill test.)


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

Powered by blists - more mailing lists