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

On Fri, Sep 4, 2026 at 5:42 AM [off-list ref] wrote:

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…

Note: the e51a/e7f2 breakage branch is wrong (verified: iproute2
rejects the literal 0 client-side, both tests pass on the patched
kernel per our tests).
Note2: The underlying point is fair, though: the error-contract change
for TCA_ETS_QUANTA_BAND == 0 deserves one changelog sentence.

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…

That matches the file's existing goal; the retired sum-wrap case is
indeed unreachable with the 1 MiB per-band ceiling (16 bands x 1 MiB
<< 2^32).

IMO, other than the changelog change for "medium" - nothing else to see here.

Phew! Cant believe i went over all that. I thought this new era was
about keeping up with huge influx of bug reports - but the influx from
the bots is higher. I am sure the s/w quality will improve but man oh
man this is too much work (change the changelog, no human has ever
asked me that!)

cheers,
jamal

--

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.

quoted

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