On Fri, Sep 4, 2026 at 5:42 AM <[email protected]> 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.
>
> > 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
