* [PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics
@ 2026-09-02 3:52 Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops Hemendra M. Naik
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-02 3:52 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
FQ-PIE runs an independent PIE controller per flow but exposes no
per-flow statistics. This series wires up fq_pie_class_ops to expose
per-flow AQM state (prob, delay, deficit, avg_dq_rate)
via 'tc -s class show', following a similar pattern as FQ-CoDel.
---
Changelog:
v5:
- Addressed Sashiko review comments:
- Omitted .tcf_block / .bind_tcf / .unbind_tcf from cl_ops (statistics
only being exported; filter attach to fq_pie is now disabled).
- Dropped empty tc_fq_pie_xqd_stats placeholder; class_stats is a direct
struct member.
- Limited flows to [1..65535] so per-flow class handles fit in a 16-bit
TC minor.
- Rewrote selftest 83c0 with TBF + fq_pie, ping traffic, and
matchCount 1 on per-flow stats output.
- Updated selftest 83be for the new flows limit.
- Dropped the tools/include UAPI mirror changes from patch 3/3.
v4: https://lore.kernel.org/netdev/20260727164056.106203-1-hemendranaik@gmail.com/
- Fixed unaligned commit message; moved typo fixes to another patch.
v3: https://lore.kernel.org/netdev/20260630183702.170798-1-hemendranaik@gmail.com/
- No changes since v2.
- Resent after the previous submission was deferred due to the
net-next tree closing during review.
- Updated corresponding iproute2 patch in response to review comments;
kernel patches unchanged.
v2: https://lore.kernel.org/netdev/20260614125000.6058-1-hemendranaik@gmail.com/
- Addressed ABI backward compatibility issue for tc_fq_pie_xstats.
v1: https://lore.kernel.org/netdev/20260531125314.22492-1-hemendranaik@gmail.com/
- Initial submission
Hemendra M. Naik (3):
net/sched: sch_fq_pie: add per-flow statistics via class ops
selftests: tc-testing: add fq_pie per-flow class stats test
net/sched: pie: correct tc_pie_xstats field documentation
include/uapi/linux/pkt_sched.h | 23 ++++-
net/sched/sch_fq_pie.c | 90 ++++++++++++++++++-
.../tc-testing/tc-tests/qdiscs/fq_pie.json | 33 ++++++-
3 files changed, 138 insertions(+), 8 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops
2026-09-02 3:52 [PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics Hemendra M. Naik
@ 2026-09-02 3:52 ` Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2026-09-02 3:52 ` [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation Hemendra M. Naik
2 siblings, 1 reply; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-02 3:52 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
FQ-PIE schedules independent PIE controllers per flow but exposes no
per-flow AQM state. Without class-level statistics there is no way to
observe the per-flow drop probability, queue delay, deficit or dequeue
rate from userspace.
Extend tc_fq_pie_xstats with a type discriminator and tc_fq_pie_cl_stats
for per-flow metrics. Append type and class_stats after the nine existing
qdisc counters so qdisc xstats offsets stay compatible with the flat
struct fq_pie already shipped.
Wire up fq_pie_class_ops with walk, dump, and dump_stats only so that
'tc -s class show' reports per-flow state (prob, delay, deficit,
avg_dq_rate, dq_rate_estimating). Filter attachment callbacks
(.tcf_block, .bind_tcf, .unbind_tcf) are omitted on purpose: this series
targets exporting statistics, not making fq_pie filter-capable.
Limit flows to [1..65535] so every per-flow class handle fits in a
16-bit TC minor (TC_H_MIN wraps at 65536).
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
include/uapi/linux/pkt_sched.h | 19 +++++++
net/sched/sch_fq_pie.c | 90 ++++++++++++++++++++++++++++++++--
2 files changed, 106 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 490efd288526..c6373f410d11 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -953,6 +953,21 @@ enum {
};
#define TCA_FQ_PIE_MAX (__TCA_FQ_PIE_MAX - 1)
+enum {
+ TCA_FQ_PIE_XSTATS_QDISC,
+ TCA_FQ_PIE_XSTATS_CLASS,
+};
+
+struct tc_fq_pie_cl_stats {
+ __u64 prob; /* current probability */
+ __u32 delay; /* current delay in microseconds */
+ __s32 deficit; /* number of remaining byte credits */
+ __u32 avg_dq_rate; /* current average dq_rate in
+ * bytes/second
+ */
+ __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
+};
+
struct tc_fq_pie_xstats {
__u32 packets_in; /* total number of packets enqueued */
__u32 dropped; /* packets dropped due to fq_pie_action */
@@ -963,6 +978,9 @@ struct tc_fq_pie_xstats {
__u32 new_flows_len; /* count of flows in new list */
__u32 old_flows_len; /* count of flows in old list */
__u32 memory_usage; /* total memory across all queues */
+ __u32 type;
+ struct tc_fq_pie_cl_stats class_stats;
+
};
/* CBS */
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index 069e1facd413..4e3eb8329d99 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -310,9 +310,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
goto flow_error;
}
q->flows_cnt = nla_get_u32(tb[TCA_FQ_PIE_FLOWS]);
- if (!q->flows_cnt || q->flows_cnt > 65536) {
+ if (!q->flows_cnt || q->flows_cnt > 65535) {
NL_SET_ERR_MSG_MOD(extack,
- "Number of flows must range in [1..65536]");
+ "Number of flows must range in [1..65535]");
goto flow_error;
}
}
@@ -509,7 +509,9 @@ static int fq_pie_dump(struct Qdisc *sch, struct sk_buff *skb)
static int fq_pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
{
struct fq_pie_sched_data *q = qdisc_priv(sch);
- struct tc_fq_pie_xstats st = { 0 };
+ struct tc_fq_pie_xstats st = {
+ .type = TCA_FQ_PIE_XSTATS_QDISC,
+ };
struct list_head *pos;
sch_tree_lock(sch);
@@ -561,7 +563,89 @@ static void fq_pie_destroy(struct Qdisc *sch)
kvfree(q->flows);
}
+static struct Qdisc *fq_pie_leaf(struct Qdisc *sch, unsigned long arg)
+{
+ return NULL;
+}
+
+static unsigned long fq_pie_find(struct Qdisc *sch, u32 classid)
+{
+ return 0;
+}
+
+static int fq_pie_dump_class(struct Qdisc *sch, unsigned long cl,
+ struct sk_buff *skb, struct tcmsg *tcm)
+{
+ tcm->tcm_handle |= TC_H_MIN(cl);
+ return 0;
+}
+
+static int fq_pie_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+ struct gnet_dump *d)
+{
+ struct fq_pie_sched_data *q = qdisc_priv(sch);
+ struct gnet_stats_queue qs = { 0 };
+ struct tc_fq_pie_xstats xstats;
+ u32 idx = cl - 1;
+
+ if (idx < q->flows_cnt) {
+ const struct fq_pie_flow *flow = &q->flows[idx];
+
+ memset(&xstats, 0, sizeof(xstats));
+ xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
+ xstats.class_stats.prob =
+ READ_ONCE(flow->vars.prob) << BITS_PER_BYTE;
+ xstats.class_stats.delay =
+ ((u32)PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay))) /
+ NSEC_PER_USEC;
+ xstats.class_stats.deficit = READ_ONCE(flow->deficit);
+ xstats.class_stats.dq_rate_estimating =
+ READ_ONCE(q->p_params.dq_rate_estimator);
+
+ if (xstats.class_stats.dq_rate_estimating) {
+ xstats.class_stats.avg_dq_rate =
+ READ_ONCE(flow->vars.avg_dq_rate) *
+ (PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
+ }
+
+ qs.qlen = READ_ONCE(flow->qlen);
+ qs.backlog = READ_ONCE(flow->backlog);
+ }
+ if (gnet_stats_copy_queue(d, NULL, &qs, qs.qlen) < 0)
+ return -1;
+ if (idx < q->flows_cnt)
+ return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
+ return 0;
+}
+
+static void fq_pie_walk(struct Qdisc *sch, struct qdisc_walker *arg)
+{
+ struct fq_pie_sched_data *q = qdisc_priv(sch);
+ unsigned int i;
+
+ if (arg->stop)
+ return;
+
+ for (i = 0; i < q->flows_cnt; i++) {
+ if (list_empty(&q->flows[i].flowchain)) {
+ arg->count++;
+ continue;
+ }
+ if (!tc_qdisc_stats_dump(sch, i + 1, arg))
+ break;
+ }
+}
+
+static const struct Qdisc_class_ops fq_pie_class_ops = {
+ .leaf = fq_pie_leaf,
+ .find = fq_pie_find,
+ .dump = fq_pie_dump_class,
+ .dump_stats = fq_pie_dump_class_stats,
+ .walk = fq_pie_walk,
+};
+
static struct Qdisc_ops fq_pie_qdisc_ops __read_mostly = {
+ .cl_ops = &fq_pie_class_ops,
.id = "fq_pie",
.priv_size = sizeof(struct fq_pie_sched_data),
.enqueue = fq_pie_qdisc_enqueue,
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test
2026-09-02 3:52 [PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops Hemendra M. Naik
@ 2026-09-02 3:52 ` Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2026-09-02 3:52 ` [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation Hemendra M. Naik
2 siblings, 1 reply; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-02 3:52 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
Add tc-testing case 83c0: create TBF + fq_pie on $DUMMY, inject traffic
with ping, then verify 'tc -s class show' prints per-flow fq_pie stats
(prob, delay, deficit) with matchCount 1. The test fails on a kernel
without fq_pie class ops.
Update case 83be to expect failure for 'flows 65536' after patch 1/3
caps the flows parameter at 65535.
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
.../tc-testing/tc-tests/qdiscs/fq_pie.json | 33 +++++++++++++++++--
1 file changed, 30 insertions(+), 3 deletions(-)
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 229fe1bf4a90..d4081fcb0933 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
@@ -1,7 +1,7 @@
[
{
"id": "83be",
- "name": "Create FQ-PIE with invalid number of flows",
+ "name": "FQ-PIE rejects flows above 65535",
"category": [
"qdisc",
"fq_pie"
@@ -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",
"teardown": [
]
},
@@ -40,5 +40,32 @@
"matchPattern": "qdisc fq_pie 1: root refcnt [0-9]+ limit 1p",
"matchCount": "1",
"teardown": ["$TC qdisc del dev $DEV1 handle 1: root"]
+ },
+ {
+ "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"
+ ],
+ "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",
+ "teardown": [
+ "$TC qdisc del dev $DUMMY handle 1: root",
+ "$IP addr del 10.10.11.10/24 dev $DUMMY || true"
+ ]
}
]
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation
2026-09-02 3:52 [PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test Hemendra M. Naik
@ 2026-09-02 3:52 ` Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2 siblings, 1 reply; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-02 3:52 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
The comments describing struct tc_pie_xstats do not match the values
exported by the kernel.
Update the delay field comment to microseconds (PSCHED_TICKS2NS() /
NSEC_PER_USEC). Update avg_dq_rate to bytes/second (avg_dq_rate *
PSCHED_TICKS_PER_SEC >> PIE_SCALE).
Documentation-only; no UAPI layout or runtime change. Touch
include/uapi/linux/pkt_sched.h only
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
include/uapi/linux/pkt_sched.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index c6373f410d11..e3f643627c05 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -920,9 +920,9 @@ enum {
struct tc_pie_xstats {
__u64 prob; /* current probability */
- __u32 delay; /* current delay in ms */
+ __u32 delay; /* current delay in microseconds */
__u32 avg_dq_rate; /* current average dq_rate in
- * bits/pie_time
+ * bytes/second
*/
__u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
__u32 packets_in; /* total number of packets enqueued */
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops
2026-09-02 3:52 ` [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops Hemendra M. Naik
@ 2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:03 ` Hemendra M. Naik
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-09-04 23:17 UTC (permalink / raw)
To: hemendranaik
Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, horms, jiri,
jhs, shuah, linux-kernel, linux-kselftest, vishy0777, tahiliani
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: sch_fq_pie: add per-flow statistics via class ops
This adds a Qdisc_class_ops table to sch_fq_pie with walk, dump and
dump_stats so that per-flow PIE state can be reported via 'tc -s class
show'. struct tc_fq_pie_xstats gains a type discriminator plus an
embedded struct tc_fq_pie_cl_stats, and the accepted flow count is
narrowed to [1..65535].
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 490efd2885267..89d825bac4bee 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -953,6 +953,21 @@ enum {
> };
> #define TCA_FQ_PIE_MAX (__TCA_FQ_PIE_MAX - 1)
>
> +enum {
> + TCA_FQ_PIE_XSTATS_QDISC,
> + TCA_FQ_PIE_XSTATS_CLASS,
> +};
> +
> +struct tc_fq_pie_cl_stats {
> + __u64 prob; /* current probability */
> + __u32 delay; /* current delay in microseconds */
> + __s32 deficit; /* number of remaining byte credits */
> + __u32 avg_dq_rate; /* current average dq_rate in
> + * bytes/second
> + */
> + __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
> +};
> +
> struct tc_fq_pie_xstats {
> __u32 packets_in; /* total number of packets enqueued */
> __u32 dropped; /* packets dropped due to fq_pie_action */
> @@ -963,6 +978,9 @@ struct tc_fq_pie_xstats {
> __u32 new_flows_len; /* count of flows in new list */
> __u32 old_flows_len; /* count of flows in old list */
> __u32 memory_usage; /* total memory across all queues */
> + __u32 type;
> + struct tc_fq_pie_cl_stats class_stats;
> +
> };
[Severity: Medium]
The changelog says the new members are appended so that "qdisc xstats
offsets stay compatible with the flat struct fq_pie already shipped".
That is true for the offsets, but does it also change the size of an
already shipped struct?
nine __u32 counters = 36 bytes
+ __u32 type = 40 bytes
+ struct tc_fq_pie_cl_stats = 64 bytes
A consumer that validates the TCA_STATS_APP payload with the usual
iproute2 idiom:
if (RTA_PAYLOAD(xstats) < sizeof(*st))
return -1;
would stop printing fq_pie xstats altogether once rebuilt against this
header and run on an older kernel that still emits 36 bytes. sch_fq
handled this by having userspace copy min(payload, sizeof(*st)). Should
the changelog mention the size change so this is a conscious decision?
Related: since fq_pie_dump_class_stats() reuses the same struct, an
iproute2 that does not know about the new type field will decode the
per-class blob using the qdisc layout and print nine all-zero qdisc
counters per flow. Does that mean the stated goal ("'tc -s class show'
reports per-flow state") depends on an iproute2 change that the changelog
does not mention? The selftest added later in the series matches
"class fq_pie 2:[0-9]+ .*prob .*delay .*deficit", which only a patched
iproute2 can produce.
[Severity: Low]
Documentation/netlink/specs/tc.yaml still describes tc-fq-pie-xstats as a
struct with only the nine original u32 members, and it is referenced as
the fixed-header for the fq_pie entry of tca-stats-app-msg:
Documentation/netlink/specs/tc.yaml
value: fq_pie
fixed-header: tc-fq-pie-xstats
Should the spec be extended in the same patch so ynl-based decoders can
see the new type discriminator and the per-flow fields? Note YAML structs
are implicitly packed, so the nested tc_fq_pie_cl_stats members would need
to be spelled out explicitly.
> diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
> index b27d95418707a..32c76d2bb74bf 100644
> --- a/net/sched/sch_fq_pie.c
> +++ b/net/sched/sch_fq_pie.c
> @@ -310,9 +310,9 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
> goto flow_error;
> }
> q->flows_cnt = nla_get_u32(tb[TCA_FQ_PIE_FLOWS]);
> - if (!q->flows_cnt || q->flows_cnt > 65536) {
> + if (!q->flows_cnt || q->flows_cnt > 65535) {
> NL_SET_ERR_MSG_MOD(extack,
> - "Number of flows must range in [1..65536]");
> + "Number of flows must range in [1..65535]");
> goto flow_error;
> }
> }
[Severity: High]
Is this a uAPI regression? fq_pie_change() has accepted flows == 65536
since sch_fq_pie was merged, and after this change the same netlink
request fails with -EINVAL:
tc qdisc add dev dummy0 handle 1: root fq_pie flows 65536
There is an in-tree consumer of exactly that command:
tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json, id 83be
"cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq_pie flows 65536",
"expExitCode": "0",
and the later selftest patch in this series flips that case to expect exit
code 2 with matchCount 0. Does that mean the series records the behaviour
change rather than avoiding it?
The stated reason is that TC_H_MIN(65536) == 0 would give the 65536th flow
a class handle of major:0 in fq_pie_dump_class():
tcm->tcm_handle |= TC_H_MIN(cl);
Nothing kernel-side truncates - fq_pie_walk() passes i + 1 as an unsigned
long straight into fq_pie_dump_class_stats(). Could the class enumeration
simply stop at 65535 flows (or the handle be computed differently) so that
existing configurations using 65536 flows keep working? The changelog
does not mention that a previously valid configuration is now refused.
> @@ -510,7 +510,9 @@ static int fq_pie_dump(struct Qdisc *sch, struct sk_buff *skb)
> static int fq_pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
> {
> struct fq_pie_sched_data *q = qdisc_priv(sch);
> - struct tc_fq_pie_xstats st = { 0 };
> + struct tc_fq_pie_xstats st = {
> + .type = TCA_FQ_PIE_XSTATS_QDISC,
> + };
> struct list_head *pos;
>
> sch_tree_lock(sch);
[ ... ]
> @@ -562,7 +564,89 @@ static void fq_pie_destroy(struct Qdisc *sch)
> kvfree(q->flows);
> }
>
> +static struct Qdisc *fq_pie_leaf(struct Qdisc *sch, unsigned long arg)
> +{
> + return NULL;
> +}
> +
> +static unsigned long fq_pie_find(struct Qdisc *sch, u32 classid)
> +{
> + return 0;
> +}
> +
> +static int fq_pie_dump_class(struct Qdisc *sch, unsigned long cl,
> + struct sk_buff *skb, struct tcmsg *tcm)
> +{
> + tcm->tcm_handle |= TC_H_MIN(cl);
> + return 0;
> +}
> +
> +static int fq_pie_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> + struct gnet_dump *d)
> +{
> + struct fq_pie_sched_data *q = qdisc_priv(sch);
> + struct gnet_stats_queue qs = { 0 };
> + struct tc_fq_pie_xstats xstats;
> + u32 idx = cl - 1;
> +
> + if (idx < q->flows_cnt) {
> + const struct fq_pie_flow *flow = &q->flows[idx];
> +
> + memset(&xstats, 0, sizeof(xstats));
> + xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
> + xstats.class_stats.prob =
> + READ_ONCE(flow->vars.prob) << BITS_PER_BYTE;
> + xstats.class_stats.delay =
> + ((u32)PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay))) /
> + NSEC_PER_USEC;
[Severity: Medium]
Does the exported delay wrap here? PSCHED_TICKS2NS() yields an s64
nanosecond value, but the cast to u32 happens before the division by
NSEC_PER_USEC, so anything above 2^32 ns (about 4.295 s) folds over - a
5 s sojourn time is reported as roughly 705032 us, while the __u32 field
could carry about 4295 s.
With dq_rate_estimator off (the default) flow->vars.qdelay is the packet
sojourn time, and multi-second sojourn times are easy to produce behind a
slow shaper; the selftest added later in this series stacks fq_pie under
'tbf rate 8bit'. Would keeping the value 64-bit until after the division
be preferable?
delay = div_u64(PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay)),
NSEC_PER_USEC);
The same expressions already exist in pie_dump_stats() in
net/sched/sch_pie.c, so the pattern is inherited, but this adds new
instances per flow.
> + xstats.class_stats.deficit = READ_ONCE(flow->deficit);
> + xstats.class_stats.dq_rate_estimating =
> + READ_ONCE(q->p_params.dq_rate_estimator);
> +
> + if (xstats.class_stats.dq_rate_estimating) {
> + xstats.class_stats.avg_dq_rate =
> + READ_ONCE(flow->vars.avg_dq_rate) *
> + (PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
> + }
[Severity: Medium]
Can this multiplication overflow on 32-bit builds? PSCHED_TICKS_PER_SEC
derives from the long constant NSEC_PER_SEC:
include/net/pkt_sched.h
#define PSCHED_TICKS_PER_SEC (NSEC_PER_SEC >> PSCHED_SHIFT)
so on ILP32 the product stays 32-bit and wraps once avg_dq_rate exceeds
about 4396, i.e. rates above roughly 16.7 MB/s, even though the final
bytes/second result would still fit in the __u32 field. Would a u64
intermediate (or an explicit saturation) be better here?
> +
> + qs.qlen = READ_ONCE(flow->qlen);
> + qs.backlog = READ_ONCE(flow->backlog);
> + }
[Severity: Medium]
Are these READ_ONCE()s paired with anything on the writer side?
All six fields read here are updated with plain stores under the qdisc
root lock:
fq_pie_qdisc_enqueue()
sel_flow->deficit = q->quantum;
sel_flow->qlen = 0;
sel_flow->backlog = 0;
...
sel_flow->qlen++;
sel_flow->backlog += pkt_len;
fq_pie_qdisc_dequeue()
flow->qlen--;
flow->deficit -= pkt_len;
flow->backlog -= pkt_len;
pie_process_dequeue(skb, &q->p_params, &flow->vars, flow->backlog);
fq_pie_timer()
spin_lock(root_lock);
...
pie_calculate_probability(&q->p_params,
&q->flows[q->flows_cursor].vars,
q->flows[q->flows_cursor].backlog);
and this new reader takes no lock at all. vars.prob is u64 and
vars.qdelay is psched_time_t (u64), so on 32-bit builds can a concurrent
read return a torn value? On all builds, can userspace observe an
inconsistent snapshot, for example qlen == 0 together with a non-zero
backlog while enqueue is in its reset window, or prob and delay from two
different tupdate rounds?
Taking the lock appears to be allowed here - tc_fill_tclass() passes NULL
as the lock to gnet_stats_start_copy_compat() before calling
cl_ops->dump_stats, and fq_pie_dump_stats() in this same file already
brackets its counters with sch_tree_lock()/sch_tree_unlock().
Note sch_fq_codel, whose class-stats reader this mirrors, converted its
writers to WRITE_ONCE() (flow->deficit, flow->head, flow->cvars.count)
precisely to pair with the lockless reader. Should fq_pie either do the
same or hold sch_tree_lock() over the snapshot?
> + if (gnet_stats_copy_queue(d, NULL, &qs, qs.qlen) < 0)
> + return -1;
> + if (idx < q->flows_cnt)
> + return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
> + return 0;
> +}
> +
> +static void fq_pie_walk(struct Qdisc *sch, struct qdisc_walker *arg)
> +{
> + struct fq_pie_sched_data *q = qdisc_priv(sch);
> + unsigned int i;
> +
> + if (arg->stop)
> + return;
> +
> + for (i = 0; i < q->flows_cnt; i++) {
> + if (list_empty(&q->flows[i].flowchain)) {
> + arg->count++;
> + continue;
> + }
> + if (!tc_qdisc_stats_dump(sch, i + 1, arg))
> + break;
> + }
> +}
> +
> +static const struct Qdisc_class_ops fq_pie_class_ops = {
> + .leaf = fq_pie_leaf,
> + .find = fq_pie_find,
> + .dump = fq_pie_dump_class,
> + .dump_stats = fq_pie_dump_class_stats,
> + .walk = fq_pie_walk,
> +};
[Severity: Low]
This isn't a bug introduced by this patch, but with cl_ops now present the
missing .tcf_block becomes more visible. sch_fq_pie already carries the
full external classifier plumbing:
fq_pie_init() -> tcf_block_get(&q->block, &q->filter_list, sch, extack)
fq_pie_classify() -> filter = rcu_dereference_bh(q->filter_list);
fq_pie_destroy() -> tcf_block_put(q->block)
but __tcf_qdisc_find() in net/sched/cls_api.c rejects every filter
operation when the class ops table has no .tcf_block:
if (!cops->tcf_block) {
NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
err = -EOPNOTSUPP;
so q->filter_list can never become non-NULL and the classifier path in
fq_pie_classify() stays unreachable, as it was before this patch. The
changelog states the omission is deliberate; would adding the trivial
.tcf_block that sch_fq_codel provides be cheap enough to make that dead
code live?
> static struct Qdisc_ops fq_pie_qdisc_ops __read_mostly = {
> + .cl_ops = &fq_pie_class_ops,
> .id = "fq_pie",
[ ... ]
Also, minor: there is a stray blank line before the closing brace of
struct tc_fq_pie_xstats in include/uapi/linux/pkt_sched.h.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test
2026-09-02 3:52 ` [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test Hemendra M. Naik
@ 2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:05 ` Hemendra M. Naik
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-09-04 23:17 UTC (permalink / raw)
To: hemendranaik
Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, horms, jiri,
jhs, shuah, linux-kernel, linux-kselftest, vishy0777, tahiliani
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"
> + ]
> }
> ]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation
2026-09-02 3:52 ` [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation Hemendra M. Naik
@ 2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:09 ` Hemendra M. Naik
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2026-09-04 23:17 UTC (permalink / raw)
To: hemendranaik
Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, horms, jiri,
jhs, shuah, linux-kernel, linux-kselftest, vishy0777, tahiliani
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: pie: correct tc_pie_xstats field documentation
This updates the comments in struct tc_pie_xstats so the delay field is
described in microseconds and avg_dq_rate in bytes/second. The change is
described as documentation-only, touching include/uapi/linux/pkt_sched.h.
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 89d825bac4bee..c1f7bde472fb2 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -920,9 +920,9 @@ enum {
>
> struct tc_pie_xstats {
> __u64 prob; /* current probability */
> - __u32 delay; /* current delay in ms */
> + __u32 delay; /* current delay in microseconds */
[Severity: Low]
Should the vendored copy in tools/include/uapi/linux/pkt_sched.h be updated
in the same patch? Its struct tc_pie_xstats still carries exactly the
wording being removed here:
tools/include/uapi/linux/pkt_sched.h:
struct tc_pie_xstats {
__u32 prob; /* current probability */
__u32 delay; /* current delay in ms */
__u32 avg_dq_rate; /* current average dq_rate in bits/pie_time */
__u32 packets_in; /* total number of packets enqueued */
...
};
So after this change the two in-tree copies of the same structure describe
the same field with different units.
That copy also looks stale in other ways: it declares __u32 prob and has no
dq_rate_estimating member, while pie_dump_stats() in net/sched/sch_pie.c
fills a __u64 prob and always sets st.dq_rate_estimating before
gnet_stats_copy_app(), and its TCA_PIE_* enum lacks
TCA_PIE_DQ_RATE_ESTIMATOR. The FQ-PIE additions exported by
fq_pie_dump_class_stats() (TCA_FQ_PIE_XSTATS_QDISC/CLASS and
struct tc_fq_pie_cl_stats) are missing there as well.
The layout drift predates this series, but the unit-comment mismatch between
the two copies is introduced by touching only one of them. Would a resync of
tools/include/uapi/linux/pkt_sched.h, or at least of the tc_pie_xstats
comments, be appropriate so the stale documentation the commit message aims
to eliminate is actually gone from the tree?
[Severity: Medium]
This isn't a bug introduced by this patch, but does the exported value always
match the newly documented microsecond unit? pie_dump_stats() in
net/sched/sch_pie.c computes:
.delay = ((u32)PSCHED_TICKS2NS(READ_ONCE(q->vars.qdelay))) /
NSEC_PER_USEC,
PSCHED_TICKS2NS() is a 64-bit shift, and the (u32) cast is applied to the
nanosecond intermediate before the division, so a qdelay whose nanosecond
value exceeds 2^32-1 (roughly 4.295 s) wraps modulo 2^32 ns and then gets
divided, reporting a small microsecond number for a large delay. The __u32
field itself can hold about 71 minutes worth of microseconds, so the
truncation comes only from where the cast sits.
The same expression is used for the per-flow delay in
fq_pie_dump_class_stats() in net/sched/sch_fq_pie.c:
xstats.class_stats.delay =
((u32)PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay))) /
NSEC_PER_USEC;
This is reachable with a deep queue on a very slow link, where tc -s qdisc
show would print a wrong (small) delay.
Would moving the cast after the division in both sch_pie.c and sch_fq_pie.c
be worth a follow-up, so the code matches the microsecond contract this
comment now states?
> __u32 avg_dq_rate; /* current average dq_rate in
> - * bits/pie_time
> + * bytes/second
> */
> __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
> __u32 packets_in; /* total number of packets enqueued */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops
2026-09-04 23:17 ` Jakub Kicinski
@ 2026-09-16 3:03 ` Hemendra M. Naik
0 siblings, 0 replies; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-16 3:03 UTC (permalink / raw)
To: kuba
Cc: davem, edumazet, hemendranaik, horms, jhs, jiri, linux-kernel,
linux-kselftest, netdev, pabeni, shuah, tahiliani, vishy0777
Hi @Jakub,
Thank you for the review. Replies are inline below.
> The changelog says the new members are appended so that "qdisc xstats
> offsets stay compatible with the flat struct fq_pie already shipped".
> That is true for the offsets, but does it also change the size of an
> already shipped struct?
>
> nine __u32 counters = 36 bytes
> + __u32 type = 40 bytes
> + struct tc_fq_pie_cl_stats = 64 bytes
>
> A consumer that validates the TCA_STATS_APP payload with the usual
> iproute2 idiom:
>
> if (RTA_PAYLOAD(xstats) < sizeof(*st))
> return -1;
>
> would stop printing fq_pie xstats altogether once rebuilt against this
> header and run on an older kernel that still emits 36 bytes. sch_fq
> handled this by having userspace copy min(payload, sizeof(*st)).
> Should the changelog mention the size change so this is a conscious
> decision?
>
> Related: since fq_pie_dump_class_stats() reuses the same struct, an
> iproute2 that does not know about the new type field will decode the
> per-class blob using the qdisc layout and print nine all-zero qdisc
> counters per flow. Does that mean the stated goal ("'tc -s class show'
> reports per-flow state") depends on an iproute2 change that the
> changelog does not mention?
Thank you for pointing this out. tc already handles this case safely:
it has copied fq_pie xstats into a zeroed local struct bounded by
RTA_PAYLOAD() since fq_pie was added, so neither an old kernel nor an
old tc will crash or read out of bounds: only there will be zeroed out
extra fields with older tc. You are right, however, that the size growth
and the iproute2 dependency were not mentioned. In v6, the commit message
will state the 36-to-64 byte growth, and reference the companion
iproute2 patch.
> Documentation/netlink/specs/tc.yaml still describes tc-fq-pie-xstats as
> a struct with only the nine original u32 members [...] Should the spec
> be extended in the same patch so ynl-based decoders can see the new
> type discriminator and the per-flow fields?
We would prefer to leave this out of v6. tc does not need it: it parses
TCA_STATS_APP directly, so 'tc -s class show' works without touching
tc.yaml. tc-pie-xstats in tc.yaml has had similarly wrong units since
2014 with no issue. We are also not certain that ynl handles a short
payload safely, so changing the spec now could break it against older
kernels.
Hope it’s fine to have tc.yaml updates in a separate patch series.
> Is this a uAPI regression? fq_pie_change() has accepted flows == 65536
> since sch_fq_pie was merged, and after this change the same netlink
> request fails with -EINVAL [...] Could the class enumeration simply
> stop at 65535 flows (or the handle be computed differently) so that
> existing configurations using 65536 flows keep working?
This cap was our response to a Low-severity comment from the v4 review,
which flagged the same TC_H_MIN(65536) == 0 display quirk and offered
"document or cap" as options; we chose to cap for v5. We agree that turns
out to be a regression. We would like to drop the cap in v6 and document
the display quirk instead, so flows still accepts [1..65536], the same
as fq_codel today. @Jakub, Could you confirm that is acceptable
before we send it?
> Does the exported delay wrap here? PSCHED_TICKS2NS() yields an s64
> nanosecond value, but the cast to u32 happens before the division by
> NSEC_PER_USEC, so anything above 2^32 ns (about 4.295 s) folds over
> [...] Would keeping the value 64-bit until after the division be
> preferable?
This is a real bug. We will fix it in v6 with div_u64() applied to
the full 64-bit value before dividing.
> Can this multiplication overflow on 32-bit builds? [...] the product
> stays 32-bit and wraps once avg_dq_rate exceeds about 4396, i.e. rates
> above roughly 16.7 MB/s [...] Would a u64 intermediate (or an explicit
> saturation) be better here?
This is also a real bug. We will fix it in v6 by widening the value
to u64 before the multiply.
> Are these READ_ONCE()s paired with anything on the writer side? [...]
> vars.prob is u64 and vars.qdelay is psched_time_t (u64), so on 32-bit
> builds can a concurrent read return a torn value? [...] Should fq_pie
> either do the same [as fq_codel's WRITE_ONCE conversion] or hold
> sch_tree_lock() over the snapshot?
That is a fair point. We would prefer to treat this as out of scope for
this patch and take it up in a subsequent patch series. We are not planning
to reintroduce sch_tree_lock() to the class dump, since it was deliberately
removed there for performance.
> This isn't a bug introduced by this patch, but with cl_ops now present
> the missing .tcf_block becomes more visible [...] would adding the
> trivial .tcf_block that sch_fq_codel provides be cheap enough to make
> that dead code live?
We would like to leave this out of v6, if that is acceptable. This
series is about statistics, not about making fq_pie filter-capable,
and that would be a separate change deserving its own review. We are
happy to send it as a quick follow-up once this lands.
> Also, minor: there is a stray blank line before the closing brace of
> struct tc_fq_pie_xstats in include/uapi/linux/pkt_sched.h.
This will already be gone in v6, since that block is being rewritten
to address the first comment above.
Thanks,
Hemendra
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test
2026-09-04 23:17 ` Jakub Kicinski
@ 2026-09-16 3:05 ` Hemendra M. Naik
0 siblings, 0 replies; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-16 3:05 UTC (permalink / raw)
To: kuba
Cc: davem, edumazet, hemendranaik, horms, jhs, jiri, linux-kernel,
linux-kselftest, netdev, pabeni, shuah, tahiliani, vishy0777
Hi @Jakub,
Thank you for the review. Replies are inline below.
> This isn't a bug, the flip is correct: patch 1 of this series does add
> the cap in fq_pie_change() [...] The only remark is about the commit
> message wording [...] 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?
This whole case will be gone in v6. We are dropping the flows-65536 cap
in patch 1, so there will be no behavior change left for this test to
flip. Case 83be will stay exactly as it is today.
> Can any of this ping traffic actually reach the fq_pie child? [...]
> 'ping -s 500' produces roughly 542-byte frames, so all 50 packets look
> like they are dropped in TBF and fq_pie never sees a packet [...]
> Would dropping '-s 500' (or raising the TBF burst above the frame
> size) be the intent here?
Confirmed, all of them were being dropped. We will fix this in v6 by
dropping -s 500, which defaults to the standard 56-byte payload used by
the other tc-testing cases with this TBF setup, and by adding -W 0.01 so
the test does not stall for about 10 seconds per run.
> Which iproute2 version prints these tokens? [...] tc routes the blob
> to q_fq_pie's existing print_xstats, which only knows the legacy
> qdisc counters [...] and has no notion of the new type discriminator.
> With any released tc the regex matches 0 times [...] and tdc has no
> iproute2 version gating, so the case fails permanently rather than
> being skipped. [...] Should it also state the required iproute2
> change, since the case cannot pass on a patched kernel with a shipped
> tc either?
You are right, no released tc prints those fields yet. We will fix this
in v6 by relaxing the match to just the class line ("class fq_pie
2:[0-9]+"), which generic tc code prints directly from the kernel's
class dump regardless of tc version. The commit message will also note
that the per-flow fields need the companion iproute2 update.
Thanks,
Hemendra
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation
2026-09-04 23:17 ` Jakub Kicinski
@ 2026-09-16 3:09 ` Hemendra M. Naik
0 siblings, 0 replies; 10+ messages in thread
From: Hemendra M. Naik @ 2026-09-16 3:09 UTC (permalink / raw)
To: kuba
Cc: davem, edumazet, hemendranaik, horms, jhs, jiri, linux-kernel,
linux-kselftest, netdev, pabeni, shuah, tahiliani, vishy0777
Hi @Jakub,
Thank you for the review. Replies are inline below.
> Should the vendored copy in tools/include/uapi/linux/pkt_sched.h be
> updated in the same patch? [...] So after this change the two in-tree
> copies of the same structure describe the same field with different
> units. [...] Would a resync of tools/include/uapi/linux/pkt_sched.h,
> or at least of the tc_pie_xstats comments, be appropriate so the stale
> documentation the commit message aims to eliminate is actually gone
> from the tree?
We would prefer to leave this out of v6. The v4 review asked us to drop
that, and we did; adding it back now would undo that. The copy is
stale well beyond this one comment (u32 prob, no dq_rate_estimating, no
FQ-PIE additions at all), so fixing a single unit comment there would
not help much. A proper resync feels like its own patch.
> This isn't a bug introduced by this patch, but does the exported value
> always match the newly documented microsecond unit? [...] a qdelay
> whose nanosecond value exceeds 2^32-1 (roughly 4.295 s) wraps modulo
> 2^32 ns and then gets divided, reporting a small microsecond number
> for a large delay. [...] Would moving the cast after the division in
> both sch_pie.c and sch_fq_pie.c be worth a follow-up, so the code
> matches the microsecond contract this comment now states?
Thank you for catching this; the bug is real, and you already flagged
the same issue for sch_fq_pie.c on patch 1 — we will fix it there with
div_u64(). No code change is needed for this patch itself; it is
comment-only and correct as posted. We will send the sch_pie.c fix as
a follow-up after this series.
One more query: I see that the counterpart iproute2 v5 patches are currently
marked as "Awaiting Upstream". In that case, is it necessary to post a v6 of
the iproute2 series, or can we wait for the v5 patch to be reviewed?
Thanks,
Hemendra
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-09-16 3:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-02 3:52 [PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:03 ` Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:05 ` Hemendra M. Naik
2026-09-02 3:52 ` [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation Hemendra M. Naik
2026-09-04 23:17 ` Jakub Kicinski
2026-09-16 3:09 ` Hemendra M. Naik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®