Re: [PATCH net repost 2/2] selftests/tc-testing: add codel/fq_codel interval boundary cases

[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: 2 · Low: 0

- [Medium] None of the eight added tdc cases exercise the code the
  previous patch ("net/sched: codel: bound the dropping loop per dequeue…
- [Medium] The subject ("add codel/fq_codel interval boundary cases"),
  the body ("the 1024us boundary is accepted") and the case names…

--

selftests/tc-testing: add codel/fq_codel interval boundary cases

This adds eight tdc cases (codel 6e44/a8c3/a695/9793 and fq_codel
1b4d/3540/49c5/3e0f) that create a codel or fq_codel qdisc with small
interval and target values and match the rendered qdisc dump line.  The
stated purpose is to lock the small-interval uAPI after the preceding
dropping-loop bound patch.

No Link: tags are present in the commit header.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> index 6d515d0e5ed69..a894e6f0e2677 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/codel.json
> @@ -213,5 +213,77 @@
>          "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1p target 5ms interval 100ms",
>          "matchCount": "1",
>          "teardown": ["$TC qdisc del dev $DEV1 handle 1: root"]
> +    },
> +    {
> +        "id": "6e44",
> +        "name": "Create CODEL with 1us interval, accepted (sub-tick, uAPI locked)",
> +        "category": [
> +            "qdisc",
> +            "codel"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel interval 1us",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 0us",
> +        "matchCount": "1",
> +        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
> +    },

[Severity: Medium]
Do any of the eight added cases actually exercise the code the previous
patch changed?  Each one has an empty setup, a bare tc qdisc add as
cmdUnderTest, and a plain tc qdisc show (no -s, no traffic, no dequeue) as
verifyCmd.

Neither codel_change() in net/sched/sch_codel.c nor fq_codel_change() gained
any validation in the previous patch, which touches only include/net/codel.h
and include/net/codel_impl.h.  So the acceptance of interval 1us/3us/1024us
and target 1us, and the dumped values interval 0us/2us/1.02ms and target 0us,
look identical on an unpatched kernel.

Would these cases stay green if the max_t(u32, 1, ...) clamp in
codel_control_law() or the CODEL_MAX_DROPS_PER_DEQUEUE bound in
codel_dequeue() were reverted?  Similarly, would 9793 and 3e0f ("target not
in control law") keep passing if target were wrongly folded into
codel_control_law()?

The changelog says:

  "These cases do not test the dropping-loop bound itself: tdc cannot
   observe per-dequeue drop counts."

Is that framing accurate for the harness as a whole?  codel.json case deb1,
in this same file, already drives traffic via scapyPlugin:

        "plugins": {
            "requires": ["nsPlugin", "scapyPlugin"]
        },
        "scapy": [
            {
                "iface": "$DEV0",
                "count": 10,
                ...

and infra/qdiscs.json case c797 configures exactly the degenerate
configuration and asserts exact counters:

        "$TC qdisc add dev $DUMMY parent 1:2 handle 10: fq_codel target 1 interval 1 flows 1",
        ...
        "verifyCmd": "$TC -s -j qdisc show dev $DUMMY",
        "matchJSON": [ ... "packets": 3, "drops": 1 ... ]

Per-dequeue granularity is indeed not observable, but aggregate drop counts
under traffic appear to be.  Could the changelog say that a dynamic case was
not written, rather than that tdc cannot express one?

[ ... ]

> +    {
> +        "id": "a695",
> +        "name": "Create CODEL with 1024us interval boundary accepted",
> +        "category": [
> +            "qdisc",
> +            "codel"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root codel interval 1024us",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc codel 1: root refcnt [0-9]+ limit 1000p target 5ms interval 1.02ms",
> +        "matchCount": "1",
> +        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
> +    },

[Severity: Medium]
Which boundary is 1024us?  The subject says "interval boundary cases", the
body says "the 1024us boundary is accepted", and this case (plus fq_codel
49c5) is named "1024us interval boundary accepted", but I cannot find a
1024us threshold in codel.

include/net/codel.h has:

  /* CoDel uses a 1024 nsec clock, encoded in u32
   * This gives a range of 2199 seconds, because of signed compares
   */
  #define CODEL_SHIFT 10

The 1024 there is nanoseconds per tick, not microseconds.  Is this a ns/us
mix-up?  1024us just converts to 1000 ticks and renders as 1.02ms; there is
no clamp or validation at that value in codel.h, codel_impl.h, sch_codel.c or
sch_fq_codel.c.

The granularity boundary in the uAPI conversion looks like it sits at 2us:

  net/sched/sch_codel.c:codel_change() {
      u32 interval = nla_get_u32(tb[TCA_CODEL_INTERVAL]);

      WRITE_ONCE(q->params.interval,
                 ((u64)interval * NSEC_PER_USEC) >> CODEL_SHIFT);
  }

giving 1us -> 0 ticks, 2us -> 1 tick, 3us -> 2 ticks.  The added cases cover
1us and 3us and skip 2us, which is the value used in the previous patch's own
reproducer ("codel interval 2us target 1ms").  Could a case at 2us be added
and the 1024us wording in the subject, body and case names be corrected?

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> index 4ce62b857fd7a..de6a1b8d954ae 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_codel.json
> @@ -316,5 +316,77 @@

[ ... ]

> +    {
> +        "id": "49c5",
> +        "name": "Create FQ_CODEL with 1024us interval boundary accepted",
> +        "category": [
> +            "qdisc",
> +            "fq_codel"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_codel interval 1024us",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc fq_codel 1: root refcnt [0-9]+ limit 10240p flows 1024 quantum [0-9]+ target 5ms interval 1.02ms memory_limit 32Mb ecn drop_batch 64",
> +        "matchCount": "1",
> +        "teardown": ["$TC qdisc del dev $DUMMY handle 1: root"]
> +    },

The same two questions apply to the fq_codel cases 1b4d/3540/49c5/3e0f: they
are config-only adds plus a show, and 49c5 carries the same 1024us boundary
naming.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-1L5H.v1.20260912080102%40mojatatu.com