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: pie: correct tc_pie_xstats field documentation
This updates the comments in struct tc_pie_xstats so the delay field is
described in microseconds and avg_dq_rate in bytes/second. The change is
described as documentation-only, touching include/uapi/linux/pkt_sched.h.
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 89d825bac4bee..c1f7bde472fb2 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -920,9 +920,9 @@ enum {
>
> struct tc_pie_xstats {
> __u64 prob; /* current probability */
> - __u32 delay; /* current delay in ms */
> + __u32 delay; /* current delay in microseconds */
[Severity: Low]
Should the vendored copy in tools/include/uapi/linux/pkt_sched.h be updated
in the same patch? Its struct tc_pie_xstats still carries exactly the
wording being removed here:
tools/include/uapi/linux/pkt_sched.h:
struct tc_pie_xstats {
__u32 prob; /* current probability */
__u32 delay; /* current delay in ms */
__u32 avg_dq_rate; /* current average dq_rate in bits/pie_time */
__u32 packets_in; /* total number of packets enqueued */
...
};
So after this change the two in-tree copies of the same structure describe
the same field with different units.
That copy also looks stale in other ways: it declares __u32 prob and has no
dq_rate_estimating member, while pie_dump_stats() in net/sched/sch_pie.c
fills a __u64 prob and always sets st.dq_rate_estimating before
gnet_stats_copy_app(), and its TCA_PIE_* enum lacks
TCA_PIE_DQ_RATE_ESTIMATOR. The FQ-PIE additions exported by
fq_pie_dump_class_stats() (TCA_FQ_PIE_XSTATS_QDISC/CLASS and
struct tc_fq_pie_cl_stats) are missing there as well.
The layout drift predates this series, but the unit-comment mismatch between
the two copies is introduced by touching only one of them. Would a resync of
tools/include/uapi/linux/pkt_sched.h, or at least of the tc_pie_xstats
comments, be appropriate so the stale documentation the commit message aims
to eliminate is actually gone from the tree?
[Severity: Medium]
This isn't a bug introduced by this patch, but does the exported value always
match the newly documented microsecond unit? pie_dump_stats() in
net/sched/sch_pie.c computes:
.delay = ((u32)PSCHED_TICKS2NS(READ_ONCE(q->vars.qdelay))) /
NSEC_PER_USEC,
PSCHED_TICKS2NS() is a 64-bit shift, and the (u32) cast is applied to the
nanosecond intermediate before the division, so a qdelay whose nanosecond
value exceeds 2^32-1 (roughly 4.295 s) wraps modulo 2^32 ns and then gets
divided, reporting a small microsecond number for a large delay. The __u32
field itself can hold about 71 minutes worth of microseconds, so the
truncation comes only from where the cast sits.
The same expression is used for the per-flow delay in
fq_pie_dump_class_stats() in net/sched/sch_fq_pie.c:
xstats.class_stats.delay =
((u32)PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay))) /
NSEC_PER_USEC;
This is reachable with a deep queue on a very slow link, where tc -s qdisc
show would print a wrong (small) delay.
Would moving the cast after the division in both sch_pie.c and sch_fq_pie.c
be worth a follow-up, so the code matches the microsecond contract this
comment now states?
> __u32 avg_dq_rate; /* current average dq_rate in
> - * bits/pie_time
> + * bytes/second
> */
> __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
> __u32 packets_in; /* total number of packets enqueued */
