Date: Fri, 7 Jun 2024 20:45:13 +0000
From: "Wang, Weilin" <weilin.wang@...el.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: Ian Rogers <irogers@...gle.com>, Arnaldo Carvalho de Melo
	<acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar
	<mingo@...hat.com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, "Hunter, Adrian" <adrian.hunter@...el.com>,
	"Kan Liang" <kan.liang@...ux.intel.com>, "linux-perf-users@...r.kernel.org"
	<linux-perf-users@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Taylor, Perry" <perry.taylor@...el.com>,
	"Alt, Samantha" <samantha.alt@...el.com>, "Biggers, Caleb"
	<caleb.biggers@...el.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.



> -----Original Message-----
> From: Namhyung Kim <namhyung@...nel.org>
> Sent: Friday, June 7, 2024 12:20 PM
> To: Wang, Weilin <weilin.wang@...el.com>
> Cc: Ian Rogers <irogers@...gle.com>; Arnaldo Carvalho de Melo
> <acme@...nel.org>; Peter Zijlstra <peterz@...radead.org>; Ingo Molnar
> <mingo@...hat.com>; Alexander Shishkin
> <alexander.shishkin@...ux.intel.com>; Jiri Olsa <jolsa@...nel.org>; Hunter,
> Adrian <adrian.hunter@...el.com>; Kan Liang <kan.liang@...ux.intel.com>;
> linux-perf-users@...r.kernel.org; linux-kernel@...r.kernel.org; Taylor, Perry
> <perry.taylor@...el.com>; Alt, Samantha <samantha.alt@...el.com>; Biggers,
> Caleb <caleb.biggers@...el.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@...nel.org>
> > > Sent: Thursday, June 6, 2024 4:21 PM
> > > To: Wang, Weilin <weilin.wang@...el.com>
> > > Cc: Ian Rogers <irogers@...gle.com>; Arnaldo Carvalho de Melo
> > > <acme@...nel.org>; Peter Zijlstra <peterz@...radead.org>; Ingo Molnar
> > > <mingo@...hat.com>; Alexander Shishkin
> > > <alexander.shishkin@...ux.intel.com>; Jiri Olsa <jolsa@...nel.org>; Hunter,
> > > Adrian <adrian.hunter@...el.com>; Kan Liang <kan.liang@...ux.intel.com>;
> > > linux-perf-users@...r.kernel.org; linux-kernel@...r.kernel.org; Taylor,
> Perry
> > > <perry.taylor@...el.com>; Alt, Samantha <samantha.alt@...el.com>;
> Biggers,
> > > Caleb <caleb.biggers@...el.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@...el.com
> wrote:
> > > > From: Weilin Wang <weilin.wang@...el.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@...el.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
> > > >

Powered by blists - more mailing lists