| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fU0N9dkJhVm4+nsBQqXNA5v4pNmTO8JGAEzRsoE0im6Fg@mail.gmail.com>
Date: Wed, 23 Apr 2025 08:03:21 -0700
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....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>,
Andreas Färber <afaerber@...e.de>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>,
Caleb Biggers <caleb.biggers@...el.com>, Weilin Wang <weilin.wang@...el.com>,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Perry Taylor <perry.taylor@...el.com>, Thomas Falcon <thomas.falcon@...el.com>
Subject: Re: [PATCH v5 14/16] perf stat: Add mean, min, max and last
--tpebs-mode options
On Wed, Apr 23, 2025 at 6:56 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> On Mon, Apr 14, 2025 at 10:41:32AM -0700, Ian Rogers wrote:
> > Add command line configuration option for how retirement latency
> > events are combined. The default "mean" gives the average of
> > retirement latency. "min" or "max" give the smallest or largest
> > retirment latency times respectively. "last" uses the last retirment
> > latency sample's time.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > Tested-by: Weilin Wang <weilin.wang@...el.com>
> > Acked-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> > tools/perf/Documentation/perf-stat.txt | 7 +++++++
> > tools/perf/builtin-stat.c | 27 ++++++++++++++++++++++++++
> > tools/perf/util/intel-tpebs.c | 20 ++++++++++++++++++-
> > tools/perf/util/intel-tpebs.h | 8 ++++++++
> > 4 files changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> > index 2bc063672486..61d091670dee 100644
> > --- a/tools/perf/Documentation/perf-stat.txt
> > +++ b/tools/perf/Documentation/perf-stat.txt
> > @@ -506,6 +506,13 @@ this option is not set. The TPEBS hardware feature starts from Intel Granite
> > Rapids microarchitecture. This option only exists in X86_64 and is meaningful on
> > Intel platforms with TPEBS feature.
> >
> > +--tpebs-mode=[mean|min|max|last]::
> > +Set how retirement latency events have their sample times
> > +combined. The default "mean" gives the average of retirement
> > +latency. "min" or "max" give the smallest or largest retirment latency
> > +times respectively. "last" uses the last retirment latency sample's
> > +time.
> > +
> > --td-level::
> > Print the top-down statistics that equal the input level. It allows
> > users to print the interested top-down metrics level instead of the
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 80e491bd775b..4adf2ae53b11 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -2327,6 +2327,30 @@ static void setup_system_wide(int forks)
> > }
> > }
> >
> > +static int parse_tpebs_mode(const struct option *opt, const char *str,
> > + int unset __maybe_unused)
> > +{
> > + enum tpebs_mode *mode = opt->value;
> > +
> > + if (!strcasecmp("mean", str)) {
> > + *mode = TPEBS_MODE__MEAN;
> > + return 0;
> > + }
> > + if (!strcasecmp("min", str)) {
> > + *mode = TPEBS_MODE__MIN;
> > + return 0;
> > + }
> > + if (!strcasecmp("max", str)) {
> > + *mode = TPEBS_MODE__MAX;
> > + return 0;
> > + }
> > + if (!strcasecmp("last", str)) {
> > + *mode = TPEBS_MODE__LAST;
> > + return 0;
> > + }
> > + return -1;
> > +}
> > +
> > int cmd_stat(int argc, const char **argv)
> > {
> > struct opt_aggr_mode opt_mode = {};
> > @@ -2431,6 +2455,9 @@ int cmd_stat(int argc, const char **argv)
> > #ifdef HAVE_ARCH_X86_64_SUPPORT
> > OPT_BOOLEAN(0, "record-tpebs", &tpebs_recording,
> > "enable recording for tpebs when retire_latency required"),
> > + OPT_CALLBACK(0, "tpebs-mode", &tpebs_mode, "tpebs-mode",
> > + "Mode of TPEBS recording: mean, min or max",
> > + parse_tpebs_mode),
> > #endif
>
> 20 5.60 debian:experimental-x-mips : FAIL gcc version 14.2.0 (Debian 14.2.0-1)
> builtin-stat.c:2330:12: error: 'parse_tpebs_mode' defined but not used [-Werror=unused-function]
> 2330 | static int parse_tpebs_mode(const struct option *opt, const char *str,
> | ^~~~~~~~~~~~~~~~
> --
>
>
> I'm enclosing parse_tpebs_mode() under #ifdef HAVE_ARCH_X86_64_SUPPORT
> to fix this.
Thanks, I agree with the fix. Longer term I think we can remove the
HAVE_ARCH_X86_64_SUPPORT. Now that events carry the retirement latency
information and hardware isn't required, there's no reason why
retirement latency couldn't be added to architectures that aren't
Intel (HAVE_ARCH_X86_64_SUPPORT is already covering AMD which lacks
support).
Thanks,
Ian
> - Arnaldo
>
> > OPT_UINTEGER(0, "td-level", &stat_config.topdown_level,
> > "Set the metrics level for the top-down statistics (0: max level)"),
> > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > index de9fea601964..6b00bd5b0af1 100644
> > --- a/tools/perf/util/intel-tpebs.c
> > +++ b/tools/perf/util/intel-tpebs.c
> > @@ -31,6 +31,7 @@
> > #define PERF_DATA "-"
> >
> > bool tpebs_recording;
> > +enum tpebs_mode tpebs_mode;
> > static LIST_HEAD(tpebs_results);
> > static pthread_t tpebs_reader_thread;
> > static struct child_process tpebs_cmd;
> > @@ -45,6 +46,8 @@ struct tpebs_retire_lat {
> > char *event;
> > /** @stats: Recorded retirement latency stats. */
> > struct stats stats;
> > + /** @last: Last retirement latency read. */
> > + uint64_t last;
> > /* Has the event been sent to perf record? */
> > bool started;
> > };
> > @@ -142,6 +145,7 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
> > * latency value will be used. Save the number of samples and the sum of
> > * retire latency value for each event.
> > */
> > + t->last = sample->retire_lat;
> > update_stats(&t->stats, sample->retire_lat);
> > mutex_unlock(tpebs_mtx_get());
> > return 0;
> > @@ -517,7 +521,21 @@ int evsel__tpebs_read(struct evsel *evsel, int cpu_map_idx, int thread)
> > return ret;
> > mutex_lock(tpebs_mtx_get());
> > }
> > - val = rint(t->stats.mean);
> > + switch (tpebs_mode) {
> > + case TPEBS_MODE__MIN:
> > + val = rint(t->stats.min);
> > + break;
> > + case TPEBS_MODE__MAX:
> > + val = rint(t->stats.max);
> > + break;
> > + case TPEBS_MODE__LAST:
> > + val = t->last;
> > + break;
> > + default:
> > + case TPEBS_MODE__MEAN:
> > + val = rint(t->stats.mean);
> > + break;
> > + }
> > mutex_unlock(tpebs_mtx_get());
> >
> > if (old_count) {
> > diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> > index 218a82866cee..9475e2e6ea74 100644
> > --- a/tools/perf/util/intel-tpebs.h
> > +++ b/tools/perf/util/intel-tpebs.h
> > @@ -8,7 +8,15 @@
> > struct evlist;
> > struct evsel;
> >
> > +enum tpebs_mode {
> > + TPEBS_MODE__MEAN,
> > + TPEBS_MODE__MIN,
> > + TPEBS_MODE__MAX,
> > + TPEBS_MODE__LAST,
> > +};
> > +
> > extern bool tpebs_recording;
> > +extern enum tpebs_mode tpebs_mode;
> >
> > int evsel__tpebs_open(struct evsel *evsel);
> > void evsel__tpebs_close(struct evsel *evsel);
> > --
> > 2.49.0.604.gff1f9ca942-goog
Powered by blists - more mailing lists