Message-ID: <20240827173905.65b8efcc@kernel.org>
Date: Tue, 27 Aug 2024 17:39:05 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, 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 RFC] selftests/net: integrate packetdrill with
 ksft

Very exciting to see packetdrill tests making their way upstream!

On Tue, 27 Aug 2024 15:32:29 -0400 Willem de Bruijn wrote:
> RFC points for discussion
> 
> ksft: the choice for this python framework introduces a dependency on
> the YNL scripts, and some non-obvious code:
> - to include the net/lib dep in tools/testing/selftests/Makefile
> - a boilerplate lib/py/__init__.py that each user of ksft will need
> It seems preferable to me to use ksft.py over reinventing the wheel,
> e.g., to print KTAP output. But perhaps we can make it more obvious
> for future ksft users, and make the dependency on YNL optional.

No preference here, the wrapper script uses little of the python
code, and use of YNL seems unlikely, so it's a coin toss whether
it's worth linking up to net/lib/py or just writing a bit of bash.

> kselftest-per-directory: copying packetdrill_ksft.py to create a
> separate script per dir is a bit of a hack. A single script is much
> simpler, optionally with nested KTAP (not supported yet by ksft). But,
> I'm afraid that running time without intermediate output will be very
> long when we integrate all packetdrill scripts.

Not sure what you mean by intermediate output (the perl wrapper prints
immediately, do you have perl?). We do have some nested ktap parsing
in nipa, but small tweaks would be necessary to "decapsulate" the first
layer completely. My bigger concern would be runtime, if the time it
takes to run all tests grows we spawn multiple VMs and load balance
the test cases. 

All in all, trying to support single script is probably not worth the
extra effort.

> nf_conntrack: we can dedup the common.sh.
> 
> *pkt files: which of the 150+ scripts on github are candidates for
> kselftests, all or a subset? To avoid change detector tests.

Other than avoiding known flakes - no concerns. Is the distinction
between "change detector" vs hard tests (uAPI change / RFC compliance),
well defined? Not sure if that's really possible but if so it would be
nice to "sort" the tests into different dirs.

> And what
> is the best way to eventually send up to 150 files, 7K LoC.

Just send them, slice into a handful (<10) of patches if you can. 
7k LoC is same order of magnitude as initial drivers we merge.

To be clear let's start with a small patch like this one.
Since this is a new target I'll have to reconfigure the runners.
So we'll see how well this works once it's merged.
Spinning up new runners is a bit tedious but here new target seems 
quite justified.

>  tools/testing/selftests/Makefile              |  7 +-
>  .../selftests/net/packetdrill/.gitignore      |  1 +
>  .../selftests/net/packetdrill/Makefile        | 28 ++++++
>  .../net/packetdrill/lib/py/__init__.py        | 15 ++++
>  .../net/packetdrill/packetdrill_ksft.py       | 90 +++++++++++++++++++
>  .../net/packetdrill/tcp/common/defaults.sh    | 63 +++++++++++++
>  .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++
>  .../net/packetdrill/tcp/inq/client.pkt        | 51 +++++++++++
>  .../net/packetdrill/tcp/inq/server.pkt        | 51 +++++++++++
>  .../tcp/md5/md5-only-on-client-ack.pkt        | 28 ++++++

prolly need a config file to enable kconfig for MD5 ?
perils of adding new targets

> diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore
> new file mode 100644
> index 0000000000000..a40f1a600eb94
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/.gitignore
> @@ -0,0 +1 @@
> +tcp*sh

Is this right or should it be tcp_*.py ?

> diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile
> new file mode 100644
> index 0000000000000..d94c51098d1f0
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/Makefile
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0

Would be nice to add a few lines here with an overview of how the
packetdrill tests get executed. People may jump in here to try to
add new tests, since that's how ksft usually operates.

> +def scriptname_to_testdir(filepath):
> +    """Extract the directory to run from this filename."""
> +
> +    suffix = ".sh"

.sh again ?

> diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh b/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh

> +# 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
> +

nit: double new line

> diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
> new file mode 100755
> index 0000000000000..5ddf456ae973a
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py

What calls this one? I can't grep it out.

Powered by blists - more mailing lists