Re: [PATCH net-next 2/2] selftests: tc-testing: add DRR deficit refill regression tests

[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

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] Both new tdc cases in tools/testing/selftests/tc-testing/tc-
  tests/qdiscs/drr.json only pass if the verify command lands inside a…
- [Medium] Test 57a1 is meant to catch a 'per-class bulk refill that
  advances each class to its own eligibility point', as the commit…

--

selftests: tc-testing: add DRR deficit refill regression tests

This adds two tdc cases to drr.json. The first checks the deficit left on a
quantum 256 class after one 1514 byte packet is served (22 bytes), while a
second packet is still queued behind a netem child. The second checks
service order: with two active quantum 256 classes, the 514 byte packet
should be dequeued before the 1514 byte packet at the list head.

> 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..bb0995d8358fc 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 @@

[ ... ]

> +    {
> +        "id": "3f3e",
> +        "name": "DRR deficit refill converges to the loop-equivalent value",

[ ... ]

> +        "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",
> +            "$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 this case fail on a kernel that is behaving correctly, purely because of
timing?

The pattern only matches inside a window about one second wide. Say ping
sends packet 1 at T0 and packet 2 at T0+1. With netem delay 2s, they are
released at T0+2 and T0+3:

  before T0+2:        deficit is still 256, tc prints "deficit 256b"
  T0+2 .. T0+3:       packet 1 served, deficit is 22
  after T0+3:         packet 2 dequeued, child qlen drops to 0

Once qlen is 0, the dump reports zero:

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

so tc prints "deficit 0b".

The verify command runs at the end of setup, plus the sleep 2.5, plus the
cost of spawning cmdUnderTest and verifyCmd through ip netns exec. That
leaves about 0.5s of slack either way for the difference between that
overhead and ping's startup time.

Test 57a1 below has the same shape. B1 is released about two seconds after
the first python3 sends. B2 is released about two seconds after the second
python3 sends. At that point 1:2 is at the head with a deficit of 254 and a
514 byte packet. The refill then serves both B2 and A1, which changes every
bytes/packets/backlog/qlen value in its matchJSON.

The verify at setup end plus sleep 1.5 has to land between those two
releases. That again depends on python3 startup and spawn overhead staying
under about 0.5s.

tdc.sh runs ./tdc.py -J"$(nproc)", so these cases run in parallel with
other tests. That adds fork/exec, python3 startup and scheduling jitter,
especially on debug kernels with KASAN or lockdep.

Most existing sleep based tdc cases only need a lower bound. These two need
both a lower and an upper bound.

> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $DUMMY root"
> +        ]
> +    },
> +    {
> +        "id": "57a1",
> +        "name": "DRR skips only rounds where no class can dequeue (keeps order)",

[ ... ]

> +        "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 class add dev $DUMMY parent 1: classid 1:2 drr quantum 256",
> +            "$TC qdisc add dev $DUMMY parent 1:1 handle 10: netem delay 2s limit 10",
> +            "$TC qdisc add dev $DUMMY parent 1:2 handle 20: netem delay 2s limit 10",

[Severity: Medium]
Does this case reliably catch the regression the commit message describes?

  A per-class bulk refill that advances each class to its own
  eligibility point would dequeue the head class instead.

To tell the two refill styles apart, A1 (in 1:1, the list head) and B1 (in
1:2) both have to be ready in the same drr_dequeue() pass.

Both netem children use delay 2s. netem_dequeue() compares time_to_send
against ktime_get_ns() at peek time. So B1 becomes sendable a small delta
after A1, where the delta is the gap between the two sendto() calls in the
same python3 process.

What happens if the hrtimer -> NET_TX softirq -> drr_dequeue() latency is
shorter than that delta?

1. At A1's release, 1:1 peeks A1 and 1514 > 256. first is set to 1:1, its
   deficit becomes 512, and list_move_tail() leaves the list as [1:2, 1:1].

2. 1:2 peeks NULL, and drr_dequeue() bails out:

		if (skb == NULL) {
			qdisc_warn_nonwc(__func__, cl->qdisc);
			goto out;
		}

   1:2 is now the list head.

3. At B1's release, 1:2 goes 256 -> 512 and 1:1 goes 512 -> 768. The bulk
   refill scan, with 1:2 at the head, computes
   rounds = min(DIV_ROUND_UP(2, 256), DIV_ROUND_UP(746, 256)) = 1. That
   gives 1:2 = 768 and 1:1 = 1024, and B1 is dequeued.

In that state, a per-class variant would give 1:2 = 768 and 1:1 = 1536. It
would still visit 1:2 first and dequeue B1, so the matchJSON below would
still match.

The test only fails as intended when B1 is already sendable at A1's
release. Then the scan starts with 1:1 at the head and both classes at 512.

The ratio between that delta and softirq latency varies across bare metal,
KVM and TCG. Could netem 20 use a slightly shorter delay than netem 10? That
way B1 is always ready when child 10's watchdog fires.

> +            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip dport 1111 0xffff flowid 1:1",
> +            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip dport 2222 0xffff flowid 1:2",

[ ... ]

> +        "matchJSON": [
> +            {
> +                "class": "drr",
> +                "handle": "1:1",
> +                "root": true,
> +                "stats": {
> +                    "bytes": 0,
> +                    "packets": 0,
> +                    "backlog": 3028,
> +                    "qlen": 2
> +                }
> +            },
> +            {
> +                "class": "drr",
> +                "handle": "1:2",
> +                "root": true,
> +                "stats": {
> +                    "bytes": 514,
> +                    "packets": 1,
> +                    "backlog": 514,
> +                    "qlen": 1
> +                }
> +            }
> +        ],

[ ... ]

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