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

[Hemendra M. Naik]

Thank you for the review. Replies are inline below. No code changes are
planned for this test; we'd like to keep it as posted.

> [Severity: Medium]
> Will this test fail with any iproute2 release available today?
>
> The kernel side, fq_pie_dump_class_stats(), only sends a binary struct
> tc_fq_pie_xstats with type TCA_FQ_PIE_XSTATS_CLASS:
>
>     xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
>     ...
>     return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
>
> The words "prob", "delay" and "deficit" only appear if tc formats that
> struct. According to the commit message, only the pending iproute2 patch
> does that.
>
> With a stock tc, re.findall() in run_one_test() in tdc.py finds 0
> matches against a matchCount of 1. tdc then marks the test as failed,
> even on a kernel where the fq_pie class stats work correctly.
>
> tdc already has a way to skip in this case: the dependsOn key handled in
> run_one_test(). Other tests use it to gate on newer iproute2 features.
> For example, in actions/mirred.json:
>
>     "dependsOn": "$TC actions add action mirred help 2>&1 | grep -q blockid",
>
> Could 83c0 get a similar dependsOn probe, so that it skips instead of
> failing when the tc in use can't print the per-flow stats?
>
> The commit message also describes the test more narrowly than it
> behaves:
>
>     verify that 'tc -s class show' reports an fq_pie class.
>
> The pattern needs the per-flow counters too, not just an fq_pie class.
> Could the description say that the test depends on the iproute2 change?

We'd like to keep the check as it is, without a dependsOn probe. There
are several cases already in this suite that were written against, and
still assume, newer tc wording with no dependsOn guard at all, e.g.:
  - tc-tests/qdiscs/fq.json (commit 3007d8712c9b, "adjust fq test to
    latest iproute2")
  - tc-tests/infra/qdiscs.json (commit 56acc7f51974, "Adapt test's
    output to HFSC's iproute2 printing changes")
  - tc-tests/actions/police.json (commit b7df2e7eaef7, "Adapt tc
    police action tests for Gb rounding changes")
  - tc-tests/actions/bpf.json (commit ac2944abe4d7, "Be compatible
    with newer tc output")

We're doing the same thing here: this test is paired with the
companion iproute2 patch, the same as those cases were paired with the
tc versions they were written for.

The commit message already says the per-flow counters are printed by
the companion iproute2 change; we don't think it needs to be repeated
in the test's own one-line description as well.