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

[netdev-bot+sashiko@kernel.org]

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.

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;
    ...
}

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?

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?

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