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

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.