Re: [PATCH v3 net-next 09/15] selftests: xsk: run one test case per xskxceiver invocation

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

On Sat, Oct 17, 2026 at 03:06:08AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · 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] test_xsk.py main()
>   (tools/testing/selftests/net/test_xsk.py:123-136) raises…
> - [Medium] The commit message says test_xsk.py "leaves those [UNALIGNED]
>   cases out when it cannot reserve them".
> - [Medium] xsk_reserve_hugepages() (lib/py/xsk.py:15-25) raises
>   vm.nr_hugepages, which sizes the pool of the *default* huge page size.
> - [Low] xsk_wait_endpoint_exit()
>   (tools/testing/selftests/net/lib/py/xsk.py:44-58) kills xskxceiver…
> 
> --
> 
> selftests: xsk: run one test case per xskxceiver invocation
> 
> This replaces test_xsk.sh and xsk_prereqs.sh with test_xsk.py. The new
> script runs xskxceiver once per mode and case, each on a fresh veth pair,
> and reports every case as its own KTAP result. xskxceiver now requires -m
> and -t and runs exactly one case, and the shared huge page, busy-poll and
> endpoint wait helpers move to net/lib/py/xsk.py.
> 
> > diff --git a/tools/testing/selftests/net/lib/py/xsk.py b/tools/testing/selftests/net/lib/py/xsk.py
> > new file mode 100644
> > index 0000000000000..50985bcb5b80c
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/lib/py/xsk.py
> > @@ -0,0 +1,58 @@
> 
> [ ... ]
> 
> > +def xsk_reserve_hugepages(saved, host=None):
> > +    """Raise vm.nr_hugepages on host to what the UNALIGNED cases need, and
> > +    return whether it has that many. Add the count to restore to saved, a
> > +    list for xsk_release_hugepages()."""
> > +    have = int(cmd("sysctl -n vm.nr_hugepages", host=host).stdout)
> 
> [Severity: Medium]
> What happens here when vm.nr_hugepages can't be read or written?
> 
> cmd() is called with the default fail=True. A non-zero exit from sysctl
> raises CmdExitFailure, and a missing sysctl binary raises
> FileNotFoundError from Popen.

Fixed in v4:
the pool is the 2mb one in /sys/kernel/mm/hugepages/hugepages-2048kB,
read with fail=False, so a host without it only loses the UNALIGNED cases

However, seems we would need to audit rest of tests relying on hugepages
being present.

> 
> main() calls this before ksft_run(), so nothing turns the exception into
> a per-case result. The whole script dies with a traceback and prints no
> KTAP plan.
> 
> This can happen with CONFIG_HUGETLB_PAGE=n (the net selftests config
> doesn't enable it), with a read-only /proc/sys in a container, or when
> procps isn't installed. The old test_xsk.sh ran in all of these setups.
> 
> The commit message says test_xsk.py:
> 
>     "leaves those cases out when it cannot reserve them"
> 
> Here, though, a failed reservation aborts every case, not only the
> UNALIGNED ones. xskxceiver already handles missing huge pages by itself:
> 
> tools/testing/selftests/net/lib/xsk/test_xsk.c:detect_ifobj_caps() {
>     ...
> 	if (hugepages_present())
> 		ifobj_set_cap(ifobj, XSK_CAP_UNALIGNED);
>     ...
> }
> 
> Should failures here be caught and treated as "no huge pages"?
> 
> > +    if have >= NR_HUGEPAGES:
> > +        return True
> > +    cmd(f"sysctl -qw vm.nr_hugepages={NR_HUGEPAGES}", host=host)
> 
> [Severity: Medium]
> Does vm.nr_hugepages size the right pool for the UNALIGNED cases?
> 
> vm.nr_hugepages sets the pool for the default huge page size. The
> UNALIGNED UMEM always asks for 2MB pages:
> 
> tools/testing/selftests/net/lib/xsk/test_xsk.c:thread_common_ops() {
>     ...
> 	if (umem->unaligned_mode)
> 		mmap_flags |= MAP_HUGETLB | MAP_HUGE_2MB;
>     ...
> }

addressed in v4 by interfacing hugepages-2048kB

> 
> The xskxceiver probe passes MAP_HUGE_2MB as the mmap offset instead of in
> the flags, so it actually checks the default-size pool:
> 
> tools/testing/selftests/net/lib/xsk/test_xsk.c:hugepages_present() {
>     ...
> 	bufs = mmap(NULL, mmap_sz, PROT_READ | PROT_WRITE,
> 		    MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, MAP_HUGE_2MB);
>     ...
> }
> 
> Before this patch nothing filled the default pool, so on a typical host
> the probe failed and the UNALIGNED cases skipped.
> 
> Now the default pool is raised to 32. Take a setup where the default huge
> page size is not 2MB, such as s390 (1MB), ppc64 hash (16MB) or arm64 with
> a 16K granule (32MB). The probe passes and XSK_CAP_UNALIGNED is set. The
> real MAP_HUGE_2MB mmap then fails with EINVAL or ENOMEM.
> 
> Would those cases now fail where they used to skip?
> 
> Larger default sizes are a second concern: x86 with default_hugepagesz=1G,
> or arm64 with a 64K granule (512MB). There, nr_hugepages=32 also reserves
> 16-32GB for the run, and the UNALIGNED cases can't use any of it.
> 
> Is /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages the pool that
> should be raised here?
> 
> > +    saved.append((host, have))
> > +    got = int(cmd("sysctl -n vm.nr_hugepages", host=host).stdout)
> > +    return got >= NR_HUGEPAGES
> 
> [ ... ]
> 
> > +def xsk_wait_endpoint_exit(endpoint, label, timeout):
> > +    """Wait up to timeout seconds for xskxceiver to exit, kill it if it does
> > +    not, and return its exit code. Log its output, a KTAP stream of its own,
> > +    prefixed with label, only if it did not pass."""
> > +    try:
> > +        endpoint.process(terminate=False, fail=False, timeout=timeout)
> > +    except subprocess.TimeoutExpired:
> > +        endpoint.proc.kill()
> 
> [Severity: Low]
> Can xskxceiver be left running when something other than TimeoutExpired
> interrupts this wait?

fixed in v4

> 
> ksft_run() installs a SIGTERM handler that raises KsftTerminate:
> 
> tools/testing/selftests/net/lib/py/ksft.py:_ksft_intr() {
>     ...
>     if term_cnt == 1:
>         raise KsftTerminate()
>     ...
> }
> 
> During a case, the main thread is almost always blocked in
> endpoint.process() here. If the runner sends SIGTERM to the Python
> process, KsftTerminate passes through this function and the child is
> never killed.
> 
> ksft_run() then runs the deferred "ip link del". That removes the veth
> pair while xskxceiver still has its sockets and XDP program bound, and
> the script exits with the child orphaned.
> 
> The child is started by _start() with cmd(background=True). The bkg()
> context manager would terminate it on any exception.
> 
> Would a try/finally, or bkg(), be a better fit here?
> 
> > +        endpoint.process(terminate=False, fail=False)
> > +        ksft_pr(f"killed after {timeout}s", line_pfx=f"{label}|")
> 
> [ ... ]
> 
> > diff --git a/tools/testing/selftests/net/test_xsk.py b/tools/testing/selftests/net/test_xsk.py
> > new file mode 100755
> > index 0000000000000..acfdba55fbae8
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/test_xsk.py
> > @@ -0,0 +1,140 @@
> 
> [ ... ]
> 
> > +def main():
> > +    # ksft_run() exits on -l and -h before main() restores the huge pages.
> > +    if "-l" in sys.argv or "-h" in sys.argv:
> > +        sys.exit(f"{sys.argv[0]}: -l and -h are not supported")
> > +    if not XSK_BIN.exists():
> > +        print(f"1..0 # SKIP {XSK_BIN} was not built")
> > +        sys.exit(SKIP_CODE)
> > +    saved_hugepages = []
> > +    hugepages = xsk_reserve_hugepages(saved_hugepages)
> > +    cases = _get_cases(hugepages)
> > +    ksft_run([ksft_variants(cases)(test_xsk),
> > +              ksft_variants(cases)(test_xsk_busy_poll)])
> > +    xsk_release_hugepages(saved_hugepages)
> 
> [Severity: Medium]
> Is the huge page count restored on every exit path?

fixed in v4, Bjorn pointed out bunch of things left uncleaned (including
hugepages) at exit/exception path at hw runner and i also addressed the
veth side (here).

> 
> The commit message says test_xsk.py "restores the count when it ends" and
> that "ksft_run() exits on -l and -h before the count is restored, so
> test_xsk.py rejects both".
> 
> The argv check only matches the exact tokens "-l" and "-h". Also,
> xsk_release_hugepages() is not in a finally block. That leaves several
> paths that exit after the sysctl has been raised.
> 
> Any unknown option makes _KsftArgs exit. That includes --help and the old
> test_xsk.sh options -m, -v and -d:
> 
> tools/testing/selftests/net/lib/py/ksft.py:_KsftArgs.__init__() {
>     ...
>         try:
>             opts, _ = getopt.getopt(sys.argv[1:], 'hlt:T:')
>         except getopt.GetoptError as e:
>             print(e, file=sys.stderr)
>             sys.exit(1)
>     ...
> }
> 
> getopt also accepts grouped short options, so -lt 'test_xsk.drv_*' or -hl
> gets past the argv check. It then reaches the sys.exit(0) for -h in
> _KsftArgs, or this one in _ksft_generate_test_cases():
> 
>     if cli_args.list_tests:
>         for _, _, name in test_cases:
>             print(name)
>         sys.exit(0)
> 
> _get_cases() also runs "xskxceiver -l" through cmd() with fail=True. A
> CmdExitFailure there, or a Ctrl-C before ksft_run(), escapes main() the
> same way.
> 
> In each case the raised vm.nr_hugepages stays on the host after the
> script exits.
> 
> SystemExit is an exception, so a try/finally around everything after
> xsk_reserve_hugepages() would cover all of these paths, plain -l and -h
> included. Would that remove the need to reject -l and -h at all?
> 
> > +    ksft_exit()
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014184539.774708-1-maciej.fijalkowski%40intel.com