Re: [PATCH net-next v5 3/3] net/sched: pie: correct tc_pie_xstats field documentation

[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: 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 */