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

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