Message-ID: <ZrIl8NCHkIVHQVt2@x1>
Date: Tue, 6 Aug 2024 10:32:32 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Namhyung Kim <namhyung@...nel.org>, weilin.wang@...el.com,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	Perry Taylor <perry.taylor@...el.com>,
	Samantha Alt <samantha.alt@...el.com>,
	Caleb Biggers <caleb.biggers@...el.com>
Subject: Re: [RFC PATCH v18 0/8] TPEBS counting mode support

On Mon, Aug 05, 2024 at 04:33:44PM -0700, Ian Rogers wrote:
> On Mon, Aug 5, 2024 at 12:34 PM Arnaldo Carvalho de Melo
> <acme@...nel.org> wrote:
> >
> > On Mon, Aug 05, 2024 at 08:10:12AM -0700, Ian Rogers wrote:
> > > On Mon, Jul 22, 2024 at 10:38 AM Namhyung Kim <namhyung@...nel.org> wrote:
> > > >
> > > > Hello Weilin,
> > > >
> > > > On Fri, Jul 19, 2024 at 11:21 PM <weilin.wang@...el.com> wrote:
> > > > >
> > > > > From: Weilin Wang <weilin.wang@...el.com>
> > > > >
> > > > > Change in v18:
> > > > >  - Update to exit 2 in TPEBS shell test when not on Intel platform.
> > > > >  - Several updates to use EVLIST_CTL_CMD_ENABLE_TAG, EVLIST_CTL_CMD_ACK_TAG, and
> > > > >  etc.
> > > > >
> > > > > Changes in v17:
> > > > >  - Add a poll on control fifo ack_fd to ensure program returns successfully when
> > > > >  perf record failed for any reason.
> > > > >  - Add a check in the tpebs test to only run on Intel platforms.
> > > > >
> > > > > Changes in v16:
> > > > >  - Update tpebs bash test code and variable name.
> > > > >  - Add a check to ensure only use "-C" option when cpumap is not "-1" when
> > > > >  forking "perf record".
> > > > >
> > > > > Changes in v15:
> > > > >  - Revert changes added for python import test failure in v14 because the code
> > > > >  works correctly with the new python build code.
> > > > >  - Update the command line option to --record-tpebs.
> > > > >
> > > > > Changes in v14:
> > > > >  - Fix the python import test failure. We cannot support PYTHON_PERF because it
> > > > >  will trigger a chain of dependency issues if we add intel-tpebs.c to it. So,
> > > > >  only enable tpebs functions in evsel and evlist when PYTHON_PERF is not
> > > > >  defined.
> > > > >  - Fix shellcheck warning for the tpebs test.
> > > > >
> > > > > Changes in v13:
> > > > >  - Add document for the command line option and fix build error in non-x86_64.
> > > > >  - Update example with non-zero retire_latency value when tpebs recording is
> > > > >  enabled.
> > > > >  - Add tpebs_stop() back to tpebs_set_evsel() because in hybrid platform, when
> > > > >  the forked perf record is not killed, the reader thread does not get any
> > > > >  sampled data from the PIPE. As a result, tpebs_set_evesel() will always return
> > > > >  zero on retire_latency values. This does not happen on my test GNR machine.
> > > > >  Since -I is not supported yet, I think we should add tpebs_stop() to ensure
> > > > >  correctness for now. More investigation is required here when we work on
> > > > >  supporting -I mode.
> > > > >  - Rebase code on top of perf-tools-next.
> > > > >
> > > > > Changes in v12:
> > > > >  - Update MTL metric JSON file to include E-Core TMA3.6 changes.
> > > > >  - Update comments and code for better code quality. Keep tpebs_start() and
> > > > >  tpebs_delete() at evsel level for now and add comments on these functions with
> > > > >  more details about potential future changes. Remove tpebs_stop() call from
> > > > >  tpebs_set_evsel(). Simplify the tpebs_start() and tpebs_stop()/tpebs_delete()
> > > > >  interfaces. Also fixed the bugs on not freed "new" pointer and the incorrect
> > > > >  variable value assignment to val instead of counter->val.
> > > > >
> > > > > Changes in v11:
> > > > >  - Make retire_latency evsels not opened for counting. This works correctly now
> > > > >  with the code Namhyung suggested that adjusting group read size when
> > > > >  retire_latency evsels included in the group.
> > > > >  - Update retire_latency value assignment using rint() to reduce precision loss
> > > > >  while keeping code generic.
> > > > >  - Fix the build error caused by not used variable in the test.
> > > > >
> > > > > Other changes in v10:
> > > > >  - Change perf record fork from perf stat to evsel. All the major operations
> > > > >  like tpebs start, stop, read_evsel should directly work through evsel.
> > > > >  - Make intel-tpebs x86_64 only. This change is cross-compiled to arm64.
> > > > >  - Put tpebs code to intel-tepbs and simplify intel-tpebs APIs to minimum number
> > > > > of functions and variables. Update funtion name and variable names to use
> > > > > consistent prefix. Also improve error handling.
> > > > >  - Integrate code patch from Ian for the :R parser.
> > > > >  - Update MTL metrics to TMA 4.8.
> > > > >
> > > > > V9: https://lore.kernel.org/all/20240521173952.3397644-1-weilin.wang@intel.com/
> > > > >
> > > > > Changes in v9:
> > > > >  - Update the retire_latency result print and metric calculation method. Plugin
> > > > > the value to evsel so that no special code is required.
> > > > >  - Update --control:fifo to use pipe instead of named pipe.
> > > > >  - Add test for TPEBS counting mode.
> > > > >  - Update Document with more details.
> > > > >
> > > > > Changes in v8:
> > > > >  - In this revision, the code is updated to base on Ian's patch on R modifier
> > > > > parser https://lore.kernel.org/lkml/20240428053616.1125891-3-irogers@google.com/
> > > > > After this change, there is no special code required for R modifier in
> > > > > metricgroup.c and metricgroup.h files.
> > > > >
> > > > > Caveat of this change:
> > > > >   Ideally, we will need to add special handling to skip counting events with R
> > > > > modifier in evsel. Currently, this is not implemented so the event with :R will
> > > > > be both counted and sampled. Usually, in a metric formula that uses retire_latency,
> > > > > it would already require to count the event. As a result, we will endup count the
> > > > > same event twice. This should be able to be handled properly when we finalize our
> > > > > design on evsel R modifier support.
> > > > >
> > > > >  - Move TPEBS specific code out from main perf stat code to separate files in
> > > > > util/intel-tpebs.c and util/intel-tpebs.h. [Namhyung]
> > > > >  - Use --control:fifo to ack perf stat from forked perf record instead of sleep(2) [Namhyung]
> > > > >  - Add introductions about TPEBS and R modifier in Documents. [Namhyung]
> > > > >
> > > > >
> > > > > Changes in v7:
> > > > >  - Update code and comments for better code quality [Namhyung]
> > > > >  - Add a separate commit for perf data [Namhyung]
> > > > >  - Update retire latency print function to improve alignment [Namhyung]
> > > > >
> > > > > Changes in v6:
> > > > >  - Update code and add comments for better code quality [Namhyung]
> > > > >  - Remove the added fd var and directly pass the opened fd to data.file.fd [Namhyung]
> > > > >  - Add kill() to stop perf record when perf stat exists early [Namhyung]
> > > > >  - Add command opt check to ensure only start perf record when -a/-C given [Namhyung]
> > > > >  - Squash commits [Namhyung]
> > > > >
> > > > > Changes in v5:
> > > > >  - Update code and add comments for better code quality [Ian]
> > > > >
> > > > > Changes in v4:
> > > > >  - Remove uncessary debug print and update code and comments for better
> > > > > readability and quality [Namhyung]
> > > > >  - Update mtl metric json file with consistent TmaL1 and TopdownL1 metricgroup
> > > > >
> > > > > Changes in v3:
> > > > >  - Remove ':' when event name has '@' [Ian]
> > > > >  - Use 'R' as the modifier instead of "retire_latency" [Ian]
> > > > >
> > > > > Changes in v2:
> > > > >  - Add MTL metric file
> > > > >  - Add more descriptions and example to the patch [Arnaldo]
> > > > >
> > > > > Here is an example of running perf stat to collect a metric that uses
> > > > > retire_latency value of event MEM_INST_RETIRED.STLB_HIT_STORES on a MTL system.
> > > > >
> > > > > In this simple example, there is no MEM_INST_RETIRED.STLB_HIT_STORES sample.
> > > > > Therefore, the MEM_INST_RETIRED.STLB_HIT_STORES:p count and retire_latency value
> > > > > are all 0.
> > > > >
> > > > > ./perf stat -M tma_dtlb_store -a -- sleep 1
> > > > >
> > > > > [ perf record: Woken up 1 times to write data ]
> > > > > [ perf record: Captured and wrote 0.000 MB - ]
> > > > >
> > > > >  Performance counter stats for 'system wide':
> > > > >
> > > > >        181,047,168      cpu_core/TOPDOWN.SLOTS/          #      0.6 %  tma_dtlb_store
> > > > >          3,195,608      cpu_core/topdown-retiring/
> > > > >         40,156,649      cpu_core/topdown-mem-bound/
> > > > >          3,550,925      cpu_core/topdown-bad-spec/
> > > > >        117,571,818      cpu_core/topdown-fe-bound/
> > > > >         57,118,087      cpu_core/topdown-be-bound/
> > > > >             69,179      cpu_core/EXE_ACTIVITY.BOUND_ON_STORES/
> > > > >              4,582      cpu_core/MEM_INST_RETIRED.STLB_HIT_STORES/
> > > > >         30,183,104      cpu_core/CPU_CLK_UNHALTED.DISTRIBUTED/
> > > > >         30,556,790      cpu_core/CPU_CLK_UNHALTED.THREAD/
> > > > >            168,486      cpu_core/DTLB_STORE_MISSES.WALK_ACTIVE/
> > > > >               0.00 MEM_INST_RETIRED.STLB_HIT_STORES:p       0        0
> > > > >
> > > > >        1.003105924 seconds time elapsed
> > > > >
> > > > > v1:
> > > > > TPEBS is one of the features provided by the next generation of Intel PMU.
> > > > > Please refer to Section 8.4.1 of "Intel® Architecture Instruction Set Extensions
> > > > > Programming Reference" [1] for more details about this feature.
> > > > >
> > > > > This set of patches supports TPEBS in counting mode. The code works in the
> > > > > following way: it forks a perf record process from perf stat when retire_latency
> > > > > of one or more events are used in a metric formula. Perf stat would send a
> > > > > SIGTERM signal to perf record before it needs the retire latency value for
> > > > > metric calculation. Perf stat will then process sample data to extract the
> > > > > retire latency data for metric calculations. Currently, the code uses the
> > > > > arithmetic average of retire latency values.
> > > > >
> > > > > [1] https://www.intel.com/content/www/us/en/content-details/812218/intel-architecture-instruction-set-extensions-programming-reference.html?wapkw=future%20features
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Ian Rogers (1):
> > > > >   perf parse-events: Add a retirement latency modifier
> > > > >
> > > > > Weilin Wang (7):
> > > > >   perf data: Allow to use given fd in data->file.fd
> > > > >   perf stat: Fork and launch perf record when perf stat needs to get
> > > > >     retire latency value for a metric.
> > > > >   perf stat: Plugin retire_lat value from sampled data to evsel
> > > > >   perf vendor events intel: Add MTL metric json files
> > > > >   perf stat: Add command line option for enabling tpebs recording
> > > > >   perf Document: Add TPEBS to Documents
> > > > >   perf test: Add test for Intel TPEBS counting mode
> > > >
> > > > Thanks for your persistence!
> > > >
> > > > Reviewed-by: Namhyung Kim <namhyung@...nel.org>
> > >
> > > Ping.
> >
> > I guess Namhyung's reviewed-by should suffice, but since you're pinging
> > and I saw previous comments about this serie, would it be possible to
> > get your Reviewed-by as well?
> 
> I'm happy with most of the changes and they can have my reviewed-by.

Thanks.

> I'm not happy with:
> perf stat: Fork and launch 'perf record' when 'perf stat' needs to get
> retire latency value for a metric.

I don't like it either, but I couldn't find the time to engage in this
discussion to try to offer an alternative patch and since Namhyung had
provided his reviewed-by and you pinged me, I felt compelled to move
forward on this.

> so please leave it off there.

So, what are you proposing, that we process up to some patch?

For now I'm removing the whole series from perf-tools-next.

- Arnaldo
 
> Context: originally the patches aimed to abstract everything inside of
> the evsel, now builtin-stat is manually starting and ending the
> sub-process and has to be aware of TPEBS. There are ugly architectural
> dependencies. My reviewed-by was correct on the earlier version, as
> you could make a tpebs evsel just like any... other, open, close, read
> it, etc. This would be useful say in a python script. This version to
> my eyes is worse as all TPEBS dependencies are made explicit and the
> caller needs to be aware of TPEBS. I understand Weilin was asked to
> make some of the changes, and there was a desire for efficiency, but I
> think things were better in earlier versions.
> 
> Thanks,
> Ian

Powered by blists - more mailing lists