Re: [PATCH v1 0/3] Make a "Setup struct perf_event_attr" a shell test

[Namhyung Kim]

On Tue, Oct 08, 2024 at 10:59:59PM -0700, Ian Rogers wrote:
> On Tue, Oct 8, 2024 at 10:38 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Tue, Oct 08, 2024 at 11:55:29AM -0700, Ian Rogers wrote:
> > > On Tue, Oct 1, 2024 at 10:19 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > >
> > > > The path detection for "Setup struct perf_event_attr" test is brittle
> > > > and leads to the test frequently not running. Running shell tests is
> > > > reasonably robust, so make the test a shell test. Move the test files
> > > > to reflect this.
> > >
> > > Ping.
> > >
> > > I think this is worthwhile cleanup for the attributes test. It should
> > > avoid problems like:
> > > https://lore.kernel.org/lkml/ZroNTkdA8XDFaDks@x1/
> >
> > Sorry, it's not clear to me what was the problem.  Can you please say it
> > again briefly?
> 
> If you build perf like:
> make -C tools/perf O=/tmp/perf
> 
> Then run the built perf test for the "Setup struct perf_event_attr" it
> skips (causing the tests to bitrot and fixes to be sent by Veronika):
> ```
> $ sudo /tmp/perf/perf test -vv perf_event_attr
> capget syscall failed (No such file or directory - 2) fall back on root check
> 17: Setup struct perf_event_attr:
> 17: Setup struct perf_event_attr:
> --- start ---
> test child forked, pid 806601
> Using CPUID GenuineIntel-6-8D-1
> ---- end(-2) ----
> 17: Setup struct perf_event_attr                                    : Skip
> ```
> 
> The issue is around the path set up, the test has a few path
> expectations but they are brittle as shown above. While we could
> endeavour to set up the path in C code, it makes sense to migrate the
> test to a shell test due to the tests smaller size, ease of
> environment variable manipulation, existing perf test support for
> better path setup, etc. Ie let's not reinvent the shell test
> infrastructure that handles python tests for the sake of one C test.
> After this change:
> ```
> $ sudo /tmp/perf/perf test attribute
> 76: Perf attribute expectations test                                : Ok
> ```

Ok, thanks for the explanation!
Namhyung

> 
> > >
> > > > Ian Rogers (3):
> > > >   perf test: Add a shell wrapper for "Setup struct perf_event_attr"
> > > >   perf test: Remove C test wrapper for attr.py
> > > >   perf test: Move attr files into shell directory where they are used
> > > >
> > > >  tools/perf/Makefile.perf                      |   5 +-
> > > >  tools/perf/perf.c                             |   2 -
> > > >  tools/perf/tests/Build                        |   1 -
> > > >  tools/perf/tests/attr.c                       | 218 ------------------
> > > >  tools/perf/tests/builtin-test.c               |   1 -
> > > >  tools/perf/tests/shell/attr.sh                |  22 ++
> > > >  tools/perf/tests/{ => shell}/attr/README      |   0
> > > >  tools/perf/tests/{ => shell}/attr/base-record |   0
> > > >  .../tests/{ => shell}/attr/base-record-spe    |   0
> > > >  tools/perf/tests/{ => shell}/attr/base-stat   |   0
> > > >  .../tests/{ => shell}/attr/system-wide-dummy  |   0
> > > >  .../tests/{ => shell}/attr/test-record-C0     |   0
> > > >  .../tests/{ => shell}/attr/test-record-basic  |   0
> > > >  .../{ => shell}/attr/test-record-branch-any   |   0
> > > >  .../attr/test-record-branch-filter-any        |   0
> > > >  .../attr/test-record-branch-filter-any_call   |   0
> > > >  .../attr/test-record-branch-filter-any_ret    |   0
> > > >  .../attr/test-record-branch-filter-hv         |   0
> > > >  .../attr/test-record-branch-filter-ind_call   |   0
> > > >  .../attr/test-record-branch-filter-k          |   0
> > > >  .../attr/test-record-branch-filter-u          |   0
> > > >  .../tests/{ => shell}/attr/test-record-count  |   0
> > > >  .../tests/{ => shell}/attr/test-record-data   |   0
> > > >  .../{ => shell}/attr/test-record-dummy-C0     |   0
> > > >  .../tests/{ => shell}/attr/test-record-freq   |   0
> > > >  .../attr/test-record-graph-default            |   0
> > > >  .../attr/test-record-graph-default-aarch64    |   0
> > > >  .../{ => shell}/attr/test-record-graph-dwarf  |   0
> > > >  .../{ => shell}/attr/test-record-graph-fp     |   0
> > > >  .../attr/test-record-graph-fp-aarch64         |   0
> > > >  .../attr/test-record-group-sampling           |   0
> > > >  .../tests/{ => shell}/attr/test-record-group1 |   0
> > > >  .../tests/{ => shell}/attr/test-record-group2 |   0
> > > >  .../{ => shell}/attr/test-record-no-buffering |   0
> > > >  .../{ => shell}/attr/test-record-no-inherit   |   0
> > > >  .../{ => shell}/attr/test-record-no-samples   |   0
> > > >  .../tests/{ => shell}/attr/test-record-period |   0
> > > >  .../{ => shell}/attr/test-record-pfm-period   |   0
> > > >  .../tests/{ => shell}/attr/test-record-raw    |   0
> > > >  .../{ => shell}/attr/test-record-spe-period   |   0
> > > >  .../attr/test-record-spe-period-term          |   0
> > > >  .../attr/test-record-spe-physical-address     |   0
> > > >  .../attr/test-record-user-regs-no-sve-aarch64 |   0
> > > >  .../test-record-user-regs-old-sve-aarch64     |   0
> > > >  .../attr/test-record-user-regs-sve-aarch64    |   0
> > > >  .../perf/tests/{ => shell}/attr/test-stat-C0  |   0
> > > >  .../tests/{ => shell}/attr/test-stat-basic    |   0
> > > >  .../tests/{ => shell}/attr/test-stat-default  |   0
> > > >  .../{ => shell}/attr/test-stat-detailed-1     |   0
> > > >  .../{ => shell}/attr/test-stat-detailed-2     |   0
> > > >  .../{ => shell}/attr/test-stat-detailed-3     |   0
> > > >  .../tests/{ => shell}/attr/test-stat-group1   |   0
> > > >  .../{ => shell}/attr/test-stat-no-inherit     |   0
> > > >  tools/perf/tests/{ => shell/lib}/attr.py      |   0
> > > >  tools/perf/tests/tests.h                      |   1 -
> > > >  tools/perf/util/evsel.c                       | 122 +++++++++-
> > > >  tools/perf/util/util.h                        |   7 -
> > > >  57 files changed, 142 insertions(+), 237 deletions(-)
> > > >  delete mode 100644 tools/perf/tests/attr.c
> > > >  create mode 100755 tools/perf/tests/shell/attr.sh
> > > >  rename tools/perf/tests/{ => shell}/attr/README (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/base-record (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/base-record-spe (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/base-stat (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/system-wide-dummy (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-C0 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-basic (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-any (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-any (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-any_call (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-any_ret (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-hv (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-ind_call (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-k (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-branch-filter-u (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-count (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-data (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-dummy-C0 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-freq (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-graph-default (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-graph-default-aarch64 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-graph-dwarf (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-graph-fp (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-graph-fp-aarch64 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-group-sampling (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-group1 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-group2 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-no-buffering (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-no-inherit (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-no-samples (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-period (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-pfm-period (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-raw (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-spe-period (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-spe-period-term (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-spe-physical-address (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-user-regs-no-sve-aarch64 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-user-regs-old-sve-aarch64 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-record-user-regs-sve-aarch64 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-C0 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-basic (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-default (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-detailed-1 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-detailed-2 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-detailed-3 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-group1 (100%)
> > > >  rename tools/perf/tests/{ => shell}/attr/test-stat-no-inherit (100%)
> > > >  rename tools/perf/tests/{ => shell/lib}/attr.py (100%)
> > > >
> > > > --
> > > > 2.46.1.824.gd892dcdcdd-goog
> > > >