From: Namhyung Kim <namhyung@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Kan Liang <kan.liang@linux.intel.com>,
James Clark <james.clark@linaro.org>,
Athira Jajeev <atrajeev@linux.vnet.ibm.com>,
zhaimingbing <zhaimingbing@cmss.chinamobile.com>,
Thomas Richter <tmricht@linux.ibm.com>,
Veronika Molnarova <vmolnaro@redhat.com>,
Leo Yan <leo.yan@linux.dev>, Howard Chu <howardchu95@gmail.com>,
Ze Gao <zegao2021@gmail.com>, Weilin Wang <weilin.wang@intel.com>,
linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
Subject: Re: [PATCH v1 0/3] Make a "Setup struct perf_event_attr" a shell test
Date: Wed, 9 Oct 2024 17:56:25 -0700 [thread overview]
Message-ID: <ZwcmOeI2NDQw96mR@google.com> (raw)
In-Reply-To: <CAP-5=fUY8d4DN+ekpj5B58kvoksrPcWpZU=iZhs+=bFfpakK3Q@mail.gmail.com>
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@kernel.org> 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@google.com> 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
> > > >
next prev parent reply other threads:[~2024-10-10 0:56 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-01 17:19 [PATCH v1 0/3] Make a "Setup struct perf_event_attr" a shell test Ian Rogers
2024-10-01 17:19 ` [PATCH v1 1/3] perf test: Add a shell wrapper for "Setup struct perf_event_attr" Ian Rogers
2024-10-01 17:19 ` [PATCH v1 2/3] perf test: Remove C test wrapper for attr.py Ian Rogers
2024-10-01 17:19 ` [PATCH v1 3/3] perf test: Move attr files into shell directory where they are used Ian Rogers
2024-10-08 18:55 ` [PATCH v1 0/3] Make a "Setup struct perf_event_attr" a shell test Ian Rogers
2024-10-09 5:38 ` Namhyung Kim
2024-10-09 5:59 ` Ian Rogers
2024-10-10 0:56 ` Namhyung Kim [this message]
2024-10-10 15:50 ` Namhyung Kim
2024-10-11 6:48 ` Namhyung Kim
2024-10-13 5:38 ` Athira Rajeev
2024-10-13 6:58 ` Athira Rajeev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZwcmOeI2NDQw96mR@google.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=howardchu95@gmail.com \
--cc=irogers@google.com \
--cc=james.clark@linaro.org \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=leo.yan@linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tmricht@linux.ibm.com \
--cc=vmolnaro@redhat.com \
--cc=weilin.wang@intel.com \
--cc=zegao2021@gmail.com \
--cc=zhaimingbing@cmss.chinamobile.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox