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

[PATCH net-next v8 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.

This patch series is accompanied by a companion iproute2 patch series that
will be posted on net-next as well.

Both the kernel and iproute2 patches can also be found in GitHub:
- https://github.com/H-N41K/linux-net-next/tree/fq_pie_class_stats/
- https://github.com/H-N41K/iproute2-next/tree/fq_pie_class_stats

---
Changelog:
v8:
- No code changes since v7.
- Reword commit message to reflect usage of div_u64().

v7: https://lore.kernel.org/netdev/20260919060125.38063-1-hemendranaik@gmail.com/
- No code changes since v6.
- Rebased over the latest net-next tree and re-submitted patch.

v6: https://lore.kernel.org/netdev/20260917204225.275251-1-hemendranaik@gmail.com/
- Address Sashiko review comments:
  - Revert the fq_pie flows range to [1..65536]. 
  - Also drop the accompanying selftest 83be change.
  - Compute the per-flow delay with div_u64() on the full 64-bit
    nanosecond value.
  - Widen avg_dq_rate to u64 before scaling it by PSCHED_TICKS_PER_SEC.
  - Document the 36 to 64 byte xstats growth in the patch 1 commit message.
  - Fix selftest 83c0. The ping payload exceeded the TBF burst so no
    packet ever reached fq_pie, and the missing -W left the run waiting
    out ping's default timeout.

v5: https://lore.kernel.org/netdev/20260902035231.81866-1-hemendranaik@gmail.com/
- 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                | 20 ++++-
 net/sched/sch_fq_pie.c                        | 86 ++++++++++++++++++-
 .../tc-testing/tc-tests/qdiscs/fq_pie.json    | 27 ++++++
 3 files changed, 130 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net-next v8 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. The nine existing __u32 counters keep their
offsets, type is appended at offset 36 and class_stats follows it,
growing the structure from 36 to 64 bytes.

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.

Derive delay with div_u64() from the full 64-bit nanosecond. 
Widen avg_dq_rate to u64 before scaling it by PSCHED_TICKS_PER_SEC.

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 | 16 +++++++
 net/sched/sch_fq_pie.c         | 86 +++++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 490efd288526..9c0c9787f0c3 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -953,6 +953,20 @@ 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 +977,8 @@ 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 5982847df8f8..3a3ef83f328b 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -511,7 +511,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);
@@ -563,7 +565,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 =
+			div_u64(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 =
+				((u64)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 v8 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 6

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] In include/uapi/linux/pkt_sched.h, the patch appends `__u32
  type` at offset 36 and then a whole `struct tc_fq_pie_cl_stats…
- [Medium] fq_pie_dump_class_stats() computes
  `((u64)READ_ONCE(flow->vars.avg_dq_rate) * PSCHED_TICKS_PER_SEC) >>…
- [Low] The commit message says the class ops are wired up "so that 'tc
  -s class show' reports per-flow state (prob, delay, deficit,…
- [Low] Setting `.cl_ops = &fq_pie_class_ops` makes the TC core treat
  fq_pie as classful.
- [Low] The patch grows `struct tc_fq_pie_xstats` in
  include/uapi/linux/pkt_sched.h from 36 to 64 bytes.
- [Low] fq_pie_walk() and fq_pie_dump_class() list classes
  `<major>:<i+1>` for every active flow, but fq_pie_find() always…
- [Low] fq_pie_change() accepts `flows_cnt` up to 65536
  (sch_fq_pie.c:313: `if (!q->flows_cnt || q->flows_cnt > 65536)`).
- [Low] fq_pie_dump_class_stats() reads per-flow state with READ_ONCE()
  only and holds no qdisc lock.

--

net/sched: sch_fq_pie: add per-flow statistics via class ops

This commit adds a type discriminator and an embedded struct
tc_fq_pie_cl_stats to struct tc_fq_pie_xstats. The struct grows from 36 to
64 bytes. It also adds a minimal Qdisc_class_ops table to fq_pie so that
each active flow is reported as a pseudo-class with its per-flow PIE state.

> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 490efd2885267..9c0c9787f0c32 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -953,6 +953,20 @@ 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? */
> +};

[Severity: Low]
Should Documentation/netlink/specs/tc.yaml be updated along with this?

In the spec, tc-fq-pie-xstats still lists only the nine original u32
members. The tca-stats-app-msg sub-message still binds fq_pie to it:

      -
        value: fq_pie
        fixed-header: tc-fq-pie-xstats

The spec has no type member, no class-stats member, no tc-fq-pie-cl-stats
struct and no enum for TCA_FQ_PIE_XSTATS_QDISC/CLASS.

A spec-driven (YNL) decoder will only decode the first 36 bytes of the new
64-byte payload. It cannot tell class records from qdisc records, and it
never shows prob, delay, deficit, avg_dq_rate or dq_rate_estimating.

tc.yaml is still unchanged at the end of the series (after "net/sched: pie:
correct tc_pie_xstats field documentation").

>  struct tc_fq_pie_xstats {
>  	__u32 packets_in;	/* total number of packets enqueued */
>  	__u32 dropped;		/* packets dropped due to fq_pie_action */
> @@ -963,6 +977,8 @@ 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]
Will this layout limit how both views can be extended later?

type sits at offset 36 and class_stats at offset 40, so the struct is 64
bytes. Once this is released:

  - a new qdisc-level counter can only be appended after class_stats
  - after that, struct tc_fq_pie_cl_stats can never grow again

Every message also carries both views. fq_pie_dump_stats() sends 24 zero
bytes of class stats, and fq_pie_dump_class_stats() sends nine zeroed qdisc
counters.

The sibling struct in the same header avoids this with a union:

struct tc_fq_codel_xstats {
	__u32	type;
	union {
		struct tc_fq_codel_qd_stats qdisc_stats;
		struct tc_fq_codel_cl_stats class_stats;
	};
};

Keeping the existing offsets means type cannot go first here. Would a union
after the discriminator still let the two views grow independently?

	__u32 type;
	union {
		struct tc_fq_pie_cl_stats class_stats;
		/* future qdisc extension */
	};

The commit message gives the new size and offsets but does not mention this
constraint.

>  
>  /* CBS */
> diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
> index 5982847df8f85..3a3ef83f328b2 100644
> --- a/net/sched/sch_fq_pie.c
> +++ b/net/sched/sch_fq_pie.c

[ ... ]

> @@ -563,7 +565,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;
> +}

[Severity: Low]
fq_pie_walk() lists a class <major>:<i+1> for every active flow, but
fq_pie_find() always returns 0. Is it intended that a class shown by
'tc class show' gets -ENOENT from a targeted RTM_GETTCLASS, such as
'tc class get dev X classid <major>:N' going through __tc_ctl_tclass()?

fq_codel_find(), sfq_find() and cake_find() also return 0 for their
pseudo-classes. A non-zero return would also send qdisc_leaf() and the
RTM_DELTCLASS paths into ops that fq_pie does not implement. So this may
well be the intended convention. It does mean per-flow stats can only be
read through a full dump.

> +
> +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);

[Severity: Low]
Can the last flow be reported under the qdisc's own handle?

fq_pie_change() accepts up to 65536 flows:

	if (!q->flows_cnt || q->flows_cnt > 65536) {

fq_pie_walk() passes i + 1 as the class id. With flows_cnt == 65536, the
flow at index 65535 is dumped with cl == 65536. TC_H_MIN() masks that to
minor 0, which gives a handle of <major>:0, the same as the qdisc itself.

fq_pie_dump_class_stats() still uses the unmasked cl, so the stats are
correct but userspace sees them under the qdisc handle.

fq_codel has the same latent issue. This patch brings it into fq_pie.

> +	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;

[Severity: Low]
The commit message says the class ops are wired up:

  so that 'tc -s class show' reports per-flow state (prob, delay, deficit,
  avg_dq_rate, dq_rate_estimating).

Does that hold with an existing tc binary?

Here the nine qdisc counters are zeroed and the per-flow data is in
class_stats at offset 40. A tc that predates the type field treats every
fq_pie xstats blob as qdisc stats. It would likely print a row of zero qdisc
counters for each active flow class and none of the per-flow values. Before
this patch fq_pie had no cl_ops, so 'tc class show' printed nothing for it.

The companion iproute2 change is only mentioned in the follow-up commit
"selftests: tc-testing: add fq_pie per-flow class stats test" ("printed
with the companion iproute2 change"). Could this commit mention it as well?

> +		xstats.class_stats.prob =
> +			READ_ONCE(flow->vars.prob) << BITS_PER_BYTE;
> +		xstats.class_stats.delay =
> +			div_u64(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 =
> +				((u64)READ_ONCE(flow->vars.avg_dq_rate)
> +				* PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
> +		}

[Severity: Medium]
Does this silently truncate the rate?

The product is computed in 64 bits and then assigned to the __u32
tc_fq_pie_cl_stats.avg_dq_rate, so the high bits are lost.

pie_vars.avg_dq_rate holds bytes per psched tick scaled by 2^8.
pie_process_dequeue() fills it from (dq_count << PIE_SCALE) / dtime, and
dtime can be only a few ticks during back-to-back GSO dequeues.

PSCHED_TICKS_PER_SEC is 15625000 and PIE_SCALE is 8. Any internal value
above about 70369 (roughly 4.29e9 B/s, about 34.4 Gbit/s) wraps. For
example, 81920 converts to 5,000,000,000 B/s but is exported as 705,032,704.

The commit message says:

  Widen avg_dq_rate to u64 before scaling it by PSCHED_TICKS_PER_SEC.

That moves the overflow to the narrowing assignment instead of avoiding it.
This is a new uAPI field. Could it be made __u64, or saturated at U32_MAX,
before it is released?

> +
> +		qs.qlen    = READ_ONCE(flow->qlen);
> +		qs.backlog = READ_ONCE(flow->backlog);
> +	}

[Severity: Low]
Are these lockless reads safe against the datapath writers?

tc_fill_tclass() passes a NULL lock to gnet_stats_start_copy_compat(), so
this function runs under RTNL only. Meanwhile enqueue, dequeue and
fq_pie_timer() update the same fields under the qdisc root lock on other
CPUs.

The writers of deficit, qlen and backlog use plain stores with no
WRITE_ONCE(). In fq_pie_qdisc_enqueue():

		if (list_empty(&sel_flow->flowchain)) {
			...
			sel_flow->deficit = q->quantum;
			sel_flow->qlen = 0;
			sel_flow->backlog = 0;
		}
		sel_flow->qlen++;
		sel_flow->backlog += pkt_len;

And in fq_pie_qdisc_dequeue():

	flow->qlen--;
	flow->deficit -= pkt_len;
	flow->backlog -= pkt_len;

KCSAN_ASSUME_PLAIN_WRITES_ATOMIC does not cover compound writes like these,
so KCSAN will likely report them. The reader can also see intermediate
values, such as qlen == 0 on a flow that was just activated. fq_codel
annotates the equivalent writers, for example WRITE_ONCE(flow->deficit,
...).

vars.prob and vars.qdelay are u64, so READ_ONCE() can tear on 32-bit
kernels.

pie_calculate_probability() also publishes an unclamped value before it
applies the overflow fix-up:

	WRITE_ONCE(vars->prob, vars->prob + delta);

	if (delta > 0) {
		/* prevent overflow */
		if (vars->prob < oldprob) {
			WRITE_ONCE(vars->prob, MAX_PROB);

A lockless reader here could therefore export a wrapped probability.

fq_pie_dump_stats() in the same file takes sch_tree_lock(). Should the
class dump take it too, or should the fq_pie writers be annotated?

[ ... ]

> +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,

[Severity: Low]
Setting .cl_ops makes the TC core treat fq_pie as classful. Is the change
in user-visible errors intended?

Filter add on parent <fq_pie>: used to fail in __tcf_qdisc_find() with
-EINVAL "Qdisc not classful". It now reaches this check instead:

	if (!cops->tcf_block) {
		NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
		err = -EOPNOTSUPP;
		goto errout_qdisc;
	}

Grafting or getting a child on parent <fq_pie>:N through qdisc_leaf() used
to return -EOPNOTSUPP "Parent qdisc is not classful". Because fq_pie_find()
always returns 0, it now returns -ENOENT "Specified class not found".

RTM_GETTCLASS and RTM_DELTCLASS in __tc_ctl_tclass() also change from
-EINVAL to -ENOENT.

fq_pie_init() still calls tcf_block_get(), and fq_pie_classify() still
reads q->filter_list and maps tcf results to flow ids 1..flows_cnt. That is
the class id space this patch exposes. With .tcf_block, .bind_tcf and
.unbind_tcf left out, the classifier path stays unreachable, as it was
before this patch.

The commit message says these callbacks are "omitted on purpose". Could it
give the reason and mention the errno changes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922205924.32173-1-hemendranaik%40gmail.com

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

[Hemendra M. Naik]

Thank you for the review. Replies are inline below. No code changes are
planned for this patch.

> [Severity: Medium]
> Will this layout limit how both views can be extended later?
>
> type sits at offset 36 and class_stats at offset 40, so the struct is 64
> bytes. Once this is released:
>
>   - a new qdisc-level counter can only be appended after class_stats
>   - after that, struct tc_fq_pie_cl_stats can never grow again
>
> Every message also carries both views. fq_pie_dump_stats() sends 24 zero
> bytes of class stats, and fq_pie_dump_class_stats() sends nine zeroed qdisc
> counters.
>
> The sibling struct in the same header avoids this with a union:
>
> struct tc_fq_codel_xstats {
> 	__u32	type;
> 	union {
> 		struct tc_fq_codel_qd_stats qdisc_stats;
> 		struct tc_fq_codel_cl_stats class_stats;
> 	};
> };
>
> Keeping the existing offsets means type cannot go first here. Would a
> union after the discriminator still let the two views grow independently?
>
> 	__u32 type;
> 	union {
> 		struct tc_fq_pie_cl_stats class_stats;
> 		/* future qdisc extension */
> 	};
>
> The commit message gives the new size and offsets but does not mention this
> constraint.

We actually posted exactly this in an earlier round:

	__u32 type;
	union {
		struct tc_fq_pie_cl_stats class_stats;
		struct tc_fq_pie_xqd_stats xqdisc_stats;
	};

It was removed after review flagged that the qdisc-side arm
(tc_fq_pie_xqd_stats) had no concrete member yet, making it an empty
struct in a UAPI header.
We flattened class_stats to a direct member specifically to avoid that
portability problem. We're not planning to reintroduce a union now,
since there's still no concrete qdisc-only field to justify the second
arm. Once one exists, wrapping class_stats and it in an anonymous union is
straightforward and won't break anything, since everything from offset
36 onward is still unshipped.


> [Severity: Medium]
> Does this silently truncate the rate?
>
> The product is computed in 64 bits and then assigned to the __u32
> tc_fq_pie_cl_stats.avg_dq_rate, so the high bits are lost.
>
> pie_vars.avg_dq_rate holds bytes per psched tick scaled by 2^8.
> pie_process_dequeue() fills it from (dq_count << PIE_SCALE) / dtime, and
> dtime can be only a few ticks during back-to-back GSO dequeues.
>
> PSCHED_TICKS_PER_SEC is 15625000 and PIE_SCALE is 8. Any internal value
> above about 70369 (roughly 4.29e9 B/s, about 34.4 Gbit/s) wraps. For
> example, 81920 converts to 5,000,000,000 B/s but is exported as 705,032,704.
>
> The commit message says:
>
>   Widen avg_dq_rate to u64 before scaling it by PSCHED_TICKS_PER_SEC.
>
> That moves the overflow to the narrowing assignment instead of avoiding it.
> This is a new uAPI field. Could it be made __u64, or saturated at U32_MAX,
> before it is released?

We're keeping avg_dq_rate as __u32 by design, to stay consistent with
the sibling field in the older tc_pie_xstats. pie_dump_stats() in
sch_pie.c computes the exact same formula into the exact same __u32
width, with the exact same ~34.4 Gbit/s wrap ceiling, and has shipped
that way, unchanged, since the original 2014 PIE merge without this
being a reported problem. We'd rather keep the two structs consistent
with each other than introduce an asymmetry between them for one field.

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

[Jakub Kicinski]

On Thu, 24 Sep 2026 21:00:39 +0000 netdev-bot+sashiko@kernel.org wrote:
> [Severity: Low]
> Should Documentation/netlink/specs/tc.yaml be updated along with this?
> 
> In the spec, tc-fq-pie-xstats still lists only the nine original u32
> members. The tca-stats-app-msg sub-message still binds fq_pie to it:
> 
>       -
>         value: fq_pie
>         fixed-header: tc-fq-pie-xstats
> 
> The spec has no type member, no class-stats member, no tc-fq-pie-cl-stats
> struct and no enum for TCA_FQ_PIE_XSTATS_QDISC/CLASS.
> 
> A spec-driven (YNL) decoder will only decode the first 36 bytes of the new
> 64-byte payload. It cannot tell class records from qdisc records, and it
> never shows prob, delay, deficit, avg_dq_rate or dq_rate_estimating.
> 
> tc.yaml is still unchanged at the end of the series (after "net/sched: pie:
> correct tc_pie_xstats field documentation").

Looks legit, please check the other Low severity review comments.

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

[Hemendra M. Naik]

Hi Jakub,

We have reviewed the low-severity comments. Please find our responses inline
below.

> [Severity: Low]
> The commit message says the class ops are wired up:
>
>   so that 'tc -s class show' reports per-flow state (prob, delay, deficit,
>   avg_dq_rate, dq_rate_estimating).
>
> Does that hold with an existing tc binary?
>
> Here the nine qdisc counters are zeroed and the per-flow data is in
> class_stats at offset 40. A tc that predates the type field treats every
> fq_pie xstats blob as qdisc stats. It would likely print a row of zero qdisc
> counters for each active flow class and none of the per-flow values. Before
> this patch fq_pie had no cl_ops, so 'tc class show' printed nothing for it.
>
> The companion iproute2 change is only mentioned in the follow-up commit
> "selftests: tc-testing: add fq_pie per-flow class stats test" ("printed
> with the companion iproute2 change"). Could this commit mention it as well?

That's expected, not a regression, and it holds in both directions.
tc has copied min(RTA_PAYLOAD(xstats), sizeof(*st)) into a zeroed
struct since fq_pie support was first added to iproute2 (commit
9dced637, "tc: add support for FQ-PIE packet scheduler", 2019) - every
released tc that knows fq_pie parses this way, so an old tc against a
new kernel degrades to a zeroed row instead of erroring out. The
companion iproute2 patch carries the same discipline forward on the
new side too: fq_pie_print_xstats() treats a missing or zero type as
the qdisc case (`if (!st->type || st->type == TCA_FQ_PIE_XSTATS_QDISC)`),
so a new tc against an old, pre-type kernel also degrades gracefully
instead of misreading the blob. The companion iproute2 dependency is already
documented in patch 2/3's commit message and in both series' cover letters; 
we don't think it needs a third copy here.

> [Severity: Low]
> Setting .cl_ops makes the TC core treat fq_pie as classful. Is the change
> in user-visible errors intended?
>
> Filter add on parent <fq_pie>: used to fail in __tcf_qdisc_find() with
> -EINVAL "Qdisc not classful". It now reaches this check instead:
>
> 	if (!cops->tcf_block) {
> 		NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
> 		err = -EOPNOTSUPP;
> 		goto errout_qdisc;
> 	}
>
> Grafting or getting a child on parent <fq_pie>:N through qdisc_leaf() used
> to return -EOPNOTSUPP "Parent qdisc is not classful". Because fq_pie_find()
> always returns 0, it now returns -ENOENT "Specified class not found".
>
> RTM_GETTCLASS and RTM_DELTCLASS in __tc_ctl_tclass() also change from
> -EINVAL to -ENOENT.
>
> fq_pie_init() still calls tcf_block_get(), and fq_pie_classify() still
> reads q->filter_list and maps tcf results to flow ids 1..flows_cnt. That is
> the class id space this patch exposes. With .tcf_block, .bind_tcf and
> .unbind_tcf left out, the classifier path stays unreachable, as it was
> before this patch.
>
> The commit message says these callbacks are "omitted on purpose". Could it
> give the reason and mention the errno changes?

Yes, intended - an unavoidable side effect of any qdisc gaining .cl_ops
for the first time. fq_codel, sfq and cake don't document the same
errno transitions either, so we don't think this needs it here.

fq_pie_class_ops has no .tcf_block. __tcf_qdisc_find()
(net/sched/cls_api.c) rejects any filter attach with -EOPNOTSUPP when
cops->tcf_block is NULL, before q->filter_list is touched. So
q->filter_list stays NULL for good, and fq_pie_classify()'s tcf branch
never runs.

That branch has no case for TC_ACT_CONSUMED - a gap shared with
fq_codel, sfq, cake, htb, hfsc, drr, qfq, multiq, prio, sfb, ets and
dualpi2, predating this series. It's unreachable here without
.tcf_block, which we're not adding, so it's out of scope.

We're not planning to fold this into the commit message - it's covered
above.

> [Severity: Low]
> Should Documentation/netlink/specs/tc.yaml be updated along with this?
>
> In the spec, tc-fq-pie-xstats still lists only the nine original u32
> members. The tca-stats-app-msg sub-message still binds fq_pie to it:
>
>       -
>         value: fq_pie
>         fixed-header: tc-fq-pie-xstats
>
> The spec has no type member, no class-stats member, no tc-fq-pie-cl-stats
> struct and no enum for TCA_FQ_PIE_XSTATS_QDISC/CLASS.
>
> A spec-driven (YNL) decoder will only decode the first 36 bytes of the new
> 64-byte payload. It cannot tell class records from qdisc records, and it
> never shows prob, delay, deficit, avg_dq_rate or dq_rate_estimating.
>
> tc.yaml is still unchanged at the end of the series (after "net/sched: pie:
> correct tc_pie_xstats field documentation").

This is a gap and we don't dispute it, but it isn't new to fq_pie.
tc-fq-codel-xstats in tc.yaml is the flat qdisc_stats view - no
class-stats member, no enum for TCA_FQ_CODEL_XSTATS_QDISC/CLASS - even
though tc_fq_codel_xstats, the union struct cited above as the pattern
to follow, has had a class_stats arm since it was added. We believe this 
would be better handled in a logically separate patch series to bring tc.yaml
up to date with the current xstats structures across all qdiscs, rather than 
addressing fq_pie in isolation here.

> [Severity: Low]
> Can the last flow be reported under the qdisc's own handle?
>
> fq_pie_change() accepts up to 65536 flows:
>
> 	if (!q->flows_cnt || q->flows_cnt > 65536) {
>
> fq_pie_walk() passes i + 1 as the class id. With flows_cnt == 65536, the
> flow at index 65535 is dumped with cl == 65536. TC_H_MIN() masks that to
> minor 0, which gives a handle of <major>:0, the same as the qdisc itself.
>
> fq_pie_dump_class_stats() still uses the unmasked cl, so the stats are
> correct but userspace sees them under the qdisc handle.
>
> fq_codel has the same latent issue. This patch brings it into fq_pie.

Agreed this exists, and we've addressed the same point in a previous patch 
before: it's a display quirk. fq_codel has had the identical 16-bit 
TC_H_MIN() wraparound at the identical 65536-flow limit since 2012, 
and it isn't reachable through any other addressable operation - only 
a dump can ever surface it. We're staying consistent with fq_codel's 
long-standing, accepted behaviour rather than special-casing fq_pie alone 
for a display-only edge case for now. A separate patch can address 
both of these together.

> [Severity: Low]
> fq_pie_walk() lists a class <major>:<i+1> for every active flow, but
> fq_pie_find() always returns 0. Is it intended that a class shown by
> 'tc class show' gets -ENOENT from a targeted RTM_GETTCLASS, such as
> 'tc class get dev X classid <major>:N' going through __tc_ctl_tclass()?
>
> fq_codel_find(), sfq_find() and cake_find() also return 0 for their
> pseudo-classes. A non-zero return would also send qdisc_leaf() and the
> RTM_DELTCLASS paths into ops that fq_pie does not implement. So this may
> well be the intended convention. It does mean per-flow stats can only be
> read through a full dump.

Confirmed intentional - same convention as fq_codel_find(), sfq_find()
and cake_find(). Pseudo-classes across this whole family are dump-only
by design; nothing to change here.

> [Severity: Low]
> Are these lockless reads safe against the datapath writers?
>
> tc_fill_tclass() passes a NULL lock to gnet_stats_start_copy_compat(), so
> this function runs under RTNL only. Meanwhile enqueue, dequeue and
> fq_pie_timer() update the same fields under the qdisc root lock on other
> CPUs.
>
> The writers of deficit, qlen and backlog use plain stores with no
> WRITE_ONCE(). In fq_pie_qdisc_enqueue():
>
> 		if (list_empty(&sel_flow->flowchain)) {
> 			...
> 			sel_flow->deficit = q->quantum;
> 			sel_flow->qlen = 0;
> 			sel_flow->backlog = 0;
> 		}
> 		sel_flow->qlen++;
> 		sel_flow->backlog += pkt_len;
>
> And in fq_pie_qdisc_dequeue():
>
> 	flow->qlen--;
> 	flow->deficit -= pkt_len;
> 	flow->backlog -= pkt_len;
>
> KCSAN_ASSUME_PLAIN_WRITES_ATOMIC does not cover compound writes like these,
> so KCSAN will likely report them. The reader can also see intermediate
> values, such as qlen == 0 on a flow that was just activated. fq_codel
> annotates the equivalent writers, for example WRITE_ONCE(flow->deficit,
> ...).
>
> vars.prob and vars.qdelay are u64, so READ_ONCE() can tear on 32-bit
> kernels.
>
> pie_calculate_probability() also publishes an unclamped value before it
> applies the overflow fix-up:
>
> 	WRITE_ONCE(vars->prob, vars->prob + delta);
>
> 	if (delta > 0) {
> 		/* prevent overflow */
> 		if (vars->prob < oldprob) {
> 			WRITE_ONCE(vars->prob, MAX_PROB);
>
> A lockless reader here could therefore export a wrapped probability.
>
> fq_pie_dump_stats() in the same file takes sch_tree_lock(). Should the
> class dump take it too, or should the fq_pie writers be annotated?

Lockless READ_ONCE() stats dumps are already accepted, existing
practice in net/sched, and sch_pie carries the same u64 and
unclamped-prob exposure on the read side. Since this patch series is 
focused on class_stats, we’d prefer to make such annotations in a separate one.


Regards,
Hemendra

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

[Jakub Kicinski]

On Wed, 23 Sep 2026 02:29:22 +0530 Hemendra M. Naik wrote:
> +	__u32 dq_rate_estimating;	/* is avg_dq_rate being calculated? */
> +};
>  struct tc_fq_pie_xstats {

nit: missing empty line between structs

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

[Hemendra M. Naik]

Add tc-testing case 83c0: build a TBF + fq_pie hierarchy on $DUMMY, push
traffic through it with ping, and verify that 'tc -s class show' reports
an fq_pie class.

The per-flow counters themselves (prob, delay, deficit) are printed with
the companion iproute2 change, "tc: fq_pie: add support for printing
per-flow PIE statistics"

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    | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

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..15b2667285eb 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
@@ -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 -W 0.01 -s 56 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 v8 2/3] selftests: tc-testing: add fq_pie per-flow class stats test

[netdev-bot+sashiko]

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

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

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] Test 83c0 in tools/testing/selftests/tc-testing/tc-
  tests/qdiscs/fq_pie.json depends on an iproute2 change that has not…
- [Low] Test 83c0 relies on a ping step, `ping -c 50 -i 0.001 -W 0.01 -s
  56 10.10.11.11 -I $DUMMY > /dev/null 2>&1 || true`, to put packets…

--

selftests: tc-testing: add fq_pie per-flow class stats test

This adds tc-testing case 83c0. It builds a tbf + fq_pie hierarchy on
$DUMMY, pushes traffic through it with ping, and checks the output of
"tc -s class show" for an fq_pie class. The commit message says the
per-flow counters are printed by a companion iproute2 change.

> 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..15b2667285eb4 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

[ ... ]

> +        "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 -W 0.01 -s 56 10.10.11.11 -I $DUMMY > /dev/null 2>&1 || true"

[Severity: Low]
This is a robustness point, not a bug. fq_pie_walk() in
net/sched/sch_fq_pie.c only dumps flows that have a non-empty flowchain:

    if (list_empty(&q->flows[i].flowchain)) {
        arg->count++;
        continue;
    }

That makes this ping the only thing that gets class 2:1 into the output.

In the standard tdc setup this works. nsPlugin runs the command inside
the namespace, dummy is NOARP, and the 98-byte frame fits under tbf's
max_size of 100.

If ping is missing, or doesn't accept -i 0.001 / -W 0.01, the "|| true"
and the redirect hide the error. The test then fails as a regex mismatch
in the verify stage, not as a setup error.

Should this step be allowed to fail visibly? The same pattern is already
used elsewhere in tc-testing (infra/qdiscs.json), so it may be fine as is.

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

[Severity: Medium]
Will this test fail with any iproute2 release available today?

The kernel side, fq_pie_dump_class_stats(), only sends a binary struct
tc_fq_pie_xstats with type TCA_FQ_PIE_XSTATS_CLASS:

    xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
    ...
    return gnet_stats_copy_app(d, &xstats, sizeof(xstats));

The words "prob", "delay" and "deficit" only appear if tc formats that
struct. According to the commit message, only the pending iproute2 patch
does that.

With a stock tc, re.findall() in run_one_test() in tdc.py finds 0
matches against a matchCount of 1. tdc then marks the test as failed,
even on a kernel where the fq_pie class stats work correctly.

tdc already has a way to skip in this case: the dependsOn key handled in
run_one_test(). Other tests use it to gate on newer iproute2 features.
For example, in actions/mirred.json:

    "dependsOn": "$TC actions add action mirred help 2>&1 | grep -q blockid",

Could 83c0 get a similar dependsOn probe, so that it skips instead of
failing when the tc in use can't print the per-flow stats?

The commit message also describes the test more narrowly than it
behaves:

    verify that 'tc -s class show' reports an fq_pie class.

The pattern needs the per-flow counters too, not just an fq_pie class.
Could the description say that the test depends on the iproute2 change?

> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $DUMMY handle 1: root",
> +            "$IP addr del 10.10.11.10/24 dev $DUMMY || true"
> +        ]
>      }
>  ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922205924.32173-1-hemendranaik%40gmail.com

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

[Hemendra M. Naik]

Thank you for the review. Replies are inline below. No code changes are
planned for this test; we'd like to keep it as posted.

> [Severity: Medium]
> Will this test fail with any iproute2 release available today?
>
> The kernel side, fq_pie_dump_class_stats(), only sends a binary struct
> tc_fq_pie_xstats with type TCA_FQ_PIE_XSTATS_CLASS:
>
>     xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
>     ...
>     return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
>
> The words "prob", "delay" and "deficit" only appear if tc formats that
> struct. According to the commit message, only the pending iproute2 patch
> does that.
>
> With a stock tc, re.findall() in run_one_test() in tdc.py finds 0
> matches against a matchCount of 1. tdc then marks the test as failed,
> even on a kernel where the fq_pie class stats work correctly.
>
> tdc already has a way to skip in this case: the dependsOn key handled in
> run_one_test(). Other tests use it to gate on newer iproute2 features.
> For example, in actions/mirred.json:
>
>     "dependsOn": "$TC actions add action mirred help 2>&1 | grep -q blockid",
>
> Could 83c0 get a similar dependsOn probe, so that it skips instead of
> failing when the tc in use can't print the per-flow stats?
>
> The commit message also describes the test more narrowly than it
> behaves:
>
>     verify that 'tc -s class show' reports an fq_pie class.
>
> The pattern needs the per-flow counters too, not just an fq_pie class.
> Could the description say that the test depends on the iproute2 change?

We'd like to keep the check as it is, without a dependsOn probe. There
are several cases already in this suite that were written against, and
still assume, newer tc wording with no dependsOn guard at all, e.g.:
  - tc-tests/qdiscs/fq.json (commit 3007d8712c9b, "adjust fq test to
    latest iproute2")
  - tc-tests/infra/qdiscs.json (commit 56acc7f51974, "Adapt test's
    output to HFSC's iproute2 printing changes")
  - tc-tests/actions/police.json (commit b7df2e7eaef7, "Adapt tc
    police action tests for Gb rounding changes")
  - tc-tests/actions/bpf.json (commit ac2944abe4d7, "Be compatible
    with newer tc output")

We're doing the same thing here: this test is paired with the
companion iproute2 patch, the same as those cases were paired with the
tc versions they were written for.

The commit message already says the per-flow counters are printed by
the companion iproute2 change; we don't think it needs to be repeated
in the test's own one-line description as well.

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

[Hemendra M. Naik]

Thank you for the review. Our response to the low-severity comment is inline below.

> [Severity: Low]
> This is a robustness point, not a bug. fq_pie_walk() in
> net/sched/sch_fq_pie.c only dumps flows that have a non-empty flowchain:
>
>     if (list_empty(&q->flows[i].flowchain)) {
>         arg->count++;
>         continue;
>     }
>
> That makes this ping the only thing that gets class 2:1 into the output.
>
> In the standard tdc setup this works. nsPlugin runs the command inside
> the namespace, dummy is NOARP, and the 98-byte frame fits under tbf's
> max_size of 100.
>
> If ping is missing, or doesn't accept -i 0.001 / -W 0.01, the "|| true"
> and the redirect hide the error. The test then fails as a regex mismatch
> in the verify stage, not as a setup error.
>
> Should this step be allowed to fail visibly? The same pattern is already
> used elsewhere in tc-testing (infra/qdiscs.json), so it may be fine as is.

Keeping it as is. tc-tests/infra/qdiscs.json (test ca5e, "Check class
delete notification for ffff:") already uses the identical pattern:

    "ping -c1 -W0.01 -I $DUMMY 10.10.10.1 || true",

as a setup step whose only job is to push a packet through the qdisc,
with the same silent-failure trade-off described above. 83c0 follows
existing practice here rather than introducing a new one.


Regards,
Hemendra

[PATCH net-next v8 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 9c0c9787f0c3..969fa30efae7 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