Re: [PATCH net v3 9/9] selftests: tc-testing: update ETS test 41f5 for clamped quanta

[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 (1)

- [Medium] Incomplete selftest update / undisclosed behavioural change:
  the accompanying commit 0bbbcd21591a ("net/sched: ets: clamp quantum…

Pre-existing issues:
- [Low] Concern that test 41f5 ("ETS offload with out-of-range quanta
  clamped") only checks the software qdisc dump via `tc qdisc show` and…

--

selftests: tc-testing: update ETS test 41f5 for clamped quanta

This updates tdc test 41f5 in qdiscs/ets.json so that its name and match
pattern reflect the new quantum clamping to [256, 1 << 20] introduced by
"net/sched: ets: clamp quantum in parse and fallback paths".  The test
still passes "quanta 4294967294 1 1" but now expects "quanta 1048576 256
256" in the qdisc dump.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> index ee09e6d6fdf3c..d2eab61c099a8 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> @@ -987,7 +987,7 @@
>      },
>      {
>          "id": "41f5",
> -        "name": "ETS offload where the sum of quanta wraps u32",
> +        "name": "ETS offload with out-of-range quanta clamped",
>          "category": [
>              "qdisc",
>              "ets"
> @@ -1002,7 +1002,7 @@
>          "cmdUnderTest": "$TC qdisc add dev $ETH root ets quanta 4294967294 1 1",
>          "expExitCode": "0",
>          "verifyCmd": "$TC qdisc show dev $ETH",
> -        "matchPattern": "qdisc ets .*bands 3 quanta 4294967294 1 1",
> +        "matchPattern": "qdisc ets .*bands 3 quanta 1048576 256 256",
>          "matchCount": "1",

[Severity: Low]
This isn't a bug introduced by this patch, but a note on what the renamed
test now covers.

The verification is only "$TC qdisc show dev $ETH", which reports the
software qdisc quanta.  The quanta actually handed to the driver are
filled in separately by ets_offload_change() in net/sched/sch_ets.c:

	qopt.replace_params.quanta[i] = quantum;
	qopt.replace_params.weights[i] = weight;

and netdevsim's nsim_setup_tc_ets() in drivers/net/netdevsim/tc.c does
nothing with them:

	case TC_ETS_REPLACE:
	case TC_ETS_DESTROY:
		break;

so nothing tdc can observe reflects the offload request itself.  That is
pre-existing and matches the idiom used by the other cases in this file.

Separately, since each quantum is now capped at 1 << 20 and there are at
most 16 bands, is the u32 sum-wrap condition the test was originally
written for still reachable at all?  If not, the rename retires that
coverage without a replacement.

[Severity: Medium]
Should the two neighbouring tests in this same file be updated in the
same patch?

The kernel-side commit in this series also removed the explicit
zero-quantum rejection from ets_quantum_parse(), which now reads:

net/sched/sch_ets.c:ets_quantum_parse() {
	*quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
	return 0;
}

The previous "ETS quantum cannot be zero" -EINVAL and extack are gone,
and nothing else in the path rejects 0: ets_quanta_policy declares
TCA_ETS_QUANTA_BAND as a bare NLA_U32 with no range, and
ets_qdisc_quanta_parse() only runs __nla_validate_nested() plus the band
count check before calling ets_quantum_parse().  A band quantum of 0 is
therefore clamped to 256 and the qdisc is created.

Tests e51a "Zero element in quanta" and e7f2 "Sole zero element in
quanta" still encode the old behaviour, for example e51a:

        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
        "expExitCode": "1",
        "verifyCmd": "$TC qdisc show dev $DUMMY",
        "matchPattern": "qdisc ets",
        "matchCount": "0"

If tc forwards the literal 0 to the kernel, which is what 41f5 suggests
given that it passes 4294967294 and 1 through unmodified, then the add
now returns 0 instead of 1 and "tc qdisc show" prints "qdisc ets", so
both tests fail.  If tc filters the 0 locally the tests keep passing but
no longer exercise any kernel check, and nothing covers the new
accept-and-clamp semantics.

Either way, could the removal of the zero-quantum -EINVAL and its extack
be mentioned in the changelog?  Neither this commit message nor the
kernel one notes that the netlink error contract for
TCA_ETS_QUANTA_BAND == 0 changed, and sch_drr.c deliberately kept its
explicit zero reject.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com