This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: tc-testing: add fq_pie per-flow class stats test
This adds tdc case 83c0, which builds a TBF parent with an fq_pie child on
$DUMMY, injects ping traffic, and then greps the output of 'tc -s class show'
for per-flow fq_pie statistics. It also flips case 83be so that 'flows 65536'
is expected to be rejected.
A couple of questions about the new case below.
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> index 229fe1bf4a906..d4081fcb09336 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
> @@ -12,10 +12,10 @@
> "setup": [
> ],
> "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_pie
> flows 65536",
> - "expExitCode": "0",
> + "expExitCode": "2",
> "verifyCmd": "$TC qdisc show dev $DUMMY",
> "matchPattern": "qdisc fq_pie 1: root refcnt 2 limit 10240p flows
> 65536",
> - "matchCount": "1",
> + "matchCount": "0",
[Severity: Low]
This isn't a bug, the flip is correct: patch 1 of this series does add
the cap in fq_pie_change():
if (!q->flows_cnt || q->flows_cnt > 65535)
The only remark is about the commit message wording:
"Update case 83be to expect failure for 'flows 65536' after patch 1/3
caps the flows parameter at 65535."
Once these land, "patch 1/3" no longer identifies anything in git history.
Could the commit message name the change or the behaviour instead, so a
backporter can pair this test flip with the kernel change?
> "teardown": [
> ]
> },
> @@ -40,5 +40,32 @@
> + {
> + "id": "83c0",
> + "name": "FQ-PIE class stats accessible via tc class show",
> + "category": [
> + "qdisc",
> + "fq_pie"
> + ],
> + "plugins": {
> + "requires": "nsPlugin"
> + },
> + "setup": [
> + "$IP link set dev $DUMMY up || true",
> + "$IP addr add 10.10.11.10/24 dev $DUMMY || true",
> + "$TC qdisc add dev $DUMMY root handle 1: tbf rate 8bit burst
> 100b latency 100ms",
> + "$TC qdisc add dev $DUMMY parent 1:1 handle 2: fq_pie limit 100
> flows 1",
> + "ping -c 50 -i 0.001 -s 500 10.10.11.11 -I $DUMMY > /dev/null
> 2>&1 || true"
> + ],
[Severity: Medium]
Can any of this ping traffic actually reach the fq_pie child?
'burst 100b' sets q->max_size to 100 in tbf_change():
net/sched/sch_tbf.c:tbf_change() {
...
if (tb[TCA_TBF_BURST]) {
max_size = nla_get_u32(tb[TCA_TBF_BURST]);
buffer = psched_l2t_ns(&rate, max_size);
...
}
and tbf_enqueue() drops oversized non-GSO packets before they are handed to
the child qdisc:
net/sched/sch_tbf.c:tbf_enqueue() {
if (qdisc_pkt_len(skb) > q->max_size) {
if (skb_is_gso(skb) &&
skb_gso_validate_mac_len(skb, q->max_size))
return tbf_segment(skb, sch, to_free);
return qdisc_drop(skb, sch, to_free);
}
...
}
'ping -s 500' produces roughly 542-byte frames (500 payload + 8 ICMP +
20 IP + 14 Ethernet), so all 50 packets look like they are dropped in TBF
and fq_pie never sees a packet. $DUMMY is IFF_NOARP (dummy_setup() in
drivers/net/dummy.c), so the neighbour goes straight to NUD_NOARP and the
ICMP frames really do reach the qdisc, and there are no ARP solicitations
that could stand in as smaller traffic.
With nothing queued, fq_pie_walk() skips the flow entirely:
net/sched/sch_fq_pie.c:fq_pie_walk() {
for (i = 0; i < q->flows_cnt; i++) {
if (list_empty(&q->flows[i].flowchain)) {
arg->count++;
continue;
}
...
}
so no 'class fq_pie 2:N' line is printed and the verify step sees 0 matches
against matchCount 1. Existing cases using the same
'tbf rate 8bit burst 100b' parent in
tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json keep the ping
payload at the default 56 bytes so the frame stays under max_size. Would
dropping '-s 500' (or raising the TBF burst above the frame size) be the
intent here?
> + "cmdUnderTest": "$TC -s class show dev $DUMMY",
> + "expExitCode": "0",
> + "verifyCmd": "$TC -s class show dev $DUMMY",
> + "matchPattern": "class fq_pie 2:[0-9]+ .*prob .*delay .*deficit",
> + "matchCount": "1",
[Severity: Medium]
Which iproute2 version prints these tokens?
The 'prob', 'delay' and 'deficit' strings correspond to the new UAPI added
by the first patch of this series:
include/uapi/linux/pkt_sched.h
struct tc_fq_pie_cl_stats {
__u64 prob;
__u32 delay;
__s32 deficit;
__u32 avg_dq_rate;
__u32 dq_rate_estimating;
};
fq_pie_dump_class_stats() exports that as an opaque TCA_STATS_APP blob:
net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
...
if (idx < q->flows_cnt)
return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
}
and tc_fill_tclass() tags the class dump with the qdisc kind:
net/sched/sch_api.c:tc_fill_tclass() {
...
if (nla_put_string(skb, TCA_KIND, q->ops->id))
goto nla_put_failure;
...
}
so tc routes the blob to q_fq_pie's existing print_xstats, which only knows
the legacy qdisc counters (pkts_in, overlimit, overmemory, dropped,
ecn_mark, new_flow_count, new_flows_len, old_flows_len, memory_used) and
has no notion of the new type discriminator. With any released tc the
regex matches 0 times while matchCount is 1, and tdc has no iproute2
version gating, so the case fails permanently rather than being skipped.
The commit message says:
"The test fails on a kernel without fq_pie class ops."
Should it also state the required iproute2 change, since the case cannot
pass on a patched kernel with a shipped tc either?
> + "teardown": [
> + "$TC qdisc del dev $DUMMY handle 1: root",
> + "$IP addr del 10.10.11.10/24 dev $DUMMY || true"
> + ]
> }
> ]
