Re: [PATCH net-next v4 3/4] selftests: drv-net: add a kperf runner

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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] KperfRunner.close() in
  tools/testing/selftests/drivers/net/lib/py/kperf.py is meant to keep…
- [Low] In KperfRunner.__enter__
  (tools/testing/selftests/drivers/net/lib/py/kperf.py), a server is…
- [Low] selftests: drv-net: KperfRunner.__enter__() misses BaseException
  cleanup

--

selftests: drv-net: add a kperf runner

This adds a KperfRunner helper to the drv-net python library. It starts
kperf servers on the local and remote hosts, runs kperf-client, and
returns the aggregate throughput of both ends. Each server gets its own
PID and log file, and close() stops the servers with kperf-server --kill
and reports any cleanup failures.

> diff --git a/tools/testing/selftests/drivers/net/lib/py/kperf.py b/tools/testing/selftests/drivers/net/lib/py/kperf.py
> new file mode 100644
> index 0000000000000..8ee8c5ba2ff7b
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/lib/py/kperf.py

[ ... ]

> +    def _server(self, remote, kill=False):
> +        host = self.env.remote if remote else None
> +        address = self.env.remote_addr if remote else self.env.addr
> +        args = ["kperf-server", "--pid-file", self._pid_file(remote)]
> +        if kill:
> +            args.append("--kill")
> +        else:
> +            args += ["--addr", address, "--port", str(self._port)]
> +            # The output of a daemon is lost, unless it can log
> +            usage = cmd("kperf-server --help", host=host, fail=False)
> +            if "--log-file" in usage.stdout + usage.stderr:
> +                args += ["--log-file", self._log_file(remote)]
> +        return cmd(shlex.join(args) if remote else args, host=host,
> +                   fail=not kill)

[ ... ]

> +    def __enter__(self):
> +        try:
> +            for remote in (False, True):
> +                self._server(remote)
> +                self._started.append(remote)

[Severity: Low]
A server is added to self._started only after self._server(remote)
returns successfully. What happens if kperf-server has already
daemonized and written its PID file, and the start command then raises?

For example, the remote ssh session could drop and exit with 255, or it
could stall past the default 20 second cmd() timeout after the daemon
has detached. CmdExitFailure or TimeoutExpired would be raised before
the append.

The except block below calls self.close(), which only walks
self._started, so --kill is never issued for that PID file. Could this
leave the daemon running, along with its PID and log files, and holding
devmem or RX steering into later tests?

Since --kill already runs with fail=False, one option is to record the
server before starting it. Another is to always issue --kill for both
PID files during cleanup.

> +            wait_port_listen(self._port)
> +            wait_port_listen(self._port, host=self.env.remote)
> +        except Exception:
> +            self.close()
> +            raise
> +        return self
> +
> +    def close(self):
> +        """Stop the started kperf servers, which waits for their sessions."""
> +        failures = []
> +        while self._started:
> +            remote = self._started.pop()
> +            command = self._server(remote, kill=True)
> +            if command.ret:
> +                failures.append(command)

[Severity: Low]
Does this rollback still run if the test is interrupted while the
servers are starting?

The kselftest SIGTERM handler, _ksft_intr() in
tools/testing/selftests/net/lib/py/ksft.py, raises KsftTerminate,
which subclasses KeyboardInterrupt. Neither of them derives from
Exception. A SIGTERM timeout from the runner, or a Ctrl-C, can arrive
while the second kperf-server is being started or while __enter__()
is polling in wait_port_listen(). In that case this handler is
skipped.

The with statement also does not call __exit__() when __enter__()
raises. So the servers already recorded in self._started never get a
kperf-server --kill. They would keep running in the background and
listening on the control port. Their PID and log files under
/tmp/ksft-kperf-* would also be left behind.

The handler re-raises anyway, so would it make sense to catch
BaseException here? That way the started servers are also stopped on
SIGTERM and KeyboardInterrupt.

[Severity: Medium]
Can the kill command raise here instead of returning a non-zero ret?

_server(remote, kill=True) calls cmd() with fail=False but keeps the
default timeout from tools/testing/selftests/net/lib/py/utils.py:

    def __init__(self, comm, shell=None, fail=True, expect_fail=False, ns=None,
                 background=False, host=None, timeout=20, ksft_ready=None,

That leads to cmd.process()->_process_terminate(), which does:

    stdout, stderr = self.proc.communicate(timeout=timeout)

fail=False only affects the return code check after communicate()
returns. If the kill takes longer than 20 seconds, subprocess.
TimeoutExpired is raised anyway. The commit message says this can
happen, since --kill waits for sessions that may still be releasing
devmem and RX steering. A stalled ssh link would trigger it too.

When that happens, the exception leaves the while self._started loop.
pop() handles the remote server first, so if the remote kill times
out:

  - the local kperf-server is never killed
  - neither log file is removed by the rm -f
  - the RuntimeError that collects the failures is skipped
  - the timed out kill subprocess keeps running, because communicate()
    does not kill the child on timeout

__exit__ calls close() only once, so would a stale server be left
holding the devmem or RX steering resources that the next test needs?

On the __enter__ error path, the TimeoutExpired from close() also
becomes the exception that is raised, and the original setup error is
only chained as context.

Would it make sense to pass a larger timeout to the kill command? The
loop could also catch exceptions per server, so the remaining servers
and log files still get cleaned up.

> +            host = self.env.remote if remote else None
> +            args = ["rm", "-f", self._log_file(remote)]
> +            cmd(shlex.join(args) if remote else args, host=host, fail=False)
> +        if failures:
> +            details = "\n".join(str(command) for command in failures)
> +            raise RuntimeError(f"failed to stop kperf server(s)\n{details}")
> +
> +    def __exit__(self, _exc_type, _exc_value, _exc_tb):
> +        self.close()

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111000246.962871-1-sdf%40fomichev.me