Re: [PATCH net-next v10 2/4] selftests: tc-testing: add fq_pie per-flow class stats test

[netdev-bot+sashiko@kernel.org]

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

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

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 83c0 in tools/testing/selftests/tc-testing/tc-
  tests/qdiscs/fq_pie.json does not do what the commit message says, and…

--

selftests: tc-testing: add fq_pie per-flow class stats test

This adds tc-testing case 83c0. The test builds a TBF + fq_pie hierarchy
on $DUMMY and pushes ping traffic through it. It then checks that
'tc -s class show' reports an fq_pie class.

> Add tc-testing case 83c0: build a TBF + fq_pie hierarchy on $DUMMY, push
> traffic through it with ping, and verify that 'tc -s class show' reports
> an fq_pie class.
>
> The per-flow counters themselves (prob, delay, deficit) are printed with
> the companion iproute2 change, "tc: fq_pie: add support for printing
> per-flow PIE statistics"

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> index 229fe1bf4a906..15b2667285eb4 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> @@ -40,5 +40,32 @@

[ ... ]

> +    },
> +    {
> +        "id": "83c0",
> +        "name": "FQ-PIE class stats accessible via tc class show",

[ ... ]

> +        "cmdUnderTest": "$TC -s class show dev $DUMMY",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC -s class show dev $DUMMY",
> +        "matchPattern": "class fq_pie 2:[0-9]+ .*prob .*delay .*deficit",
> +        "matchCount": "1",

[Severity: Medium]
Will this test fail, rather than skip, on any iproute2 that doesn't have
the companion patch?

The commit message says the test checks that an fq_pie class is reported.
The matchPattern, however, also needs the words prob, delay and deficit
after the class line, and only the out-of-tree iproute2 change prints
them.

On the kernel side, fq_pie_dump_class_stats() in net/sched/sch_fq_pie.c
only sends a binary blob:

net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
    ...
	xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
    ...
	return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
}

Current iproute2 decodes fq_pie xstats with the qdisc layout (pkts_in,
overlimit, overmemory, dropped, ecn_mark, new_flow_count and so on). None
of those labels is prob, delay or deficit. The tbf class 1:1 is printed
before the fq_pie class, so nothing after "class fq_pie 2:1" can supply
those words either.

That means re.findall() gets 0 matches against a matchCount of 1, and
run_one_test() in tdc.py marks the test as failed:

tools/testing/selftests/tc-testing/tdc.py:run_one_test() {
    ...
	if len(match_index) != int(tidx["matchCount"]):
	    res.set_result(ResultState.fail)
    ...
}

tdc can already skip tests that depend on a userspace feature, through
the dependsOn key, which run_one_test() checks. mirred.json uses it, for
example:

    "dependsOn": "$TC actions add action mirred help 2>&1 | grep -q blockid",

Test 83c0 has no dependsOn probe. As a result, a correct kernel tested
with stock iproute2 would show a new tc-testing failure. The selftest
would also depend on an output format that only exists in an unmerged
userspace patch.

Would it make sense to either add a dependsOn probe for the new iproute2
output, or narrow the pattern to just the "class fq_pie 2:N" line as the
commit message describes? The commit message could then be updated to
say which approach the test uses.

> +        "teardown": [
> +            "$TC qdisc del dev $DUMMY handle 1: root",
> +            "$IP addr del 10.10.11.10/24 dev $DUMMY || true"
> +        ]
>      }
>  ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109071017.438478-1-hemendranaik%40gmail.com