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

[netdev-bot+sashiko@kernel.org]

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