[PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics

[PATCH net-next v5 0/3] net/sched: sch_fq_pie: add per-flow class statistics

[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

[PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops

[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

Re: [PATCH net-next v5 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops

[Jakub Kicinski]

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

[PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test

[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

Re: [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test

[Jakub Kicinski]

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"
> +        ]
>      }
>  ]

[PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation

[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

Re: [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation

[Jakub Kicinski]

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 */