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

[Jakub Kicinski <kuba@kernel.org>]

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