Re: [PATCH v5 09/16] perf intel-tpebs: Refactor tpebs_results list

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

On Mon, Apr 14, 2025 at 10:41:27AM -0700, Ian Rogers wrote:
> evsel names and metric-ids are used for matching but this can be
> problematic, for example, multiple occurrences of the same retirement
> latency event become a single event for the record. Change the name of
> the record events so they are unique and reflect the evsel of the
> retirement latency event that opens them (the retirement latency
> event's evsel address is embedded within them). This allows an evsel
> based close to close the event when the retirement latency event is
> closed. This is important as perf stat has an evlist and the session
> listen to the record events has an evlist, knowing which event should
> remove the tpebs_retire_lat can't be tied to an evlist list as there
> is more than 1, so closing which evlist should cause the tpebs to
> stop? Using the evsel and the last one out doing the tpebs_stop is
> cleaner.
> 
> 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/builtin-stat.c     |   2 -
>  tools/perf/util/evlist.c      |   1 -
>  tools/perf/util/evsel.c       |   2 +-
>  tools/perf/util/intel-tpebs.c | 150 +++++++++++++++++++++-------------
>  tools/perf/util/intel-tpebs.h |   2 +-
>  5 files changed, 93 insertions(+), 64 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 68ea7589c143..80e491bd775b 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -681,8 +681,6 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>  	if (child_pid != -1)
>  		kill(child_pid, SIGTERM);
>  
> -	tpebs_delete();
> -
>  	return COUNTER_FATAL;
>  }
>  
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c1a04141aed0..0a21da4f990f 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -183,7 +183,6 @@ void evlist__delete(struct evlist *evlist)
>  	if (evlist == NULL)
>  		return;
>  
> -	tpebs_delete();
>  	evlist__free_stats(evlist);
>  	evlist__munmap(evlist);
>  	evlist__close(evlist);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 121283f2f382..554252ed1aab 100644
> --- a/tools/perf/util/evsel.c
<SNIP>

>  static struct tpebs_retire_lat *tpebs_retire_lat__find(struct evsel *evsel)
>  {
>  	struct tpebs_retire_lat *t;
> +	uint64_t num;
> +	const char *evsel_name;
>  
> +	/*
> +	 * Evsels will match for evlist with the retirement latency event. The
> +	 * name with "tpebs_event_" prefix will be present on events being read
> +	 * from `perf record`.
> +	 */
> +	if (evsel__is_retire_lat(evsel)) {
> +		list_for_each_entry(t, &tpebs_results, nd) {
> +			if (t->evsel == evsel)
> +				return t;
> +		}
> +		return NULL;
> +	}
> +	evsel_name = strstr(evsel->name, "tpebs_event_");
> +	if (!evsel_name) {
> +		/* Unexpected that the perf record should have other events. */
> +		return NULL;
> +	}
> +	errno = 0;
> +	num = strtoull(evsel_name + 12, NULL, 16);
> +	if (errno) {
> +		pr_err("Bad evsel for tpebs find '%s'\n", evsel->name);
> +		return NULL;
> +	}
>  	list_for_each_entry(t, &tpebs_results, nd) {
> -		if (t->tpebs_name == evsel->name ||
> -		    !strcmp(t->tpebs_name, evsel->name) ||
> -		    (evsel->metric_id && !strcmp(t->tpebs_name, evsel->metric_id)))
> +		if ((uint64_t)t->evsel == num)
>  			return t;

I'm adding the following patch to address building on 32-bit systems:

  20     4.97 debian:experimental-x-mips    : FAIL gcc version 14.2.0 (Debian 14.2.0-13) 
    util/intel-tpebs.c: In function 'tpebs_retire_lat__find':
    util/intel-tpebs.c:377:21: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
      377 |                 if ((uint64_t)t->evsel == num)
          |                     ^
    cc1: all warnings being treated as errors

- Arnaldo

⬢ [acme@toolbx perf-tools-next]$ git diff
diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
index a723687e67f6d7b3..b48f3692c798f924 100644
--- a/tools/perf/util/intel-tpebs.c
+++ b/tools/perf/util/intel-tpebs.c
@@ -242,7 +242,7 @@ static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
 static struct tpebs_retire_lat *tpebs_retire_lat__find(struct evsel *evsel)
 {
        struct tpebs_retire_lat *t;
-       uint64_t num;
+       unsigned long num;
        const char *evsel_name;
 
        /*
@@ -269,7 +269,7 @@ static struct tpebs_retire_lat *tpebs_retire_lat__find(struct evsel *evsel)
                return NULL;
        }
        list_for_each_entry(t, &tpebs_results, nd) {
-               if ((uint64_t)t->evsel == num)
+               if ((unsigned long)t->evsel == num)
                        return t;
        }
        return NULL;