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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@google.com>
> Tested-by: Weilin Wang <weilin.wang@intel.com>
> Acked-by: Namhyung Kim <namhyung@kernel.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.

- 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