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

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

- [Medium] Test 83c0 in tools/testing/selftests/tc-testing/tc-
  tests/qdiscs/fq_pie.json depends on an iproute2 change that has not…
- [Low] Test 83c0 relies on a ping step, `ping -c 50 -i 0.001 -W 0.01 -s
  56 10.10.11.11 -I $DUMMY > /dev/null 2>&1 || true`, to put packets…

--

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

This adds tc-testing case 83c0. It builds a tbf + fq_pie hierarchy on
$DUMMY, pushes traffic through it with ping, and checks the output of
"tc -s class show" for an fq_pie class. The commit message says the
per-flow counters are printed by a companion iproute2 change.

> 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

[ ... ]

> +        "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: Low]
This is a robustness point, not a bug. fq_pie_walk() in
net/sched/sch_fq_pie.c only dumps flows that have a non-empty flowchain:

    if (list_empty(&q->flows[i].flowchain)) {
        arg->count++;
        continue;
    }

That makes this ping the only thing that gets class 2:1 into the output.

In the standard tdc setup this works. nsPlugin runs the command inside
the namespace, dummy is NOARP, and the 98-byte frame fits under tbf's
max_size of 100.

If ping is missing, or doesn't accept -i 0.001 / -W 0.01, the "|| true"
and the redirect hide the error. The test then fails as a regex mismatch
in the verify stage, not as a setup error.

Should this step be allowed to fail visibly? The same pattern is already
used elsewhere in tc-testing (infra/qdiscs.json), so it may be fine as is.

> +        ],
> +        "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",

[Severity: Medium]
Will this test fail with any iproute2 release available today?

The kernel side, fq_pie_dump_class_stats(), only sends a binary struct
tc_fq_pie_xstats with type TCA_FQ_PIE_XSTATS_CLASS:

    xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
    ...
    return gnet_stats_copy_app(d, &xstats, sizeof(xstats));

The words "prob", "delay" and "deficit" only appear if tc formats that
struct. According to the commit message, only the pending iproute2 patch
does that.

With a stock tc, re.findall() in run_one_test() in tdc.py finds 0
matches against a matchCount of 1. tdc then marks the test as failed,
even on a kernel where the fq_pie class stats work correctly.

tdc already has a way to skip in this case: the dependsOn key handled in
run_one_test(). Other tests use it to gate on newer iproute2 features.
For example, in actions/mirred.json:

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

Could 83c0 get a similar dependsOn probe, so that it skips instead of
failing when the tc in use can't print the per-flow stats?

The commit message also describes the test more narrowly than it
behaves:

    verify that 'tc -s class show' reports an fq_pie class.

The pattern needs the per-flow counters too, not just an fq_pie class.
Could the description say that the test depends on the iproute2 change?

> +        "matchCount": "1",
> +        "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/20260922205924.32173-1-hemendranaik%40gmail.com