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
