Re: [PATCH net-next v8 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops

["Hemendra M. Naik" <hemendranaik@gmail.com>]

Thank you for the review. Replies are inline below. No code changes are
planned for this patch.

> [Severity: Medium]
> Will this layout limit how both views can be extended later?
>
> type sits at offset 36 and class_stats at offset 40, so the struct is 64
> bytes. Once this is released:
>
>   - a new qdisc-level counter can only be appended after class_stats
>   - after that, struct tc_fq_pie_cl_stats can never grow again
>
> Every message also carries both views. fq_pie_dump_stats() sends 24 zero
> bytes of class stats, and fq_pie_dump_class_stats() sends nine zeroed qdisc
> counters.
>
> The sibling struct in the same header avoids this with a union:
>
> struct tc_fq_codel_xstats {
> 	__u32	type;
> 	union {
> 		struct tc_fq_codel_qd_stats qdisc_stats;
> 		struct tc_fq_codel_cl_stats class_stats;
> 	};
> };
>
> Keeping the existing offsets means type cannot go first here. Would a
> union after the discriminator still let the two views grow independently?
>
> 	__u32 type;
> 	union {
> 		struct tc_fq_pie_cl_stats class_stats;
> 		/* future qdisc extension */
> 	};
>
> The commit message gives the new size and offsets but does not mention this
> constraint.

We actually posted exactly this in an earlier round:

	__u32 type;
	union {
		struct tc_fq_pie_cl_stats class_stats;
		struct tc_fq_pie_xqd_stats xqdisc_stats;
	};

It was removed after review flagged that the qdisc-side arm
(tc_fq_pie_xqd_stats) had no concrete member yet, making it an empty
struct in a UAPI header.
We flattened class_stats to a direct member specifically to avoid that
portability problem. We're not planning to reintroduce a union now,
since there's still no concrete qdisc-only field to justify the second
arm. Once one exists, wrapping class_stats and it in an anonymous union is
straightforward and won't break anything, since everything from offset
36 onward is still unshipped.


> [Severity: Medium]
> Does this silently truncate the rate?
>
> The product is computed in 64 bits and then assigned to the __u32
> tc_fq_pie_cl_stats.avg_dq_rate, so the high bits are lost.
>
> pie_vars.avg_dq_rate holds bytes per psched tick scaled by 2^8.
> pie_process_dequeue() fills it from (dq_count << PIE_SCALE) / dtime, and
> dtime can be only a few ticks during back-to-back GSO dequeues.
>
> PSCHED_TICKS_PER_SEC is 15625000 and PIE_SCALE is 8. Any internal value
> above about 70369 (roughly 4.29e9 B/s, about 34.4 Gbit/s) wraps. For
> example, 81920 converts to 5,000,000,000 B/s but is exported as 705,032,704.
>
> The commit message says:
>
>   Widen avg_dq_rate to u64 before scaling it by PSCHED_TICKS_PER_SEC.
>
> That moves the overflow to the narrowing assignment instead of avoiding it.
> This is a new uAPI field. Could it be made __u64, or saturated at U32_MAX,
> before it is released?

We're keeping avg_dq_rate as __u32 by design, to stay consistent with
the sibling field in the older tc_pie_xstats. pie_dump_stats() in
sch_pie.c computes the exact same formula into the exact same __u32
width, with the exact same ~34.4 Gbit/s wrap ceiling, and has shipped
that way, unchanged, since the original 2014 PIE merge without this
being a reported problem. We'd rather keep the two structs consistent
with each other than introduce an asymmetry between them for one field.