Message-ID: <7457b450-9f13-4528-b5e1-2f10f0a7be12@linux.intel.com>
Date: Mon, 7 Apr 2025 15:33:03 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 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>,
 Weilin Wang <weilin.wang@...el.com>, James Clark <james.clark@...aro.org>,
 Xu Yang <xu.yang_2@....com>, John Garry <john.g.garry@...cle.com>,
 Howard Chu <howardchu95@...il.com>, Levi Yun <yeoreum.yun@....com>,
 Dominique Martinet <asmadeus@...ewreck.org>,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 09/16] perf intel-tpebs: Refactor tpebs_results list



On 2025-04-07 1:00 a.m., 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@...gle.com>
> ---
>  tools/perf/builtin-stat.c     |   2 -
>  tools/perf/util/evlist.c      |   1 -
>  tools/perf/util/evsel.c       |   2 +-
>  tools/perf/util/intel-tpebs.c | 152 ++++++++++++++++++++--------------
>  tools/perf/util/intel-tpebs.h |   2 +-
>  5 files changed, 94 insertions(+), 65 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
> +++ b/tools/perf/util/evsel.c
> @@ -2759,7 +2759,7 @@ int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
>  void evsel__close(struct evsel *evsel)
>  {
>  	if (evsel__is_retire_lat(evsel))
> -		tpebs_delete();
> +		evsel__tpebs_close(evsel);
>  	perf_evsel__close(&evsel->core);
>  	perf_evsel__free_id(&evsel->core);
>  }
> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index e42f3ec39a64..e3227646a9cc 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -35,10 +35,10 @@ static struct child_process tpebs_cmd;
>  
>  struct tpebs_retire_lat {
>  	struct list_head nd;
> -	/* Event name */
> -	char *name;
> -	/* Event name with the TPEBS modifier R */
> -	const char *tpebs_name;
> +	/** @evsel: The evsel that opened the retire_lat event. */
> +	struct evsel *evsel;
> +	/** @event: Event passed to perf record. */
> +	char *event;
>  	/* Count of retire_latency values found in sample data */
>  	size_t count;
>  	/* Sum of all the retire_latency values in sample data */
> @@ -49,6 +49,8 @@ struct tpebs_retire_lat {
>  	bool started;
>  };
>  
> +static struct tpebs_retire_lat *tpebs_retire_lat__find(struct evsel *evsel);
> +
>  static int evsel__tpebs_start_perf_record(struct evsel *evsel, int control_fd[], int ack_fd[])
>  {
>  	const char **record_argv;
> @@ -85,7 +87,7 @@ static int evsel__tpebs_start_perf_record(struct evsel *evsel, int control_fd[],
>  
>  	list_for_each_entry(t, &tpebs_results, nd) {
>  		record_argv[i++] = "-e";
> -		record_argv[i++] = t->name;
> +		record_argv[i++] = t->event;
>  	}
>  	record_argv[i++] = NULL;
>  	assert(i == 10 + 2 * tpebs_event_size || i == 8 + 2 * tpebs_event_size);
> @@ -108,27 +110,20 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
>  				struct evsel *evsel,
>  				struct machine *machine __maybe_unused)
>  {
> -	int ret = 0;
> -	const char *evname;
>  	struct tpebs_retire_lat *t;
>  
> -	evname = evsel__name(evsel);
> -
> +	t = tpebs_retire_lat__find(evsel);
> +	if (!t)
> +		return -EINVAL;
>  	/*
>  	 * Need to handle per core results? We are assuming average retire
>  	 * latency value will be used. Save the number of samples and the sum of
>  	 * retire latency value for each event.
>  	 */
> -	list_for_each_entry(t, &tpebs_results, nd) {
> -		if (!strcmp(evname, t->name)) {
> -			t->count += 1;
> -			t->sum += sample->retire_lat;
> -			t->val = (double) t->sum / t->count;
> -			break;
> -		}
> -	}
> -
> -	return ret;
> +	t->count += 1;
> +	t->sum += sample->retire_lat;
> +	t->val = (double) t->sum / t->count;
> +	return 0;
>  }
>  
>  static int process_feature_event(struct perf_session *session,
> @@ -183,50 +178,98 @@ static int tpebs_stop(void)
>  	return ret;
>  }
>  
> -static char *evsel__tpebs_name(struct evsel *evsel)
> +/**
> + * evsel__tpebs_event() - Create string event encoding to pass to `perf record`.
> + */
> +static int evsel__tpebs_event(struct evsel *evsel, char **event)
>  {
>  	char *name, *modifier;
> +	int ret;
>  
>  	name = strdup(evsel->name);
> -	if (!name)
> -		return NULL;
> +	if (!*name)
> +		return -ENOMEM;

if (!name)?

Thanks,
Kan

>  
>  	modifier = strrchr(name, 'R');
>  	if (!modifier) {
> -		pr_err("Tpebs event missing modifier '%s'\n", name);
> -		free(name);
> -		return NULL;
> +		ret = -EINVAL;
> +		goto out;
>  	}
> -
>  	*modifier = 'p';
> -	return name;
> +	modifier = strchr(name, ':');
> +	if (!modifier)
> +		modifier = strrchr(name, '/');
> +	if (!modifier) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*modifier = '\0';
> +	if (asprintf(event, "%s/name=tpebs_event_%p/%s", name, evsel, modifier + 1) > 0)
> +		ret = 0;
> +	else
> +		ret = -ENOMEM;
> +out:
> +	if (ret)
> +		pr_err("Tpebs event modifier broken '%s'\n", evsel->name);
> +	free(name);
> +	return ret;
>  }
>  
>  static struct tpebs_retire_lat *tpebs_retire_lat__new(struct evsel *evsel)
>  {
>  	struct tpebs_retire_lat *result = zalloc(sizeof(*result));
> +	int ret;
>  
>  	if (!result)
>  		return NULL;
>  
> -	result->tpebs_name = evsel->name;
> -	result->name = evsel__tpebs_name(evsel);
> -	if (!result->name) {
> +	ret = evsel__tpebs_event(evsel, &result->event);
> +	if (ret) {
>  		free(result);
>  		return NULL;
>  	}
> +	result->evsel = evsel;
>  	list_add_tail(&result->nd, &tpebs_results);
>  	return result;
>  }
>  
> +static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> +{
> +	zfree(&r->event);
> +	free(r);
> +}
> +
>  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;
>  	}
>  	return NULL;
> @@ -363,8 +406,12 @@ int evsel__tpebs_open(struct evsel *evsel)
>  		close(ack_fd[0]);
>  		close(ack_fd[1]);
>  	}
> -	if (ret)
> -		tpebs_delete();
> +	if (ret) {
> +		struct tpebs_retire_lat *t = tpebs_retire_lat__find(evsel);
> +
> +		list_del_init(&t->nd);
> +		tpebs_retire_lat__delete(t);
> +	}
>  	return ret;
>  }
>  
> @@ -414,34 +461,19 @@ int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
>  	return 0;
>  }
>  
> -static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> -{
> -	zfree(&r->name);
> -	free(r);
> -}
> -
> -
> -/*
> - * tpebs_delete - delete tpebs related data and stop the created thread and
> - * process by calling tpebs_stop().
> - *
> - * This function is called from evlist_delete() and also from builtin-stat
> - * stat_handle_error(). If tpebs_start() is called from places other then perf
> - * stat, need to ensure tpebs_delete() is also called to safely free mem and
> - * close the data read thread and the forked perf record process.
> +/**
> + * evsel__tpebs_close() - delete tpebs related data. If the last event, stop the
> + * created thread and process by calling tpebs_stop().
>   *
> - * This function is also called in evsel__close() to be symmetric with
> - * tpebs_start() being called in evsel__open(). We will update this call site
> - * when move tpebs_start() to evlist level.
> + * This function is called in evsel__close() to be symmetric with
> + * evsel__tpebs_open() being called in evsel__open().
>   */
> -void tpebs_delete(void)
> +void evsel__tpebs_close(struct evsel *evsel)
>  {
> -	struct tpebs_retire_lat *r, *rtmp;
> +	struct tpebs_retire_lat *t = tpebs_retire_lat__find(evsel);
>  
> -	tpebs_stop();
> +	tpebs_retire_lat__delete(t);
>  
> -	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
> -		list_del_init(&r->nd);
> -		tpebs_retire_lat__delete(r);
> -	}
> +	if (list_empty(&tpebs_results))
> +		tpebs_stop();
>  }
> diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> index cc98203719c8..5c671181ec60 100644
> --- a/tools/perf/util/intel-tpebs.h
> +++ b/tools/perf/util/intel-tpebs.h
> @@ -11,7 +11,7 @@ struct evsel;
>  extern bool tpebs_recording;
>  
>  int evsel__tpebs_open(struct evsel *evsel);
> -void tpebs_delete(void);
> +void evsel__tpebs_close(struct evsel *evsel);
>  int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
>  
>  #endif /* __INTEL_TPEBS_H */

Powered by blists - more mailing lists