Re: [PATCH v2 net-next 14/14] selftests: xsk: document generic and hardware endpoint runs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new README describes XSK_REMOTE_DEPLOY and XSK_REMOTE_BIN as
  if each works on its own, but xsk.py does not handle them that way.
- [Low] The 'Generic veth test' section of the new README.rst ends with
  "The generic frame format is unchanged." That is a changelog note, not…
- [Low] The README says "The XDP programs redirect only the test flow and
  pass all other traffic to the stack, so the tested link does not have…
- [Low] The README says "The remote listens on all addresses for
  control." In test_xsk()…

--

selftests: xsk: document generic and hardware endpoint runs

This adds a README.rst under net/lib/xsk. It describes the per-case veth
launcher in test_xsk.sh and the two-host hardware setup driven by
drivers/net/hw/xsk.py, where the DUT uses AF_XDP zero-copy and the remote
xskxceiver uses SKB mode. It also points to the new document from
drivers/net/README.rst and xskxceiver.c.

> diff --git a/tools/testing/selftests/net/lib/xsk/README.rst b/tools/testing/selftests/net/lib/xsk/README.rst
> new file mode 100644
> index 0000000000000..d0a0a902ed783
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/xsk/README.rst
> @@ -0,0 +1,138 @@

[ ... ]

> +The shell wrapper creates the veth pair. ``-m skb|drv`` selects a mode,
> +``-t`` selects a test by the name or number shown by ``xskxceiver -l``,
> +and ``-p N`` selects a fixed control port instead of the default random
> +port. The generic frame format is unchanged.

[Severity: Low]
This isn't a bug, but "The generic frame format is unchanged" reads like a
changelog note. Once the series is merged, someone reading this file on
its own has nothing to compare it against.

Could this sentence either describe the frame format, or move to the
commit message or cover letter?

[ ... ]

> +Both XSK endpoints generate and validate raw IPv4/UDP frames with one fixed
> +UDP port, so ntuple rules can steer the test flow. They do not use AF_INET
> +sockets. On DUT RX cases ``xsk.py`` reserves the last RX queue outside RSS
> +and steers test traffic to it. On DUT TX cases it steers traffic to queue 0
> +on the remote receiver. The XDP programs redirect only the test flow and
> +pass all other traffic to the stack, so the tested link does not have to
> +be otherwise idle.

[Severity: Low]
Does "does not have to be otherwise idle" hold for DUT TX cases with
REMOTE_TYPE=ssh?

In _setup_case() in drivers/net/hw/xsk.py, the TX branch reserves a queue
only for a netns remote:

    queue = _reserve_queue(cfg) if _netns_remote(cfg) else 0

With an SSH remote, the DUT zero-copy socket binds to queue 0, and queue 0
stays in the RSS indirection table. A later paragraph of this README notes
that the DUT TX socket fills no RX buffers, so its zero-copy queue drops
what it receives.

If unrelated traffic on the tested link hashes to DUT queue 0, wouldn't it
be dropped before XDP_PASS can hand it to the stack? That includes non-IP
frames such as ARP or LLDP, which many NICs put on queue 0.

The test still passes, because _control_addr() sends the SSH control
channel over the management link. Even so, this guarantee only seems to
hold for netns remotes.

[ ... ]

> +Configure the usual driver test variables in
> +``tools/testing/selftests/drivers/net/hw/net.config`` or the environment::
> +
> +  NETIF=eth0
> +  LOCAL_V4=192.0.2.1
> +  REMOTE_V4=192.0.2.2
> +  REMOTE_TYPE=ssh
> +  REMOTE_ARGS=user@remote.example.com
> +  XSK_REMOTE_BIN=/path/to/remote/xskxceiver
> +  XSK_REMOTE_SUDO=1

[ ... ]

> +The remote host needs AF_XDP in SKB mode and BPF. ``xsk.py`` copies the
> +DUT's ``xskxceiver`` binary to the remote, so both hosts must have
> +compatible architectures and libraries. ``XSK_REMOTE_BIN`` chooses a fixed
> +destination path for that copy. Set ``XSK_REMOTE_DEPLOY=0`` to use a binary
> +built on the remote from the same test case definitions instead.

[Severity: Low]
Does this match how xsk.py handles these two variables?

In _remote_binary() in drivers/net/hw/xsk.py, XSK_REMOTE_DEPLOY is only
checked once XSK_REMOTE_BIN is set:

    binary = cfg.env.get("XSK_REMOTE_BIN")
    if not binary:
        return cfg.remote.deploy(local_binary)
    if cfg.env.get("REMOTE_TYPE") != "ssh":
        raise KsftSkipEx("XSK_REMOTE_BIN requires the SSH remote backend")
    if cfg.env.get("XSK_REMOTE_DEPLOY", "1") == "0":

If only XSK_REMOTE_DEPLOY=0 is set, as this paragraph suggests, wouldn't
the DUT binary still be copied to the remote without any warning?

Also, XSK_REMOTE_BIN causes a skip for any remote that isn't SSH, and the
example config above sets it. The netns paragraph doesn't say to unset it.
If someone copies the example and switches REMOTE_TYPE to netns, wouldn't
every case be skipped with "XSK_REMOTE_BIN requires the SSH remote
backend"?

Could the README say that XSK_REMOTE_DEPLOY=0 needs XSK_REMOTE_BIN, and
that XSK_REMOTE_BIN only works with REMOTE_TYPE=ssh?

[ ... ]

> +With SSH, the DUT uses the SSH host name to connect to the remote control
> +listener.
> +``XSK_UDP_PORT`` changes the fixed test UDP port (default 42567). The remote
> +listens on all addresses for control.

[Severity: Low]
Is "listens on all addresses" accurate? test_xsk() in drivers/net/hw/xsk.py
starts the remote listener with:

    "-p", "0.0.0.0", "-P", str(control_port), *udp_args,

peer_open_tcp() in net/lib/xsk/xsk_peer.c resolves this through
getaddrinfo() with AF_UNSPEC. For "0.0.0.0" that returns only an AF_INET
wildcard, so the listener binds to IPv4 only.

With an SSH remote, _control_addr() makes the DUT connect to the host part
of REMOTE_ARGS. If REMOTE_ARGS names an IPv6 literal or a host with only
AAAA records, can the DUT reach the listener?

The remote still prints its listening line, so the case would time out or
fail rather than skip.

Should the README say the listener is IPv4 only, or should the listener
also bind an IPv6 wildcard?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008114909.734364-1-maciej.fijalkowski%40intel.com