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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch now documents tc_pie_xstats.avg_dq_rate
  unconditionally as "bytes/second", but pie_dump_stats()…
- [Medium] The patch now documents tc_pie_xstats.delay as "current delay
  in microseconds" with no range caveat, but pie_dump_stats()…
- [Low] Missing `Fixes:` tag.

--

net/sched: pie: correct tc_pie_xstats field documentation

This updates the comments on struct tc_pie_xstats so the documented units
for the delay and avg_dq_rate fields match what pie_dump_stats() actually
exports.  It is described as documentation-only, touching
include/uapi/linux/pkt_sched.h alone.

A few questions about the newly documented contract below.

> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 9c0c9787f0c32..969fa30efae7f 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: Medium]
Is the microsecond description accurate for all values the kernel can
report here?  pie_dump_stats() in net/sched/sch_pie.c truncates the
nanosecond value to u32 before dividing:

net/sched/sch_pie.c:pie_dump_stats() {
	struct tc_pie_xstats st = {
		.prob		= READ_ONCE(q->vars.prob) << BITS_PER_BYTE,
		.delay		= ((u32)PSCHED_TICKS2NS(READ_ONCE(q->vars.qdelay))) /
				   NSEC_PER_USEC,
	...
}

PSCHED_TICKS2NS() in include/net/pkt_sched.h is:

#define PSCHED_TICKS2NS(x)		((s64)(x) << PSCHED_SHIFT)

so the (u32) cast discards the high bits, and a qdelay above roughly
4.295 s wraps modulo 2^32 ns.  A 5 s sojourn time would be reported as
(5e9 mod 2^32) / 1000 = 705032 us rather than 5000000 us, even though the
__u32 field can hold up to about 4294 s.  Delays in that range look
reachable with a large user-settable TCA_PIE_LIMIT or in ECN mode on a
low-rate link.

The peer producer of the identically documented field does the division on
the full 64-bit value:

net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
	...
		xstats.class_stats.delay =
			div_u64(PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay)),
				NSEC_PER_USEC);
	...
}

The preceding patch in this series, "net/sched: sch_fq_pie: add per-flow
statistics via class ops", calls out this same truncation ("which wrapped
for sojourn times above ~4.295 s") and fixes it only in sch_fq_pie.c.

Would it make sense to convert sch_pie.c to div_u64() in the same series so
the two ABI producers agree with the comment being added here?

>  	__u32 avg_dq_rate;		/* current average dq_rate in
> -					 * bits/pie_time
> +					 * bytes/second
>  					 */

[Severity: Medium]
Does the bytes/second wording hold on 32-bit kernels?  pie_dump_stats()
does the scaling without widening:

net/sched/sch_pie.c:pie_dump_stats() {
	...
	/* unscale and return dq_rate in bytes per sec */
	if (st.dq_rate_estimating)
		st.avg_dq_rate = READ_ONCE(q->vars.avg_dq_rate) *
				 (PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
	...
}

q->vars.avg_dq_rate is u32 (include/net/pie.h) and PSCHED_TICKS_PER_SEC is

#define PSCHED_TICKS_PER_SEC		PSCHED_NS2TICKS(NSEC_PER_SEC)

which is 15625000 of type long.  On ILP32 both operands stay 32-bit, so the
product wraps once avg_dq_rate exceeds 4294967295 / 15625000, about 274,
i.e. a real drain rate above roughly 16.8 MB/s.  With an internal
avg_dq_rate of 512 the expected export is 512 * 15625000 >> 8 = 31250000
bytes/s, which fits __u32, but the value actually dumped is
(512 * 15625000 mod 2^32) >> 8 = 14472784.

sch_fq_pie.c widens before scaling:

net/sched/sch_fq_pie.c:fq_pie_dump_class_stats() {
	...
			xstats.class_stats.avg_dq_rate =
				((u64)READ_ONCE(flow->vars.avg_dq_rate)
				* PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
	...
}

The earlier patch in this series states it "widen[s] avg_dq_rate to u64
before scaling it by PSCHED_TICKS_PER_SEC", but that widening is still
missing in sch_pie.c while this patch asserts the bytes/second contract for
it.  Could the same (u64) cast be added to pie_dump_stats()?

[Severity: Low]
This isn't a bug, but should this carry a Fixes: tag?  The wrong units in
struct tc_pie_xstats date back to the original PIE submission: commit
d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme") added both the header
comments claiming ms and bits/pie_time and the pie_dump_stats() code that
already exported microseconds and "dq_rate in bytes per sec".  A

Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")

line would record where the user-facing units first went wrong.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204225.275251-1-hemendranaik%40gmail.com