Re: [PATCH net-next v3 3/3] selftests: tc-testing: pin DRR and ETS deficit refill semantics

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

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] All three new tests pass or fail based on wall-clock timing,
  with roughly ±0.5 s of margin, and nothing synchronises them with when…
- [Low] The commit message says only 57a1 and 2177 put netem under the
  classful qdisc and log the "non-work-conserving?" warning.
- [Low] In ETS test 2177, band 1:1 is expected to show "bytes": 1514,
  "packets": 1, although the band was never served (backlog 3028, qlen…
- [Low] The commit says it pins the DRR/ETS deficit refill 'so a change
  to the refill loop can't silently alter the result'.

--

selftests: tc-testing: pin DRR and ETS deficit refill semantics

This commit adds three tc-testing cases. 3f3e checks the DRR deficit after
a 1514-byte packet at quantum 256. 57a1 and 2177 check that DRR and ETS
serve a 514-byte packet before a 1514-byte one, using netem delay under the
classful qdisc to hold packets while stats are inspected.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
> index 2b61d8d79bde8..d7ac32294738e 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
> @@ -86,5 +86,87 @@

[ ... ]

> +        "setup": [
> +            "$IP addr add 10.10.11.10/24 dev $DUMMY",
> +            "$TC qdisc add dev $DUMMY root handle 1: drr",
> +            "$TC class add dev $DUMMY parent 1: classid 1:1 drr quantum 256",
> +            "$TC qdisc add dev $DUMMY parent 1:1 handle 10: netem delay 2s limit 10",

[Severity: Low]
The commit message says:

    57a1 and 2177 put netem under the classful qdisc, so they log the usual
    "non-work-conserving?" warning.

3f3e also puts netem under DRR class 1:1 here, so doesn't it log the same
warning? The first ping packet is enqueued and qdisc_run() calls
drr_dequeue(). netem's peek, qdisc_peek_dequeued()->netem_dequeue(),
returns NULL because the packet is not due for 2s:

net/sched/sch_drr.c:drr_dequeue() {
    ...
		skb = cl->qdisc->ops->peek(cl->qdisc);
		if (skb == NULL) {
			qdisc_warn_nonwc(__func__, cl->qdisc);
			goto out;
		}
    ...
}

Each test creates a new netem qdisc with TCQ_F_WARN_NONWC clear, so it
looks like all three tests print the warning, not two.

Should the commit message say that all three tests log it?

> +            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match u32 0 0 flowid 1:1",
> +            "ping -c 2 -s 1472 -i 1 10.10.11.11 -I $DUMMY -W 0.01 >/dev/null 2>&1 &"
> +        ],
> +        "cmdUnderTest": "sleep 2.5",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC -s class show dev $DUMMY",
> +        "matchPattern": "deficit 22b",

[Severity: Medium]
Can these tests fail intermittently because of scheduling latency?

nsPlugin runs the traffic generators in the background with & through ip
netns exec. The verify step then runs after a fixed sleep 2.5. Nothing
ties the verify step to the time the packets are actually enqueued.

In 3f3e, ping sends at T+d1 and T+d1+1, and netem releases each packet 2s
later. drr_dump_class_stats() reports the deficit only while the class is
backlogged:

net/sched/sch_drr.c:drr_dump_class_stats() {
    ...
	if (qlen)
		xstats.deficit = READ_ONCE(cl->deficit);
    ...
}

So "deficit 22b" only matches while the first packet has been dequeued and
the second is still held by netem. If ping start-up or tdc overhead moves
things by more than about 0.5s, tc prints either deficit 256b (nothing
released yet) or deficit 0b (class drained).

57a1 and 2177 have the same problem. Batch 1 is released at about T0+p1+3,
and verify runs at about T0+1+2.5 plus overhead:

- If starting ip netns exec plus python3 takes more than about 0.5s,
  batch 1 is still held at verify time. Class 1:2 then shows bytes 0 with
  backlog 1028 / qlen 2 instead of 514 / 1.
- If tdc overhead between setup and verify is too large, batch 2 is
  released and backlog/qlen change.

tdc.sh runs tdc.py -J"$(nproc)", which adds CPU contention. Debug kernels
such as KASAN or lockdep slow start-up further.

Could the tests wait for the packets to appear in the qdisc backlog before
the timed window starts? Alternatively, could the margin between the netem
delay and the verify time be made wider?

[ ... ]

> +        "cmdUnderTest": "sleep 2.5",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC -j -s class show dev $DUMMY",
> +        "matchJSON": [
> +            {
> +                "class": "drr",
> +                "handle": "1:1",
> +                "root": true,
> +                "stats": {
> +                    "bytes": 0,
> +                    "packets": 0,
> +                    "backlog": 3028,
> +                    "qlen": 2
> +                }
> +            },

[Severity: Low]
The commit message says the tests ensure "a change to the refill loop
can't silently alter the result". Does 57a1 actually pin the multi-class
refill?

This matchJSON only checks bytes, packets, backlog and qlen, which only
shows the service order. The batched refill in drr_dequeue() adds
rounds * quantum to every active class:

net/sched/sch_drr.c:drr_dequeue() {
    ...
			if (skip && rounds != UINT_MAX) {
				list_for_each_entry(iter, &q->active, alist)
					WRITE_ONCE(iter->deficit,
						   iter->deficit + rounds *
						   READ_ONCE(iter->quantum));
    ...
}

Suppose that loop were changed to refill only the first class. B would
still be dequeued first, and the asserted stats would match exactly.
Class 1:1's deficit, though, would end at 1280 (or 768) instead of 1024.
Class 1:2 ends at 254 in both cases. 3f3e has only one class, so it can't
tell these cases apart either.

Class 1:1 still has qlen 2 at verify time, so drr_dump_class_stats()
exports its deficit. Could 57a1 also check the per-class deficits (1024
and 254)? ETS does not export a deficit, so 2177 can only check the order.

[ ... ]

> 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 d2eab61c099a8..ea3adba2f06c7 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json
> @@ -1007,5 +1007,59 @@

[ ... ]

> +        "verifyCmd": "$TC -j -s class show dev $DUMMY",
> +        "matchJSON": [
> +            {
> +                "class": "ets",
> +                "handle": "1:1",
> +                "root": true,
> +                "stats": {
> +                    "bytes": 1514,
> +                    "packets": 1,
> +                    "backlog": 3028,
> +                    "qlen": 2
> +                }
> +            },

[Severity: Low]
Is "bytes": 1514, "packets": 1 the intended expectation for band 1:1?

Band 1:1 still has backlog 3028 and qlen 2. Both 1514-byte packets are
still queued, so the band was never served. The count seems to come from
netem's accounting at peek time. ets_class_dump_stats() reports the
child's bstats:

net/sched/sch_ets.c:ets_class_dump_stats() {
    ...
	if (gnet_stats_copy_basic(d, NULL, &cl_q->bstats, true) < 0 ||
    ...
}

netem's peek is qdisc_peek_dequeued(), which calls netem_dequeue(). That
updates bstats before the skb is stashed back in gso_skb:

net/sched/sch_netem.c:netem_dequeue() {
    ...
deliver:
		qdisc_qstats_backlog_dec(sch, skb);
		qdisc_bstats_update(sch, skb);
		return skb;
    ...
}

So a single ETS peek of the first A packet sets band 1:1 to 1514/1.

The DRR version, 57a1, runs the same scenario and expects bytes 0 /
packets 0. That is because drr_dump_class_stats() reports cl->bstats,
which is only updated on a real dequeue.

The expectation here depends on how netem accounts a peek. Could band 1:1
check only backlog/qlen, so the test does not break if that accounting
changes while ETS scheduling stays the same?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-TIAE.v3.r0.20261109173518%40mojatatu.com