Re: [PATCH net-next v6 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.

> 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