Re: [PATCH v3 net-next 06/15] selftests: xsk: move the AF_XDP test suite to selftests/net

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

On Sat, Oct 17, 2026 at 03:06:06AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> 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] Undisclosed relicensing.
> - [Low] In tools/testing/selftests/net/lib/Makefile, the xskxceiver
>   binary is only removed by `make clean` when bpftool happens to work at…
> 
> --
> 
> selftests: xsk: move the AF_XDP test suite to selftests/net
> 
> This commit moves the xskxceiver engine, the xsk userspace library, the
> test definitions and the XDP program from selftests/bpf to
> selftests/net/lib/xsk. It also moves test_xsk.sh and xsk_prereqs.sh to
> selftests/net, and net/lib now builds xskxceiver only when a working
> bpftool is found. The two ethtool ring helpers move from
> bpf/network_helpers.c into xsk.c.
> 
> > diff --git a/tools/testing/selftests/net/lib/Makefile b/tools/testing/selftests/net/lib/Makefile
> > index ff83603397d06..c26088da04892 100644
> > --- a/tools/testing/selftests/net/lib/Makefile
> > +++ b/tools/testing/selftests/net/lib/Makefile
> > @@ -5,6 +5,15 @@ CFLAGS += -I../../../../../usr/include/ $(KHDR_INCLUDES)
> >  # Additional include paths needed by kselftest.h
> >  CFLAGS += -I../../
> >  
> > +# xskxceiver needs a working bpftool to generate its skeleton
> > +BPFTOOL ?= bpftool
> > +HAS_BPFTOOL := $(shell $(BPFTOOL) version >/dev/null 2>&1 && echo y)
> > +ifeq ($(HAS_BPFTOOL),y)
> > +COND_GEN_FILES += xskxceiver
> > +else
> > +$(warning excluding xskxceiver, bpftool not installed or not working)
> > +endif
> > +
> 
> [ ... ]
> 
> > @@ -13,6 +22,7 @@ TEST_FILES := \
> >  
> >  TEST_GEN_FILES := \
> >  	$(patsubst %.c,%.o,$(wildcard *.bpf.c)) \
> > +	$(COND_GEN_FILES) \
> >  	csum \
> >  	gro \
> >  	xdp_helper \
> > @@ -23,3 +33,28 @@ TEST_INCLUDES := $(wildcard py/*.py sh/*.sh)
> 
> [ ... ]
> 
> > +$(OUTPUT)/xskxceiver: $(XSK_SOURCES) $(XSK_HEADERS) $(XSK_SKEL) $(BPFOBJ)
> > +	$(call msg,CC,,$@)
> > +	$(Q)$(CC) -I$(top_srcdir)/tools/include $(CFLAGS) -I$(OUTPUT) \
> > +		-I$(SCRATCH_DIR)/include \
> > +		$(XSK_SOURCES) $(BPFOBJ) $(LDLIBS) $(XSK_LDLIBS) -o $@
> > +
> > +EXTRA_CLEAN += $(XSK_BPF_OBJ) $(XSK_SKEL)
> 
> [Severity: Low]
> Will make clean leave lib/xskxceiver behind if bpftool can't be used
> when clean runs?
> 
> HAS_BPFTOOL is evaluated when the Makefile is parsed. xskxceiver only
> ends up in TEST_GEN_FILES if "$(BPFTOOL) version" succeeds during that
> same make invocation.
> 
> The clean target in lib.mk removes:
> 
> 	$(RM) -r $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) $(EXTRA_CLEAN)
> 
> Here EXTRA_CLEAN only lists the .bpf.o and the skeleton.
> 
> Say the binary was built with a working bpftool, and make clean later
> runs where bpftool doesn't work. That can happen with a different PATH,
> a BPFTOOL= override, or a cross environment. The intermediates get
> removed, but $(OUTPUT)/xskxceiver stays.
> 
> test_xsk.sh only checks:
> 
> 	if [ ! -x "./${XSKOBJ}" ]; then
> 
> So it would run the stale binary instead of skipping.

Stale binary is still a valid xskxceiver...test_xsk.sh is later gone so
this check is not something we should worry about.

However, adding it to EXTRA_CLEAN would not hurt, as a follow-up. Maybe we
should audit also other binaries (iou-zcrx) while at it.

> 
> Would it work to add $(OUTPUT)/xskxceiver to EXTRA_CLEAN
> unconditionally? The same Makefile logic is still there at the end of
> the series.
> 
> > diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/net/lib/xsk/xsk.c
> > similarity index 95%
> > rename from tools/testing/selftests/bpf/xsk.c
> > rename to tools/testing/selftests/net/lib/xsk/xsk.c
> > index 25d568abf0f20..6bd32caf5d085 100644
> > --- a/tools/testing/selftests/bpf/xsk.c
> > +++ b/tools/testing/selftests/net/lib/xsk/xsk.c
> 
> [ ... ]
> 
> > @@ -779,3 +778,51 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> >  		close(xsk->fd);
> >  	free(xsk);
> >  }
> > +
> > +int get_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
> > +{
> 
> [Severity: Medium]
> Does this move change the licence of these helpers?
> 
> get_hw_ring_size() and set_hw_ring_size() come from
> tools/testing/selftests/bpf/network_helpers.c, which has:
> 
> 	// SPDX-License-Identifier: GPL-2.0-only
> 
> xsk.c starts with:
> 
> 	// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> 
> Tushar Vyavahare added both helpers in commits 90a695c3d31e1c and
> bee3a7b0762422 under GPL-2.0-only. After the move they are unchanged,
> but they now sit in a file that offers its contents under LGPL-2.1 or
> BSD-2-Clause, with no per-function licence note.
> 
> The commit message only says "Move the two ethtool ring helpers from
> network_helpers into xsk.c". It doesn't mention the licence change, and
> there is no Ack from the original author. This is still true at the end
> of the series.
> 
> Could the helpers go into one of the GPL-2.0 files in the same
> directory instead, such as test_xsk.c or xskxceiver.c? If they need to
> stay in xsk.c, could the relicensing be stated explicitly, with an Ack
> from the copyright holder?

libbpf was at latter dual license AFAIR, I did not pay much attention that
apparently network_helpers.c had it different.

If Tushar would insist as original author I could fixup licenses, but I
don't see it as a show-stopper.

> 
> > +	struct ifreq ifr = {0};
> > +	int sockfd, err;
> 
> [ ... ]
> 
> > +int set_hw_ring_size(char *ifname, struct ethtool_ringparam *ring_param)
> > +{
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014184539.774708-1-maciej.fijalkowski%40intel.com