Re: [PATCH v3 net-next 14/15] selftests: drv-net: test AF_XDP zero-copy with an SKB peer

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Sat, Oct 17, 2026 at 03:06:11AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 6 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In tools/testing/selftests/drivers/net/hw/xsk.py, main()
>   changes state on both hosts and has no try/finally to undo it.
> - [Medium] In xsk.py, _run() starts the DUT zero-copy endpoint with
>   `local = cmd(local_args, background=True, fail=False)` at line 142 and…
> - [Medium] main() in xsk.py calls `xsk_reserve_hugepages(saved_hugepages,
>   cfg.remote)` (line 227), and xsk_reserve_hugepages()…
> - [Medium] If an endpoint is killed, the NIC's TX/RX descriptor ring
>   sizes are never restored.
> - [Medium] Taking the zero-copy queue out of RSS (_reserve_queue(),
>   xsk.py line 74) is the only thing keeping non-test traffic off it.
> - [Medium] _reserve_queue() in xsk.py assumes every reported channel can
>   be addressed by RSS.
> - [Low] The commit message says 'A host without ntuple support skips' and
>   'A DUT with fewer than two such queues skips'.
> - [Low] In xsk.py, _steer_udp() runs `ethtool -K <if> ntuple-filters on`
>   (line 86) when the feature is inactive and not fixed.
> 

I believe here I am done with responding to comments from our beloved
bot. There is one issue that indeed needs a fix before getting this in
(see below at ATR comment). Let me know if any fixups are possible...

> --
> 
> selftests: drv-net: test AF_XDP zero-copy with an SKB peer
> 
> This adds drivers/net/hw/xsk.py. It runs the xskxceiver cases as pairs of
> processes: a zero-copy endpoint on the DUT and an SKB-mode endpoint on the
> remote. main() sets up the shared state once (an RSS-excluded queue, ntuple
> rules on both hosts, huge pages) and undoes it after ksft_run().
> 
> > diff --git a/tools/testing/selftests/drivers/net/hw/xsk.py b/tools/testing/selftests/drivers/net/hw/xsk.py
> > new file mode 100755
> > index 000000000000..fd708bb3420d
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/xsk.py
> > @@ -0,0 +1,263 @@
> 
> [ ... ]
> 
> > +def _reserve_queue(cfg):
> > +    """Take the last queue with both an RX and a TX ring out of RSS. A TX
> > +    endpoint fills no RX buffers, so in zero-copy mode its queue drops what
> > +    it receives, the control connection included."""
> > +    ch = cfg.ethnl.channels_get({"header": {"dev-index": cfg.ifindex}})
> > +    count = ch.get("combined-count", 0) + min(ch.get("rx-count", 0),
> > +                                              ch.get("tx-count", 0))
> 
> [Severity: Medium]
> Does this assume that the RSS indirection table can address every reported
> channel?
> 
> On pre-X550 ixgbe (82599/X540) with ATR enabled, which is the default,
> ixgbe_get_channels() reports the Flow Director queue count:
> 
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:ixgbe_get_channels() {
>     ...
> 	/* report flow director queues as maximum channels */
> 	ch->combined_count = adapter->ring_feature[RING_F_FDIR].indices;
>     ...
> }
> 
> That count can be as high as 64. The RSS table on those parts can only
> address 16 queues:
> 
> ixgbe_rss_indir_tbl_max() {
> 	if (adapter->hw.mac.type < ixgbe_mac_X550)
> 		return 16;
>     ...
> }
> 
> ixgbe_set_rxfh() rejects entries beyond that limit:
> 
> 	for (i = 0; i < reta_entries; i++)
> 		if (rxfh->indir[i] >= max_queues)
> 			return -EINVAL;
> 
> On a host with more than 17 CPUs, would "ethtool -X <netif> equal 63" fail
> and raise CmdExitFailure during setup? That would abort the suite on a NIC
> that advertises AF_XDP zero-copy. In this configuration the chosen queue is
> already outside RSS, so the table doesn't need to change.
> 
> > +    if count < 2:
> > +        return None
> > +    queue = count - 1
> > +    ethtool(f"-X {cfg.ifname} equal {queue}")

in v4 rejected ethtool -X skips the run instead of raising exitfailure and
any previous setup is undone.

Current get_channels() output interpretation is targeted towards modern
NICs, but later on if there is really such need we could check if chosen
queue is already out of RSS...?

> 
> [Severity: Medium]
> On NICs that steer flows based on TX, is taking the queue out of RSS
> enough to keep the TCP control connection off it?
> 
> Flow Director ATR is on by default on i40e. i40e_atr() samples TCP TX
> packets, including the SYN. It then programs an FD filter that steers the
> flow's RX to the queue of the TX ring:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
>     ...
> 	flex_ptype = FIELD_PREP(I40E_TXD_FLTR_QW0_QINDEX_MASK,
> 				tx_ring->queue_index);
>     ...
> }

SO CLOSE! This one has to be addressed as atr is turned on by default and
this could cause i40e runs being stochastic. I must admit I did not have
i40e setup on my side, but it has full AF_XDP feature support (including
mbuf zc) so its coverage matters.

Fix would be:

diff --git a/tools/testing/selftests/drivers/net/hw/xsk.py b/tools/testing/selftests/drivers/net/hw/xsk.py
--- a/tools/testing/selftests/drivers/net/hw/xsk.py
+++ b/tools/testing/selftests/drivers/net/hw/xsk.py
@@ -115,6 +115,16 @@ def _reserve_queue(cfg, undo):
     return queue
 
 
+def _disable_atr(cfg, undo):
+    """i40e ATR steers what a TCP connection receives to the queue that it
+    sends from, which can be the zero-copy one, so turn it off until undo."""
+    flags = cmd(f"ethtool --show-priv-flags {cfg.ifname}", fail=False).stdout
+    if re.search(r"^flow-director-atr\s*: on$", flags, re.MULTILINE):
+        priv = f"--set-priv-flags {cfg.ifname} flow-director-atr"
+        ethtool(f"{priv} off")
+        undo.callback(ethtool, f"{priv} on")
+
+
 def _steer_udp(cfg, undo, ifname, queue, host=None):
     """Steer the test flow to queue until undo, return False if ifname
     cannot."""
@@ -313,6 +323,7 @@ def main():
         cfg.queue = _reserve_queue(cfg, undo)
         if cfg.queue is None:
             sys.exit(SKIP_CODE)
+        _disable_atr(cfg, undo)
 
         cfg.udp_port = rand_port()
         steered = (_steer_udp(cfg, undo, cfg.ifname, cfg.queue) and

> 
> FD filters take priority over RSS. Suppose XPS picks the last TX queue for
> the CPU that the DUT xskxceiver runs on. The remote's replies on the
> control connection would then land on the zero-copy queue.
> 
> In TX cases that queue has no fill buffers. Wouldn't the control
> connection stall, so the case fails or times out depending on CPU
> placement?
> 
> Only a udp4 ntuple rule is installed. Unlike a TCP4 sideband rule, it
> doesn't disable ATR automatically, and the flow-director-atr private flag
> is not cleared.
> 
> Some drivers gate aRFS on NETIF_F_NTUPLE, which _steer_udp() may turn on.
> When RFS is configured, aRFS could steer flows the same way.
> 
> > +    return queue
> > +
> > +
> > +def _steer_udp(cfg, ifname, port, queue, host=None):
> > +    """Steer the test flow to queue, return False if ifname cannot."""
> > +    ntuple = ethtool(f"-k {ifname}", json=True, host=host)[0]["ntuple-filters"]
> > +    if not ntuple["active"]:
> > +        if ntuple["fixed"]:
> > +            print(f"1..0 # SKIP {ifname} does not support ntuple-filters")
> > +            return False
> > +
> > +        ethtool(f"-K {ifname} ntuple-filters on", host=host)
> > +
> > +    out = ethtool(f"-N {ifname} flow-type udp4 dst-port {port} action {queue}",
> > +                  host=host).stdout
> > +    rule = re.search(r"ID (\d+)", out).group(1)
> 
> [Severity: Low]
> The commit message says:
> 
>     A host without ntuple support skips.
> 
> and:
> 
>     A DUT with fewer than two such queues skips
> 
> Here a skip happens only when ntuple-filters is fixed off. There are two
> other failure cases:
> 
> - The feature can be toggled but the NIC rejects the udp4 dst-port rule.
>   ethtool -N then raises CmdExitFailure.
> - The output has no "ID N". The re.search(...).group(1) then raises
>   AttributeError.
> 
> Would either of these bypass the skip branch in main()? The run would then
> error out instead of skipping, and the _unsteer_udp() and "ethtool -X
> default" calls in that branch would never run. If only the remote rule
> fails, the DUT rule that was already added would also be left behind.
> 
> Similarly, _reserve_queue() runs "ethtool -X equal" with fail=True. A DUT
> that rejects the RSS table update, such as the pre-X550 ixgbe case above,
> would crash during setup instead of skipping.
> 
> > +    cfg.rules.append((host, ifname, rule, ntuple["active"]))
> 
> [Severity: Low]
> When ntuple-filters was inactive, "ethtool -K ... ntuple-filters on" turns
> it on before the rule is added. The toggle is recorded only here, after
> both ethtool -N and the ID parse succeed.
> 
> If either of those raises, the toggle is never recorded. _unsteer_udp()
> turns the feature off only for entries in cfg.rules.
> 
> Could ntuple-filters be left enabled on that host after a failed run?
> Recording the undo right after the toggle would avoid that.
> 
> > +    return True
> 
> [ ... ]
> 
> > +def _setup(cfg, busy_poll):
> 
> [ ... ]
> 
> > +    fallbacks = [
> > +        defer(ip, f"link set dev {cfg.ifname} mtu {cfg.dev['mtu']}"),
> > +        defer(ip, f"link set dev {cfg.remote_ifname} "
> > +                  f"mtu {cfg.remote_dev['mtu']}", host=cfg.remote),
> > +        defer(ip, f"link set dev {cfg.ifname} xdpdrv off"),
> > +        defer(ip, f"link set dev {cfg.remote_ifname} xdpgeneric off",
> > +              host=cfg.remote),
> > +    ]
> 
> [Severity: Medium]
> What restores the NIC ring sizes when an endpoint is killed?
> 
> xskxceiver changes ring sizes with set_hw_ring_size(). The only restore
> happens in cleanup_iface(), on normal exit:

Addressed in v4 by moving ring changes into  xsk.py . The runner saves the
sizes and registers their restoration before starting the endpoints, so
restoration does not depend on endpoint cleanup running.

> 
> tools/testing/selftests/net/lib/xsk/xskxceiver.c:cleanup_iface() {
>     ...
> 	if (ifobj_has_cap(ifobj, XSK_CAP_HW_RING))
> 		hw_ring_size_reset(ifobj);
>     ...
> }
> 
> xskxceiver has no signal handling, so this restore is skipped in two
> cases:
> 
> - xsk_wait_endpoint_exit() sends SIGKILL to the DUT endpoint after
>   CASE_TIMEOUT.
> - timeout(1) sends SIGTERM to the remote endpoint.
> 
> These fallbacks cover only the MTU and the XDP detach.
> 
> The next endpoint's capability probe in test_xsk.c then adopts the
> leftover size as its default:
> 
> 	if (!get_hw_ring_size(ifobj->ifname, &ifobj->ring)) {
> 		ifobj_set_cap(ifobj, XSK_CAP_HW_RING);
> 		...
> 		ifobj->set_ring.default_tx = ifobj->ring.tx_pending;
> 		ifobj->set_ring.default_rx = ifobj->ring.rx_pending;
> 
> Would a hung ring size case then leave the NIC with a min or max
> descriptor ring size for the rest of the run, and after it?
> 
> > +    return fallbacks
> 
> [ ... ]
> 
> > +def _run(cfg, local_args, remote_args, ctrl_port):
> 
> [ ... ]
> 
> > +    try:
> > +        head, listening = xsk_wait_listen(remote, ctrl_port, LISTEN_TIMEOUT)
> > +        if listening:
> > +            local = cmd(local_args, background=True, fail=False)
> > +            dut_ret = xsk_wait_endpoint_exit(local, "dut", CASE_TIMEOUT)
> > +    finally:
> > +        # Even if the DUT endpoint did not run, so that the remote one is
> > +        # gone before the next case starts.
> > +        remote_ret = xsk_wait_endpoint_exit(remote, "remote", REMOTE_TIMEOUT,
> > +                                            head)
> 
> [Severity: Medium]
> Can the DUT endpoint be orphaned here? The finally reaps only the remote
> endpoint, and xsk_wait_endpoint_exit() kills the process only on a
> timeout:
> 
> tools/testing/selftests/net/lib/py/xsk.py:xsk_wait_endpoint_exit() {
>     try:
>         endpoint.process(terminate=False, fail=False, timeout=timeout)
>     except subprocess.TimeoutExpired:
>         endpoint.proc.kill()
>     ...
> }

Addressed in v4 in the shared helper: interruption kills and reaps the
endpoint before propagating the exception.

> 
> When the kselftest runner times out, it sends SIGTERM to the Python
> process only. _ksft_intr then raises KsftTerminate inside the wait on
> local, and the exception propagates while local.proc is still running.
> 
> ksft_run() catches the exception and runs ksft_flush_defer(). That
> detaches the XDP program and restores the MTU while the AF_XDP socket is
> still bound. main() then deletes the ntuple rules and resets RSS.
> 
> xskxceiver would keep running after Python exits. It would later run its
> own cleanup_iface() against state that the runner has already changed.
> 
> Should local also be killed in the finally?
> 
> > +    return dut_ret, remote_ret
> 
> [ ... ]
> 
> > +def main():
> > +    # ksft_run() exits on -l and -h before main() can undo the setup.
> > +    if "-l" in sys.argv or "-h" in sys.argv:
> > +        sys.exit(f"{sys.argv[0]}: -l and -h are not supported")
> 
> [ ... ]
> 
> > +        saved_hugepages = []
> > +        cfg.hugepages = (xsk_reserve_hugepages(saved_hugepages) and
> > +                         xsk_reserve_hugepages(saved_hugepages, cfg.remote))
> 
> [Severity: Medium]
> What happens when the remote kernel is built without CONFIG_HUGETLBFS?
> xsk_reserve_hugepages() starts with:

addressed in v4 and described on veth runner comments

> 
>     have = int(cmd("sysctl -n vm.nr_hugepages", host=host).stdout)
> 
> vm.nr_hugepages exists only with CONFIG_HUGETLBFS (mm/hugetlb_sysctl.c).
> Without it the sysctl fails, and cmd() raises CmdExitFailure instead of
> the helper returning False.
> 
> The commit message says the UNALIGNED cases are "left out without them".
> Wouldn't the whole suite abort before ksft_run() instead, leaving the RSS
> change and ntuple rules in place?
> 
> The drivers/net/hw config covers the DUT, but nothing constrains the
> remote kernel.
> 
> > +
> > +        cases = _get_cases(cfg)
> 
> [ ... ]
> 
> > +        ksft_run([ksft_variants(cases)(test_xsk),
> > +                  ksft_variants(cases)(test_xsk_busy_poll)], args=(cfg,))
> > +
> > +        xsk_release_hugepages(saved_hugepages)
> > +        _unsteer_udp(cfg)
> > +        ethtool(f"-X {cfg.ifname} default")
> 
> [Severity: Medium]
> If main() leaves early, does anything undo the RSS change, the ntuple
> rules and the huge page reservation? These three calls run only when
> ksft_run() returns normally, and the setup has no try/finally around it.
> NetDrvEpEnv.__exit__ doesn't know about cfg.rules or saved_hugepages, and
> defer() only works inside a case.
> 
> Any exception after _reserve_queue() skips all three calls. Some examples:

addressed in v4 via ExitStack introduction, suggested by Bjorn

> 
> - a CmdExitFailure from ethtool -N on either host
> - the AttributeError from the rule ID parse
> - a failing remote sysctl
> - a failing "xskxceiver -L" in _get_cases()
> 
> The "-l"/"-h" guard at the top of main() matches only those exact tokens.
> ksft_run() parses arguments with:
> 
>     opts, _ = getopt.getopt(sys.argv[1:], 'hlt:T:')
> 
> This accepts clustered options like "-lt NAME" or "-hl". It also calls
> sys.exit(1) on a GetoptError, such as an unknown option, --help, or -t
> with no value. All of these exit from inside ksft_run(), after the setup
> has already run.
> 
> The commit message gives the reason for the guard as:
> 
>     so xsk.py rejects -l and -h, on which ksft_run() exits without
>     returning
> 
> sys.exit() raises SystemExit. Would a try/finally around the setup and
> ksft_run() cover these paths and the -l/-h case as well?
> 
> There are two smaller points about the teardown itself:
> 
> - "ethtool -X default" installs the driver's default indirection table,
>   not the one that was in place before the run. A custom table set by
>   the admin is lost.
> - If xsk_release_hugepages() fails, for example over ssh, the rule
>   deletes and the RSS reset after it are skipped.
> 
> > +    ksft_exit()
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014184539.774708-1-maciej.fijalkowski%40intel.com