Hi @Jakub,
Thank you for the review. Replies are inline below.
> 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?
Thank you for pointing this out. tc already handles this case safely:
it has copied fq_pie xstats into a zeroed local struct bounded by
RTA_PAYLOAD() since fq_pie was added, so neither an old kernel nor an
old tc will crash or read out of bounds: only there will be zeroed out
extra fields with older tc. You are right, however, that the size growth
and the iproute2 dependency were not mentioned. In v6, the commit message
will state the 36-to-64 byte growth, and reference the companion
iproute2 patch.
> Documentation/netlink/specs/tc.yaml still describes tc-fq-pie-xstats as
> a struct with only the nine original u32 members [...] Should the spec
> be extended in the same patch so ynl-based decoders can see the new
> type discriminator and the per-flow fields?
We would prefer to leave this out of v6. tc does not need it: it parses
TCA_STATS_APP directly, so 'tc -s class show' works without touching
tc.yaml. tc-pie-xstats in tc.yaml has had similarly wrong units since
2014 with no issue. We are also not certain that ynl handles a short
payload safely, so changing the spec now could break it against older
kernels.
Hope it’s fine to have tc.yaml updates in a separate patch series.
> 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 [...] Could the class enumeration simply
> stop at 65535 flows (or the handle be computed differently) so that
> existing configurations using 65536 flows keep working?
This cap was our response to a Low-severity comment from the v4 review,
which flagged the same TC_H_MIN(65536) == 0 display quirk and offered
"document or cap" as options; we chose to cap for v5. We agree that turns
out to be a regression. We would like to drop the cap in v6 and document
the display quirk instead, so flows still accepts [1..65536], the same
as fq_codel today. @Jakub, Could you confirm that is acceptable
before we send it?
> 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
> [...] Would keeping the value 64-bit until after the division be
> preferable?
This is a real bug. We will fix it in v6 with div_u64() applied to
the full 64-bit value before dividing.
> Can this multiplication overflow on 32-bit builds? [...] the product
> stays 32-bit and wraps once avg_dq_rate exceeds about 4396, i.e. rates
> above roughly 16.7 MB/s [...] Would a u64 intermediate (or an explicit
> saturation) be better here?
This is also a real bug. We will fix it in v6 by widening the value
to u64 before the multiply.
> Are these READ_ONCE()s paired with anything on the writer side? [...]
> 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? [...] Should fq_pie
> either do the same [as fq_codel's WRITE_ONCE conversion] or hold
> sch_tree_lock() over the snapshot?
That is a fair point. We would prefer to treat this as out of scope for
this patch and take it up in a subsequent patch series. We are not planning
to reintroduce sch_tree_lock() to the class dump, since it was deliberately
removed there for performance.
> This isn't a bug introduced by this patch, but with cl_ops now present
> the missing .tcf_block becomes more visible [...] would adding the
> trivial .tcf_block that sch_fq_codel provides be cheap enough to make
> that dead code live?
We would like to leave this out of v6, if that is acceptable. This
series is about statistics, not about making fq_pie filter-capable,
and that would be a separate change deserving its own review. We are
happy to send it as a quick follow-up once this lands.
> 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.
This will already be gone in v6, since that block is being rewritten
to address the first comment above.
Thanks,
Hemendra
