From: Weilin Wang <weilin.wang@intel.com>

When retire_latency value is used in a metric formula, evsel would fork a perf
record process with "-e" and "-W" options. Perf record will collect required
retire_latency values in parallel while perf stat is collecting counting values.

At the point of time that perf stat stops counting, evsel would stop perf record
by sending sigterm signal to perf record process. Sampled data will be process
to get retire latency value. Another thread is required to synchronize between
perf stat and perf record when we pass data through pipe.

Retire_latency evsel is not opened for perf stat so that there is no counter
wasted on it. This commit includes code suggested by Namhyung to adjust reading
size for groups that include retire_latency evsels.

Signed-off-by: Weilin Wang <weilin.wang@intel.com>
---
 tools/perf/builtin-stat.c     |   6 +
 tools/perf/util/Build         |   1 +
 tools/perf/util/evsel.c       |  66 +++++-
 tools/perf/util/intel-tpebs.c | 397 ++++++++++++++++++++++++++++++++++
 tools/perf/util/intel-tpebs.h |  48 ++++
 5 files changed, 516 insertions(+), 2 deletions(-)
 create mode 100644 tools/perf/util/intel-tpebs.c
 create mode 100644 tools/perf/util/intel-tpebs.h

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 428e9721b908..b09cb2c6e9c2 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -70,6 +70,7 @@
 #include "util/bpf_counter.h"
 #include "util/iostat.h"
 #include "util/util.h"
+#include "util/intel-tpebs.h"
 #include "asm/bug.h"
 
 #include <linux/time64.h>
@@ -653,6 +654,9 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
 
 	if (child_pid != -1)
 		kill(child_pid, SIGTERM);
+
+	tpebs_stop_delete();
+
 	return COUNTER_FATAL;
 }
 
@@ -985,6 +989,8 @@ static void sig_atexit(void)
 	if (child_pid != -1)
 		kill(child_pid, SIGTERM);
 
+	tpebs_stop();
+
 	sigprocmask(SIG_SETMASK, &oset, NULL);
 
 	if (signr == -1)
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 292170a99ab6..79adf39e0d8f 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -153,6 +153,7 @@ perf-y += clockid.o
 perf-y += list_sort.o
 perf-y += mutex.o
 perf-y += sharded_mutex.o
+perf-$(CONFIG_X86_64) += intel-tpebs.o
 
 perf-$(CONFIG_LIBBPF) += bpf_map.o
 perf-$(CONFIG_PERF_BPF_SKEL) += bpf_counter.o
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a0a8aee7d6b9..bd3627819afe 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1538,6 +1538,60 @@ static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
 	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread, true);
 }
 
+static bool evsel__group_has_tpebs(struct evsel *leader)
+{
+	struct evsel *evsel;
+
+	for_each_group_evsel(evsel, leader) {
+		if (evsel__is_retire_lat(evsel))
+			return true;
+	}
+	return false;
+}
+
+static u64 evsel__group_read_nr_members(struct evsel *leader)
+{
+	u64 nr = leader->core.nr_members;
+	struct evsel *evsel;
+
+	for_each_group_evsel(evsel, leader) {
+		if (evsel__is_retire_lat(evsel))
+			nr--;
+	}
+	return nr;
+}
+
+static u64 evsel__group_read_size(struct evsel *leader)
+{
+	u64 read_format = leader->core.attr.read_format;
+	int entry = sizeof(u64); /* value */
+	int size = 0;
+	int nr = 1;
+
+	if (!evsel__group_has_tpebs(leader))
+		return perf_evsel__read_size(&leader->core);
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+		size += sizeof(u64);
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+		size += sizeof(u64);
+
+	if (read_format & PERF_FORMAT_ID)
+		entry += sizeof(u64);
+
+	if (read_format & PERF_FORMAT_LOST)
+		entry += sizeof(u64);
+
+	if (read_format & PERF_FORMAT_GROUP) {
+		nr = evsel__group_read_nr_members(leader);
+		size += sizeof(u64);
+	}
+
+	size += entry * nr;
+	return size;
+}
+
 static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int thread, u64 *data)
 {
 	u64 read_format = leader->core.attr.read_format;
@@ -1546,7 +1600,7 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
 
 	nr = *data++;
 
-	if (nr != (u64) leader->core.nr_members)
+	if (nr != evsel__group_read_nr_members(leader))
 		return -EINVAL;
 
 	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
@@ -1576,7 +1630,7 @@ static int evsel__read_group(struct evsel *leader, int cpu_map_idx, int thread)
 {
 	struct perf_stat_evsel *ps = leader->stats;
 	u64 read_format = leader->core.attr.read_format;
-	int size = perf_evsel__read_size(&leader->core);
+	int size = evsel__group_read_size(leader);
 	u64 *data = ps->group_data;
 
 	if (!(read_format & PERF_FORMAT_ID))
@@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
 		return 0;
 	}
 
+	if (evsel__is_retire_lat(evsel))
+		return tpebs_start(evsel->evlist, cpus);
+
 	err = __evsel__prepare_open(evsel, cpus, threads);
 	if (err)
 		return err;
@@ -2376,6 +2433,8 @@ 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();
 	perf_evsel__close(&evsel->core);
 	perf_evsel__free_id(&evsel->core);
 }
@@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel, struct evlist *evlist)
 {
 	int cpu_map_idx, thread;
 
+	if (evsel__is_retire_lat(evsel))
+		return 0;
+
 	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd); cpu_map_idx++) {
 		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
 		     thread++) {
diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
new file mode 100644
index 000000000000..37b7a4f92dd9
--- /dev/null
+++ b/tools/perf/util/intel-tpebs.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * intel_tpebs.c: Intel TPEBS support
+ */
+
+
+#include <sys/param.h>
+#include <subcmd/run-command.h>
+#include <thread.h>
+#include "intel-tpebs.h"
+#include <linux/list.h>
+#include <linux/zalloc.h>
+#include <linux/err.h>
+#include "sample.h"
+#include "debug.h"
+#include "evlist.h"
+#include "evsel.h"
+#include "session.h"
+#include "tool.h"
+#include "cpumap.h"
+#include "metricgroup.h"
+#include <sys/stat.h>
+#include <sys/file.h>
+#include <poll.h>
+#include <math.h>
+
+#define PERF_DATA		"-"
+
+bool tpebs_recording;
+static pid_t tpebs_pid = -1;
+static size_t tpebs_event_size;
+static pthread_t tpebs_reader_thread;
+static struct child_process *tpebs_cmd;
+static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);
+
+struct tpebs_retire_lat {
+	struct list_head nd;
+	/* Event name */
+	const char *name;
+	/* Event name with the TPEBS modifier R */
+	const char *tpebs_name;
+	/* Count of retire_latency values found in sample data */
+	size_t count;
+	/* Sum of all the retire_latency values in sample data */
+	int sum;
+	/* Average of retire_latency, val = sum / count */
+	double val;
+};
+
+static int get_perf_record_args(const char **record_argv, char buf[],
+				const char *cpumap_buf)
+{
+	struct tpebs_retire_lat *e;
+	int i = 0;
+
+	pr_debug("Prepare perf record for retire_latency\n");
+
+	record_argv[i++] = "perf";
+	record_argv[i++] = "record";
+	record_argv[i++] = "-W";
+	record_argv[i++] = "--synth=no";
+	record_argv[i++] = buf;
+
+	if (cpumap_buf) {
+		record_argv[i++] = "-C";
+		record_argv[i++] = cpumap_buf;
+	}
+
+	record_argv[i++] = "-a";
+
+	if (!cpumap_buf) {
+		pr_err("Require cpumap list to run sampling.\n");
+		return -ECANCELED;
+	}
+
+	list_for_each_entry(e, &tpebs_results, nd) {
+		record_argv[i++] = "-e";
+		record_argv[i++] = e->name;
+	}
+
+	record_argv[i++] = "-o";
+	record_argv[i++] = PERF_DATA;
+
+	return 0;
+}
+
+static int prepare_run_command(const char **argv)
+{
+	tpebs_cmd = zalloc(sizeof(struct child_process));
+	if (!tpebs_cmd)
+		return -ENOMEM;
+	tpebs_cmd->argv = argv;
+	tpebs_cmd->out = -1;
+	return 0;
+}
+
+static int start_perf_record(int control_fd[], int ack_fd[],
+				const char *cpumap_buf)
+{
+	const char **record_argv;
+	int ret;
+	char buf[32];
+
+	scnprintf(buf, sizeof(buf), "--control=fd:%d,%d", control_fd[0], ack_fd[1]);
+
+	record_argv = calloc(12 + 2 * tpebs_event_size, sizeof(char *));
+	if (!record_argv)
+		return -ENOMEM;
+
+	ret = get_perf_record_args(record_argv, buf, cpumap_buf);
+	if (ret)
+		goto out;
+
+	ret = prepare_run_command(record_argv);
+	if (ret)
+		goto out;
+	ret = start_command(tpebs_cmd);
+out:
+	free(record_argv);
+	return ret;
+}
+
+static int process_sample_event(struct perf_tool *tool __maybe_unused,
+				union perf_event *event __maybe_unused,
+				struct perf_sample *sample,
+				struct evsel *evsel,
+				struct machine *machine __maybe_unused)
+{
+	int ret = 0;
+	const char *evname;
+	struct tpebs_retire_lat *t;
+
+	evname = evsel__name(evsel);
+
+	/*
+	 * 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;
+}
+
+static int process_feature_event(struct perf_session *session,
+				 union perf_event *event)
+{
+	if (event->feat.feat_id < HEADER_LAST_FEATURE)
+		return perf_event__process_feature(session, event);
+	return 0;
+}
+
+static void *__sample_reader(void *arg)
+{
+	struct child_process *child = arg;
+	struct perf_session *session;
+	struct perf_data data = {
+		.mode = PERF_DATA_MODE_READ,
+		.path = PERF_DATA,
+		.file.fd = child->out,
+	};
+	struct perf_tool tool = {
+		.sample = process_sample_event,
+		.feature = process_feature_event,
+		.attr = perf_event__process_attr,
+	};
+
+	session = perf_session__new(&data, &tool);
+	if (IS_ERR(session))
+		return NULL;
+	perf_session__process_events(session);
+	perf_session__delete(session);
+
+	return NULL;
+}
+
+int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus)
+{
+	int ret = 0;
+	struct evsel *evsel;
+	char cpumap_buf[50];
+
+	/*
+	 * We should only run tpebs_start when tpebs_recording is enabled.
+	 * And we should only run it once with all the required events.
+	 */
+	if (tpebs_pid != -1 || !tpebs_recording)
+		return 0;
+
+	cpu_map__snprint(cpus, cpumap_buf, sizeof(cpumap_buf));
+	pr_debug("cpu map: %s\n", cpumap_buf);
+
+	/*
+	 * Prepare perf record for sampling event retire_latency before fork and
+	 * prepare workload
+	 */
+	evlist__for_each_entry(evsel_list, evsel) {
+		struct tpebs_retire_lat *new = zalloc(sizeof(*new));
+		char *name;
+		int i;
+
+		if (!evsel->retire_lat)
+			continue;
+
+		pr_debug("perf stat retire latency of event %s required\n", evsel->name);
+		if (!new) {
+			ret = -1;
+			goto err;
+		}
+		for (i = strlen(evsel->name) - 1; i > 0; i--) {
+			if (evsel->name[i] == 'R')
+				break;
+		}
+		if (i <= 0 || evsel->name[i] != 'R') {
+			ret = -1;
+			goto err;
+		}
+
+		name = strdup(evsel->name);
+		if (!name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+		name[i] = 'p';
+		new->name = name;
+		new->tpebs_name = strdup(evsel->name);
+		if (!new->tpebs_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+		list_add_tail(&new->nd, &tpebs_results);
+		tpebs_event_size += 1;
+	}
+
+	if (tpebs_event_size > 0) {
+		int control_fd[2], ack_fd[2], len;
+		char ack_buf[8];
+
+		/*Create control and ack fd for --control*/
+		if (pipe(control_fd) < 0) {
+			pr_err("Failed to create control fifo");
+			ret = -1;
+			goto out;
+		}
+		if (pipe(ack_fd) < 0) {
+			pr_err("Failed to create control fifo");
+			ret = -1;
+			goto out;
+		}
+
+		ret = start_perf_record(control_fd, ack_fd, cpumap_buf);
+		if (ret)
+			goto out;
+		tpebs_pid = tpebs_cmd->pid;
+		if (pthread_create(&tpebs_reader_thread, NULL, __sample_reader, tpebs_cmd)) {
+			kill(tpebs_cmd->pid, SIGTERM);
+			close(tpebs_cmd->out);
+			pr_err("Could not create thread to process sample data.\n");
+			ret = -1;
+			goto out;
+		}
+		/* Wait for perf record initialization.*/
+		len = strlen("enable");
+		ret = write(control_fd[1], "enable", len);
+		if (ret != len) {
+			pr_err("perf record control write control message failed\n");
+			goto out;
+		}
+
+		ret = read(ack_fd[0], ack_buf, sizeof(ack_buf));
+		if (ret > 0)
+			ret = strcmp(ack_buf, "ack\n");
+		else {
+			pr_err("perf record control ack failed\n");
+			goto out;
+		}
+		pr_debug("Received ack from perf record\n");
+out:
+		close(control_fd[0]);
+		close(control_fd[1]);
+		close(ack_fd[0]);
+		close(ack_fd[1]);
+	}
+err:
+	if (ret)
+		tpebs_delete();
+	return ret;
+}
+
+int tpebs_stop(void)
+{
+	int ret = 0;
+
+	/* Like tpebs_start, we should only run tpebs_end once. */
+	if (tpebs_pid != -1) {
+		kill(tpebs_cmd->pid, SIGTERM);
+		tpebs_pid = -1;
+		pthread_join(tpebs_reader_thread, NULL);
+		close(tpebs_cmd->out);
+		ret = finish_command(tpebs_cmd);
+		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
+			ret = 0;
+	}
+	return ret;
+}
+
+int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
+{
+	struct perf_counts_values *count;
+	struct tpebs_retire_lat *t;
+	bool found = false;
+	__u64 val;
+	int ret;
+
+	/* Non reitre_latency evsel should never enter this function. */
+	if (!evsel__is_retire_lat(evsel))
+		return -1;
+
+	ret = tpebs_stop();
+	if (ret)
+		return ret;
+
+	count = perf_counts(evsel->counts, cpu_map_idx, thread);
+
+	list_for_each_entry(t, &tpebs_results, nd) {
+		if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t->tpebs_name, evsel->metric_id)) {
+			found = true;
+			break;
+		}
+	}
+
+	/* Set ena and run to non-zero */
+	count->ena = count->run = 1;
+	count->lost = 0;
+
+	if (!found) {
+		/*
+		 * Set default value or 0 when retire_latency for this event is
+		 * not found from sampling data (enable_tpebs_recording not set
+		 * or 0 sample recorded).
+		 */
+		val = 0;
+		return 0;
+	}
+
+	/*
+	 * Only set retire_latency value to the first CPU and thread.
+	 */
+	if (cpu_map_idx == 0 && thread == 0)
+		val = rint(t->val);
+	else
+		val = 0;
+
+	count->val = val;
+	return 0;
+}
+
+static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
+{
+	zfree(&r->name);
+	zfree(&r->tpebs_name);
+	free(r);
+}
+
+void tpebs_delete(void)
+{
+	struct tpebs_retire_lat *r, *rtmp;
+
+	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
+		list_del_init(&r->nd);
+		tpebs_retire_lat__delete(r);
+	}
+
+	if (tpebs_cmd) {
+		free(tpebs_cmd);
+		tpebs_cmd = NULL;
+	}
+}
+
+int tpebs_stop_delete(void)
+{
+	int ret;
+
+	if (tpebs_pid == -1)
+		return 0;
+
+	ret = tpebs_stop();
+	tpebs_delete();
+	return ret;
+}
diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
new file mode 100644
index 000000000000..73c1e5219522
--- /dev/null
+++ b/tools/perf/util/intel-tpebs.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * intel_tpebs.h: Intel TEPBS support
+ */
+#ifndef INCLUDE__PERF_INTEL_TPEBS_H__
+#define INCLUDE__PERF_INTEL_TPEBS_H__
+
+#include "stat.h"
+#include "evsel.h"
+
+#ifdef HAVE_ARCH_X86_64_SUPPORT
+
+extern bool tpebs_recording;
+int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus);
+int tpebs_stop(void);
+void tpebs_delete(void);
+int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
+int tpebs_stop_delete(void);
+
+#else
+
+static inline int tpebs_start(struct evlist *evsel_list __maybe_unused,
+				struct perf_cpu_map *cpus  __maybe_unused)
+{
+	return 0;
+}
+
+static inline int tpebs_stop(void)
+{
+	return 0;
+}
+
+static inline void tpebs_delete(void) {};
+
+static inline int tpebs_set_evsel(struct evsel *evsel  __maybe_unused,
+				int cpu_map_idx  __maybe_unused,
+				int thread  __maybe_unused)
+{
+	return 0;
+}
+
+static inline int tpebs_stop_delete(void)
+{
+	return 0;
+}
+
+#endif
+#endif
-- 
2.43.0

Hello,

On Wed, Jun 05, 2024 at 01:21:44AM -0400, weilin.wang@intel.com wrote:
> From: Weilin Wang <weilin.wang@intel.com>
> 
> When retire_latency value is used in a metric formula, evsel would fork a perf
> record process with "-e" and "-W" options. Perf record will collect required
> retire_latency values in parallel while perf stat is collecting counting values.
> 
> At the point of time that perf stat stops counting, evsel would stop perf record
> by sending sigterm signal to perf record process. Sampled data will be process
> to get retire latency value. Another thread is required to synchronize between
> perf stat and perf record when we pass data through pipe.
> 
> Retire_latency evsel is not opened for perf stat so that there is no counter
> wasted on it. This commit includes code suggested by Namhyung to adjust reading
> size for groups that include retire_latency evsels.
> 
> Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> ---
>  tools/perf/builtin-stat.c     |   6 +
>  tools/perf/util/Build         |   1 +
>  tools/perf/util/evsel.c       |  66 +++++-
>  tools/perf/util/intel-tpebs.c | 397 ++++++++++++++++++++++++++++++++++
>  tools/perf/util/intel-tpebs.h |  48 ++++
>  5 files changed, 516 insertions(+), 2 deletions(-)
>  create mode 100644 tools/perf/util/intel-tpebs.c
>  create mode 100644 tools/perf/util/intel-tpebs.h
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 428e9721b908..b09cb2c6e9c2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -70,6 +70,7 @@
>  #include "util/bpf_counter.h"
>  #include "util/iostat.h"
>  #include "util/util.h"
> +#include "util/intel-tpebs.h"
>  #include "asm/bug.h"
>  
>  #include <linux/time64.h>
> @@ -653,6 +654,9 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>  
>  	if (child_pid != -1)
>  		kill(child_pid, SIGTERM);
> +
> +	tpebs_stop_delete();
> +
>  	return COUNTER_FATAL;
>  }
>  
> @@ -985,6 +989,8 @@ static void sig_atexit(void)
>  	if (child_pid != -1)
>  		kill(child_pid, SIGTERM);
>  
> +	tpebs_stop();
> +
>  	sigprocmask(SIG_SETMASK, &oset, NULL);
>  
>  	if (signr == -1)
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 292170a99ab6..79adf39e0d8f 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -153,6 +153,7 @@ perf-y += clockid.o
>  perf-y += list_sort.o
>  perf-y += mutex.o
>  perf-y += sharded_mutex.o
> +perf-$(CONFIG_X86_64) += intel-tpebs.o
>  
>  perf-$(CONFIG_LIBBPF) += bpf_map.o
>  perf-$(CONFIG_PERF_BPF_SKEL) += bpf_counter.o
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a0a8aee7d6b9..bd3627819afe 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1538,6 +1538,60 @@ static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
>  	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread, true);
>  }
>  
> +static bool evsel__group_has_tpebs(struct evsel *leader)
> +{
> +	struct evsel *evsel;
> +
> +	for_each_group_evsel(evsel, leader) {
> +		if (evsel__is_retire_lat(evsel))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static u64 evsel__group_read_nr_members(struct evsel *leader)
> +{
> +	u64 nr = leader->core.nr_members;
> +	struct evsel *evsel;
> +
> +	for_each_group_evsel(evsel, leader) {
> +		if (evsel__is_retire_lat(evsel))
> +			nr--;
> +	}
> +	return nr;
> +}
> +
> +static u64 evsel__group_read_size(struct evsel *leader)
> +{
> +	u64 read_format = leader->core.attr.read_format;
> +	int entry = sizeof(u64); /* value */
> +	int size = 0;
> +	int nr = 1;
> +
> +	if (!evsel__group_has_tpebs(leader))
> +		return perf_evsel__read_size(&leader->core);
> +
> +	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> +		size += sizeof(u64);
> +
> +	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> +		size += sizeof(u64);
> +
> +	if (read_format & PERF_FORMAT_ID)
> +		entry += sizeof(u64);
> +
> +	if (read_format & PERF_FORMAT_LOST)
> +		entry += sizeof(u64);
> +
> +	if (read_format & PERF_FORMAT_GROUP) {
> +		nr = evsel__group_read_nr_members(leader);
> +		size += sizeof(u64);
> +	}
> +
> +	size += entry * nr;
> +	return size;
> +}
> +
>  static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int thread, u64 *data)
>  {
>  	u64 read_format = leader->core.attr.read_format;
> @@ -1546,7 +1600,7 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
>  
>  	nr = *data++;
>  
> -	if (nr != (u64) leader->core.nr_members)
> +	if (nr != evsel__group_read_nr_members(leader))
>  		return -EINVAL;
>  
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> @@ -1576,7 +1630,7 @@ static int evsel__read_group(struct evsel *leader, int cpu_map_idx, int thread)
>  {
>  	struct perf_stat_evsel *ps = leader->stats;
>  	u64 read_format = leader->core.attr.read_format;
> -	int size = perf_evsel__read_size(&leader->core);
> +	int size = evsel__group_read_size(leader);
>  	u64 *data = ps->group_data;
>  
>  	if (!(read_format & PERF_FORMAT_ID))
> @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>  		return 0;
>  	}
>  
> +	if (evsel__is_retire_lat(evsel))
> +		return tpebs_start(evsel->evlist, cpus);

As it works with evlist, I think it's better to put this code there.
But it seems perf stat doesn't call the evlist API for open, then we
can add this to somewhere in __run_perf_stat() directly.

> +
>  	err = __evsel__prepare_open(evsel, cpus, threads);
>  	if (err)
>  		return err;
> @@ -2376,6 +2433,8 @@ 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();

Ditto.

>  	perf_evsel__close(&evsel->core);
>  	perf_evsel__free_id(&evsel->core);
>  }
> @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel, struct evlist *evlist)
>  {
>  	int cpu_map_idx, thread;
>  
> +	if (evsel__is_retire_lat(evsel))
> +		return 0;
> +
>  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd); cpu_map_idx++) {
>  		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
>  		     thread++) {
> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> new file mode 100644
> index 000000000000..37b7a4f92dd9
> --- /dev/null
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * intel_tpebs.c: Intel TPEBS support
> + */
> +
> +
> +#include <sys/param.h>
> +#include <subcmd/run-command.h>
> +#include <thread.h>
> +#include "intel-tpebs.h"
> +#include <linux/list.h>
> +#include <linux/zalloc.h>
> +#include <linux/err.h>
> +#include "sample.h"
> +#include "debug.h"
> +#include "evlist.h"
> +#include "evsel.h"
> +#include "session.h"
> +#include "tool.h"
> +#include "cpumap.h"
> +#include "metricgroup.h"
> +#include <sys/stat.h>
> +#include <sys/file.h>
> +#include <poll.h>
> +#include <math.h>
> +
> +#define PERF_DATA		"-"
> +
> +bool tpebs_recording;
> +static pid_t tpebs_pid = -1;
> +static size_t tpebs_event_size;
> +static pthread_t tpebs_reader_thread;
> +static struct child_process *tpebs_cmd;
> +static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);

It can be 'static LIST_HEAD(tpebs_results);'

> +
> +struct tpebs_retire_lat {
> +	struct list_head nd;
> +	/* Event name */
> +	const char *name;
> +	/* Event name with the TPEBS modifier R */
> +	const char *tpebs_name;
> +	/* Count of retire_latency values found in sample data */
> +	size_t count;
> +	/* Sum of all the retire_latency values in sample data */
> +	int sum;
> +	/* Average of retire_latency, val = sum / count */
> +	double val;
> +};
> +
> +static int get_perf_record_args(const char **record_argv, char buf[],
> +				const char *cpumap_buf)
> +{
> +	struct tpebs_retire_lat *e;
> +	int i = 0;
> +
> +	pr_debug("Prepare perf record for retire_latency\n");
> +
> +	record_argv[i++] = "perf";
> +	record_argv[i++] = "record";
> +	record_argv[i++] = "-W";
> +	record_argv[i++] = "--synth=no";
> +	record_argv[i++] = buf;
> +
> +	if (cpumap_buf) {
> +		record_argv[i++] = "-C";
> +		record_argv[i++] = cpumap_buf;
> +	}
> +
> +	record_argv[i++] = "-a";
> +
> +	if (!cpumap_buf) {
> +		pr_err("Require cpumap list to run sampling.\n");
> +		return -ECANCELED;
> +	}

Hmm.. I thought you supported system wide collection, not sure if it has
a cpumap.  Anyway this check makes the earlier one meaningless - you
need the cpumap always, right?

> +
> +	list_for_each_entry(e, &tpebs_results, nd) {
> +		record_argv[i++] = "-e";
> +		record_argv[i++] = e->name;
> +	}
> +
> +	record_argv[i++] = "-o";
> +	record_argv[i++] = PERF_DATA;
> +
> +	return 0;
> +}
> +
> +static int prepare_run_command(const char **argv)
> +{
> +	tpebs_cmd = zalloc(sizeof(struct child_process));
> +	if (!tpebs_cmd)
> +		return -ENOMEM;
> +	tpebs_cmd->argv = argv;
> +	tpebs_cmd->out = -1;
> +	return 0;
> +}
> +
> +static int start_perf_record(int control_fd[], int ack_fd[],
> +				const char *cpumap_buf)
> +{
> +	const char **record_argv;
> +	int ret;
> +	char buf[32];
> +
> +	scnprintf(buf, sizeof(buf), "--control=fd:%d,%d", control_fd[0], ack_fd[1]);
> +
> +	record_argv = calloc(12 + 2 * tpebs_event_size, sizeof(char *));
> +	if (!record_argv)
> +		return -ENOMEM;
> +
> +	ret = get_perf_record_args(record_argv, buf, cpumap_buf);
> +	if (ret)
> +		goto out;
> +
> +	ret = prepare_run_command(record_argv);
> +	if (ret)
> +		goto out;
> +	ret = start_command(tpebs_cmd);
> +out:
> +	free(record_argv);
> +	return ret;
> +}
> +
> +static int process_sample_event(struct perf_tool *tool __maybe_unused,
> +				union perf_event *event __maybe_unused,
> +				struct perf_sample *sample,
> +				struct evsel *evsel,
> +				struct machine *machine __maybe_unused)
> +{
> +	int ret = 0;
> +	const char *evname;
> +	struct tpebs_retire_lat *t;
> +
> +	evname = evsel__name(evsel);
> +
> +	/*
> +	 * 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;
> +}
> +
> +static int process_feature_event(struct perf_session *session,
> +				 union perf_event *event)
> +{
> +	if (event->feat.feat_id < HEADER_LAST_FEATURE)
> +		return perf_event__process_feature(session, event);
> +	return 0;
> +}
> +
> +static void *__sample_reader(void *arg)
> +{
> +	struct child_process *child = arg;
> +	struct perf_session *session;
> +	struct perf_data data = {
> +		.mode = PERF_DATA_MODE_READ,
> +		.path = PERF_DATA,
> +		.file.fd = child->out,
> +	};
> +	struct perf_tool tool = {
> +		.sample = process_sample_event,
> +		.feature = process_feature_event,
> +		.attr = perf_event__process_attr,
> +	};
> +
> +	session = perf_session__new(&data, &tool);
> +	if (IS_ERR(session))
> +		return NULL;
> +	perf_session__process_events(session);
> +	perf_session__delete(session);
> +
> +	return NULL;
> +}
> +
> +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus)
> +{
> +	int ret = 0;
> +	struct evsel *evsel;
> +	char cpumap_buf[50];
> +
> +	/*
> +	 * We should only run tpebs_start when tpebs_recording is enabled.
> +	 * And we should only run it once with all the required events.
> +	 */
> +	if (tpebs_pid != -1 || !tpebs_recording)
> +		return 0;
> +
> +	cpu_map__snprint(cpus, cpumap_buf, sizeof(cpumap_buf));
> +	pr_debug("cpu map: %s\n", cpumap_buf);

Can you please remove unnecessary debug prints?  If you really want it,
then make it more meaningful like with more context.

> +
> +	/*
> +	 * Prepare perf record for sampling event retire_latency before fork and
> +	 * prepare workload
> +	 */
> +	evlist__for_each_entry(evsel_list, evsel) {
> +		struct tpebs_retire_lat *new = zalloc(sizeof(*new));
> +		char *name;
> +		int i;
> +
> +		if (!evsel->retire_lat)
> +			continue;

Please move the allocation after this.

> +
> +		pr_debug("perf stat retire latency of event %s required\n", evsel->name);
> +		if (!new) {
> +			ret = -1;
> +			goto err;
> +		}
> +		for (i = strlen(evsel->name) - 1; i > 0; i--) {
> +			if (evsel->name[i] == 'R')
> +				break;

I think you also need to check ':' or '/'..

> +		}
> +		if (i <= 0 || evsel->name[i] != 'R') {
> +			ret = -1;
> +			goto err;
> +		}
> +
> +		name = strdup(evsel->name);
> +		if (!name) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +		name[i] = 'p';
> +		new->name = name;
> +		new->tpebs_name = strdup(evsel->name);
> +		if (!new->tpebs_name) {
> +			ret = -ENOMEM;
> +			goto err;

This error handling (including above) will leak 'new' because it's not
linked to the list yet.

> +		}
> +		list_add_tail(&new->nd, &tpebs_results);
> +		tpebs_event_size += 1;
> +	}
> +
> +	if (tpebs_event_size > 0) {
> +		int control_fd[2], ack_fd[2], len;
> +		char ack_buf[8];
> +
> +		/*Create control and ack fd for --control*/
> +		if (pipe(control_fd) < 0) {
> +			pr_err("Failed to create control fifo");
> +			ret = -1;
> +			goto out;
> +		}
> +		if (pipe(ack_fd) < 0) {
> +			pr_err("Failed to create control fifo");
> +			ret = -1;
> +			goto out;
> +		}
> +
> +		ret = start_perf_record(control_fd, ack_fd, cpumap_buf);
> +		if (ret)
> +			goto out;
> +		tpebs_pid = tpebs_cmd->pid;
> +		if (pthread_create(&tpebs_reader_thread, NULL, __sample_reader, tpebs_cmd)) {
> +			kill(tpebs_cmd->pid, SIGTERM);
> +			close(tpebs_cmd->out);
> +			pr_err("Could not create thread to process sample data.\n");
> +			ret = -1;
> +			goto out;
> +		}
> +		/* Wait for perf record initialization.*/
> +		len = strlen("enable");
> +		ret = write(control_fd[1], "enable", len);
> +		if (ret != len) {
> +			pr_err("perf record control write control message failed\n");
> +			goto out;
> +		}
> +
> +		ret = read(ack_fd[0], ack_buf, sizeof(ack_buf));
> +		if (ret > 0)
> +			ret = strcmp(ack_buf, "ack\n");
> +		else {
> +			pr_err("perf record control ack failed\n");
> +			goto out;
> +		}
> +		pr_debug("Received ack from perf record\n");
> +out:
> +		close(control_fd[0]);
> +		close(control_fd[1]);
> +		close(ack_fd[0]);
> +		close(ack_fd[1]);
> +	}
> +err:
> +	if (ret)
> +		tpebs_delete();
> +	return ret;
> +}
> +
> +int tpebs_stop(void)
> +{
> +	int ret = 0;
> +
> +	/* Like tpebs_start, we should only run tpebs_end once. */
> +	if (tpebs_pid != -1) {
> +		kill(tpebs_cmd->pid, SIGTERM);
> +		tpebs_pid = -1;
> +		pthread_join(tpebs_reader_thread, NULL);
> +		close(tpebs_cmd->out);
> +		ret = finish_command(tpebs_cmd);
> +		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> +			ret = 0;
> +	}
> +	return ret;
> +}
> +
> +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
> +{
> +	struct perf_counts_values *count;
> +	struct tpebs_retire_lat *t;
> +	bool found = false;
> +	__u64 val;
> +	int ret;
> +
> +	/* Non reitre_latency evsel should never enter this function. */
> +	if (!evsel__is_retire_lat(evsel))
> +		return -1;
> +
> +	ret = tpebs_stop();

I think it's better to call this in the upper layer.


> +	if (ret)
> +		return ret;
> +
> +	count = perf_counts(evsel->counts, cpu_map_idx, thread);
> +
> +	list_for_each_entry(t, &tpebs_results, nd) {
> +		if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t->tpebs_name, evsel->metric_id)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	/* Set ena and run to non-zero */
> +	count->ena = count->run = 1;
> +	count->lost = 0;
> +
> +	if (!found) {
> +		/*
> +		 * Set default value or 0 when retire_latency for this event is
> +		 * not found from sampling data (enable_tpebs_recording not set
> +		 * or 0 sample recorded).
> +		 */
> +		val = 0;

Shouldn't it set count->val to 0?

Thanks,
Namhyung


> +		return 0;
> +	}
> +
> +	/*
> +	 * Only set retire_latency value to the first CPU and thread.
> +	 */
> +	if (cpu_map_idx == 0 && thread == 0)
> +		val = rint(t->val);
> +	else
> +		val = 0;
> +
> +	count->val = val;
> +	return 0;
> +}
> +
> +static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> +{
> +	zfree(&r->name);
> +	zfree(&r->tpebs_name);
> +	free(r);
> +}
> +
> +void tpebs_delete(void)
> +{
> +	struct tpebs_retire_lat *r, *rtmp;
> +
> +	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
> +		list_del_init(&r->nd);
> +		tpebs_retire_lat__delete(r);
> +	}
> +
> +	if (tpebs_cmd) {
> +		free(tpebs_cmd);
> +		tpebs_cmd = NULL;
> +	}
> +}
> +
> +int tpebs_stop_delete(void)
> +{
> +	int ret;
> +
> +	if (tpebs_pid == -1)
> +		return 0;
> +
> +	ret = tpebs_stop();
> +	tpebs_delete();
> +	return ret;
> +}
> diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> new file mode 100644
> index 000000000000..73c1e5219522
> --- /dev/null
> +++ b/tools/perf/util/intel-tpebs.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * intel_tpebs.h: Intel TEPBS support
> + */
> +#ifndef INCLUDE__PERF_INTEL_TPEBS_H__
> +#define INCLUDE__PERF_INTEL_TPEBS_H__
> +
> +#include "stat.h"
> +#include "evsel.h"
> +
> +#ifdef HAVE_ARCH_X86_64_SUPPORT
> +
> +extern bool tpebs_recording;
> +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus);
> +int tpebs_stop(void);
> +void tpebs_delete(void);
> +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
> +int tpebs_stop_delete(void);
> +
> +#else
> +
> +static inline int tpebs_start(struct evlist *evsel_list __maybe_unused,
> +				struct perf_cpu_map *cpus  __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static inline int tpebs_stop(void)
> +{
> +	return 0;
> +}
> +
> +static inline void tpebs_delete(void) {};
> +
> +static inline int tpebs_set_evsel(struct evsel *evsel  __maybe_unused,
> +				int cpu_map_idx  __maybe_unused,
> +				int thread  __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static inline int tpebs_stop_delete(void)
> +{
> +	return 0;
> +}
> +
> +#endif
> +#endif
> -- 
> 2.43.0
>

> -----Original Message-----
> From: Namhyung Kim <namhyung@kernel.org>
> Sent: Thursday, June 6, 2024 4:21 PM
> To: Wang, Weilin <weilin.wang@intel.com>
> Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> <mingo@redhat.com>; Alexander Shishkin
> <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> Caleb <caleb.biggers@intel.com>
> Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> perf stat needs to get retire latency value for a metric.
> 
> Hello,
> 
> On Wed, Jun 05, 2024 at 01:21:44AM -0400, weilin.wang@intel.com wrote:
> > From: Weilin Wang <weilin.wang@intel.com>
> >
> > When retire_latency value is used in a metric formula, evsel would fork a perf
> > record process with "-e" and "-W" options. Perf record will collect required
> > retire_latency values in parallel while perf stat is collecting counting values.
> >
> > At the point of time that perf stat stops counting, evsel would stop perf
> record
> > by sending sigterm signal to perf record process. Sampled data will be
> process
> > to get retire latency value. Another thread is required to synchronize
> between
> > perf stat and perf record when we pass data through pipe.
> >
> > Retire_latency evsel is not opened for perf stat so that there is no counter
> > wasted on it. This commit includes code suggested by Namhyung to adjust
> reading
> > size for groups that include retire_latency evsels.
> >
> > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> > ---
> >  tools/perf/builtin-stat.c     |   6 +
> >  tools/perf/util/Build         |   1 +
> >  tools/perf/util/evsel.c       |  66 +++++-
> >  tools/perf/util/intel-tpebs.c | 397
> ++++++++++++++++++++++++++++++++++
> >  tools/perf/util/intel-tpebs.h |  48 ++++
> >  5 files changed, 516 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/perf/util/intel-tpebs.c
> >  create mode 100644 tools/perf/util/intel-tpebs.h
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 428e9721b908..b09cb2c6e9c2 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -70,6 +70,7 @@
> >  #include "util/bpf_counter.h"
> >  #include "util/iostat.h"
> >  #include "util/util.h"
> > +#include "util/intel-tpebs.h"
> >  #include "asm/bug.h"
> >
> >  #include <linux/time64.h>
> > @@ -653,6 +654,9 @@ static enum counter_recovery
> stat_handle_error(struct evsel *counter)
> >
> >  	if (child_pid != -1)
> >  		kill(child_pid, SIGTERM);
> > +
> > +	tpebs_stop_delete();
> > +
> >  	return COUNTER_FATAL;
> >  }
> >
> > @@ -985,6 +989,8 @@ static void sig_atexit(void)
> >  	if (child_pid != -1)
> >  		kill(child_pid, SIGTERM);
> >
> > +	tpebs_stop();
> > +
> >  	sigprocmask(SIG_SETMASK, &oset, NULL);
> >
> >  	if (signr == -1)
> > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > index 292170a99ab6..79adf39e0d8f 100644
> > --- a/tools/perf/util/Build
> > +++ b/tools/perf/util/Build
> > @@ -153,6 +153,7 @@ perf-y += clockid.o
> >  perf-y += list_sort.o
> >  perf-y += mutex.o
> >  perf-y += sharded_mutex.o
> > +perf-$(CONFIG_X86_64) += intel-tpebs.o
> >
> >  perf-$(CONFIG_LIBBPF) += bpf_map.o
> >  perf-$(CONFIG_PERF_BPF_SKEL) += bpf_counter.o
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index a0a8aee7d6b9..bd3627819afe 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1538,6 +1538,60 @@ static void evsel__set_count(struct evsel
> *counter, int cpu_map_idx, int thread,
> >  	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread,
> true);
> >  }
> >
> > +static bool evsel__group_has_tpebs(struct evsel *leader)
> > +{
> > +	struct evsel *evsel;
> > +
> > +	for_each_group_evsel(evsel, leader) {
> > +		if (evsel__is_retire_lat(evsel))
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +static u64 evsel__group_read_nr_members(struct evsel *leader)
> > +{
> > +	u64 nr = leader->core.nr_members;
> > +	struct evsel *evsel;
> > +
> > +	for_each_group_evsel(evsel, leader) {
> > +		if (evsel__is_retire_lat(evsel))
> > +			nr--;
> > +	}
> > +	return nr;
> > +}
> > +
> > +static u64 evsel__group_read_size(struct evsel *leader)
> > +{
> > +	u64 read_format = leader->core.attr.read_format;
> > +	int entry = sizeof(u64); /* value */
> > +	int size = 0;
> > +	int nr = 1;
> > +
> > +	if (!evsel__group_has_tpebs(leader))
> > +		return perf_evsel__read_size(&leader->core);
> > +
> > +	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > +		size += sizeof(u64);
> > +
> > +	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> > +		size += sizeof(u64);
> > +
> > +	if (read_format & PERF_FORMAT_ID)
> > +		entry += sizeof(u64);
> > +
> > +	if (read_format & PERF_FORMAT_LOST)
> > +		entry += sizeof(u64);
> > +
> > +	if (read_format & PERF_FORMAT_GROUP) {
> > +		nr = evsel__group_read_nr_members(leader);
> > +		size += sizeof(u64);
> > +	}
> > +
> > +	size += entry * nr;
> > +	return size;
> > +}
> > +
> >  static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx,
> int thread, u64 *data)
> >  {
> >  	u64 read_format = leader->core.attr.read_format;
> > @@ -1546,7 +1600,7 @@ static int evsel__process_group_data(struct evsel
> *leader, int cpu_map_idx, int
> >
> >  	nr = *data++;
> >
> > -	if (nr != (u64) leader->core.nr_members)
> > +	if (nr != evsel__group_read_nr_members(leader))
> >  		return -EINVAL;
> >
> >  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > @@ -1576,7 +1630,7 @@ static int evsel__read_group(struct evsel *leader,
> int cpu_map_idx, int thread)
> >  {
> >  	struct perf_stat_evsel *ps = leader->stats;
> >  	u64 read_format = leader->core.attr.read_format;
> > -	int size = perf_evsel__read_size(&leader->core);
> > +	int size = evsel__group_read_size(leader);
> >  	u64 *data = ps->group_data;
> >
> >  	if (!(read_format & PERF_FORMAT_ID))
> > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel,
> struct perf_cpu_map *cpus,
> >  		return 0;
> >  	}
> >
> > +	if (evsel__is_retire_lat(evsel))
> > +		return tpebs_start(evsel->evlist, cpus);
> 
> As it works with evlist, I think it's better to put this code there.
> But it seems perf stat doesn't call the evlist API for open, then we
> can add this to somewhere in __run_perf_stat() directly.
> 
> > +
> >  	err = __evsel__prepare_open(evsel, cpus, threads);
> >  	if (err)
> >  		return err;
> > @@ -2376,6 +2433,8 @@ 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();
> 
> Ditto.

Hi Namhyung,

I hope both this and the one above on open could stay in evsel level because
these are operations on retire_latency evsel. At the same time, a lot of the 
previous several versions of work was to move TPEBS code out from perf stat to 
evsel to make it more generic. I think move these back to __run_perf_stat() are 
opposite to that goal. 


> 
> >  	perf_evsel__close(&evsel->core);
> >  	perf_evsel__free_id(&evsel->core);
> >  }
> > @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel,
> struct evlist *evlist)
> >  {
> >  	int cpu_map_idx, thread;
> >
> > +	if (evsel__is_retire_lat(evsel))
> > +		return 0;
> > +
> >  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd);
> cpu_map_idx++) {
> >  		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
> >  		     thread++) {
> > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > new file mode 100644
> > index 000000000000..37b7a4f92dd9
> > --- /dev/null
> > +++ b/tools/perf/util/intel-tpebs.c
> > @@ -0,0 +1,397 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * intel_tpebs.c: Intel TPEBS support
> > + */
> > +
> > +
> > +#include <sys/param.h>
> > +#include <subcmd/run-command.h>
> > +#include <thread.h>
> > +#include "intel-tpebs.h"
> > +#include <linux/list.h>
> > +#include <linux/zalloc.h>
> > +#include <linux/err.h>
> > +#include "sample.h"
> > +#include "debug.h"
> > +#include "evlist.h"
> > +#include "evsel.h"
> > +#include "session.h"
> > +#include "tool.h"
> > +#include "cpumap.h"
> > +#include "metricgroup.h"
> > +#include <sys/stat.h>
> > +#include <sys/file.h>
> > +#include <poll.h>
> > +#include <math.h>
> > +
> > +#define PERF_DATA		"-"
> > +
> > +bool tpebs_recording;
> > +static pid_t tpebs_pid = -1;
> > +static size_t tpebs_event_size;
> > +static pthread_t tpebs_reader_thread;
> > +static struct child_process *tpebs_cmd;
> > +static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);
> 
> It can be 'static LIST_HEAD(tpebs_results);'
> 
> > +
> > +struct tpebs_retire_lat {
> > +	struct list_head nd;
> > +	/* Event name */
> > +	const char *name;
> > +	/* Event name with the TPEBS modifier R */
> > +	const char *tpebs_name;
> > +	/* Count of retire_latency values found in sample data */
> > +	size_t count;
> > +	/* Sum of all the retire_latency values in sample data */
> > +	int sum;
> > +	/* Average of retire_latency, val = sum / count */
> > +	double val;
> > +};
> > +
> > +static int get_perf_record_args(const char **record_argv, char buf[],
> > +				const char *cpumap_buf)
> > +{
> > +	struct tpebs_retire_lat *e;
> > +	int i = 0;
> > +
> > +	pr_debug("Prepare perf record for retire_latency\n");
> > +
> > +	record_argv[i++] = "perf";
> > +	record_argv[i++] = "record";
> > +	record_argv[i++] = "-W";
> > +	record_argv[i++] = "--synth=no";
> > +	record_argv[i++] = buf;
> > +
> > +	if (cpumap_buf) {
> > +		record_argv[i++] = "-C";
> > +		record_argv[i++] = cpumap_buf;
> > +	}
> > +
> > +	record_argv[i++] = "-a";
> > +
> > +	if (!cpumap_buf) {
> > +		pr_err("Require cpumap list to run sampling.\n");
> > +		return -ECANCELED;
> > +	}
> 
> Hmm.. I thought you supported system wide collection, not sure if it has
> a cpumap.  Anyway this check makes the earlier one meaningless - you
> need the cpumap always, right?

TPEBS should be work with "-a" or "-C". I'm not sure what the cpumap would be 
for "-a". Would it make sense to add "-a" only when cpumap_buf is NULL?

> 
> > +
> > +	list_for_each_entry(e, &tpebs_results, nd) {
> > +		record_argv[i++] = "-e";
> > +		record_argv[i++] = e->name;
> > +	}
> > +
> > +	record_argv[i++] = "-o";
> > +	record_argv[i++] = PERF_DATA;
> > +
> > +	return 0;
> > +}
> > +
> > +static int prepare_run_command(const char **argv)
> > +{
> > +	tpebs_cmd = zalloc(sizeof(struct child_process));
> > +	if (!tpebs_cmd)
> > +		return -ENOMEM;
> > +	tpebs_cmd->argv = argv;
> > +	tpebs_cmd->out = -1;
> > +	return 0;
> > +}
> > +
> > +static int start_perf_record(int control_fd[], int ack_fd[],
> > +				const char *cpumap_buf)
> > +{
> > +	const char **record_argv;
> > +	int ret;
> > +	char buf[32];
> > +
> > +	scnprintf(buf, sizeof(buf), "--control=fd:%d,%d", control_fd[0],
> ack_fd[1]);
> > +
> > +	record_argv = calloc(12 + 2 * tpebs_event_size, sizeof(char *));
> > +	if (!record_argv)
> > +		return -ENOMEM;
> > +
> > +	ret = get_perf_record_args(record_argv, buf, cpumap_buf);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = prepare_run_command(record_argv);
> > +	if (ret)
> > +		goto out;
> > +	ret = start_command(tpebs_cmd);
> > +out:
> > +	free(record_argv);
> > +	return ret;
> > +}
> > +
> > +static int process_sample_event(struct perf_tool *tool __maybe_unused,
> > +				union perf_event *event __maybe_unused,
> > +				struct perf_sample *sample,
> > +				struct evsel *evsel,
> > +				struct machine *machine __maybe_unused)
> > +{
> > +	int ret = 0;
> > +	const char *evname;
> > +	struct tpebs_retire_lat *t;
> > +
> > +	evname = evsel__name(evsel);
> > +
> > +	/*
> > +	 * 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;
> > +}
> > +
> > +static int process_feature_event(struct perf_session *session,
> > +				 union perf_event *event)
> > +{
> > +	if (event->feat.feat_id < HEADER_LAST_FEATURE)
> > +		return perf_event__process_feature(session, event);
> > +	return 0;
> > +}
> > +
> > +static void *__sample_reader(void *arg)
> > +{
> > +	struct child_process *child = arg;
> > +	struct perf_session *session;
> > +	struct perf_data data = {
> > +		.mode = PERF_DATA_MODE_READ,
> > +		.path = PERF_DATA,
> > +		.file.fd = child->out,
> > +	};
> > +	struct perf_tool tool = {
> > +		.sample = process_sample_event,
> > +		.feature = process_feature_event,
> > +		.attr = perf_event__process_attr,
> > +	};
> > +
> > +	session = perf_session__new(&data, &tool);
> > +	if (IS_ERR(session))
> > +		return NULL;
> > +	perf_session__process_events(session);
> > +	perf_session__delete(session);
> > +
> > +	return NULL;
> > +}
> > +
> > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus)
> > +{
> > +	int ret = 0;
> > +	struct evsel *evsel;
> > +	char cpumap_buf[50];
> > +
> > +	/*
> > +	 * We should only run tpebs_start when tpebs_recording is enabled.
> > +	 * And we should only run it once with all the required events.
> > +	 */
> > +	if (tpebs_pid != -1 || !tpebs_recording)
> > +		return 0;
> > +
> > +	cpu_map__snprint(cpus, cpumap_buf, sizeof(cpumap_buf));
> > +	pr_debug("cpu map: %s\n", cpumap_buf);
> 
> Can you please remove unnecessary debug prints?  If you really want it,
> then make it more meaningful like with more context.
> 
> > +
> > +	/*
> > +	 * Prepare perf record for sampling event retire_latency before fork
> and
> > +	 * prepare workload
> > +	 */
> > +	evlist__for_each_entry(evsel_list, evsel) {
> > +		struct tpebs_retire_lat *new = zalloc(sizeof(*new));
> > +		char *name;
> > +		int i;
> > +
> > +		if (!evsel->retire_lat)
> > +			continue;
> 
> Please move the allocation after this.
> 
> > +
> > +		pr_debug("perf stat retire latency of event %s required\n",
> evsel->name);
> > +		if (!new) {
> > +			ret = -1;
> > +			goto err;
> > +		}
> > +		for (i = strlen(evsel->name) - 1; i > 0; i--) {
> > +			if (evsel->name[i] == 'R')
> > +				break;
> 
> I think you also need to check ':' or '/'..

I think the ':' and '/' have already been checked by the evsel parser to make 
this evsel a retire_latency evsel. Do we need to check them again here?

> 
> > +		}
> > +		if (i <= 0 || evsel->name[i] != 'R') {
> > +			ret = -1;
> > +			goto err;
> > +		}
> > +
> > +		name = strdup(evsel->name);
> > +		if (!name) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +		name[i] = 'p';
> > +		new->name = name;
> > +		new->tpebs_name = strdup(evsel->name);
> > +		if (!new->tpebs_name) {
> > +			ret = -ENOMEM;
> > +			goto err;
> 
> This error handling (including above) will leak 'new' because it's not
> linked to the list yet.

Yes, you are right. I will fix this. 

> 
> > +		}
> > +		list_add_tail(&new->nd, &tpebs_results);
> > +		tpebs_event_size += 1;
> > +	}
> > +
> > +	if (tpebs_event_size > 0) {
> > +		int control_fd[2], ack_fd[2], len;
> > +		char ack_buf[8];
> > +
> > +		/*Create control and ack fd for --control*/
> > +		if (pipe(control_fd) < 0) {
> > +			pr_err("Failed to create control fifo");
> > +			ret = -1;
> > +			goto out;
> > +		}
> > +		if (pipe(ack_fd) < 0) {
> > +			pr_err("Failed to create control fifo");
> > +			ret = -1;
> > +			goto out;
> > +		}
> > +
> > +		ret = start_perf_record(control_fd, ack_fd, cpumap_buf);
> > +		if (ret)
> > +			goto out;
> > +		tpebs_pid = tpebs_cmd->pid;
> > +		if (pthread_create(&tpebs_reader_thread, NULL,
> __sample_reader, tpebs_cmd)) {
> > +			kill(tpebs_cmd->pid, SIGTERM);
> > +			close(tpebs_cmd->out);
> > +			pr_err("Could not create thread to process sample
> data.\n");
> > +			ret = -1;
> > +			goto out;
> > +		}
> > +		/* Wait for perf record initialization.*/
> > +		len = strlen("enable");
> > +		ret = write(control_fd[1], "enable", len);
> > +		if (ret != len) {
> > +			pr_err("perf record control write control message
> failed\n");
> > +			goto out;
> > +		}
> > +
> > +		ret = read(ack_fd[0], ack_buf, sizeof(ack_buf));
> > +		if (ret > 0)
> > +			ret = strcmp(ack_buf, "ack\n");
> > +		else {
> > +			pr_err("perf record control ack failed\n");
> > +			goto out;
> > +		}
> > +		pr_debug("Received ack from perf record\n");
> > +out:
> > +		close(control_fd[0]);
> > +		close(control_fd[1]);
> > +		close(ack_fd[0]);
> > +		close(ack_fd[1]);
> > +	}
> > +err:
> > +	if (ret)
> > +		tpebs_delete();
> > +	return ret;
> > +}
> > +
> > +int tpebs_stop(void)
> > +{
> > +	int ret = 0;
> > +
> > +	/* Like tpebs_start, we should only run tpebs_end once. */
> > +	if (tpebs_pid != -1) {
> > +		kill(tpebs_cmd->pid, SIGTERM);
> > +		tpebs_pid = -1;
> > +		pthread_join(tpebs_reader_thread, NULL);
> > +		close(tpebs_cmd->out);
> > +		ret = finish_command(tpebs_cmd);
> > +		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> > +			ret = 0;
> > +	}
> > +	return ret;
> > +}
> > +
> > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
> > +{
> > +	struct perf_counts_values *count;
> > +	struct tpebs_retire_lat *t;
> > +	bool found = false;
> > +	__u64 val;
> > +	int ret;
> > +
> > +	/* Non reitre_latency evsel should never enter this function. */
> > +	if (!evsel__is_retire_lat(evsel))
> > +		return -1;
> > +
> > +	ret = tpebs_stop();
> 
> I think it's better to call this in the upper layer.

The tpebs_stop() is also called in sig_atexit() in perf stat. I want to make this function 
safe to be called multiple times and add one tpebs_stop() here in case tpebs_set_evsel()
is invoked before tpebs_stop(). I hope this makes sense.

> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	count = perf_counts(evsel->counts, cpu_map_idx, thread);
> > +
> > +	list_for_each_entry(t, &tpebs_results, nd) {
> > +		if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t-
> >tpebs_name, evsel->metric_id)) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* Set ena and run to non-zero */
> > +	count->ena = count->run = 1;
> > +	count->lost = 0;
> > +
> > +	if (!found) {
> > +		/*
> > +		 * Set default value or 0 when retire_latency for this event is
> > +		 * not found from sampling data (enable_tpebs_recording not
> set
> > +		 * or 0 sample recorded).
> > +		 */
> > +		val = 0;
> 
> Shouldn't it set count->val to 0?

Yes, I will fix this one.

Thanks,
Weilin
> 
> Thanks,
> Namhyung
> 
> 
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * Only set retire_latency value to the first CPU and thread.
> > +	 */
> > +	if (cpu_map_idx == 0 && thread == 0)
> > +		val = rint(t->val);
> > +	else
> > +		val = 0;
> > +
> > +	count->val = val;
> > +	return 0;
> > +}
> > +
> > +static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> > +{
> > +	zfree(&r->name);
> > +	zfree(&r->tpebs_name);
> > +	free(r);
> > +}
> > +
> > +void tpebs_delete(void)
> > +{
> > +	struct tpebs_retire_lat *r, *rtmp;
> > +
> > +	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
> > +		list_del_init(&r->nd);
> > +		tpebs_retire_lat__delete(r);
> > +	}
> > +
> > +	if (tpebs_cmd) {
> > +		free(tpebs_cmd);
> > +		tpebs_cmd = NULL;
> > +	}
> > +}
> > +
> > +int tpebs_stop_delete(void)
> > +{
> > +	int ret;
> > +
> > +	if (tpebs_pid == -1)
> > +		return 0;
> > +
> > +	ret = tpebs_stop();
> > +	tpebs_delete();
> > +	return ret;
> > +}
> > diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> > new file mode 100644
> > index 000000000000..73c1e5219522
> > --- /dev/null
> > +++ b/tools/perf/util/intel-tpebs.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * intel_tpebs.h: Intel TEPBS support
> > + */
> > +#ifndef INCLUDE__PERF_INTEL_TPEBS_H__
> > +#define INCLUDE__PERF_INTEL_TPEBS_H__
> > +
> > +#include "stat.h"
> > +#include "evsel.h"
> > +
> > +#ifdef HAVE_ARCH_X86_64_SUPPORT
> > +
> > +extern bool tpebs_recording;
> > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus);
> > +int tpebs_stop(void);
> > +void tpebs_delete(void);
> > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
> > +int tpebs_stop_delete(void);
> > +
> > +#else
> > +
> > +static inline int tpebs_start(struct evlist *evsel_list __maybe_unused,
> > +				struct perf_cpu_map *cpus
> __maybe_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline int tpebs_stop(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void tpebs_delete(void) {};
> > +
> > +static inline int tpebs_set_evsel(struct evsel *evsel  __maybe_unused,
> > +				int cpu_map_idx  __maybe_unused,
> > +				int thread  __maybe_unused)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline int tpebs_stop_delete(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +#endif
> > +#endif
> > --
> > 2.43.0
> >

On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
> 
> 
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@kernel.org>
> > Sent: Thursday, June 6, 2024 4:21 PM
> > To: Wang, Weilin <weilin.wang@intel.com>
> > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > <mingo@redhat.com>; Alexander Shishkin
> > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> > Caleb <caleb.biggers@intel.com>
> > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> > perf stat needs to get retire latency value for a metric.
> > 
> > Hello,
> > 
> > On Wed, Jun 05, 2024 at 01:21:44AM -0400, weilin.wang@intel.com wrote:
> > > From: Weilin Wang <weilin.wang@intel.com>
> > >
> > > When retire_latency value is used in a metric formula, evsel would fork a perf
> > > record process with "-e" and "-W" options. Perf record will collect required
> > > retire_latency values in parallel while perf stat is collecting counting values.
> > >
> > > At the point of time that perf stat stops counting, evsel would stop perf
> > record
> > > by sending sigterm signal to perf record process. Sampled data will be
> > process
> > > to get retire latency value. Another thread is required to synchronize
> > between
> > > perf stat and perf record when we pass data through pipe.
> > >
> > > Retire_latency evsel is not opened for perf stat so that there is no counter
> > > wasted on it. This commit includes code suggested by Namhyung to adjust
> > reading
> > > size for groups that include retire_latency evsels.
> > >
> > > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> > > ---
> > >  tools/perf/builtin-stat.c     |   6 +
> > >  tools/perf/util/Build         |   1 +
> > >  tools/perf/util/evsel.c       |  66 +++++-
> > >  tools/perf/util/intel-tpebs.c | 397
> > ++++++++++++++++++++++++++++++++++
> > >  tools/perf/util/intel-tpebs.h |  48 ++++
> > >  5 files changed, 516 insertions(+), 2 deletions(-)
> > >  create mode 100644 tools/perf/util/intel-tpebs.c
> > >  create mode 100644 tools/perf/util/intel-tpebs.h
> > >
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index 428e9721b908..b09cb2c6e9c2 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -70,6 +70,7 @@
> > >  #include "util/bpf_counter.h"
> > >  #include "util/iostat.h"
> > >  #include "util/util.h"
> > > +#include "util/intel-tpebs.h"
> > >  #include "asm/bug.h"
> > >
> > >  #include <linux/time64.h>
> > > @@ -653,6 +654,9 @@ static enum counter_recovery
> > stat_handle_error(struct evsel *counter)
> > >
> > >  	if (child_pid != -1)
> > >  		kill(child_pid, SIGTERM);
> > > +
> > > +	tpebs_stop_delete();
> > > +
> > >  	return COUNTER_FATAL;
> > >  }
> > >
> > > @@ -985,6 +989,8 @@ static void sig_atexit(void)
> > >  	if (child_pid != -1)
> > >  		kill(child_pid, SIGTERM);
> > >
> > > +	tpebs_stop();
> > > +
> > >  	sigprocmask(SIG_SETMASK, &oset, NULL);
> > >
> > >  	if (signr == -1)
> > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > > index 292170a99ab6..79adf39e0d8f 100644
> > > --- a/tools/perf/util/Build
> > > +++ b/tools/perf/util/Build
> > > @@ -153,6 +153,7 @@ perf-y += clockid.o
> > >  perf-y += list_sort.o
> > >  perf-y += mutex.o
> > >  perf-y += sharded_mutex.o
> > > +perf-$(CONFIG_X86_64) += intel-tpebs.o
> > >
> > >  perf-$(CONFIG_LIBBPF) += bpf_map.o
> > >  perf-$(CONFIG_PERF_BPF_SKEL) += bpf_counter.o
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index a0a8aee7d6b9..bd3627819afe 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1538,6 +1538,60 @@ static void evsel__set_count(struct evsel
> > *counter, int cpu_map_idx, int thread,
> > >  	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread,
> > true);
> > >  }
> > >
> > > +static bool evsel__group_has_tpebs(struct evsel *leader)
> > > +{
> > > +	struct evsel *evsel;
> > > +
> > > +	for_each_group_evsel(evsel, leader) {
> > > +		if (evsel__is_retire_lat(evsel))
> > > +			return true;
> > > +	}
> > > +	return false;
> > > +}
> > > +
> > > +static u64 evsel__group_read_nr_members(struct evsel *leader)
> > > +{
> > > +	u64 nr = leader->core.nr_members;
> > > +	struct evsel *evsel;
> > > +
> > > +	for_each_group_evsel(evsel, leader) {
> > > +		if (evsel__is_retire_lat(evsel))
> > > +			nr--;
> > > +	}
> > > +	return nr;
> > > +}
> > > +
> > > +static u64 evsel__group_read_size(struct evsel *leader)
> > > +{
> > > +	u64 read_format = leader->core.attr.read_format;
> > > +	int entry = sizeof(u64); /* value */
> > > +	int size = 0;
> > > +	int nr = 1;
> > > +
> > > +	if (!evsel__group_has_tpebs(leader))
> > > +		return perf_evsel__read_size(&leader->core);
> > > +
> > > +	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > > +		size += sizeof(u64);
> > > +
> > > +	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> > > +		size += sizeof(u64);
> > > +
> > > +	if (read_format & PERF_FORMAT_ID)
> > > +		entry += sizeof(u64);
> > > +
> > > +	if (read_format & PERF_FORMAT_LOST)
> > > +		entry += sizeof(u64);
> > > +
> > > +	if (read_format & PERF_FORMAT_GROUP) {
> > > +		nr = evsel__group_read_nr_members(leader);
> > > +		size += sizeof(u64);
> > > +	}
> > > +
> > > +	size += entry * nr;
> > > +	return size;
> > > +}
> > > +
> > >  static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx,
> > int thread, u64 *data)
> > >  {
> > >  	u64 read_format = leader->core.attr.read_format;
> > > @@ -1546,7 +1600,7 @@ static int evsel__process_group_data(struct evsel
> > *leader, int cpu_map_idx, int
> > >
> > >  	nr = *data++;
> > >
> > > -	if (nr != (u64) leader->core.nr_members)
> > > +	if (nr != evsel__group_read_nr_members(leader))
> > >  		return -EINVAL;
> > >
> > >  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > > @@ -1576,7 +1630,7 @@ static int evsel__read_group(struct evsel *leader,
> > int cpu_map_idx, int thread)
> > >  {
> > >  	struct perf_stat_evsel *ps = leader->stats;
> > >  	u64 read_format = leader->core.attr.read_format;
> > > -	int size = perf_evsel__read_size(&leader->core);
> > > +	int size = evsel__group_read_size(leader);
> > >  	u64 *data = ps->group_data;
> > >
> > >  	if (!(read_format & PERF_FORMAT_ID))
> > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel,
> > struct perf_cpu_map *cpus,
> > >  		return 0;
> > >  	}
> > >
> > > +	if (evsel__is_retire_lat(evsel))
> > > +		return tpebs_start(evsel->evlist, cpus);
> > 
> > As it works with evlist, I think it's better to put this code there.
> > But it seems perf stat doesn't call the evlist API for open, then we
> > can add this to somewhere in __run_perf_stat() directly.
> > 
> > > +
> > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > >  	if (err)
> > >  		return err;
> > > @@ -2376,6 +2433,8 @@ 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();
> > 
> > Ditto.
> 
> Hi Namhyung,
> 
> I hope both this and the one above on open could stay in evsel level because
> these are operations on retire_latency evsel.

Then I think you need to remove the specific evsel not the all tpebs
events.

> At the same time, a lot of the 
> previous several versions of work was to move TPEBS code out from perf stat to 
> evsel to make it more generic. I think move these back to __run_perf_stat() are 
> opposite to that goal. 

Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
in evlist__close() but we don't use evlist__open() for perf stat so it's
not symmetric. :(

Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
think it's natural that they are called for the whole list, not for an
event/evsel.

> 
> 
> > 
> > >  	perf_evsel__close(&evsel->core);
> > >  	perf_evsel__free_id(&evsel->core);
> > >  }
> > > @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel,
> > struct evlist *evlist)
> > >  {
> > >  	int cpu_map_idx, thread;
> > >
> > > +	if (evsel__is_retire_lat(evsel))
> > > +		return 0;
> > > +
> > >  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd);
> > cpu_map_idx++) {
> > >  		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
> > >  		     thread++) {
> > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > new file mode 100644
> > > index 000000000000..37b7a4f92dd9
> > > --- /dev/null
> > > +++ b/tools/perf/util/intel-tpebs.c
> > > @@ -0,0 +1,397 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * intel_tpebs.c: Intel TPEBS support
> > > + */
> > > +
> > > +
> > > +#include <sys/param.h>
> > > +#include <subcmd/run-command.h>
> > > +#include <thread.h>
> > > +#include "intel-tpebs.h"
> > > +#include <linux/list.h>
> > > +#include <linux/zalloc.h>
> > > +#include <linux/err.h>
> > > +#include "sample.h"
> > > +#include "debug.h"
> > > +#include "evlist.h"
> > > +#include "evsel.h"
> > > +#include "session.h"
> > > +#include "tool.h"
> > > +#include "cpumap.h"
> > > +#include "metricgroup.h"
> > > +#include <sys/stat.h>
> > > +#include <sys/file.h>
> > > +#include <poll.h>
> > > +#include <math.h>
> > > +
> > > +#define PERF_DATA		"-"
> > > +
> > > +bool tpebs_recording;
> > > +static pid_t tpebs_pid = -1;
> > > +static size_t tpebs_event_size;
> > > +static pthread_t tpebs_reader_thread;
> > > +static struct child_process *tpebs_cmd;
> > > +static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);
> > 
> > It can be 'static LIST_HEAD(tpebs_results);'
> > 
> > > +
> > > +struct tpebs_retire_lat {
> > > +	struct list_head nd;
> > > +	/* Event name */
> > > +	const char *name;
> > > +	/* Event name with the TPEBS modifier R */
> > > +	const char *tpebs_name;
> > > +	/* Count of retire_latency values found in sample data */
> > > +	size_t count;
> > > +	/* Sum of all the retire_latency values in sample data */
> > > +	int sum;
> > > +	/* Average of retire_latency, val = sum / count */
> > > +	double val;
> > > +};
> > > +
> > > +static int get_perf_record_args(const char **record_argv, char buf[],
> > > +				const char *cpumap_buf)
> > > +{
> > > +	struct tpebs_retire_lat *e;
> > > +	int i = 0;
> > > +
> > > +	pr_debug("Prepare perf record for retire_latency\n");
> > > +
> > > +	record_argv[i++] = "perf";
> > > +	record_argv[i++] = "record";
> > > +	record_argv[i++] = "-W";
> > > +	record_argv[i++] = "--synth=no";
> > > +	record_argv[i++] = buf;
> > > +
> > > +	if (cpumap_buf) {
> > > +		record_argv[i++] = "-C";
> > > +		record_argv[i++] = cpumap_buf;
> > > +	}
> > > +
> > > +	record_argv[i++] = "-a";
> > > +
> > > +	if (!cpumap_buf) {
> > > +		pr_err("Require cpumap list to run sampling.\n");
> > > +		return -ECANCELED;
> > > +	}
> > 
> > Hmm.. I thought you supported system wide collection, not sure if it has
> > a cpumap.  Anyway this check makes the earlier one meaningless - you
> > need the cpumap always, right?
> 
> TPEBS should be work with "-a" or "-C". I'm not sure what the cpumap would be 
> for "-a". Would it make sense to add "-a" only when cpumap_buf is NULL?

I think the best way is to check target__has_cpu().

> 
> > 
> > > +
> > > +	list_for_each_entry(e, &tpebs_results, nd) {
> > > +		record_argv[i++] = "-e";
> > > +		record_argv[i++] = e->name;
> > > +	}
> > > +
> > > +	record_argv[i++] = "-o";
> > > +	record_argv[i++] = PERF_DATA;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int prepare_run_command(const char **argv)
> > > +{
> > > +	tpebs_cmd = zalloc(sizeof(struct child_process));
> > > +	if (!tpebs_cmd)
> > > +		return -ENOMEM;
> > > +	tpebs_cmd->argv = argv;
> > > +	tpebs_cmd->out = -1;
> > > +	return 0;
> > > +}
> > > +
> > > +static int start_perf_record(int control_fd[], int ack_fd[],
> > > +				const char *cpumap_buf)
> > > +{
> > > +	const char **record_argv;
> > > +	int ret;
> > > +	char buf[32];
> > > +
> > > +	scnprintf(buf, sizeof(buf), "--control=fd:%d,%d", control_fd[0],
> > ack_fd[1]);
> > > +
> > > +	record_argv = calloc(12 + 2 * tpebs_event_size, sizeof(char *));
> > > +	if (!record_argv)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = get_perf_record_args(record_argv, buf, cpumap_buf);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	ret = prepare_run_command(record_argv);
> > > +	if (ret)
> > > +		goto out;
> > > +	ret = start_command(tpebs_cmd);
> > > +out:
> > > +	free(record_argv);
> > > +	return ret;
> > > +}
> > > +
> > > +static int process_sample_event(struct perf_tool *tool __maybe_unused,
> > > +				union perf_event *event __maybe_unused,
> > > +				struct perf_sample *sample,
> > > +				struct evsel *evsel,
> > > +				struct machine *machine __maybe_unused)
> > > +{
> > > +	int ret = 0;
> > > +	const char *evname;
> > > +	struct tpebs_retire_lat *t;
> > > +
> > > +	evname = evsel__name(evsel);
> > > +
> > > +	/*
> > > +	 * 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;
> > > +}
> > > +
> > > +static int process_feature_event(struct perf_session *session,
> > > +				 union perf_event *event)
> > > +{
> > > +	if (event->feat.feat_id < HEADER_LAST_FEATURE)
> > > +		return perf_event__process_feature(session, event);
> > > +	return 0;
> > > +}
> > > +
> > > +static void *__sample_reader(void *arg)
> > > +{
> > > +	struct child_process *child = arg;
> > > +	struct perf_session *session;
> > > +	struct perf_data data = {
> > > +		.mode = PERF_DATA_MODE_READ,
> > > +		.path = PERF_DATA,
> > > +		.file.fd = child->out,
> > > +	};
> > > +	struct perf_tool tool = {
> > > +		.sample = process_sample_event,
> > > +		.feature = process_feature_event,
> > > +		.attr = perf_event__process_attr,
> > > +	};
> > > +
> > > +	session = perf_session__new(&data, &tool);
> > > +	if (IS_ERR(session))
> > > +		return NULL;
> > > +	perf_session__process_events(session);
> > > +	perf_session__delete(session);
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus)
> > > +{
> > > +	int ret = 0;
> > > +	struct evsel *evsel;
> > > +	char cpumap_buf[50];
> > > +
> > > +	/*
> > > +	 * We should only run tpebs_start when tpebs_recording is enabled.
> > > +	 * And we should only run it once with all the required events.
> > > +	 */
> > > +	if (tpebs_pid != -1 || !tpebs_recording)
> > > +		return 0;
> > > +
> > > +	cpu_map__snprint(cpus, cpumap_buf, sizeof(cpumap_buf));
> > > +	pr_debug("cpu map: %s\n", cpumap_buf);
> > 
> > Can you please remove unnecessary debug prints?  If you really want it,
> > then make it more meaningful like with more context.
> > 
> > > +
> > > +	/*
> > > +	 * Prepare perf record for sampling event retire_latency before fork
> > and
> > > +	 * prepare workload
> > > +	 */
> > > +	evlist__for_each_entry(evsel_list, evsel) {
> > > +		struct tpebs_retire_lat *new = zalloc(sizeof(*new));
> > > +		char *name;
> > > +		int i;
> > > +
> > > +		if (!evsel->retire_lat)
> > > +			continue;
> > 
> > Please move the allocation after this.
> > 
> > > +
> > > +		pr_debug("perf stat retire latency of event %s required\n",
> > evsel->name);
> > > +		if (!new) {
> > > +			ret = -1;
> > > +			goto err;
> > > +		}
> > > +		for (i = strlen(evsel->name) - 1; i > 0; i--) {
> > > +			if (evsel->name[i] == 'R')
> > > +				break;
> > 
> > I think you also need to check ':' or '/'..
> 
> I think the ':' and '/' have already been checked by the evsel parser to make 
> this evsel a retire_latency evsel. Do we need to check them again here?

Ok, it's fine if it's checked already.  Then I thought you could just
use strrchr() but it seems you want the index of 'R' to modify the
copied string.

> 
> > 
> > > +		}
> > > +		if (i <= 0 || evsel->name[i] != 'R') {
> > > +			ret = -1;
> > > +			goto err;
> > > +		}
> > > +
> > > +		name = strdup(evsel->name);
> > > +		if (!name) {
> > > +			ret = -ENOMEM;
> > > +			goto err;
> > > +		}
> > > +		name[i] = 'p';
> > > +		new->name = name;
> > > +		new->tpebs_name = strdup(evsel->name);

Is this just to find the original evsel later?  Then I think you can
compare the pointer directly instead of name.


> > > +		if (!new->tpebs_name) {
> > > +			ret = -ENOMEM;
> > > +			goto err;
> > 
> > This error handling (including above) will leak 'new' because it's not
> > linked to the list yet.
> 
> Yes, you are right. I will fix this. 
> 
> > 
> > > +		}
> > > +		list_add_tail(&new->nd, &tpebs_results);
> > > +		tpebs_event_size += 1;
> > > +	}
> > > +
> > > +	if (tpebs_event_size > 0) {
> > > +		int control_fd[2], ack_fd[2], len;
> > > +		char ack_buf[8];
> > > +
> > > +		/*Create control and ack fd for --control*/
> > > +		if (pipe(control_fd) < 0) {
> > > +			pr_err("Failed to create control fifo");
> > > +			ret = -1;
> > > +			goto out;
> > > +		}
> > > +		if (pipe(ack_fd) < 0) {
> > > +			pr_err("Failed to create control fifo");
> > > +			ret = -1;
> > > +			goto out;
> > > +		}
> > > +
> > > +		ret = start_perf_record(control_fd, ack_fd, cpumap_buf);
> > > +		if (ret)
> > > +			goto out;
> > > +		tpebs_pid = tpebs_cmd->pid;
> > > +		if (pthread_create(&tpebs_reader_thread, NULL,
> > __sample_reader, tpebs_cmd)) {
> > > +			kill(tpebs_cmd->pid, SIGTERM);
> > > +			close(tpebs_cmd->out);
> > > +			pr_err("Could not create thread to process sample
> > data.\n");
> > > +			ret = -1;
> > > +			goto out;
> > > +		}
> > > +		/* Wait for perf record initialization.*/
> > > +		len = strlen("enable");
> > > +		ret = write(control_fd[1], "enable", len);
> > > +		if (ret != len) {
> > > +			pr_err("perf record control write control message
> > failed\n");
> > > +			goto out;
> > > +		}
> > > +
> > > +		ret = read(ack_fd[0], ack_buf, sizeof(ack_buf));
> > > +		if (ret > 0)
> > > +			ret = strcmp(ack_buf, "ack\n");
> > > +		else {
> > > +			pr_err("perf record control ack failed\n");
> > > +			goto out;
> > > +		}
> > > +		pr_debug("Received ack from perf record\n");
> > > +out:
> > > +		close(control_fd[0]);
> > > +		close(control_fd[1]);
> > > +		close(ack_fd[0]);
> > > +		close(ack_fd[1]);
> > > +	}
> > > +err:
> > > +	if (ret)
> > > +		tpebs_delete();
> > > +	return ret;
> > > +}
> > > +
> > > +int tpebs_stop(void)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	/* Like tpebs_start, we should only run tpebs_end once. */
> > > +	if (tpebs_pid != -1) {
> > > +		kill(tpebs_cmd->pid, SIGTERM);
> > > +		tpebs_pid = -1;
> > > +		pthread_join(tpebs_reader_thread, NULL);
> > > +		close(tpebs_cmd->out);
> > > +		ret = finish_command(tpebs_cmd);
> > > +		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> > > +			ret = 0;
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
> > > +{
> > > +	struct perf_counts_values *count;
> > > +	struct tpebs_retire_lat *t;
> > > +	bool found = false;
> > > +	__u64 val;
> > > +	int ret;
> > > +
> > > +	/* Non reitre_latency evsel should never enter this function. */
> > > +	if (!evsel__is_retire_lat(evsel))
> > > +		return -1;
> > > +
> > > +	ret = tpebs_stop();
> > 
> > I think it's better to call this in the upper layer.
> 
> The tpebs_stop() is also called in sig_atexit() in perf stat. I want to make this function 
> safe to be called multiple times and add one tpebs_stop() here in case tpebs_set_evsel()
> is invoked before tpebs_stop(). I hope this makes sense.

I guess it's ok to call tpebs_set_evsel() even if it's not stopped.  It
seems you use tpebs_retire_lat->val only and it's an average so it won't
change much if you read either before or after concurrent changes.

It can be useful to the interval mode if you keep the tpebs background
process after read. 

Thanks,
Namhyung

> 
> > 
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	count = perf_counts(evsel->counts, cpu_map_idx, thread);
> > > +
> > > +	list_for_each_entry(t, &tpebs_results, nd) {
> > > +		if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t-
> > >tpebs_name, evsel->metric_id)) {
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	/* Set ena and run to non-zero */
> > > +	count->ena = count->run = 1;
> > > +	count->lost = 0;
> > > +
> > > +	if (!found) {
> > > +		/*
> > > +		 * Set default value or 0 when retire_latency for this event is
> > > +		 * not found from sampling data (enable_tpebs_recording not
> > set
> > > +		 * or 0 sample recorded).
> > > +		 */
> > > +		val = 0;
> > 
> > Shouldn't it set count->val to 0?
> 
> Yes, I will fix this one.
> 
> Thanks,
> Weilin
> > 
> > Thanks,
> > Namhyung
> > 
> > 
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Only set retire_latency value to the first CPU and thread.
> > > +	 */
> > > +	if (cpu_map_idx == 0 && thread == 0)
> > > +		val = rint(t->val);
> > > +	else
> > > +		val = 0;
> > > +
> > > +	count->val = val;
> > > +	return 0;
> > > +}
> > > +
> > > +static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> > > +{
> > > +	zfree(&r->name);
> > > +	zfree(&r->tpebs_name);
> > > +	free(r);
> > > +}
> > > +
> > > +void tpebs_delete(void)
> > > +{
> > > +	struct tpebs_retire_lat *r, *rtmp;
> > > +
> > > +	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
> > > +		list_del_init(&r->nd);
> > > +		tpebs_retire_lat__delete(r);
> > > +	}
> > > +
> > > +	if (tpebs_cmd) {
> > > +		free(tpebs_cmd);
> > > +		tpebs_cmd = NULL;
> > > +	}
> > > +}
> > > +
> > > +int tpebs_stop_delete(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (tpebs_pid == -1)
> > > +		return 0;
> > > +
> > > +	ret = tpebs_stop();
> > > +	tpebs_delete();
> > > +	return ret;
> > > +}
> > > diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> > > new file mode 100644
> > > index 000000000000..73c1e5219522
> > > --- /dev/null
> > > +++ b/tools/perf/util/intel-tpebs.h
> > > @@ -0,0 +1,48 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * intel_tpebs.h: Intel TEPBS support
> > > + */
> > > +#ifndef INCLUDE__PERF_INTEL_TPEBS_H__
> > > +#define INCLUDE__PERF_INTEL_TPEBS_H__
> > > +
> > > +#include "stat.h"
> > > +#include "evsel.h"
> > > +
> > > +#ifdef HAVE_ARCH_X86_64_SUPPORT
> > > +
> > > +extern bool tpebs_recording;
> > > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus);
> > > +int tpebs_stop(void);
> > > +void tpebs_delete(void);
> > > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
> > > +int tpebs_stop_delete(void);
> > > +
> > > +#else
> > > +
> > > +static inline int tpebs_start(struct evlist *evsel_list __maybe_unused,
> > > +				struct perf_cpu_map *cpus
> > __maybe_unused)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline int tpebs_stop(void)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void tpebs_delete(void) {};
> > > +
> > > +static inline int tpebs_set_evsel(struct evsel *evsel  __maybe_unused,
> > > +				int cpu_map_idx  __maybe_unused,
> > > +				int thread  __maybe_unused)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline int tpebs_stop_delete(void)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +#endif
> > > +#endif
> > > --
> > > 2.43.0
> > >

> -----Original Message-----
> From: Namhyung Kim <namhyung@kernel.org>
> Sent: Friday, June 7, 2024 12:20 PM
> To: Wang, Weilin <weilin.wang@intel.com>
> Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> <mingo@redhat.com>; Alexander Shishkin
> <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> Caleb <caleb.biggers@intel.com>
> Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> perf stat needs to get retire latency value for a metric.
> 
> On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Namhyung Kim <namhyung@kernel.org>
> > > Sent: Thursday, June 6, 2024 4:21 PM
> > > To: Wang, Weilin <weilin.wang@intel.com>
> > > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > > <mingo@redhat.com>; Alexander Shishkin
> > > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor,
> Perry
> > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>;
> Biggers,
> > > Caleb <caleb.biggers@intel.com>
> > > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record
> when
> > > perf stat needs to get retire latency value for a metric.
> > >
> > > Hello,
> > >
> > > On Wed, Jun 05, 2024 at 01:21:44AM -0400, weilin.wang@intel.com
> wrote:
> > > > From: Weilin Wang <weilin.wang@intel.com>
> > > >
> > > > When retire_latency value is used in a metric formula, evsel would fork a
> perf
> > > > record process with "-e" and "-W" options. Perf record will collect
> required
> > > > retire_latency values in parallel while perf stat is collecting counting
> values.
> > > >
> > > > At the point of time that perf stat stops counting, evsel would stop perf
> > > record
> > > > by sending sigterm signal to perf record process. Sampled data will be
> > > process
> > > > to get retire latency value. Another thread is required to synchronize
> > > between
> > > > perf stat and perf record when we pass data through pipe.
> > > >
> > > > Retire_latency evsel is not opened for perf stat so that there is no counter
> > > > wasted on it. This commit includes code suggested by Namhyung to
> adjust
> > > reading
> > > > size for groups that include retire_latency evsels.
> > > >
> > > > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> > > > ---
> > > >  tools/perf/builtin-stat.c     |   6 +
> > > >  tools/perf/util/Build         |   1 +
> > > >  tools/perf/util/evsel.c       |  66 +++++-
> > > >  tools/perf/util/intel-tpebs.c | 397
> > > ++++++++++++++++++++++++++++++++++
> > > >  tools/perf/util/intel-tpebs.h |  48 ++++
> > > >  5 files changed, 516 insertions(+), 2 deletions(-)
> > > >  create mode 100644 tools/perf/util/intel-tpebs.c
> > > >  create mode 100644 tools/perf/util/intel-tpebs.h
> > > >
> > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > > index 428e9721b908..b09cb2c6e9c2 100644
> > > > --- a/tools/perf/builtin-stat.c
> > > > +++ b/tools/perf/builtin-stat.c
> > > > @@ -70,6 +70,7 @@
> > > >  #include "util/bpf_counter.h"
> > > >  #include "util/iostat.h"
> > > >  #include "util/util.h"
> > > > +#include "util/intel-tpebs.h"
> > > >  #include "asm/bug.h"
> > > >
> > > >  #include <linux/time64.h>
> > > > @@ -653,6 +654,9 @@ static enum counter_recovery
> > > stat_handle_error(struct evsel *counter)
> > > >
> > > >  	if (child_pid != -1)
> > > >  		kill(child_pid, SIGTERM);
> > > > +
> > > > +	tpebs_stop_delete();
> > > > +
> > > >  	return COUNTER_FATAL;
> > > >  }
> > > >
> > > > @@ -985,6 +989,8 @@ static void sig_atexit(void)
> > > >  	if (child_pid != -1)
> > > >  		kill(child_pid, SIGTERM);
> > > >
> > > > +	tpebs_stop();
> > > > +
> > > >  	sigprocmask(SIG_SETMASK, &oset, NULL);
> > > >
> > > >  	if (signr == -1)
> > > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > > > index 292170a99ab6..79adf39e0d8f 100644
> > > > --- a/tools/perf/util/Build
> > > > +++ b/tools/perf/util/Build
> > > > @@ -153,6 +153,7 @@ perf-y += clockid.o
> > > >  perf-y += list_sort.o
> > > >  perf-y += mutex.o
> > > >  perf-y += sharded_mutex.o
> > > > +perf-$(CONFIG_X86_64) += intel-tpebs.o
> > > >
> > > >  perf-$(CONFIG_LIBBPF) += bpf_map.o
> > > >  perf-$(CONFIG_PERF_BPF_SKEL) += bpf_counter.o
> > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > > index a0a8aee7d6b9..bd3627819afe 100644
> > > > --- a/tools/perf/util/evsel.c
> > > > +++ b/tools/perf/util/evsel.c
> > > > @@ -1538,6 +1538,60 @@ static void evsel__set_count(struct evsel
> > > *counter, int cpu_map_idx, int thread,
> > > >  	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread,
> > > true);
> > > >  }
> > > >
> > > > +static bool evsel__group_has_tpebs(struct evsel *leader)
> > > > +{
> > > > +	struct evsel *evsel;
> > > > +
> > > > +	for_each_group_evsel(evsel, leader) {
> > > > +		if (evsel__is_retire_lat(evsel))
> > > > +			return true;
> > > > +	}
> > > > +	return false;
> > > > +}
> > > > +
> > > > +static u64 evsel__group_read_nr_members(struct evsel *leader)
> > > > +{
> > > > +	u64 nr = leader->core.nr_members;
> > > > +	struct evsel *evsel;
> > > > +
> > > > +	for_each_group_evsel(evsel, leader) {
> > > > +		if (evsel__is_retire_lat(evsel))
> > > > +			nr--;
> > > > +	}
> > > > +	return nr;
> > > > +}
> > > > +
> > > > +static u64 evsel__group_read_size(struct evsel *leader)
> > > > +{
> > > > +	u64 read_format = leader->core.attr.read_format;
> > > > +	int entry = sizeof(u64); /* value */
> > > > +	int size = 0;
> > > > +	int nr = 1;
> > > > +
> > > > +	if (!evsel__group_has_tpebs(leader))
> > > > +		return perf_evsel__read_size(&leader->core);
> > > > +
> > > > +	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > > > +		size += sizeof(u64);
> > > > +
> > > > +	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> > > > +		size += sizeof(u64);
> > > > +
> > > > +	if (read_format & PERF_FORMAT_ID)
> > > > +		entry += sizeof(u64);
> > > > +
> > > > +	if (read_format & PERF_FORMAT_LOST)
> > > > +		entry += sizeof(u64);
> > > > +
> > > > +	if (read_format & PERF_FORMAT_GROUP) {
> > > > +		nr = evsel__group_read_nr_members(leader);
> > > > +		size += sizeof(u64);
> > > > +	}
> > > > +
> > > > +	size += entry * nr;
> > > > +	return size;
> > > > +}
> > > > +
> > > >  static int evsel__process_group_data(struct evsel *leader, int
> cpu_map_idx,
> > > int thread, u64 *data)
> > > >  {
> > > >  	u64 read_format = leader->core.attr.read_format;
> > > > @@ -1546,7 +1600,7 @@ static int evsel__process_group_data(struct
> evsel
> > > *leader, int cpu_map_idx, int
> > > >
> > > >  	nr = *data++;
> > > >
> > > > -	if (nr != (u64) leader->core.nr_members)
> > > > +	if (nr != evsel__group_read_nr_members(leader))
> > > >  		return -EINVAL;
> > > >
> > > >  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > > > @@ -1576,7 +1630,7 @@ static int evsel__read_group(struct evsel
> *leader,
> > > int cpu_map_idx, int thread)
> > > >  {
> > > >  	struct perf_stat_evsel *ps = leader->stats;
> > > >  	u64 read_format = leader->core.attr.read_format;
> > > > -	int size = perf_evsel__read_size(&leader->core);
> > > > +	int size = evsel__group_read_size(leader);
> > > >  	u64 *data = ps->group_data;
> > > >
> > > >  	if (!(read_format & PERF_FORMAT_ID))
> > > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel,
> > > struct perf_cpu_map *cpus,
> > > >  		return 0;
> > > >  	}
> > > >
> > > > +	if (evsel__is_retire_lat(evsel))
> > > > +		return tpebs_start(evsel->evlist, cpus);
> > >
> > > As it works with evlist, I think it's better to put this code there.
> > > But it seems perf stat doesn't call the evlist API for open, then we
> > > can add this to somewhere in __run_perf_stat() directly.
> > >
> > > > +
> > > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > > >  	if (err)
> > > >  		return err;
> > > > @@ -2376,6 +2433,8 @@ 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();
> > >
> > > Ditto.
> >
> > Hi Namhyung,
> >
> > I hope both this and the one above on open could stay in evsel level because
> > these are operations on retire_latency evsel.
> 
> Then I think you need to remove the specific evsel not the all tpebs
> events.
> 
> > At the same time, a lot of the
> > previous several versions of work was to move TPEBS code out from perf
> stat to
> > evsel to make it more generic. I think move these back to __run_perf_stat()
> are
> > opposite to that goal.
> 
> Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
> to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
> in evlist__close() but we don't use evlist__open() for perf stat so it's
> not symmetric. :(
> 
> Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
> think it's natural that they are called for the whole list, not for an
> event/evsel.

I think we're trying to work at evsel level and open(remove) or close one 
retire_latency evsel at a time. In addition to that, we put all the required retire_latency
together in one perf record launch in order to reduce overhead to fork multiple perf 
record. I hope this makes sense. 

> 
> >
> >
> > >
> > > >  	perf_evsel__close(&evsel->core);
> > > >  	perf_evsel__free_id(&evsel->core);
> > > >  }
> > > > @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel,
> > > struct evlist *evlist)
> > > >  {
> > > >  	int cpu_map_idx, thread;
> > > >
> > > > +	if (evsel__is_retire_lat(evsel))
> > > > +		return 0;
> > > > +
> > > >  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd);
> > > cpu_map_idx++) {
> > > >  		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
> > > >  		     thread++) {
> > > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > > new file mode 100644
> > > > index 000000000000..37b7a4f92dd9
> > > > --- /dev/null
> > > > +++ b/tools/perf/util/intel-tpebs.c
> > > > @@ -0,0 +1,397 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * intel_tpebs.c: Intel TPEBS support
> > > > + */
> > > > +
> > > > +
> > > > +#include <sys/param.h>
> > > > +#include <subcmd/run-command.h>
> > > > +#include <thread.h>
> > > > +#include "intel-tpebs.h"
> > > > +#include <linux/list.h>
> > > > +#include <linux/zalloc.h>
> > > > +#include <linux/err.h>
> > > > +#include "sample.h"
> > > > +#include "debug.h"
> > > > +#include "evlist.h"
> > > > +#include "evsel.h"
> > > > +#include "session.h"
> > > > +#include "tool.h"
> > > > +#include "cpumap.h"
> > > > +#include "metricgroup.h"
> > > > +#include <sys/stat.h>
> > > > +#include <sys/file.h>
> > > > +#include <poll.h>
> > > > +#include <math.h>
> > > > +
> > > > +#define PERF_DATA		"-"
> > > > +
> > > > +bool tpebs_recording;
> > > > +static pid_t tpebs_pid = -1;
> > > > +static size_t tpebs_event_size;
> > > > +static pthread_t tpebs_reader_thread;
> > > > +static struct child_process *tpebs_cmd;
> > > > +static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);
> > >
> > > It can be 'static LIST_HEAD(tpebs_results);'
> > >
> > > > +
> > > > +struct tpebs_retire_lat {
> > > > +	struct list_head nd;
> > > > +	/* Event name */
> > > > +	const char *name;
> > > > +	/* Event name with the TPEBS modifier R */
> > > > +	const char *tpebs_name;
> > > > +	/* Count of retire_latency values found in sample data */
> > > > +	size_t count;
> > > > +	/* Sum of all the retire_latency values in sample data */
> > > > +	int sum;
> > > > +	/* Average of retire_latency, val = sum / count */
> > > > +	double val;
> > > > +};
> > > > +
> > > > +static int get_perf_record_args(const char **record_argv, char buf[],
> > > > +				const char *cpumap_buf)
> > > > +{
> > > > +	struct tpebs_retire_lat *e;
> > > > +	int i = 0;
> > > > +
> > > > +	pr_debug("Prepare perf record for retire_latency\n");
> > > > +
> > > > +	record_argv[i++] = "perf";
> > > > +	record_argv[i++] = "record";
> > > > +	record_argv[i++] = "-W";
> > > > +	record_argv[i++] = "--synth=no";
> > > > +	record_argv[i++] = buf;
> > > > +
> > > > +	if (cpumap_buf) {
> > > > +		record_argv[i++] = "-C";
> > > > +		record_argv[i++] = cpumap_buf;
> > > > +	}
> > > > +
> > > > +	record_argv[i++] = "-a";
> > > > +
> > > > +	if (!cpumap_buf) {
> > > > +		pr_err("Require cpumap list to run sampling.\n");
> > > > +		return -ECANCELED;
> > > > +	}
> > >
> > > Hmm.. I thought you supported system wide collection, not sure if it has
> > > a cpumap.  Anyway this check makes the earlier one meaningless - you
> > > need the cpumap always, right?
> >
> > TPEBS should be work with "-a" or "-C". I'm not sure what the cpumap
> would be
> > for "-a". Would it make sense to add "-a" only when cpumap_buf is NULL?
> 
> I think the best way is to check target__has_cpu().
Yes this is an ideal way to check. But since the tpebs_start() is called in evsel, I'm
wondering do we still have target here?

> 
> >
> > >
> > > > +
> > > > +	list_for_each_entry(e, &tpebs_results, nd) {
> > > > +		record_argv[i++] = "-e";
> > > > +		record_argv[i++] = e->name;
> > > > +	}
> > > > +
> > > > +	record_argv[i++] = "-o";
> > > > +	record_argv[i++] = PERF_DATA;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int prepare_run_command(const char **argv)
> > > > +{
> > > > +	tpebs_cmd = zalloc(sizeof(struct child_process));
> > > > +	if (!tpebs_cmd)
> > > > +		return -ENOMEM;
> > > > +	tpebs_cmd->argv = argv;
> > > > +	tpebs_cmd->out = -1;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int start_perf_record(int control_fd[], int ack_fd[],
> > > > +				const char *cpumap_buf)
> > > > +{
> > > > +	const char **record_argv;
> > > > +	int ret;
> > > > +	char buf[32];
> > > > +
> > > > +	scnprintf(buf, sizeof(buf), "--control=fd:%d,%d", control_fd[0],
> > > ack_fd[1]);
> > > > +
> > > > +	record_argv = calloc(12 + 2 * tpebs_event_size, sizeof(char *));
> > > > +	if (!record_argv)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = get_perf_record_args(record_argv, buf, cpumap_buf);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	ret = prepare_run_command(record_argv);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +	ret = start_command(tpebs_cmd);
> > > > +out:
> > > > +	free(record_argv);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int process_sample_event(struct perf_tool *tool
> __maybe_unused,
> > > > +				union perf_event *event __maybe_unused,
> > > > +				struct perf_sample *sample,
> > > > +				struct evsel *evsel,
> > > > +				struct machine *machine __maybe_unused)
> > > > +{
> > > > +	int ret = 0;
> > > > +	const char *evname;
> > > > +	struct tpebs_retire_lat *t;
> > > > +
> > > > +	evname = evsel__name(evsel);
> > > > +
> > > > +	/*
> > > > +	 * 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;
> > > > +}
> > > > +
> > > > +static int process_feature_event(struct perf_session *session,
> > > > +				 union perf_event *event)
> > > > +{
> > > > +	if (event->feat.feat_id < HEADER_LAST_FEATURE)
> > > > +		return perf_event__process_feature(session, event);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void *__sample_reader(void *arg)
> > > > +{
> > > > +	struct child_process *child = arg;
> > > > +	struct perf_session *session;
> > > > +	struct perf_data data = {
> > > > +		.mode = PERF_DATA_MODE_READ,
> > > > +		.path = PERF_DATA,
> > > > +		.file.fd = child->out,
> > > > +	};
> > > > +	struct perf_tool tool = {
> > > > +		.sample = process_sample_event,
> > > > +		.feature = process_feature_event,
> > > > +		.attr = perf_event__process_attr,
> > > > +	};
> > > > +
> > > > +	session = perf_session__new(&data, &tool);
> > > > +	if (IS_ERR(session))
> > > > +		return NULL;
> > > > +	perf_session__process_events(session);
> > > > +	perf_session__delete(session);
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +
> > > > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus)
> > > > +{
> > > > +	int ret = 0;
> > > > +	struct evsel *evsel;
> > > > +	char cpumap_buf[50];
> > > > +
> > > > +	/*
> > > > +	 * We should only run tpebs_start when tpebs_recording is enabled.
> > > > +	 * And we should only run it once with all the required events.
> > > > +	 */
> > > > +	if (tpebs_pid != -1 || !tpebs_recording)
> > > > +		return 0;
> > > > +
> > > > +	cpu_map__snprint(cpus, cpumap_buf, sizeof(cpumap_buf));
> > > > +	pr_debug("cpu map: %s\n", cpumap_buf);
> > >
> > > Can you please remove unnecessary debug prints?  If you really want it,
> > > then make it more meaningful like with more context.
> > >
> > > > +
> > > > +	/*
> > > > +	 * Prepare perf record for sampling event retire_latency before fork
> > > and
> > > > +	 * prepare workload
> > > > +	 */
> > > > +	evlist__for_each_entry(evsel_list, evsel) {
> > > > +		struct tpebs_retire_lat *new = zalloc(sizeof(*new));
> > > > +		char *name;
> > > > +		int i;
> > > > +
> > > > +		if (!evsel->retire_lat)
> > > > +			continue;
> > >
> > > Please move the allocation after this.
> > >
> > > > +
> > > > +		pr_debug("perf stat retire latency of event %s required\n",
> > > evsel->name);
> > > > +		if (!new) {
> > > > +			ret = -1;
> > > > +			goto err;
> > > > +		}
> > > > +		for (i = strlen(evsel->name) - 1; i > 0; i--) {
> > > > +			if (evsel->name[i] == 'R')
> > > > +				break;
> > >
> > > I think you also need to check ':' or '/'..
> >
> > I think the ':' and '/' have already been checked by the evsel parser to make
> > this evsel a retire_latency evsel. Do we need to check them again here?
> 
> Ok, it's fine if it's checked already.  Then I thought you could just
> use strrchr() but it seems you want the index of 'R' to modify the
> copied string.

Yes, I'm trying to get the index and replace it with 'p' for sampling.

> 
> >
> > >
> > > > +		}
> > > > +		if (i <= 0 || evsel->name[i] != 'R') {
> > > > +			ret = -1;
> > > > +			goto err;
> > > > +		}
> > > > +
> > > > +		name = strdup(evsel->name);
> > > > +		if (!name) {
> > > > +			ret = -ENOMEM;
> > > > +			goto err;
> > > > +		}
> > > > +		name[i] = 'p';
> > > > +		new->name = name;
> > > > +		new->tpebs_name = strdup(evsel->name);
> 
> Is this just to find the original evsel later?  Then I think you can
> compare the pointer directly instead of name.
> 
> 
> > > > +		if (!new->tpebs_name) {
> > > > +			ret = -ENOMEM;
> > > > +			goto err;
> > >
> > > This error handling (including above) will leak 'new' because it's not
> > > linked to the list yet.
> >
> > Yes, you are right. I will fix this.
> >
> > >
> > > > +		}
> > > > +		list_add_tail(&new->nd, &tpebs_results);
> > > > +		tpebs_event_size += 1;
> > > > +	}
> > > > +
> > > > +	if (tpebs_event_size > 0) {
> > > > +		int control_fd[2], ack_fd[2], len;
> > > > +		char ack_buf[8];
> > > > +
> > > > +		/*Create control and ack fd for --control*/
> > > > +		if (pipe(control_fd) < 0) {
> > > > +			pr_err("Failed to create control fifo");
> > > > +			ret = -1;
> > > > +			goto out;
> > > > +		}
> > > > +		if (pipe(ack_fd) < 0) {
> > > > +			pr_err("Failed to create control fifo");
> > > > +			ret = -1;
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		ret = start_perf_record(control_fd, ack_fd, cpumap_buf);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +		tpebs_pid = tpebs_cmd->pid;
> > > > +		if (pthread_create(&tpebs_reader_thread, NULL,
> > > __sample_reader, tpebs_cmd)) {
> > > > +			kill(tpebs_cmd->pid, SIGTERM);
> > > > +			close(tpebs_cmd->out);
> > > > +			pr_err("Could not create thread to process sample
> > > data.\n");
> > > > +			ret = -1;
> > > > +			goto out;
> > > > +		}
> > > > +		/* Wait for perf record initialization.*/
> > > > +		len = strlen("enable");
> > > > +		ret = write(control_fd[1], "enable", len);
> > > > +		if (ret != len) {
> > > > +			pr_err("perf record control write control message
> > > failed\n");
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		ret = read(ack_fd[0], ack_buf, sizeof(ack_buf));
> > > > +		if (ret > 0)
> > > > +			ret = strcmp(ack_buf, "ack\n");
> > > > +		else {
> > > > +			pr_err("perf record control ack failed\n");
> > > > +			goto out;
> > > > +		}
> > > > +		pr_debug("Received ack from perf record\n");
> > > > +out:
> > > > +		close(control_fd[0]);
> > > > +		close(control_fd[1]);
> > > > +		close(ack_fd[0]);
> > > > +		close(ack_fd[1]);
> > > > +	}
> > > > +err:
> > > > +	if (ret)
> > > > +		tpebs_delete();
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +int tpebs_stop(void)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	/* Like tpebs_start, we should only run tpebs_end once. */
> > > > +	if (tpebs_pid != -1) {
> > > > +		kill(tpebs_cmd->pid, SIGTERM);
> > > > +		tpebs_pid = -1;
> > > > +		pthread_join(tpebs_reader_thread, NULL);
> > > > +		close(tpebs_cmd->out);
> > > > +		ret = finish_command(tpebs_cmd);
> > > > +		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> > > > +			ret = 0;
> > > > +	}
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread)
> > > > +{
> > > > +	struct perf_counts_values *count;
> > > > +	struct tpebs_retire_lat *t;
> > > > +	bool found = false;
> > > > +	__u64 val;
> > > > +	int ret;
> > > > +
> > > > +	/* Non reitre_latency evsel should never enter this function. */
> > > > +	if (!evsel__is_retire_lat(evsel))
> > > > +		return -1;
> > > > +
> > > > +	ret = tpebs_stop();
> > >
> > > I think it's better to call this in the upper layer.
> >
> > The tpebs_stop() is also called in sig_atexit() in perf stat. I want to make this
> function
> > safe to be called multiple times and add one tpebs_stop() here in case
> tpebs_set_evsel()
> > is invoked before tpebs_stop(). I hope this makes sense.
> 
> I guess it's ok to call tpebs_set_evsel() even if it's not stopped.  It
> seems you use tpebs_retire_lat->val only and it's an average so it won't
> change much if you read either before or after concurrent changes.
> 
> It can be useful to the interval mode if you keep the tpebs background
> process after read.
> 

Ok, I will remove the tpebs_stop() here if that works fine. 

Thanks,
Weilin


> Thanks,
> Namhyung
> 
> >
> > >
> > >
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	count = perf_counts(evsel->counts, cpu_map_idx, thread);
> > > > +
> > > > +	list_for_each_entry(t, &tpebs_results, nd) {
> > > > +		if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t-
> > > >tpebs_name, evsel->metric_id)) {
> > > > +			found = true;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* Set ena and run to non-zero */
> > > > +	count->ena = count->run = 1;
> > > > +	count->lost = 0;
> > > > +
> > > > +	if (!found) {
> > > > +		/*
> > > > +		 * Set default value or 0 when retire_latency for this event is
> > > > +		 * not found from sampling data (enable_tpebs_recording not
> > > set
> > > > +		 * or 0 sample recorded).
> > > > +		 */
> > > > +		val = 0;
> > >
> > > Shouldn't it set count->val to 0?
> >
> > Yes, I will fix this one.
> >
> > Thanks,
> > Weilin
> > >
> > > Thanks,
> > > Namhyung
> > >
> > >
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Only set retire_latency value to the first CPU and thread.
> > > > +	 */
> > > > +	if (cpu_map_idx == 0 && thread == 0)
> > > > +		val = rint(t->val);
> > > > +	else
> > > > +		val = 0;
> > > > +
> > > > +	count->val = val;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void tpebs_retire_lat__delete(struct tpebs_retire_lat *r)
> > > > +{
> > > > +	zfree(&r->name);
> > > > +	zfree(&r->tpebs_name);
> > > > +	free(r);
> > > > +}
> > > > +
> > > > +void tpebs_delete(void)
> > > > +{
> > > > +	struct tpebs_retire_lat *r, *rtmp;
> > > > +
> > > > +	list_for_each_entry_safe(r, rtmp, &tpebs_results, nd) {
> > > > +		list_del_init(&r->nd);
> > > > +		tpebs_retire_lat__delete(r);
> > > > +	}
> > > > +
> > > > +	if (tpebs_cmd) {
> > > > +		free(tpebs_cmd);
> > > > +		tpebs_cmd = NULL;
> > > > +	}
> > > > +}
> > > > +
> > > > +int tpebs_stop_delete(void)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (tpebs_pid == -1)
> > > > +		return 0;
> > > > +
> > > > +	ret = tpebs_stop();
> > > > +	tpebs_delete();
> > > > +	return ret;
> > > > +}
> > > > diff --git a/tools/perf/util/intel-tpebs.h b/tools/perf/util/intel-tpebs.h
> > > > new file mode 100644
> > > > index 000000000000..73c1e5219522
> > > > --- /dev/null
> > > > +++ b/tools/perf/util/intel-tpebs.h
> > > > @@ -0,0 +1,48 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * intel_tpebs.h: Intel TEPBS support
> > > > + */
> > > > +#ifndef INCLUDE__PERF_INTEL_TPEBS_H__
> > > > +#define INCLUDE__PERF_INTEL_TPEBS_H__
> > > > +
> > > > +#include "stat.h"
> > > > +#include "evsel.h"
> > > > +
> > > > +#ifdef HAVE_ARCH_X86_64_SUPPORT
> > > > +
> > > > +extern bool tpebs_recording;
> > > > +int tpebs_start(struct evlist *evsel_list, struct perf_cpu_map *cpus);
> > > > +int tpebs_stop(void);
> > > > +void tpebs_delete(void);
> > > > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread);
> > > > +int tpebs_stop_delete(void);
> > > > +
> > > > +#else
> > > > +
> > > > +static inline int tpebs_start(struct evlist *evsel_list __maybe_unused,
> > > > +				struct perf_cpu_map *cpus
> > > __maybe_unused)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static inline int tpebs_stop(void)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static inline void tpebs_delete(void) {};
> > > > +
> > > > +static inline int tpebs_set_evsel(struct evsel *evsel  __maybe_unused,
> > > > +				int cpu_map_idx  __maybe_unused,
> > > > +				int thread  __maybe_unused)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static inline int tpebs_stop_delete(void)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +#endif
> > > > +#endif
> > > > --
> > > > 2.43.0
> > > >

On Fri, Jun 07, 2024 at 08:45:13PM +0000, Wang, Weilin wrote:
> 
> 
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@kernel.org>
> > Sent: Friday, June 7, 2024 12:20 PM
> > To: Wang, Weilin <weilin.wang@intel.com>
> > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > <mingo@redhat.com>; Alexander Shishkin
> > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> > Caleb <caleb.biggers@intel.com>
> > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> > perf stat needs to get retire latency value for a metric.
> > 
> > On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
[SNIP]
> > > > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel *evsel,
> > > > struct perf_cpu_map *cpus,
> > > > >  		return 0;
> > > > >  	}
> > > > >
> > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > +		return tpebs_start(evsel->evlist, cpus);
> > > >
> > > > As it works with evlist, I think it's better to put this code there.
> > > > But it seems perf stat doesn't call the evlist API for open, then we
> > > > can add this to somewhere in __run_perf_stat() directly.
> > > >
> > > > > +
> > > > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > > > >  	if (err)
> > > > >  		return err;
> > > > > @@ -2376,6 +2433,8 @@ 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();
> > > >
> > > > Ditto.
> > >
> > > Hi Namhyung,
> > >
> > > I hope both this and the one above on open could stay in evsel level because
> > > these are operations on retire_latency evsel.
> > 
> > Then I think you need to remove the specific evsel not the all tpebs
> > events.
> > 
> > > At the same time, a lot of the
> > > previous several versions of work was to move TPEBS code out from perf
> > stat to
> > > evsel to make it more generic. I think move these back to __run_perf_stat()
> > are
> > > opposite to that goal.
> > 
> > Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
> > to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
> > in evlist__close() but we don't use evlist__open() for perf stat so it's
> > not symmetric. :(
> > 
> > Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
> > think it's natural that they are called for the whole list, not for an
> > event/evsel.
> 
> I think we're trying to work at evsel level and open(remove) or close one 
> retire_latency evsel at a time. In addition to that, we put all the required retire_latency
> together in one perf record launch in order to reduce overhead to fork multiple perf 
> record. I hope this makes sense. 

Well.. I think we can do something like this in the current code.

__run_perf_stat():
  ...

  tpebs__start(evlist, target);

  evlist__for_each_cpu(...) {
      if (create_perf_steat_counter() < 0) {
          ....

instead of doing it in the evsel__open().  What's the issue with this
approach?

> 
> > 
> > >
> > >
> > > >
> > > > >  	perf_evsel__close(&evsel->core);
> > > > >  	perf_evsel__free_id(&evsel->core);
> > > > >  }
> > > > > @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel *evsel,
> > > > struct evlist *evlist)
> > > > >  {
> > > > >  	int cpu_map_idx, thread;
> > > > >
> > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > +		return 0;
> > > > > +
> > > > >  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel->core.fd);
> > > > cpu_map_idx++) {
> > > > >  		for (thread = 0; thread < xyarray__max_y(evsel->core.fd);
> > > > >  		     thread++) {
> > > > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > > > new file mode 100644
> > > > > index 000000000000..37b7a4f92dd9
> > > > > --- /dev/null
> > > > > +++ b/tools/perf/util/intel-tpebs.c
> > > > > @@ -0,0 +1,397 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * intel_tpebs.c: Intel TPEBS support
> > > > > + */
> > > > > +
> > > > > +
> > > > > +#include <sys/param.h>
> > > > > +#include <subcmd/run-command.h>
> > > > > +#include <thread.h>
> > > > > +#include "intel-tpebs.h"
> > > > > +#include <linux/list.h>
> > > > > +#include <linux/zalloc.h>
> > > > > +#include <linux/err.h>
> > > > > +#include "sample.h"
> > > > > +#include "debug.h"
> > > > > +#include "evlist.h"
> > > > > +#include "evsel.h"
> > > > > +#include "session.h"
> > > > > +#include "tool.h"
> > > > > +#include "cpumap.h"
> > > > > +#include "metricgroup.h"
> > > > > +#include <sys/stat.h>
> > > > > +#include <sys/file.h>
> > > > > +#include <poll.h>
> > > > > +#include <math.h>
> > > > > +
> > > > > +#define PERF_DATA		"-"
> > > > > +
> > > > > +bool tpebs_recording;
> > > > > +static pid_t tpebs_pid = -1;
> > > > > +static size_t tpebs_event_size;
> > > > > +static pthread_t tpebs_reader_thread;
> > > > > +static struct child_process *tpebs_cmd;
> > > > > +static struct list_head tpebs_results = LIST_HEAD_INIT(tpebs_results);
> > > >
> > > > It can be 'static LIST_HEAD(tpebs_results);'
> > > >
> > > > > +
> > > > > +struct tpebs_retire_lat {
> > > > > +	struct list_head nd;
> > > > > +	/* Event name */
> > > > > +	const char *name;
> > > > > +	/* Event name with the TPEBS modifier R */
> > > > > +	const char *tpebs_name;
> > > > > +	/* Count of retire_latency values found in sample data */
> > > > > +	size_t count;
> > > > > +	/* Sum of all the retire_latency values in sample data */
> > > > > +	int sum;
> > > > > +	/* Average of retire_latency, val = sum / count */
> > > > > +	double val;
> > > > > +};
> > > > > +
> > > > > +static int get_perf_record_args(const char **record_argv, char buf[],
> > > > > +				const char *cpumap_buf)
> > > > > +{
> > > > > +	struct tpebs_retire_lat *e;
> > > > > +	int i = 0;
> > > > > +
> > > > > +	pr_debug("Prepare perf record for retire_latency\n");
> > > > > +
> > > > > +	record_argv[i++] = "perf";
> > > > > +	record_argv[i++] = "record";
> > > > > +	record_argv[i++] = "-W";
> > > > > +	record_argv[i++] = "--synth=no";
> > > > > +	record_argv[i++] = buf;
> > > > > +
> > > > > +	if (cpumap_buf) {
> > > > > +		record_argv[i++] = "-C";
> > > > > +		record_argv[i++] = cpumap_buf;
> > > > > +	}
> > > > > +
> > > > > +	record_argv[i++] = "-a";
> > > > > +
> > > > > +	if (!cpumap_buf) {
> > > > > +		pr_err("Require cpumap list to run sampling.\n");
> > > > > +		return -ECANCELED;
> > > > > +	}
> > > >
> > > > Hmm.. I thought you supported system wide collection, not sure if it has
> > > > a cpumap.  Anyway this check makes the earlier one meaningless - you
> > > > need the cpumap always, right?
> > >
> > > TPEBS should be work with "-a" or "-C". I'm not sure what the cpumap
> > would be
> > > for "-a". Would it make sense to add "-a" only when cpumap_buf is NULL?
> > 
> > I think the best way is to check target__has_cpu().
> Yes this is an ideal way to check. But since the tpebs_start() is called in evsel, I'm
> wondering do we still have target here?

Please see above.

Thanks,
Namhyung

> 
> > 
> > >
> > > >
> > > > > +
> > > > > +	list_for_each_entry(e, &tpebs_results, nd) {
> > > > > +		record_argv[i++] = "-e";
> > > > > +		record_argv[i++] = e->name;
> > > > > +	}
> > > > > +
> > > > > +	record_argv[i++] = "-o";
> > > > > +	record_argv[i++] = PERF_DATA;
> > > > > +
> > > > > +	return 0;
> > > > > +}

> -----Original Message-----
> From: Namhyung Kim <namhyung@kernel.org>
> Sent: Saturday, June 8, 2024 7:28 PM
> To: Wang, Weilin <weilin.wang@intel.com>
> Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> <mingo@redhat.com>; Alexander Shishkin
> <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> Caleb <caleb.biggers@intel.com>
> Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> perf stat needs to get retire latency value for a metric.
> 
> On Fri, Jun 07, 2024 at 08:45:13PM +0000, Wang, Weilin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Namhyung Kim <namhyung@kernel.org>
> > > Sent: Friday, June 7, 2024 12:20 PM
> > > To: Wang, Weilin <weilin.wang@intel.com>
> > > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > > <mingo@redhat.com>; Alexander Shishkin
> > > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor,
> Perry
> > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>;
> Biggers,
> > > Caleb <caleb.biggers@intel.com>
> > > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record
> when
> > > perf stat needs to get retire latency value for a metric.
> > >
> > > On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
> [SNIP]
> > > > > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel
> *evsel,
> > > > > struct perf_cpu_map *cpus,
> > > > > >  		return 0;
> > > > > >  	}
> > > > > >
> > > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > > +		return tpebs_start(evsel->evlist, cpus);
> > > > >
> > > > > As it works with evlist, I think it's better to put this code there.
> > > > > But it seems perf stat doesn't call the evlist API for open, then we
> > > > > can add this to somewhere in __run_perf_stat() directly.
> > > > >
> > > > > > +
> > > > > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > > > > >  	if (err)
> > > > > >  		return err;
> > > > > > @@ -2376,6 +2433,8 @@ 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();
> > > > >
> > > > > Ditto.
> > > >
> > > > Hi Namhyung,
> > > >
> > > > I hope both this and the one above on open could stay in evsel level
> because
> > > > these are operations on retire_latency evsel.
> > >
> > > Then I think you need to remove the specific evsel not the all tpebs
> > > events.
> > >
> > > > At the same time, a lot of the
> > > > previous several versions of work was to move TPEBS code out from perf
> > > stat to
> > > > evsel to make it more generic. I think move these back to
> __run_perf_stat()
> > > are
> > > > opposite to that goal.
> > >
> > > Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
> > > to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
> > > in evlist__close() but we don't use evlist__open() for perf stat so it's
> > > not symmetric. :(
> > >
> > > Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
> > > think it's natural that they are called for the whole list, not for an
> > > event/evsel.
> >
> > I think we're trying to work at evsel level and open(remove) or close one
> > retire_latency evsel at a time. In addition to that, we put all the required
> retire_latency
> > together in one perf record launch in order to reduce overhead to fork
> multiple perf
> > record. I hope this makes sense.
> 
> Well.. I think we can do something like this in the current code.
> 
> __run_perf_stat():
>   ...
> 
>   tpebs__start(evlist, target);
> 
>   evlist__for_each_cpu(...) {
>       if (create_perf_steat_counter() < 0) {
>           ....
> 
> instead of doing it in the evsel__open().  What's the issue with this
> approach?

This is basically how tpebs__start() was invoked in v9 (https://lore.kernel.org/all/CAM9d7ci7tgjR8LVNx+ZrFKMGo+OZn=eFSksPL56MeP_Q84PkMw@mail.gmail.com/)

I changed it in v10 so that it works at evsel level. 

Ian, could you please let me know what do you think about this? 

Thanks,
Weilin

> 
> >
> > >
> > > >
> > > >
> > > > >
> > > > > >  	perf_evsel__close(&evsel->core);
> > > > > >  	perf_evsel__free_id(&evsel->core);
> > > > > >  }
> > > > > > @@ -3341,6 +3400,9 @@ static int store_evsel_ids(struct evsel
> *evsel,
> > > > > struct evlist *evlist)
> > > > > >  {
> > > > > >  	int cpu_map_idx, thread;
> > > > > >
> > > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > > +		return 0;
> > > > > > +
> > > > > >  	for (cpu_map_idx = 0; cpu_map_idx < xyarray__max_x(evsel-
> >core.fd);
> > > > > cpu_map_idx++) {
> > > > > >  		for (thread = 0; thread < xyarray__max_y(evsel-
> >core.fd);
> > > > > >  		     thread++) {
> > > > > > diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..37b7a4f92dd9
> > > > > > --- /dev/null
> > > > > > +++ b/tools/perf/util/intel-tpebs.c
> > > > > > @@ -0,0 +1,397 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +/*
> > > > > > + * intel_tpebs.c: Intel TPEBS support
> > > > > > + */
> > > > > > +
> > > > > > +
> > > > > > +#include <sys/param.h>
> > > > > > +#include <subcmd/run-command.h>
> > > > > > +#include <thread.h>
> > > > > > +#include "intel-tpebs.h"
> > > > > > +#include <linux/list.h>
> > > > > > +#include <linux/zalloc.h>
> > > > > > +#include <linux/err.h>
> > > > > > +#include "sample.h"
> > > > > > +#include "debug.h"
> > > > > > +#include "evlist.h"
> > > > > > +#include "evsel.h"
> > > > > > +#include "session.h"
> > > > > > +#include "tool.h"
> > > > > > +#include "cpumap.h"
> > > > > > +#include "metricgroup.h"
> > > > > > +#include <sys/stat.h>
> > > > > > +#include <sys/file.h>
> > > > > > +#include <poll.h>
> > > > > > +#include <math.h>
> > > > > > +
> > > > > > +#define PERF_DATA		"-"
> > > > > > +
> > > > > > +bool tpebs_recording;
> > > > > > +static pid_t tpebs_pid = -1;
> > > > > > +static size_t tpebs_event_size;
> > > > > > +static pthread_t tpebs_reader_thread;
> > > > > > +static struct child_process *tpebs_cmd;
> > > > > > +static struct list_head tpebs_results =
> LIST_HEAD_INIT(tpebs_results);
> > > > >
> > > > > It can be 'static LIST_HEAD(tpebs_results);'
> > > > >
> > > > > > +
> > > > > > +struct tpebs_retire_lat {
> > > > > > +	struct list_head nd;
> > > > > > +	/* Event name */
> > > > > > +	const char *name;
> > > > > > +	/* Event name with the TPEBS modifier R */
> > > > > > +	const char *tpebs_name;
> > > > > > +	/* Count of retire_latency values found in sample data */
> > > > > > +	size_t count;
> > > > > > +	/* Sum of all the retire_latency values in sample data */
> > > > > > +	int sum;
> > > > > > +	/* Average of retire_latency, val = sum / count */
> > > > > > +	double val;
> > > > > > +};
> > > > > > +
> > > > > > +static int get_perf_record_args(const char **record_argv, char buf[],
> > > > > > +				const char *cpumap_buf)
> > > > > > +{
> > > > > > +	struct tpebs_retire_lat *e;
> > > > > > +	int i = 0;
> > > > > > +
> > > > > > +	pr_debug("Prepare perf record for retire_latency\n");
> > > > > > +
> > > > > > +	record_argv[i++] = "perf";
> > > > > > +	record_argv[i++] = "record";
> > > > > > +	record_argv[i++] = "-W";
> > > > > > +	record_argv[i++] = "--synth=no";
> > > > > > +	record_argv[i++] = buf;
> > > > > > +
> > > > > > +	if (cpumap_buf) {
> > > > > > +		record_argv[i++] = "-C";
> > > > > > +		record_argv[i++] = cpumap_buf;
> > > > > > +	}
> > > > > > +
> > > > > > +	record_argv[i++] = "-a";
> > > > > > +
> > > > > > +	if (!cpumap_buf) {
> > > > > > +		pr_err("Require cpumap list to run sampling.\n");
> > > > > > +		return -ECANCELED;
> > > > > > +	}
> > > > >
> > > > > Hmm.. I thought you supported system wide collection, not sure if it
> has
> > > > > a cpumap.  Anyway this check makes the earlier one meaningless - you
> > > > > need the cpumap always, right?
> > > >
> > > > TPEBS should be work with "-a" or "-C". I'm not sure what the cpumap
> > > would be
> > > > for "-a". Would it make sense to add "-a" only when cpumap_buf is
> NULL?
> > >
> > > I think the best way is to check target__has_cpu().
> > Yes this is an ideal way to check. But since the tpebs_start() is called in evsel,
> I'm
> > wondering do we still have target here?
> 
> Please see above.
> 
> Thanks,
> Namhyung
> 
> >
> > >
> > > >
> > > > >
> > > > > > +
> > > > > > +	list_for_each_entry(e, &tpebs_results, nd) {
> > > > > > +		record_argv[i++] = "-e";
> > > > > > +		record_argv[i++] = e->name;
> > > > > > +	}
> > > > > > +
> > > > > > +	record_argv[i++] = "-o";
> > > > > > +	record_argv[i++] = PERF_DATA;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}

On Sun, Jun 09, 2024 at 03:02:21AM +0000, Wang, Weilin wrote:
> 
> 
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@kernel.org>
> > Sent: Saturday, June 8, 2024 7:28 PM
> > To: Wang, Weilin <weilin.wang@intel.com>
> > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > <mingo@redhat.com>; Alexander Shishkin
> > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> > Caleb <caleb.biggers@intel.com>
> > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record when
> > perf stat needs to get retire latency value for a metric.
> > 
> > On Fri, Jun 07, 2024 at 08:45:13PM +0000, Wang, Weilin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Namhyung Kim <namhyung@kernel.org>
> > > > Sent: Friday, June 7, 2024 12:20 PM
> > > > To: Wang, Weilin <weilin.wang@intel.com>
> > > > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > > > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > > > <mingo@redhat.com>; Alexander Shishkin
> > > > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > > > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor,
> > Perry
> > > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>;
> > Biggers,
> > > > Caleb <caleb.biggers@intel.com>
> > > > Subject: Re: [RFC PATCH v11 3/8] perf stat: Fork and launch perf record
> > when
> > > > perf stat needs to get retire latency value for a metric.
> > > >
> > > > On Fri, Jun 07, 2024 at 01:07:12AM +0000, Wang, Weilin wrote:
> > [SNIP]
> > > > > > > @@ -2186,6 +2240,9 @@ static int evsel__open_cpu(struct evsel
> > *evsel,
> > > > > > struct perf_cpu_map *cpus,
> > > > > > >  		return 0;
> > > > > > >  	}
> > > > > > >
> > > > > > > +	if (evsel__is_retire_lat(evsel))
> > > > > > > +		return tpebs_start(evsel->evlist, cpus);
> > > > > >
> > > > > > As it works with evlist, I think it's better to put this code there.
> > > > > > But it seems perf stat doesn't call the evlist API for open, then we
> > > > > > can add this to somewhere in __run_perf_stat() directly.
> > > > > >
> > > > > > > +
> > > > > > >  	err = __evsel__prepare_open(evsel, cpus, threads);
> > > > > > >  	if (err)
> > > > > > >  		return err;
> > > > > > > @@ -2376,6 +2433,8 @@ 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();
> > > > > >
> > > > > > Ditto.
> > > > >
> > > > > Hi Namhyung,
> > > > >
> > > > > I hope both this and the one above on open could stay in evsel level
> > because
> > > > > these are operations on retire_latency evsel.
> > > >
> > > > Then I think you need to remove the specific evsel not the all tpebs
> > > > events.
> > > >
> > > > > At the same time, a lot of the
> > > > > previous several versions of work was to move TPEBS code out from perf
> > > > stat to
> > > > > evsel to make it more generic. I think move these back to
> > __run_perf_stat()
> > > > are
> > > > > opposite to that goal.
> > > >
> > > > Oh, I meant you can have the logic in utils/intel-tpebs.c but add a call
> > > > to tpebs_delete() in __run_perf_stat().  I think it'd better to keep it
> > > > in evlist__close() but we don't use evlist__open() for perf stat so it's
> > > > not symmetric. :(
> > > >
> > > > Anyway, all I want to say is that tpebs APIs work on evlist level.  So I
> > > > think it's natural that they are called for the whole list, not for an
> > > > event/evsel.
> > >
> > > I think we're trying to work at evsel level and open(remove) or close one
> > > retire_latency evsel at a time. In addition to that, we put all the required
> > retire_latency
> > > together in one perf record launch in order to reduce overhead to fork
> > multiple perf
> > > record. I hope this makes sense.
> > 
> > Well.. I think we can do something like this in the current code.
> > 
> > __run_perf_stat():
> >   ...
> > 
> >   tpebs__start(evlist, target);
> > 
> >   evlist__for_each_cpu(...) {
> >       if (create_perf_steat_counter() < 0) {
> >           ....
> > 
> > instead of doing it in the evsel__open().  What's the issue with this
> > approach?
> 
> This is basically how tpebs__start() was invoked in v9 (https://lore.kernel.org/all/CAM9d7ci7tgjR8LVNx+ZrFKMGo+OZn=eFSksPL56MeP_Q84PkMw@mail.gmail.com/)
> 
> I changed it in v10 so that it works at evsel level. 
> 
> Ian, could you please let me know what do you think about this? 

Ok, we sync-ed offline and agreed to have it in evsel level.  I still
think it's better to handle it in evlist level (at least for TPEBS) but
unfortunately we don't use evlist__open() consistently and there are
places it's not called.  Probably we need to convert the all call sites
to open evsel to be from evlist__open() then move tpebs__start() there.

Thanks,
Namhyung