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 <[email protected]>` 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 <[email protected]> > Signed-off-by: Kuba Pawlak <[email protected]> [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
