[PATCH net-next] selftests: net: Skip so_incoming_cpu on single-CPU systems

Kuba Pawlak posted 1 patch 3 weeks ago
tools/testing/selftests/net/so_incoming_cpu.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
[PATCH net-next] selftests: net: Skip so_incoming_cpu on single-CPU systems
Posted by Kuba Pawlak 3 weeks ago
The test requires at least two CPUs to exercise the distribution of
connections between per-CPU listeners.  It currently enforces this
prerequisite with an assertion, causing all test cases to fail on
single-vCPU systems even though no kernel functionality was tested.

Report the unsupported environment as a skip instead.  Check the
prerequisite before setting up the network namespace so skipped tests do
not make unnecessary system changes.

Link: https://bugs.launchpad.net/ubuntu-kernel-tests/+bug/2023546

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Kuba Pawlak <kuba.pawlak@canonical.com>
---
 tools/testing/selftests/net/so_incoming_cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/so_incoming_cpu.c b/tools/testing/selftests/net/so_incoming_cpu.c
index 4740701f1..1b7344238 100644
--- a/tools/testing/selftests/net/so_incoming_cpu.c
+++ b/tools/testing/selftests/net/so_incoming_cpu.c
@@ -81,10 +81,11 @@ static int nr_client_per_server, nr_server, nr_client;
 
 FIXTURE_SETUP(so_incoming_cpu)
 {
-	setup_netns(_metadata);
-
 	nr_server = get_nprocs();
-	ASSERT_LE(2, nr_server);
+	if (nr_server < 2)
+		SKIP(return, "requires at least two CPUs");
+
+	setup_netns(_metadata);
 
 	if (NR_CLIENT_PER_SERVER_DEFAULT * nr_server < NR_PORT)
 		nr_client_per_server = NR_CLIENT_PER_SERVER_DEFAULT;
-- 
2.43.0
Re: [PATCH net-next] selftests: net: Skip so_incoming_cpu on single-CPU systems
Posted by netdev-bot+sashiko@kernel.org 2 weeks, 2 days ago
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 2

- [Medium] The new skip gate in FIXTURE_SETUP(so_incoming_cpu)
  (tools/testing/selftests/net/so_incoming_cpu.c:84-86) tests only the…
- [Low] Missing `Fixes:` tag on a user-visible selftest bug fix.
- [Low] The commit message uses the non-standard trailer `Co-authored-by:
  Copilot <223556219+Copilot@users.noreply.github.com>` with no…

--

selftests: net: Skip so_incoming_cpu on single-CPU systems

The change replaces the ASSERT_LE(2, nr_server) prerequisite in
FIXTURE_SETUP(so_incoming_cpu) with a SKIP, and moves the check ahead of
setup_netns() so a skipped run does not create a network namespace.

Link: https://bugs.launchpad.net/ubuntu-kernel-tests/+bug/2023546

A couple of questions about the commit trailers first, then one about the
new prerequisite check.

> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
> Signed-off-by: Kuba Pawlak <kuba.pawlak@canonical.com>

[Severity: Low]
Should this trailer be Co-developed-by: instead?
Documentation/process/submitting-patches.rst documents Co-developed-by:
for shared authorship and states:

    Since Co-developed-by: denotes authorship, every Co-developed-by:
    must be immediately followed by a Signed-off-by: of the associated
    co-author.

Co-authored-by: is not recognised by the kernel tooling, and there is no
Signed-off-by: for the named co-author here. Since a bot account cannot
provide a DCO sign-off, would it be better to mention the AI assistance in
the changelog text rather than as an authorship trailer?

[Severity: Low]
Would a Fixes: tag be appropriate here? The assertion being converted came
in with commit 6df96146b2025e ("selftest: Add test for SO_INCOMING_CPU.")
as:

    ASSERT_LE(2, self->nproc);

and commit 97de5a15edf2d2 later moved it into its current form:

    ASSERT_LE(2, nr_server);

Without a Fixes: trailer, backport tooling will not associate this fix with
either commit, so single-vCPU runs on maintained trees keep reporting the
failures described in the Launchpad bug.

> diff --git a/tools/testing/selftests/net/so_incoming_cpu.c b/tools/testing/selftests/net/so_incoming_cpu.c
> index 4740701f1a9ac..1b73442386aef 100644
> --- a/tools/testing/selftests/net/so_incoming_cpu.c
> +++ b/tools/testing/selftests/net/so_incoming_cpu.c
> @@ -81,10 +81,11 @@ static int nr_client_per_server, nr_server, nr_client;
>  
>  FIXTURE_SETUP(so_incoming_cpu)
>  {
> -	setup_netns(_metadata);
> -
>  	nr_server = get_nprocs();
> -	ASSERT_LE(2, nr_server);
> +	if (nr_server < 2)
> +		SKIP(return, "requires at least two CPUs");

[Severity: Medium]
Is the count of online CPUs the right prerequisite to test here? The code
that consumes nr_server needs the CPU ids 0..nr_server-1 to be individually
usable by the task, not just a count of two or more.

create_clients() pins itself to each id in turn:

    for (i = 0; i < nr_server; i++) {
            CPU_ZERO(&cpu_set);
            CPU_SET(i, &cpu_set);
            ...
            ret = sched_setaffinity(0, sizeof(cpu_set), &cpu_set);
            ASSERT_EQ(ret, 0);

and create_servers() assigns SO_INCOMING_CPU over the same dense range:

    for (i = 0; i < nr_server; i++) {
            self->servers[i] = create_server(_metadata, self, variant, i);

So can the test still report FAIL rather than SKIP when get_nprocs()
returns two or more but some id below nr_server is not runnable?

Two cases come to mind:

- CPUs 0 and 3 online, 1 and 2 offline: get_nprocs() returns 2, the gate is
  not taken, and for i = 1 the mask contains only offline CPU 1, so
  sched_setaffinity() returns EINVAL and ASSERT_EQ(ret, 0) fails.

- A container or CI job started with taskset -c 2,3: for i = 0 the mask
  contains no permitted CPU, sched_setaffinity() returns EINVAL, and the
  test fails again.

Would deriving the prerequisite from sched_getaffinity()/CPU_COUNT(), and
iterating over the ids actually set in that mask, cover the same class of
unsupported environment the commit message describes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904140334.562593-1-kuba.pawlak%40canonical.com