Re: [PATCH v3 net-next 11/15] selftests: xsk: add a hardware mode to xskxceiver

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

On Sat, Oct 17, 2026 at 03:06:09AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 4
> 
> 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] A hardware SKB-mode endpoint still gates every multi-buffer
>   case on XSK_CAP_MBUF.
> - [Medium] The comment in wait_link_up() says 'Each endpoint makes its
>   changes before it is ready', but the TX endpoint creates its UMEM and…
> - [Medium] cleanup_iface() in xskxceiver.c now fails the endpoint when
>   xsk_set_mtu() cannot restore orig_mtu, but it still ignores the return…
> - [Low] At this commit, hardware mode switches the test frames to
>   UDP/IPv4, but the XDP programs (xsk_def_prog, xsk_xdp_drop,…
> - [Low] testapp_too_many_frags() builds the stream from
>   max_frags_override no matter which mode the local TX endpoint runs in.
> - [Low] parse_command_line() rejects --listen and --max-frags without
>   --hw, but it silently accepts --udp-src, --udp-dst, --udp-port and…
> - [Low] In hardware mode, sync_endpoints() exchanges READY and then each
>   endpoint calls wait_link_up() on its own interface only, once.
> 
> --
> 
> selftests: xsk: add a hardware mode to xskxceiver
> 
> This commit adds a --hw mode to xskxceiver. In this mode,
> drivers/net/hw/xsk.py can run AF_XDP cases between a zero-copy DUT and an
> SKB-mode remote over a physical link, using UDP/IPv4 frames between the
> real MACs. It also adds --queue, --listen and --max-frags, waits for the
> link after endpoint synchronization, defaults cases to ETH_DATA_LEN, and
> restores the original MTU in cleanup_iface().
> 
> > diff --git a/tools/testing/selftests/net/lib/xsk/test_xsk.c b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> > index f67a32c0260a3..a280956d53f4a 100644
> > --- a/tools/testing/selftests/net/lib/xsk/test_xsk.c
> > +++ b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> 
> [ ... ]
> 
> > @@ -84,11 +97,72 @@ static void write_payload(void *dest, u32 pkt_nb, u32 start, u32 size)
> 
> [ ... ]
> 
> >  	memcpy(eth_hdr->h_dest, xsk->dst_mac, ETH_ALEN);
> >  	memcpy(eth_hdr->h_source, xsk->src_mac, ETH_ALEN);
> > -	eth_hdr->h_proto = htons(ETH_P_LOOPBACK);
> > +	if (!udp_packets) {
> > +		eth_hdr->h_proto = htons(ETH_P_LOOPBACK);
> > +		return;
> > +	}
> > +
> > +	eth_hdr->h_proto = htons(ETH_P_IP);
> 
> [Severity: Low]
> At this commit hardware mode sends UDP/IPv4 test frames, but xsk_def_prog,
> xsk_xdp_drop and xsk_xdp_populate_metadata in xsk_xdp_progs.bpf.c still act
> on every frame received on the queue:
> 
>     return bpf_redirect_map(&xsk, 0, XDP_DROP);
> 
> On a physical link, could non-test traffic such as IPv6 ND/MLD or LLDP
> reach the socket on the bound queue? Could it also shift drop_idx or the
> metadata count?
> 
> This is resolved later in the series. The commit "selftests: xsk: pass
> non-test traffic to the stack in hardware mode" adds is_test_packet(), and
> xsk.py adds ntuple steering. drivers/net/hw/xsk.py does not exist yet at
> this commit, so no runner can hit the gap here.
> 
> [ ... ]
> 
> > @@ -1703,14 +1829,36 @@ static int testapp_validate_rx_endpoint(struct test_spec *test,
> 
> [ ... ]
> 
> > +/* An MTU or XDP change can reset a NIC, which takes the link down on both
> > + * hosts. Each endpoint makes its changes before it is ready, so once both are,
> > + * wait for the local link before any traffic.
> > + */
> 
> [Severity: Medium]
> Does this hold for the TX endpoint? worker_testapp_validate_tx() runs the
> barrier first, and only creates the UMEM and the XDP_ZEROCOPY socket after
> it:
> 
> worker_testapp_validate_tx()
>   sync_endpoints()
>     xsk_peer_ready()
>     wait_link_up()
>   testapp_validate_tx_endpoint()
>     thread_common_ops()
>       xsk_configure()
>         xsk_socket__create()
>     send_pkts()
> 
> The RX side binds in testapp_prepare_rx_endpoint() before sync_endpoints(),
> so only the TX bind happens after the barrier.

corrected comment in v4

> 
> On some zero-copy drivers the pool setup restarts the interface. For
> example, dpaa2_xsk_enable_pool() in dpaa2-xsk.c does:
> 
> 	up = netif_running(dev);
> 	if (up)
> 		dev_close(dev);
> 	...
> 	if (up)
> 		dev_open(dev, NULL);
> 
> dpaa2 advertises NETDEV_XDP_ACT_XSK_ZEROCOPY. Can send_pkts() then
> transmit while the carrier is still coming back? That would lose the
> frames and leave RX to hit the 20s timeout in hardware TX-role cases. The
> code is the same at the end of the series.
> 
> > +static int wait_link_up(struct test_spec *test)
> 
> [ ... ]
> 
> >  static int sync_endpoints(struct test_spec *test)
> >  {
> >  	int err = xsk_peer_ready(ctrl_peer);
> >  
> > -	if (err)
> > +	if (err) {
> >  		ksft_print_msg("Endpoint synchronization failed at step %u: %d (%s)\n",
> >  			       test->current_step, err, strerror(-err));
> > -	return err;
> > +		return err;
> > +	}
> > +	return hw_test ? wait_link_up(test) : 0;
> 
> [Severity: Low]
> Each endpoint checks only its own interface here, and nothing comes after
> the link check. TX goes into send_pkts() as soon as its own ETHTOOL_GLINK
> reads up, and pacing_tx_sync() does not block. xsk_wait_link_up() returns
> on the first sample that reads up:
> 
> 		if (err || edata.data)
> 			return err;
> 
> Suppose the RX host's driver applies an MTU, XDP or ring change through an
> asynchronous reset, and the link drops only after READY was delivered.
> Could TX then act on a stale up state and transmit into the reset?
> 
> At the end of the series the control connection runs over the tested link.
> That covers drivers that reset synchronously, which is what ice, i40e,
> mlx5, igb and ixgbe do. No mainline driver with an asynchronous reset was
> identified, so this may only be theoretical.
> 
> [ ... ]
> 
> > @@ -2284,7 +2444,9 @@ int testapp_too_many_frags(struct test_spec *test)
> >  	u32 max_frags, i;
> >  	int ret = TEST_FAILURE;
> >  
> > -	if (test->mode == TEST_MODE_ZC) {
> > +	if (max_frags_override) {
> > +		max_frags = max_frags_override;
> > +	} else if (test->mode == TEST_MODE_ZC) {
> 
> [Severity: Low]
> The override replaces the limit no matter which mode the transmitting
> endpoint runs in. If the SKB-mode remote were TX with the DUT's zero-copy
> limit, wouldn't the copy-mode TX path send the "too many" packet anyway?
> 
> At the end of the series TOO_MANY_FRAGS is listed only in hw_tx_tests[].
> xsk.py always runs it with the zero-copy DUT as TX, so the mismatch does
> not appear reachable there. Would a short comment noting that assumption
> next to the override help?
> 
> [ ... ]
> 
> > @@ -2378,6 +2540,7 @@ static int detect_ifobj_caps(struct ifobject *ifobj)
> >  	if (query_opts.feature_flags & NETDEV_XDP_ACT_RX_SG)
> >  		ifobj_set_cap(ifobj, XSK_CAP_MBUF);
> >  	if (query_opts.feature_flags & NETDEV_XDP_ACT_XSK_ZEROCOPY) {
> > +		ifobj_set_cap(ifobj, XSK_CAP_ZC_ADVERTISED);
> 
> [Severity: Medium]
> A hardware SKB-mode endpoint still gates every multi-buffer case on
> XSK_CAP_MBUF. That cap comes from the native driver NETDEV_XDP_ACT_RX_SG
> flag, queried with XDP_FLAGS_DRV_MODE. In __testapp_validate_traffic():
> 
> 	mbuf_cap = test->mode == TEST_MODE_ZC ? XSK_CAP_MBUF_ZC :
> 						XSK_CAP_MBUF;
> 
> 	if (!ifobj_has_cap(ifobj1, mbuf_cap) ||
> 	    (ifobj2 && !ifobj_has_cap(ifobj2, mbuf_cap))) {
> 		...
> 		return TEST_SKIP;
> 	}
> 
> Generic XDP does not need native RX_SG for xdp.frags programs.
> netif_receive_generic_xdp() handles non-linear skbs through
> skb_cow_data_for_xdp().
> 
> Take a remote NIC that does not set NETDEV_XDP_ACT_RX_SG, such as igb, igc
> or ixgbe. The remote then skips SEND_RECEIVE_9K_PACKETS, UNALIGNED_9K,
> XDP_METADATA_COPY_MULTI_BUFF, the ADJUST_TAIL_*_MULTI_BUFF and
> *_INV_DESC_MULTI_BUFF cases, and TOO_MANY_FRAGS. The zero-copy DUT checks
> XSK_CAP_MBUF_ZC instead and continues.
> 
> Does the DUT then fail in sync_endpoints() once the remote closes the
> control channel? At the end of the series xsk.py reports this one-sided
> skip as SKIP, so multi-buffer coverage is lost whenever the remote is a
> non-SG NIC.
> 
> The commit message says:
> 
>     - a hardware endpoint probes only the mode it runs in, taking zero-copy
>       support from NETDEV_XDP_ACT_XSK_ZEROCOPY
> 
> Should XSK_CAP_MBUF follow the same rule for an SKB-mode endpoint?

I assume this implies mixed NICs setup, say ice<->mlx5. mlx5 supports
RX_SG but not ZC_MBUF, so mbuf cases for ice as DUT will run.

I view this as corner case of funky setup, better to be addressed but
again it is rather minor, I haven't seen in netdev CI a setup which would
be currently offended by this.

> 
> [ ... ]
> 
> > diff --git a/tools/testing/selftests/net/lib/xsk/xskxceiver.c b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> > index 769718cad8cd7..7044822f23c0a 100644
> > --- a/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> > +++ b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> 
> [ ... ]
> 
> > @@ -306,6 +378,10 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
> >  	    !*opt_peer_host || !opt_peer_port || opt_run_test == RUN_ALL_TESTS ||
> >  	    opt_mode == TEST_MODE_ALL)
> >  		print_usage(argv);
> > +	if (opt_hw && (!opt_udp_src || !opt_udp_dst || !opt_udp_port || !opt_peer_mac))
> > +		print_usage(argv);
> > +	if ((opt_listen || opt_max_frags) && !opt_hw)
> > +		print_usage(argv);
> >  }
> 
> [Severity: Low]
> This isn't a bug, but --udp-src, --udp-dst, --udp-port and --peer-mac are
> accepted without --hw and then silently ignored. main() calls
> xsk_set_udp_packet_format() only under opt_hw, and setup_peer() copies
> opt_peer_mac only under opt_hw.
> 
> Should these options be rejected without --hw, like --listen and
> --max-frags are? print_usage() also describes them as if they always apply.
> 
> [ ... ]
> 
> > @@ -401,8 +487,16 @@ static void cleanup_iface(struct ifobject *ifobj)
> >  		xsk_detach_xdp_program(ifobj->ifindex,
> >  				       ifobj->mode == TEST_MODE_SKB ?
> >  				       XDP_FLAGS_SKB_MODE : XDP_FLAGS_DRV_MODE);
> > +	/* After the detach, as an attached program can cap the MTU. */
> > +	if (ifobj->dev_mtu != ifobj->orig_mtu) {
> > +		err = xsk_set_mtu(ifobj->ifindex, ifobj->orig_mtu);
> > +		if (err)
> > +			ksft_print_msg("Failed to restore MTU %d on %s\n",
> > +				       ifobj->orig_mtu, ifobj->ifname);
> > +	}
> >  unload:
> >  	xsk_unload_xdp_programs(ifobj);
> > +	return err;
> >  }
> 
> [Severity: Medium]
> A failed MTU restore now fails the endpoint. The ring restore just above
> this hunk still ignores its return value:
> 
> 	if (ifobj_has_cap(ifobj, XSK_CAP_HW_RING))
> 		hw_ring_size_reset(ifobj);
> 
> set_hw_ring_size() can fail with a non-EBUSY error, or after the EBUSY
> retries run out. In that case hw_ring_size_reset() keeps the changed sizes
> for a later reset. No later reset happens, because the process exits, and
> the endpoint still reports PASS.
> 
> This patch makes that more likely. __test_spec_init() now resets rings on
> any local ifobj, and testapp_hw_sw_max_ring_size() now sets the receiver's
> rx_pending to rx_max_pending. The final xsk.py relies on the endpoints to
> restore ring sizes.
> 
> Can a failed restore leave the NIC with enlarged rings? The next run's
> detect_ifobj_caps() would then record those as set_ring.default_tx and
> set_ring.default_rx. Should the result of hw_ring_size_reset() also feed
> err here?

In v4 the runner sets and restores the rings, not xskxceiver, including
when an endpoint is killed.

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