Re: [PATCH v2 14/16] perf stat: Add mean, min, max and last --tpebs-mode options

Ian Rogers
2025-04-07T20:10:52Z

On Mon, Apr 7, 2025 at 12:56 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2025-04-07 1:00 a.m., 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@google.com>
> > ---
> > 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
> > 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 f9584b00fb05..e1bdab4bba6f 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.mean);
>
> t->stats.min?
>
> > + break;
> > + case TPEBS_MODE__MAX:
> > + val = rint(t->stats.mean);
>
> t->stats.max?

Good catch, will fix in v3.

Thanks,
Ian

> Thanks,
> Kan
>
> > + 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);
>