Re: [PATCH net-next v5 2/3] selftests: tc-testing: add fq_pie per-flow class stats test

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

Hi @Jakub,

Thank you for the review. Replies are inline below.

> This isn't a bug, the flip is correct: patch 1 of this series does add
> the cap in fq_pie_change() [...] The only remark is about the commit
> message wording [...] Once these land, "patch 1/3" no longer
> identifies anything in git history. Could the commit message name the
> change or the behaviour instead, so a backporter can pair this test
> flip with the kernel change?

This whole case will be gone in v6. We are dropping the flows-65536 cap
in patch 1, so there will be no behavior change left for this test to
flip. Case 83be will stay exactly as it is today.

> Can any of this ping traffic actually reach the fq_pie child? [...]
> 'ping -s 500' produces roughly 542-byte frames, so all 50 packets look
> like they are dropped in TBF and fq_pie never sees a packet [...]
> Would dropping '-s 500' (or raising the TBF burst above the frame
> size) be the intent here?

Confirmed, all of them were being dropped. We will fix this in v6 by
dropping -s 500, which defaults to the standard 56-byte payload used by
the other tc-testing cases with this TBF setup, and by adding -W 0.01 so
the test does not stall for about 10 seconds per run.

> Which iproute2 version prints these tokens? [...] tc routes the blob
> to q_fq_pie's existing print_xstats, which only knows the legacy
> qdisc counters [...] and has no notion of the new type discriminator.
> With any released tc the regex matches 0 times [...] and tdc has no
> iproute2 version gating, so the case fails permanently rather than
> being skipped. [...] Should it also state the required iproute2
> change, since the case cannot pass on a patched kernel with a shipped
> tc either?

You are right, no released tc prints those fields yet. We will fix this
in v6 by relaxing the match to just the class line ("class fq_pie
2:[0-9]+"), which generic tc code prints directly from the kernel's
class dump regardless of tc version. The commit message will also note
that the per-flow fields need the companion iproute2 update.

Thanks,
Hemendra