Re: [PATCH net-next v6 2/3] 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 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · 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).

- [High] New tdc case 83c0 in tools/testing/selftests/tc-testing/tc-
  tests/qdiscs/fq_pie.json asserts `matchPattern` = "class fq_pie…
- [Medium] Case 83c0's assertion depends on fq_pie having a backlogged
  flow at the exact instant the dump runs, because fq_pie_walk()…
- [Low] Case 83c0 claims to cover the new per-flow fq_pie statistics but
  only checks that the literal label words "prob", "delay" and "deficit"…

--

Verified against the tree. Report follows.

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

This adds tdc case 83c0, which builds a TBF plus fq_pie hierarchy on
$DUMMY, drives traffic through it with ping, and then greps the output of
'tc -s class show' for an fq_pie class line carrying per-flow counters.

> 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 @@
> +        "setup": [
> +            "$IP link set dev $DUMMY up || true",
> +            "$IP addr add 10.10.11.10/24 dev $DUMMY || true",
> +            "$TC qdisc add dev $DUMMY root handle 1: tbf rate 8bit burst 100b latency 100ms",
> +            "$TC qdisc add dev $DUMMY parent 1:1 handle 2: fq_pie limit 100 flows 1",
> +            "ping -c 50 -i 0.001 -W 0.01 -s 56 10.10.11.11 -I $DUMMY > /dev/null 2>&1 || true"
> +        ],

[Severity: Medium]
Can this case fail intermittently depending on the host environment?  The
assertion needs a backlogged flow at the instant the dump runs, because
fq_pie_walk() in net/sched/sch_fq_pie.c skips every flow whose flowchain is
empty:

	for (i = 0; i < q->flows_cnt; i++) {
		if (list_empty(&q->flows[i].flowchain)) {
			arg->count++;
			continue;
		}
		if (!tc_qdisc_stats_dump(sch, i + 1, arg))
			break;
	}

The only step that creates that backlog is the ping line, and its exit
status is discarded with '|| true'.  If ping is not installed, or the
installed iputils rejects the fractional -i 0.001 / -W 0.01 arguments, the
fq_pie queue stays empty and 'tc -s class show' prints no fq_pie class at
all.  tdc.py then takes this branch:

	elif int(tidx["matchCount"]) != 0:
	    res.set_result(ResultState.fail)
	    res.set_failmsg('No output generated by verify command.')

which reports 'No output generated by verify command' rather than pointing
at the missing traffic precondition.

There is also a timing aspect in the passing case: TBF is configured with
'rate 8bit burst 100b', so the bucket starts full and the first frame (an
~42-byte ARP request or a ~98-byte ICMP echo) is dequeued right away.  A run
where only one frame gets enqueued leaves the flow list empty at dump time
even though ping itself "succeeded".  Would it be better to check the ping
result, or to enqueue traffic in a way that does not depend on neighbour
solicitation timing?

[Severity: Low]
This isn't a bug, but with 'flows 1' there is exactly one flow, so the
class-id to flow-index contract between fq_pie_walk() and
fq_pie_dump_class_stats() is not really exercised:

net/sched/sch_fq_pie.c:fq_pie_walk() {
	...
		if (!tc_qdisc_stats_dump(sch, i + 1, arg))
	...
}

net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
	u32 idx = cl - 1;
	...
}

A kernel that attributed statistics to the wrong flow would still satisfy
the pattern.  Two of the five fields in struct tc_fq_pie_cl_stats,
avg_dq_rate and dq_rate_estimating, are not referenced by the test at all,
and none of the unit conversions (probability scaling, PSCHED ticks to
microseconds, byte credits, avg_dq_rate scaling) are checked.  Would using
more than one flow plus matchJSON on 'tc -s -j class show' with numeric
assertions cover the kernel side more directly?

> +        "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: High]
Can this case ever pass with a released iproute2?  The kernel exports the
per-flow counters only as a binary TCA_STATS_APP payload:

net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
	...
	if (idx < q->flows_cnt)
		return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
	...
}

Turning that blob into the literal tokens "prob", "delay" and "deficit"
requires the companion iproute2 change mentioned in the commit message,
"tc: fq_pie: add support for printing per-flow PIE statistics", which is not
merged.  Shipped tc prints only the qdisc-level names (pkts_in, overlimit,
dropped, ecn_mark and friends), so re.findall() returns zero hits and
tdc.py fails the case:

	if len(match_index) != int(tidx["matchCount"]):
	    res.set_result(ResultState.fail)
	    res.set_failmsg('Could not match regex pattern. Verify command output:\n{}'.format(procout))

So the test appears to land in the tree permanently red on every tc-testing
run.  tdc has a probe mechanism for exactly this situation:

	if 'dependsOn' in tidx:
	    ...
	    if (p.returncode != 0):
	        res.set_result(ResultState.skip)
	        res.set_errormsg('probe command: test skipped.')

Would it make sense to add a 'dependsOn' probe so the case skips when the
userspace support is absent, or to assert only what current tc prints?

A related question: the pattern hard-codes the exact label text and ordering
of the not-yet-reviewed userspace formatter.  If the iproute2 side lands with
different wording or field order (say "drop_prob", or JSON-only output), does
this kernel selftest then break while the kernel itself is unchanged?

The commit message says the counters "are printed only after the companion
iproute2 change", but the case has no fallback pattern and no skip
condition, so that dependency reads as an aside rather than as the reason the
test cannot pass today.

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