Thank you for the review. Replies are inline below. > Is this paragraph describing a change made by this patch? The diff is > purely additive and net/sched/sch_fq_pie.c had no delay or avg_dq_rate > conversion before it, so the new fq_pie_dump_class_stats() is not > replacing any truncating computation. > > The code that actually contains both of the described defects is > pie_dump_stats() in net/sched/sch_pie.c, which this patch does not touch > and does not mention, and there is no Fixes: tag. > > Could the paragraph either be reworded as a deliberate difference from > the pie qdisc, or the series extended to fix pie_dump_stats() as well?
As you noted, the diff here is purely additive, so this paragraph isn't describing a fix to existing code in this file - there's nothing in this patch's history to carry a Fixes: tag for. It's explaining why the new code uses div_u64() and a widened u64 intermediate: to avoid a known overflow pattern, not to describe a prior bug in this file. The bug you found in pie_dump_stats() is real, but it's independent, pre-existing (net/sched/sch_pie.c, ~2014), and unrelated to whether this new ABI addition is correct. We'd rather not fold a fix for that into a series about adding per-flow statistics - it deserves its own patch, separately from this series. We can make the paragraph read as a comparison rather than a claim about this file - e.g. "which can wrap" instead of "which wrapped" - a wording-only change, no scope change. > Growing struct tc_fq_pie_xstats from 36 to 64 bytes is visible to > userspace in both directions [...] Does the accompanying iproute2 > change parse defensively, e.g. copying min(RTA_PAYLOAD(xstats), > sizeof(st)) bytes into a zeroed local struct rather than bailing out? > [...] Could that reasoning be added to the changelog? > > The alternative would be to leave tc_fq_pie_xstats at 36 bytes entirely > and emit a separate struct [...] Was that option considered and > rejected? Yes to both. iproute2's fq_pie_print_xstats() already does exactly this: it copies into a zeroed local struct and memcpy()s only RTA_PAYLOAD() bytes when the reply is short, so a short/zero-filled read still resolves to TCA_FQ_PIE_XSTATS_QDISC (0) and prints normally. This reasoning is already in the changelog - it's there now, not something we're planning to add. We did consider a separate struct and rejected it. fq_codel can put its discriminator first only because it was designed in from day one; tc_fq_pie_xstats already has nine fixed-offset __u32 counters that predate this patch, so a discriminator can only be appended here, not prepended, without moving those offsets. A new, separate struct would still need its own attribute and its own print callback wired up in tc, splitting one qdisc's stats across two code paths for no added compatibility benefit. Extending the existing struct was thus better. pw-bot: cr
