[PATCH 0/8] perf tools: Assorted fixes

[PATCH 0/8] perf tools: Assorted fixes

[Jiri Olsa]

hi,
sending assorted general fixes that queued
up in my other branches.

It contains all my previous posted single patches,
(except for the dir data patchset).

Also available in here:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/fixes

thanks,
jirka


---
Jiri Olsa (8):
      perf c2c: Fix c2c report for empty numa node
      perf tools: Add error path into hist_entry__init
      perf hist: Fix memory leak of srcline
      perf tools: Read and store caps/max_precise in perf_pmu
      perf tools: Get precise_ip from the pmu config
      perf tools: Probe for precise_ip with simple attr
      perf tools: Fix double free in perf_data__close
      perf tools: Force perf_data__open|close zero data->file.path

 tools/perf/builtin-c2c.c  |  8 ++++++--
 tools/perf/util/data.c    |  4 ++--
 tools/perf/util/evlist.c  | 25 ++++++++++++++++++++-----
 tools/perf/util/evsel.c   | 36 +++++++++++++++++++++++++++---------
 tools/perf/util/hist.c    | 51 ++++++++++++++++++++++++++++++---------------------
 tools/perf/util/pmu.c     | 14 ++++++++++++++
 tools/perf/util/pmu.h     |  1 +
 tools/perf/util/session.c |  4 +---
 tools/perf/util/setup.py  |  2 +-
 9 files changed, 102 insertions(+), 43 deletions(-)

[PATCH 1/8] perf c2c: Fix c2c report for empty numa node

[Jiri Olsa]

From: Jiri Olsa <jolsa@redhat.com>

Ravi Bangoria reported that we fail with empty
numa node with following message:

  $ lscpu
  NUMA node0 CPU(s):
  NUMA node1 CPU(s):   0-4

  $ sudo ./perf c2c report
  node/cpu topology bugFailed setup nodes

Fixing this by detecting empty node and keeping
its cpu set empty.

Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Link: http://lkml.kernel.org/n/tip-dyq5jo6pn1j3yqavb5ukjwwu@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-c2c.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 4272763a5e96..9e6cc868bdb4 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2056,6 +2056,12 @@ static int setup_nodes(struct perf_session *session)
 		if (!set)
 			return -ENOMEM;
 
+		nodes[node] = set;
+
+		/* empty node, skip */
+		if (cpu_map__empty(map))
+			continue;
+
 		for (cpu = 0; cpu < map->nr; cpu++) {
 			set_bit(map->map[cpu], set);
 
@@ -2064,8 +2070,6 @@ static int setup_nodes(struct perf_session *session)
 
 			cpu2node[map->map[cpu]] = node;
 		}
-
-		nodes[node] = set;
 	}
 
 	setup_nodes_header();
-- 
2.17.2

[PATCH 2/8] perf tools: Add error path into hist_entry__init

[Jiri Olsa]

Adding error path into hist_entry__init to unify error
handling, so every new member does not need to free
everything else.

Link: http://lkml.kernel.org/n/tip-v0ssws4hsr0tozb7lm0k5f70@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 669f961316f0..74e307d17c49 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -396,11 +396,8 @@ static int hist_entry__init(struct hist_entry *he,
 		 * adding new entries.  So we need to save a copy.
 		 */
 		he->branch_info = malloc(sizeof(*he->branch_info));
-		if (he->branch_info == NULL) {
-			map__zput(he->ms.map);
-			free(he->stat_acc);
-			return -ENOMEM;
-		}
+		if (he->branch_info == NULL)
+			goto err;
 
 		memcpy(he->branch_info, template->branch_info,
 		       sizeof(*he->branch_info));
@@ -419,21 +416,8 @@ static int hist_entry__init(struct hist_entry *he,
 
 	if (he->raw_data) {
 		he->raw_data = memdup(he->raw_data, he->raw_size);
-
-		if (he->raw_data == NULL) {
-			map__put(he->ms.map);
-			if (he->branch_info) {
-				map__put(he->branch_info->from.map);
-				map__put(he->branch_info->to.map);
-				free(he->branch_info);
-			}
-			if (he->mem_info) {
-				map__put(he->mem_info->iaddr.map);
-				map__put(he->mem_info->daddr.map);
-			}
-			free(he->stat_acc);
-			return -ENOMEM;
-		}
+		if (he->raw_data == NULL)
+			goto err_infos;
 	}
 	INIT_LIST_HEAD(&he->pairs.node);
 	thread__get(he->thread);
@@ -444,6 +428,21 @@ static int hist_entry__init(struct hist_entry *he,
 		he->leaf = true;
 
 	return 0;
+
+err_infos:
+	if (he->branch_info) {
+		map__put(he->branch_info->from.map);
+		map__put(he->branch_info->to.map);
+		free(he->branch_info);
+	}
+	if (he->mem_info) {
+		map__put(he->mem_info->iaddr.map);
+		map__put(he->mem_info->daddr.map);
+	}
+err:
+	map__zput(he->ms.map);
+	free(he->stat_acc);
+	return -ENOMEM;
 }
 
 static void *hist_entry__zalloc(size_t size)
-- 
2.17.2

[PATCH 3/8] perf hist: Fix memory leak of srcline

[Jiri Olsa]

From: Jiri Olsa <jolsa@redhat.com>

We can't allocate he->srcline unconditionaly, only when
new hist_entry is created. Moving he->srcline allocation
into hist_entry__init function.

Original-patch-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/n/tip-2ijaiiwlzmk4cae3zs97pq0x@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 74e307d17c49..f9eb95bf3938 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -419,6 +419,13 @@ static int hist_entry__init(struct hist_entry *he,
 		if (he->raw_data == NULL)
 			goto err_infos;
 	}
+
+	if (he->srcline) {
+		he->srcline = strdup(he->srcline);
+		if (he->srcline == NULL)
+			goto err_rawdata;
+	}
+
 	INIT_LIST_HEAD(&he->pairs.node);
 	thread__get(he->thread);
 	he->hroot_in  = RB_ROOT_CACHED;
@@ -429,6 +436,9 @@ static int hist_entry__init(struct hist_entry *he,
 
 	return 0;
 
+err_rawdata:
+	free(he->raw_data);
+
 err_infos:
 	if (he->branch_info) {
 		map__put(he->branch_info->from.map);
@@ -605,7 +615,7 @@ __hists__add_entry(struct hists *hists,
 			.map	= al->map,
 			.sym	= al->sym,
 		},
-		.srcline = al->srcline ? strdup(al->srcline) : NULL,
+		.srcline = (char *) al->srcline,
 		.socket	 = al->socket,
 		.cpu	 = al->cpu,
 		.cpumode = al->cpumode,
@@ -962,7 +972,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 			.map = al->map,
 			.sym = al->sym,
 		},
-		.srcline = al->srcline ? strdup(al->srcline) : NULL,
+		.srcline = (char *) al->srcline,
 		.parent = iter->parent,
 		.raw_data = sample->raw_data,
 		.raw_size = sample->raw_size,
-- 
2.17.2

[PATCH 4/8] perf tools: Read and store caps/max_precise in perf_pmu

[Jiri Olsa]

Read the caps/max_precise value and store it in struct
perf_pmu to be used when setting the maximum precise_ip
field in following patch.

Link: http://lkml.kernel.org/n/tip-y47h37fzs5t8hy9t0j5g9pyj@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/pmu.c | 14 ++++++++++++++
 tools/perf/util/pmu.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 51d437f55d18..6199a3174ab9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -752,6 +752,19 @@ perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
 	return NULL;
 }
 
+static int pmu_max_precise(const char *name)
+{
+	char path[PATH_MAX];
+	int max_precise = -1;
+
+	scnprintf(path, PATH_MAX,
+		 "bus/event_source/devices/%s/caps/max_precise",
+		 name);
+
+	sysfs__read_int(path, &max_precise);
+	return max_precise;
+}
+
 static struct perf_pmu *pmu_lookup(const char *name)
 {
 	struct perf_pmu *pmu;
@@ -784,6 +797,7 @@ static struct perf_pmu *pmu_lookup(const char *name)
 	pmu->name = strdup(name);
 	pmu->type = type;
 	pmu->is_uncore = pmu_is_uncore(name);
+	pmu->max_precise = pmu_max_precise(name);
 	pmu_add_cpu_aliases(&aliases, pmu);
 
 	INIT_LIST_HEAD(&pmu->format);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 47253c3daf55..bd9ec2704a57 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -26,6 +26,7 @@ struct perf_pmu {
 	__u32 type;
 	bool selectable;
 	bool is_uncore;
+	int max_precise;
 	struct perf_event_attr *default_config;
 	struct cpu_map *cpus;
 	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
-- 
2.17.2

[PATCH 5/8] perf tools: Get precise_ip from the pmu config

[Jiri Olsa]

Getting precise_ip field from the perf_pmu::max_precise
config read from sysfs. If it's not available falling
back to current detection function.

This fixes perf c2c usage of 'P' modifier on memory events,
where the standard detection fails, because of way too many
new perf_event_attr features added recently.

Use PYTHON_MOD to distinguish python perf module build
to exclude pmu.o object call being compiled in. There's
the flex/bison (and other) object dependencies, that
we'd need to teach setup.py about.

Link: http://lkml.kernel.org/n/tip-zyegnaq8xlkhh4sgfl4tnvui@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evsel.c  | 28 +++++++++++++++++++++++++++-
 tools/perf/util/setup.py |  2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dfe2958e6287..eec542bab815 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -870,6 +870,32 @@ static bool is_dummy_event(struct perf_evsel *evsel)
 	       (evsel->attr.config == PERF_COUNT_SW_DUMMY);
 }
 
+#ifndef PYTHON_MOD
+static int pmu_get_precise(const char *pmu_name)
+{
+	struct perf_pmu *pmu = pmu_name ? perf_pmu__find(pmu_name) : NULL;
+
+	/* If unsupported pmu->max_precise is -1. */
+	return pmu ? pmu->max_precise : -1;
+}
+#else
+static int pmu_get_precise(const char *pmu_name __maybe_unused)
+{
+	return 0;
+}
+#endif
+
+static void perf_evsel__set_max_precise_ip(struct perf_evsel *evsel)
+{
+	int max_precise = pmu_get_precise(evsel->pmu_name);
+	struct perf_event_attr *attr = &evsel->attr;
+
+	if (max_precise >= 0)
+		attr->precise_ip = max_precise;
+	else
+		perf_event_attr__set_max_precise_ip(attr);
+}
+
 /*
  * The enable_on_exec/disabled value strategy:
  *
@@ -1091,7 +1117,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	}
 
 	if (evsel->precise_max)
-		perf_event_attr__set_max_precise_ip(attr);
+		perf_evsel__set_max_precise_ip(evsel);
 
 	if (opts->all_user) {
 		attr->exclude_kernel = 1;
diff --git a/tools/perf/util/setup.py b/tools/perf/util/setup.py
index 5b5a167b43ce..9eee3467ac5d 100644
--- a/tools/perf/util/setup.py
+++ b/tools/perf/util/setup.py
@@ -37,7 +37,7 @@ class install_lib(_install_lib):
 
 cflags = getenv('CFLAGS', '').split()
 # switch off several checks (need to be at the end of cflags list)
-cflags += ['-fno-strict-aliasing', '-Wno-write-strings', '-Wno-unused-parameter', '-Wno-redundant-decls' ]
+cflags += ['-fno-strict-aliasing', '-Wno-write-strings', '-Wno-unused-parameter', '-Wno-redundant-decls', '-DPYTHON_MOD' ]
 if cc != "clang":
     cflags += ['-Wno-cast-function-type' ]
 
-- 
2.17.2

[PATCH 6/8] perf tools: Probe for precise_ip with simple attr

[Jiri Olsa]

Currently we probe for precise_ip with user specified
perf_event_attr, which might fail because of unsupported
kernel features, which would get disabled during the
open time anyway.

Switching the probe to take place on simple hw cycles,
so the following record sets proper precise_ip:

  # perf record -e cycles:P ls
  # perf evlist -v
  cycles:P: size: 112, ... precise_ip: 3, ...

Link: http://lkml.kernel.org/n/tip-rwncfxifbhnmd89yx0va5zg0@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evlist.c | 25 ++++++++++++++++++++-----
 tools/perf/util/evsel.c  |  8 --------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 08cedb643ea6..ed20f4379956 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -230,18 +230,33 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 	}
 }
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr)
+void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
 {
-	attr->precise_ip = 3;
+	struct perf_event_attr attr = {
+		.type		= PERF_TYPE_HARDWARE,
+		.config		= PERF_COUNT_HW_CPU_CYCLES,
+		.exclude_kernel	= 1,
+		.precise_ip	= 3,
+	};
 
-	while (attr->precise_ip != 0) {
-		int fd = sys_perf_event_open(attr, 0, -1, -1, 0);
+	event_attr_init(&attr);
+
+	/*
+	 * Unnamed union member, not supported as struct member named
+	 * initializer in older compilers such as gcc 4.4.7
+	 */
+	attr.sample_period = 1;
+
+	while (attr.precise_ip != 0) {
+		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
 		if (fd != -1) {
 			close(fd);
 			break;
 		}
-		--attr->precise_ip;
+		--attr.precise_ip;
 	}
+
+	pattr->precise_ip = attr.precise_ip;
 }
 
 int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index eec542bab815..2ef229e24b6f 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -294,20 +294,12 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 
 	if (!precise)
 		goto new_event;
-	/*
-	 * Unnamed union member, not supported as struct member named
-	 * initializer in older compilers such as gcc 4.4.7
-	 *
-	 * Just for probing the precise_ip:
-	 */
-	attr.sample_period = 1;
 
 	perf_event_attr__set_max_precise_ip(&attr);
 	/*
 	 * Now let the usual logic to set up the perf_event_attr defaults
 	 * to kick in when we return and before perf_evsel__open() is called.
 	 */
-	attr.sample_period = 0;
 new_event:
 	evsel = perf_evsel__new(&attr);
 	if (evsel == NULL)
-- 
2.17.2

[PATCH 7/8] perf tools: Fix double free in perf_data__close

[Jiri Olsa]

We can't call perf_data__close and subsequently
perf_session__delete, because it will call
perf_data__close again and cause double free
for data->file.path.

  $ perf report -i .
  incompatible file format (rerun with -v to learn more)
  free(): double free detected in tcache 2
  Aborted (core dumped)

In fact we don't need to call perf_data__close
at all, because at the time the got out_close
is reached, session->data is already initialized,
so the perf_data__close call will be triggered
from perf_session__delete.

Fixes: 2d4f27999b88 ("perf data: Add global path holder")
Link: http://lkml.kernel.org/n/tip-2g1zaoxmgmaxbuz8yvopq32v@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/session.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c764bbc91009..db643f3c2b95 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -140,7 +140,7 @@ struct perf_session *perf_session__new(struct perf_data *data,
 
 		if (perf_data__is_read(data)) {
 			if (perf_session__open(session) < 0)
-				goto out_close;
+				goto out_delete;
 
 			/*
 			 * set session attributes that are present in perf.data
@@ -181,8 +181,6 @@ struct perf_session *perf_session__new(struct perf_data *data,
 
 	return session;
 
- out_close:
-	perf_data__close(data);
  out_delete:
 	perf_session__delete(session);
  out:
-- 
2.17.2

[PATCH 8/8] perf tools: Force perf_data__open|close zero data->file.path

[Jiri Olsa]

Making sure the data->file.path is zeroed on perf_data__open
error path and in perf_data__close, so we don't double free
it in case someone call it twice.

Link: http://lkml.kernel.org/n/tip-yt0yz1y9h5vdppsj8nkiuh07@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 7bd5ddeb7a41..e098e189f93e 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -237,7 +237,7 @@ static int open_file(struct perf_data *data)
 	     open_file_read(data) : open_file_write(data);
 
 	if (fd < 0) {
-		free(data->file.path);
+		zfree(&data->file.path);
 		return -1;
 	}
 
@@ -270,7 +270,7 @@ int perf_data__open(struct perf_data *data)
 
 void perf_data__close(struct perf_data *data)
 {
-	free(data->file.path);
+	zfree(&data->file.path);
 	close(data->file.fd);
 }
 
-- 
2.17.2

Re: [PATCH 5/8] perf tools: Get precise_ip from the pmu config

[Andi Kleen]

On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> Getting precise_ip field from the perf_pmu::max_precise
> config read from sysfs. If it's not available falling
> back to current detection function.

max_precise depends on the event.  This won't work for all
events. For example only instructions and cycles support
ppp

The previous method handled it by event.

-Andi

Re: [PATCH 5/8] perf tools: Get precise_ip from the pmu config

[Jiri Olsa]

On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > Getting precise_ip field from the perf_pmu::max_precise
> > config read from sysfs. If it's not available falling
> > back to current detection function.
> 
> max_precise depends on the event.  This won't work for all
> events. For example only instructions and cycles support
> ppp

I'm getting  precise_ip=3 on mem-* events as well, that's why I
was fixing this.. now it's not working for any event

> 
> The previous method handled it by event.

how about we use empty template with just type/config
of the original event, like in the change below,
that way we eliminate unsupported features failing
the probing.. maybe we could use oldest attr version

also if precise_ip is event based, we shouldn't use the
caps/max_precise then

jirka


---
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ed20f4379956..cee2f83feb89 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -233,8 +233,8 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
 {
 	struct perf_event_attr attr = {
-		.type		= PERF_TYPE_HARDWARE,
-		.config		= PERF_COUNT_HW_CPU_CYCLES,
+		.type		= pattr->type,
+		.config		= pattr->config,
 		.exclude_kernel	= 1,
 		.precise_ip	= 3,
 	};

Re: [PATCH 5/8] perf tools: Get precise_ip from the pmu config

[Andi Kleen]

On Tue, Mar 05, 2019 at 05:28:54PM +0100, Jiri Olsa wrote:
> On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> > On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > > Getting precise_ip field from the perf_pmu::max_precise
> > > config read from sysfs. If it's not available falling
> > > back to current detection function.
> > 
> > max_precise depends on the event.  This won't work for all
> > events. For example only instructions and cycles support
> > ppp
> 
> I'm getting  precise_ip=3 on mem-* events as well, that's why I
> was fixing this.. now it's not working for any event

I don't think it means anything for mem-*

There's some support for it on Goldmont plus for other events,
but it doesn't support mem-*. On big core it's only
for instructions and cycles, all implemented with the same
event. All other PEBS events only have two levels
switching between the two IPs.

-Andi

[PATCHv2 5/8] perf tools: Get precise_ip from the pmu config

[Jiri Olsa]

On Tue, Mar 05, 2019 at 08:40:17AM -0800, Andi Kleen wrote:
> On Tue, Mar 05, 2019 at 05:28:54PM +0100, Jiri Olsa wrote:
> > On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> > > On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > > > Getting precise_ip field from the perf_pmu::max_precise
> > > > config read from sysfs. If it's not available falling
> > > > back to current detection function.
> > > 
> > > max_precise depends on the event.  This won't work for all
> > > events. For example only instructions and cycles support
> > > ppp
> > 
> > I'm getting  precise_ip=3 on mem-* events as well, that's why I
> > was fixing this.. now it's not working for any event
> 
> I don't think it means anything for mem-*
> 
> There's some support for it on Goldmont plus for other events,
> but it doesn't support mem-*. On big core it's only
> for instructions and cycles, all implemented with the same
> event. All other PEBS events only have two levels
> switching between the two IPs.

ok, so how about this, it's the change I posted merged with the patch

jirka


---
Currently we probe for precise_ip with user specified
perf_event_attr, which might fail because of unsupported
kernel features, which would get disabled during the
open time anyway.

Switching the probe to take place on simple event,
only configured with users type/config, so the
following record sets proper precise_ip:

  # perf record -e cycles:P ls
  # perf evlist -v
  cycles:P: size: 112, ... precise_ip: 3, ...

Link: http://lkml.kernel.org/n/tip-rwncfxifbhnmd89yx0va5zg0@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evlist.c | 25 ++++++++++++++++++++-----
 tools/perf/util/evsel.c  |  8 --------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 08cedb643ea6..cee2f83feb89 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -230,18 +230,33 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 	}
 }
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr)
+void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
 {
-	attr->precise_ip = 3;
+	struct perf_event_attr attr = {
+		.type		= pattr->type,
+		.config		= pattr->config,
+		.exclude_kernel	= 1,
+		.precise_ip	= 3,
+	};
 
-	while (attr->precise_ip != 0) {
-		int fd = sys_perf_event_open(attr, 0, -1, -1, 0);
+	event_attr_init(&attr);
+
+	/*
+	 * Unnamed union member, not supported as struct member named
+	 * initializer in older compilers such as gcc 4.4.7
+	 */
+	attr.sample_period = 1;
+
+	while (attr.precise_ip != 0) {
+		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
 		if (fd != -1) {
 			close(fd);
 			break;
 		}
-		--attr->precise_ip;
+		--attr.precise_ip;
 	}
+
+	pattr->precise_ip = attr.precise_ip;
 }
 
 int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index eec542bab815..2ef229e24b6f 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -294,20 +294,12 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 
 	if (!precise)
 		goto new_event;
-	/*
-	 * Unnamed union member, not supported as struct member named
-	 * initializer in older compilers such as gcc 4.4.7
-	 *
-	 * Just for probing the precise_ip:
-	 */
-	attr.sample_period = 1;
 
 	perf_event_attr__set_max_precise_ip(&attr);
 	/*
 	 * Now let the usual logic to set up the perf_event_attr defaults
 	 * to kick in when we return and before perf_evsel__open() is called.
 	 */
-	attr.sample_period = 0;
 new_event:
 	evsel = perf_evsel__new(&attr);
 	if (evsel == NULL)
-- 
2.17.2

Re: [PATCHv2 5/8] perf tools: Get precise_ip from the pmu config

[Andi Kleen]

On Thu, Mar 07, 2019 at 04:35:00PM +0100, Jiri Olsa wrote:
> On Tue, Mar 05, 2019 at 08:40:17AM -0800, Andi Kleen wrote:
> > On Tue, Mar 05, 2019 at 05:28:54PM +0100, Jiri Olsa wrote:
> > > On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> > > > On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > > > > Getting precise_ip field from the perf_pmu::max_precise
> > > > > config read from sysfs. If it's not available falling
> > > > > back to current detection function.
> > > > 
> > > > max_precise depends on the event.  This won't work for all
> > > > events. For example only instructions and cycles support
> > > > ppp
> > > 
> > > I'm getting  precise_ip=3 on mem-* events as well, that's why I
> > > was fixing this.. now it's not working for any event
> > 
> > I don't think it means anything for mem-*
> > 
> > There's some support for it on Goldmont plus for other events,
> > but it doesn't support mem-*. On big core it's only
> > for instructions and cycles, all implemented with the same
> > event. All other PEBS events only have two levels
> > switching between the two IPs.
> 
> ok, so how about this, it's the change I posted merged with the patch

Still seems like a hack. Even though I don't know of a case that
would break now. But it would be better to have the precise probing
in the open retry loop, instead of trying to reinvent it here.

-Andi

Re: [PATCHv2 5/8] perf tools: Get precise_ip from the pmu config

[Jiri Olsa]

On Thu, Mar 07, 2019 at 08:51:23AM -0800, Andi Kleen wrote:
> On Thu, Mar 07, 2019 at 04:35:00PM +0100, Jiri Olsa wrote:
> > On Tue, Mar 05, 2019 at 08:40:17AM -0800, Andi Kleen wrote:
> > > On Tue, Mar 05, 2019 at 05:28:54PM +0100, Jiri Olsa wrote:
> > > > On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> > > > > On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > > > > > Getting precise_ip field from the perf_pmu::max_precise
> > > > > > config read from sysfs. If it's not available falling
> > > > > > back to current detection function.
> > > > > 
> > > > > max_precise depends on the event.  This won't work for all
> > > > > events. For example only instructions and cycles support
> > > > > ppp
> > > > 
> > > > I'm getting  precise_ip=3 on mem-* events as well, that's why I
> > > > was fixing this.. now it's not working for any event
> > > 
> > > I don't think it means anything for mem-*
> > > 
> > > There's some support for it on Goldmont plus for other events,
> > > but it doesn't support mem-*. On big core it's only
> > > for instructions and cycles, all implemented with the same
> > > event. All other PEBS events only have two levels
> > > switching between the two IPs.
> > 
> > ok, so how about this, it's the change I posted merged with the patch
> 
> Still seems like a hack. Even though I don't know of a case that
> would break now. But it would be better to have the precise probing
> in the open retry loop, instead of trying to reinvent it here.

right, I guess we could just set precise_ip hard to 3 for :P
and add fallback part for the precise_ip in perf_evsel__open

jirka

[tip:perf/urgent] perf c2c: Fix c2c report for empty numa node

[tip-bot for Jiri Olsa]

Commit-ID:  e34c940245437f36d2c492edd1f8237eff391064
Gitweb:     https://git.kernel.org/tip/e34c940245437f36d2c492edd1f8237eff391064
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 5 Mar 2019 16:25:29 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:15:24 -0300

perf c2c: Fix c2c report for empty numa node

Ravi Bangoria reported that we fail with an empty NUMA node with the
following message:

  $ lscpu
  NUMA node0 CPU(s):
  NUMA node1 CPU(s):   0-4

  $ sudo ./perf c2c report
  node/cpu topology bugFailed setup nodes

Fix this by detecting the empty node and keeping its CPU set empty.

Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20190305152536.21035-2-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 4272763a5e96..9e6cc868bdb4 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2056,6 +2056,12 @@ static int setup_nodes(struct perf_session *session)
 		if (!set)
 			return -ENOMEM;
 
+		nodes[node] = set;
+
+		/* empty node, skip */
+		if (cpu_map__empty(map))
+			continue;
+
 		for (cpu = 0; cpu < map->nr; cpu++) {
 			set_bit(map->map[cpu], set);
 
@@ -2064,8 +2070,6 @@ static int setup_nodes(struct perf_session *session)
 
 			cpu2node[map->map[cpu]] = node;
 		}
-
-		nodes[node] = set;
 	}
 
 	setup_nodes_header();

[tip:perf/urgent] perf hist: Add error path into hist_entry__init

[tip-bot for Jiri Olsa]

Commit-ID:  c57589106fd6d996dbf3757708baa4a3fb91850f
Gitweb:     https://git.kernel.org/tip/c57589106fd6d996dbf3757708baa4a3fb91850f
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 5 Mar 2019 16:25:30 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:16:30 -0300

perf hist: Add error path into hist_entry__init

Adding error path into hist_entry__init to unify error handling, so
every new member does not need to free everything else.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: nageswara r sastry <nasastry@in.ibm.com>
Link: http://lkml.kernel.org/r/20190305152536.21035-3-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 669f961316f0..74e307d17c49 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -396,11 +396,8 @@ static int hist_entry__init(struct hist_entry *he,
 		 * adding new entries.  So we need to save a copy.
 		 */
 		he->branch_info = malloc(sizeof(*he->branch_info));
-		if (he->branch_info == NULL) {
-			map__zput(he->ms.map);
-			free(he->stat_acc);
-			return -ENOMEM;
-		}
+		if (he->branch_info == NULL)
+			goto err;
 
 		memcpy(he->branch_info, template->branch_info,
 		       sizeof(*he->branch_info));
@@ -419,21 +416,8 @@ static int hist_entry__init(struct hist_entry *he,
 
 	if (he->raw_data) {
 		he->raw_data = memdup(he->raw_data, he->raw_size);
-
-		if (he->raw_data == NULL) {
-			map__put(he->ms.map);
-			if (he->branch_info) {
-				map__put(he->branch_info->from.map);
-				map__put(he->branch_info->to.map);
-				free(he->branch_info);
-			}
-			if (he->mem_info) {
-				map__put(he->mem_info->iaddr.map);
-				map__put(he->mem_info->daddr.map);
-			}
-			free(he->stat_acc);
-			return -ENOMEM;
-		}
+		if (he->raw_data == NULL)
+			goto err_infos;
 	}
 	INIT_LIST_HEAD(&he->pairs.node);
 	thread__get(he->thread);
@@ -444,6 +428,21 @@ static int hist_entry__init(struct hist_entry *he,
 		he->leaf = true;
 
 	return 0;
+
+err_infos:
+	if (he->branch_info) {
+		map__put(he->branch_info->from.map);
+		map__put(he->branch_info->to.map);
+		free(he->branch_info);
+	}
+	if (he->mem_info) {
+		map__put(he->mem_info->iaddr.map);
+		map__put(he->mem_info->daddr.map);
+	}
+err:
+	map__zput(he->ms.map);
+	free(he->stat_acc);
+	return -ENOMEM;
 }
 
 static void *hist_entry__zalloc(size_t size)

[tip:perf/urgent] perf hist: Fix memory leak of srcline

[tip-bot for Jiri Olsa]

Commit-ID:  2634958586368dcbf09c0d2a17dee02d1fc53e0d
Gitweb:     https://git.kernel.org/tip/2634958586368dcbf09c0d2a17dee02d1fc53e0d
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 5 Mar 2019 16:25:31 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:16:57 -0300

perf hist: Fix memory leak of srcline

We can't allocate he->srcline unconditionaly, only when new hist_entry
is created. Moving he->srcline allocation into hist_entry__init
function.

Original-patch-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Nageswara R Sastry <nasastry@in.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Link: http://lkml.kernel.org/r/20190305152536.21035-4-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 74e307d17c49..f9eb95bf3938 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -419,6 +419,13 @@ static int hist_entry__init(struct hist_entry *he,
 		if (he->raw_data == NULL)
 			goto err_infos;
 	}
+
+	if (he->srcline) {
+		he->srcline = strdup(he->srcline);
+		if (he->srcline == NULL)
+			goto err_rawdata;
+	}
+
 	INIT_LIST_HEAD(&he->pairs.node);
 	thread__get(he->thread);
 	he->hroot_in  = RB_ROOT_CACHED;
@@ -429,6 +436,9 @@ static int hist_entry__init(struct hist_entry *he,
 
 	return 0;
 
+err_rawdata:
+	free(he->raw_data);
+
 err_infos:
 	if (he->branch_info) {
 		map__put(he->branch_info->from.map);
@@ -605,7 +615,7 @@ __hists__add_entry(struct hists *hists,
 			.map	= al->map,
 			.sym	= al->sym,
 		},
-		.srcline = al->srcline ? strdup(al->srcline) : NULL,
+		.srcline = (char *) al->srcline,
 		.socket	 = al->socket,
 		.cpu	 = al->cpu,
 		.cpumode = al->cpumode,
@@ -962,7 +972,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 			.map = al->map,
 			.sym = al->sym,
 		},
-		.srcline = al->srcline ? strdup(al->srcline) : NULL,
+		.srcline = (char *) al->srcline,
 		.parent = iter->parent,
 		.raw_data = sample->raw_data,
 		.raw_size = sample->raw_size,

[tip:perf/urgent] perf tools: Read and store caps/max_precise in perf_pmu

[tip-bot for Jiri Olsa]

Commit-ID:  90a86bde97ba050cb3c9ccb215252ee2d2d705fa
Gitweb:     https://git.kernel.org/tip/90a86bde97ba050cb3c9ccb215252ee2d2d705fa
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 5 Mar 2019 16:25:32 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:18:17 -0300

perf tools: Read and store caps/max_precise in perf_pmu

Read the caps/max_precise value and store it in struct perf_pmu to be
used when setting the maximum precise_ip field in following patch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Nageswara R Sastry <nasastry@in.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Link: http://lkml.kernel.org/r/20190305152536.21035-5-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/pmu.c | 14 ++++++++++++++
 tools/perf/util/pmu.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 51d437f55d18..6199a3174ab9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -752,6 +752,19 @@ perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
 	return NULL;
 }
 
+static int pmu_max_precise(const char *name)
+{
+	char path[PATH_MAX];
+	int max_precise = -1;
+
+	scnprintf(path, PATH_MAX,
+		 "bus/event_source/devices/%s/caps/max_precise",
+		 name);
+
+	sysfs__read_int(path, &max_precise);
+	return max_precise;
+}
+
 static struct perf_pmu *pmu_lookup(const char *name)
 {
 	struct perf_pmu *pmu;
@@ -784,6 +797,7 @@ static struct perf_pmu *pmu_lookup(const char *name)
 	pmu->name = strdup(name);
 	pmu->type = type;
 	pmu->is_uncore = pmu_is_uncore(name);
+	pmu->max_precise = pmu_max_precise(name);
 	pmu_add_cpu_aliases(&aliases, pmu);
 
 	INIT_LIST_HEAD(&pmu->format);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 47253c3daf55..bd9ec2704a57 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -26,6 +26,7 @@ struct perf_pmu {
 	__u32 type;
 	bool selectable;
 	bool is_uncore;
+	int max_precise;
 	struct perf_event_attr *default_config;
 	struct cpu_map *cpus;
 	struct list_head format;  /* HEAD struct perf_pmu_format -> list */

[tip:perf/urgent] perf evsel: Probe for precise_ip with simple attr

[tip-bot for Jiri Olsa]

Commit-ID:  5b61adb16599be04346e7e943c1b5113b57485ad
Gitweb:     https://git.kernel.org/tip/5b61adb16599be04346e7e943c1b5113b57485ad
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 5 Mar 2019 16:25:34 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:19:45 -0300

perf evsel: Probe for precise_ip with simple attr

Currently we probe for precise_ip with user specified perf_event_attr,
which might fail because of unsupported kernel features, which would get
disabled during the open time anyway.

Switching the probe to take place on simple hw cycles, so the following
record sets proper precise_ip:

  # perf record -e cycles:P ls
  # perf evlist -v
  cycles:P: size: 112, ... precise_ip: 3, ...

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Nageswara R Sastry <nasastry@in.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Link: http://lkml.kernel.org/r/20190305152536.21035-7-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 25 ++++++++++++++++++++-----
 tools/perf/util/evsel.c  |  8 --------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 08cedb643ea6..ed20f4379956 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -230,18 +230,33 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 	}
 }
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr)
+void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
 {
-	attr->precise_ip = 3;
+	struct perf_event_attr attr = {
+		.type		= PERF_TYPE_HARDWARE,
+		.config		= PERF_COUNT_HW_CPU_CYCLES,
+		.exclude_kernel	= 1,
+		.precise_ip	= 3,
+	};
 
-	while (attr->precise_ip != 0) {
-		int fd = sys_perf_event_open(attr, 0, -1, -1, 0);
+	event_attr_init(&attr);
+
+	/*
+	 * Unnamed union member, not supported as struct member named
+	 * initializer in older compilers such as gcc 4.4.7
+	 */
+	attr.sample_period = 1;
+
+	while (attr.precise_ip != 0) {
+		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
 		if (fd != -1) {
 			close(fd);
 			break;
 		}
-		--attr->precise_ip;
+		--attr.precise_ip;
 	}
+
+	pattr->precise_ip = attr.precise_ip;
 }
 
 int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dfe2958e6287..3bbf73e979c0 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -294,20 +294,12 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 
 	if (!precise)
 		goto new_event;
-	/*
-	 * Unnamed union member, not supported as struct member named
-	 * initializer in older compilers such as gcc 4.4.7
-	 *
-	 * Just for probing the precise_ip:
-	 */
-	attr.sample_period = 1;
 
 	perf_event_attr__set_max_precise_ip(&attr);
 	/*
 	 * Now let the usual logic to set up the perf_event_attr defaults
 	 * to kick in when we return and before perf_evsel__open() is called.
 	 */
-	attr.sample_period = 0;
 new_event:
 	evsel = perf_evsel__new(&attr);
 	if (evsel == NULL)

[tip:perf/urgent] perf session: Fix double free in perf_data__close

[tip-bot for Jiri Olsa]

Commit-ID:  befa09b61f8bf1d7c34b8e6405f08d804640573c
Gitweb:     https://git.kernel.org/tip/befa09b61f8bf1d7c34b8e6405f08d804640573c
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 5 Mar 2019 16:25:35 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:20:33 -0300

perf session: Fix double free in perf_data__close

We can't call perf_data__close and subsequently perf_session__delete,
because it will call perf_data__close again and cause double free for
data->file.path.

  $ perf report -i .
  incompatible file format (rerun with -v to learn more)
  free(): double free detected in tcache 2
  Aborted (core dumped)

In fact we don't need to call perf_data__close at all, because at the
time the got out_close is reached, session->data is already initialized,
so the perf_data__close call will be triggered from
perf_session__delete.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Nageswara R Sastry <nasastry@in.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Fixes: 2d4f27999b88 ("perf data: Add global path holder")
Link: http://lkml.kernel.org/r/20190305152536.21035-8-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c764bbc91009..db643f3c2b95 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -140,7 +140,7 @@ struct perf_session *perf_session__new(struct perf_data *data,
 
 		if (perf_data__is_read(data)) {
 			if (perf_session__open(session) < 0)
-				goto out_close;
+				goto out_delete;
 
 			/*
 			 * set session attributes that are present in perf.data
@@ -181,8 +181,6 @@ struct perf_session *perf_session__new(struct perf_data *data,
 
 	return session;
 
- out_close:
-	perf_data__close(data);
  out_delete:
 	perf_session__delete(session);
  out:

[tip:perf/urgent] perf data: Force perf_data__open|close zero data->file.path

[tip-bot for Jiri Olsa]

Commit-ID:  b8f7d86b5849ea7bb84bddc0345a3799049764d4
Gitweb:     https://git.kernel.org/tip/b8f7d86b5849ea7bb84bddc0345a3799049764d4
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 5 Mar 2019 16:25:36 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 6 Mar 2019 18:21:00 -0300

perf data: Force perf_data__open|close zero data->file.path

Making sure the data->file.path is zeroed on perf_data__open error path
and in perf_data__close, so we don't double free it in case someone call
it twice.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Cc: Nageswara R Sastry <nasastry@in.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Link: http://lkml.kernel.org/r/20190305152536.21035-9-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 7bd5ddeb7a41..e098e189f93e 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -237,7 +237,7 @@ static int open_file(struct perf_data *data)
 	     open_file_read(data) : open_file_write(data);
 
 	if (fd < 0) {
-		free(data->file.path);
+		zfree(&data->file.path);
 		return -1;
 	}
 
@@ -270,7 +270,7 @@ int perf_data__open(struct perf_data *data)
 
 void perf_data__close(struct perf_data *data)
 {
-	free(data->file.path);
+	zfree(&data->file.path);
 	close(data->file.fd);
 }
 

Re: [PATCHv2 5/8] perf tools: Get precise_ip from the pmu config

[Jiri Olsa]

On Thu, Mar 07, 2019 at 08:51:23AM -0800, Andi Kleen wrote:
> On Thu, Mar 07, 2019 at 04:35:00PM +0100, Jiri Olsa wrote:
> > On Tue, Mar 05, 2019 at 08:40:17AM -0800, Andi Kleen wrote:
> > > On Tue, Mar 05, 2019 at 05:28:54PM +0100, Jiri Olsa wrote:
> > > > On Tue, Mar 05, 2019 at 08:13:19AM -0800, Andi Kleen wrote:
> > > > > On Tue, Mar 05, 2019 at 04:25:33PM +0100, Jiri Olsa wrote:
> > > > > > Getting precise_ip field from the perf_pmu::max_precise
> > > > > > config read from sysfs. If it's not available falling
> > > > > > back to current detection function.
> > > > > 
> > > > > max_precise depends on the event.  This won't work for all
> > > > > events. For example only instructions and cycles support
> > > > > ppp
> > > > 
> > > > I'm getting  precise_ip=3 on mem-* events as well, that's why I
> > > > was fixing this.. now it's not working for any event
> > > 
> > > I don't think it means anything for mem-*
> > > 
> > > There's some support for it on Goldmont plus for other events,
> > > but it doesn't support mem-*. On big core it's only
> > > for instructions and cycles, all implemented with the same
> > > event. All other PEBS events only have two levels
> > > switching between the two IPs.
> > 
> > ok, so how about this, it's the change I posted merged with the patch
> 
> Still seems like a hack. Even though I don't know of a case that
> would break now. But it would be better to have the precise probing
> in the open retry loop, instead of trying to reinvent it here.

how about something like below?

jirka


---
 tools/perf/util/evlist.c | 29 ----------------
 tools/perf/util/evlist.h |  2 --
 tools/perf/util/evsel.c  | 73 +++++++++++++++++++++++++++++++++-------
 3 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ed20f4379956..3a243c249c29 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -230,35 +230,6 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 	}
 }
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
-{
-	struct perf_event_attr attr = {
-		.type		= PERF_TYPE_HARDWARE,
-		.config		= PERF_COUNT_HW_CPU_CYCLES,
-		.exclude_kernel	= 1,
-		.precise_ip	= 3,
-	};
-
-	event_attr_init(&attr);
-
-	/*
-	 * Unnamed union member, not supported as struct member named
-	 * initializer in older compilers such as gcc 4.4.7
-	 */
-	attr.sample_period = 1;
-
-	while (attr.precise_ip != 0) {
-		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
-		if (fd != -1) {
-			close(fd);
-			break;
-		}
-		--attr.precise_ip;
-	}
-
-	pattr->precise_ip = attr.precise_ip;
-}
-
 int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
 {
 	struct perf_evsel *evsel = perf_evsel__new_cycles(precise);
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 744906dd4887..cb15e3366eba 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -303,8 +303,6 @@ void perf_evlist__to_front(struct perf_evlist *evlist,
 void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
 				     struct perf_evsel *tracking_evsel);
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr);
-
 struct perf_evsel *
 perf_evlist__find_evsel_by_str(struct perf_evlist *evlist, const char *str);
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 3bbf73e979c0..7cd70f99201a 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -295,7 +295,6 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 	if (!precise)
 		goto new_event;
 
-	perf_event_attr__set_max_precise_ip(&attr);
 	/*
 	 * Now let the usual logic to set up the perf_event_attr defaults
 	 * to kick in when we return and before perf_evsel__open() is called.
@@ -305,6 +304,8 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 	if (evsel == NULL)
 		goto out;
 
+	evsel->precise_max = true;
+
 	/* use asprintf() because free(evsel) assumes name is allocated */
 	if (asprintf(&evsel->name, "cycles%s%s%.*s",
 		     (attr.precise_ip || attr.exclude_kernel) ? ":" : "",
@@ -1083,7 +1084,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	}
 
 	if (evsel->precise_max)
-		perf_event_attr__set_max_precise_ip(attr);
+		attr->precise_ip = 3;
 
 	if (opts->all_user) {
 		attr->exclude_kernel = 1;
@@ -1749,6 +1750,60 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
 	return true;
 }
 
+static void display_attr(struct perf_event_attr *attr)
+{
+	if (verbose >= 2) {
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+		fprintf(stderr, "perf_event_attr:\n");
+		perf_event_attr__fprintf(stderr, attr, __open_attr__fprintf, NULL);
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+	}
+}
+
+static int perf_event_open(struct perf_evsel *evsel,
+			   pid_t pid, int cpu, int group_fd,
+			   unsigned long flags)
+{
+	int precise_ip = evsel->attr.precise_ip;
+	int fd;
+
+	while (1) {
+		pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
+			  pid, cpu, group_fd, flags);
+
+		fd = sys_perf_event_open(&evsel->attr, pid, cpu, group_fd, flags);
+		if (fd >= 0)
+			break;
+
+		/*
+		 * Do quick precise_ip fallback if requested:
+		 *  - there is some precise_ip
+		 *  - maximum precise is requested
+		 *  - we failed with ENOTSUP error, which is
+		 *    associated with wrong precise_ip config
+		 */
+		if (!precise_ip || !evsel->precise_max || (errno != ENOTSUP))
+			break;
+
+		/*
+		 * We tried all the precise_ip values, and it's
+		 * still failing, so leave it to standard fallabck.
+		 */
+		if (!evsel->attr.precise_ip) {
+			evsel->attr.precise_ip = precise_ip;
+			break;
+		}
+
+		pr_debug2("\nsys_perf_event_open failed, error %d\n", -ENOTSUP);
+
+		evsel->attr.precise_ip--;
+		pr_debug2("decreasing precise_ip by one (%d)\n", evsel->attr.precise_ip);
+		display_attr(&evsel->attr);
+	}
+
+	return fd;
+}
+
 int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 		     struct thread_map *threads)
 {
@@ -1824,12 +1879,7 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 	if (perf_missing_features.sample_id_all)
 		evsel->attr.sample_id_all = 0;
 
-	if (verbose >= 2) {
-		fprintf(stderr, "%.60s\n", graph_dotted_line);
-		fprintf(stderr, "perf_event_attr:\n");
-		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
-		fprintf(stderr, "%.60s\n", graph_dotted_line);
-	}
+	display_attr(&evsel->attr);
 
 	for (cpu = 0; cpu < cpus->nr; cpu++) {
 
@@ -1841,13 +1891,10 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 			group_fd = get_group_fd(evsel, cpu, thread);
 retry_open:
-			pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
-				  pid, cpus->map[cpu], group_fd, flags);
-
 			test_attr__ready();
 
-			fd = sys_perf_event_open(&evsel->attr, pid, cpus->map[cpu],
-						 group_fd, flags);
+			fd = perf_event_open(evsel, pid, cpus->map[cpu],
+					     group_fd, flags);
 
 			FD(evsel, cpu, thread) = fd;
 
-- 
2.17.2

Re: [PATCHv2 5/8] perf tools: Get precise_ip from the pmu config

[Andi Kleen]

> > Still seems like a hack. Even though I don't know of a case that
> > would break now. But it would be better to have the precise probing
> > in the open retry loop, instead of trying to reinvent it here.
> 
> how about something like below?

Yes looks reasonable.

-Andi

[PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Jiri Olsa]

On Thu, Mar 14, 2019 at 08:49:11AM -0700, Andi Kleen wrote:
> > > Still seems like a hack. Even though I don't know of a case that
> > > would break now. But it would be better to have the precise probing
> > > in the open retry loop, instead of trying to reinvent it here.
> > 
> > how about something like below?
> 
> Yes looks reasonable.

full patch attached,

jirka


---
After discussion with Andi, moving the precise_ip detection
for maximum precise config (via :P modifier or for default
cycles event) into perf_evsel__open code.

The current detection code in perf_event_attr__set_max_precise_ip
function is tricky, because precise_ip config is specific for
given event and it currently checks only hw cycles.

We now check for valid precise_ip value right after failing
sys_perf_event_open for specific event, before any of the
perf_event_attr fallback code gets executed. This way we get
the proper config in perf_event_attr together with allowed
precise_ip settings.

We can see that code activity with -vv, like:

  $ perf record -vv ls
  ...
  ------------------------------------------------------------
  perf_event_attr:
    size                             112
    { sample_period, sample_freq }   4000
    ...
    precise_ip                       3
    sample_id_all                    1
    exclude_guest                    1
    mmap2                            1
    comm_exec                        1
    ksymbol                          1
  ------------------------------------------------------------
  sys_perf_event_open: pid 9926  cpu 0  group_fd -1  flags 0x8
  sys_perf_event_open failed, error -95
  decreasing precise_ip by one (2)
  ------------------------------------------------------------
  perf_event_attr:
    size                             112
    { sample_period, sample_freq }   4000
    ...
    precise_ip                       2
    sample_id_all                    1
    exclude_guest                    1
    mmap2                            1
    comm_exec                        1
    ksymbol                          1
  ------------------------------------------------------------
  sys_perf_event_open: pid 9926  cpu 0  group_fd -1  flags 0x8 = 4
  ...

Suggested-by: Andi Kleen <ak@linux.intel.com>
Link: http://lkml.kernel.org/n/tip-dkvxxbeg7lu74155d4jhlmc9@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evlist.c | 29 ----------------
 tools/perf/util/evlist.h |  2 --
 tools/perf/util/evsel.c  | 72 ++++++++++++++++++++++++++++++++--------
 3 files changed, 59 insertions(+), 44 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ed20f4379956..3a243c249c29 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -230,35 +230,6 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
 	}
 }
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
-{
-	struct perf_event_attr attr = {
-		.type		= PERF_TYPE_HARDWARE,
-		.config		= PERF_COUNT_HW_CPU_CYCLES,
-		.exclude_kernel	= 1,
-		.precise_ip	= 3,
-	};
-
-	event_attr_init(&attr);
-
-	/*
-	 * Unnamed union member, not supported as struct member named
-	 * initializer in older compilers such as gcc 4.4.7
-	 */
-	attr.sample_period = 1;
-
-	while (attr.precise_ip != 0) {
-		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
-		if (fd != -1) {
-			close(fd);
-			break;
-		}
-		--attr.precise_ip;
-	}
-
-	pattr->precise_ip = attr.precise_ip;
-}
-
 int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
 {
 	struct perf_evsel *evsel = perf_evsel__new_cycles(precise);
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 744906dd4887..cb15e3366eba 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -303,8 +303,6 @@ void perf_evlist__to_front(struct perf_evlist *evlist,
 void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
 				     struct perf_evsel *tracking_evsel);
 
-void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr);
-
 struct perf_evsel *
 perf_evlist__find_evsel_by_str(struct perf_evlist *evlist, const char *str);
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 3bbf73e979c0..f606e8a81f0e 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -295,7 +295,6 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 	if (!precise)
 		goto new_event;
 
-	perf_event_attr__set_max_precise_ip(&attr);
 	/*
 	 * Now let the usual logic to set up the perf_event_attr defaults
 	 * to kick in when we return and before perf_evsel__open() is called.
@@ -305,6 +304,8 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
 	if (evsel == NULL)
 		goto out;
 
+	evsel->precise_max = true;
+
 	/* use asprintf() because free(evsel) assumes name is allocated */
 	if (asprintf(&evsel->name, "cycles%s%s%.*s",
 		     (attr.precise_ip || attr.exclude_kernel) ? ":" : "",
@@ -1083,7 +1084,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	}
 
 	if (evsel->precise_max)
-		perf_event_attr__set_max_precise_ip(attr);
+		attr->precise_ip = 3;
 
 	if (opts->all_user) {
 		attr->exclude_kernel = 1;
@@ -1749,6 +1750,59 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
 	return true;
 }
 
+static void display_attr(struct perf_event_attr *attr)
+{
+	if (verbose >= 2) {
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+		fprintf(stderr, "perf_event_attr:\n");
+		perf_event_attr__fprintf(stderr, attr, __open_attr__fprintf, NULL);
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+	}
+}
+
+static int perf_event_open(struct perf_evsel *evsel,
+			   pid_t pid, int cpu, int group_fd,
+			   unsigned long flags)
+{
+	int precise_ip = evsel->attr.precise_ip;
+	int fd;
+
+	while (1) {
+		pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
+			  pid, cpu, group_fd, flags);
+
+		fd = sys_perf_event_open(&evsel->attr, pid, cpu, group_fd, flags);
+		if (fd >= 0)
+			break;
+
+		/*
+		 * Do quick precise_ip fallback if:
+		 *  - there is precise_ip set in perf_event_attr
+		 *  - maximum precise is requested
+		 *  - sys_perf_event_open failed with ENOTSUP error,
+		 *    which is associated with wrong precise_ip
+		 */
+		if (!precise_ip || !evsel->precise_max || (errno != ENOTSUP))
+			break;
+
+		/*
+		 * We tried all the precise_ip values, and it's
+		 * still failing, so leave it to standard fallback.
+		 */
+		if (!evsel->attr.precise_ip) {
+			evsel->attr.precise_ip = precise_ip;
+			break;
+		}
+
+		pr_debug2("\nsys_perf_event_open failed, error %d\n", -ENOTSUP);
+		evsel->attr.precise_ip--;
+		pr_debug2("decreasing precise_ip by one (%d)\n", evsel->attr.precise_ip);
+		display_attr(&evsel->attr);
+	}
+
+	return fd;
+}
+
 int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 		     struct thread_map *threads)
 {
@@ -1824,12 +1878,7 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 	if (perf_missing_features.sample_id_all)
 		evsel->attr.sample_id_all = 0;
 
-	if (verbose >= 2) {
-		fprintf(stderr, "%.60s\n", graph_dotted_line);
-		fprintf(stderr, "perf_event_attr:\n");
-		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
-		fprintf(stderr, "%.60s\n", graph_dotted_line);
-	}
+	display_attr(&evsel->attr);
 
 	for (cpu = 0; cpu < cpus->nr; cpu++) {
 
@@ -1841,13 +1890,10 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 			group_fd = get_group_fd(evsel, cpu, thread);
 retry_open:
-			pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
-				  pid, cpus->map[cpu], group_fd, flags);
-
 			test_attr__ready();
 
-			fd = sys_perf_event_open(&evsel->attr, pid, cpus->map[cpu],
-						 group_fd, flags);
+			fd = perf_event_open(evsel, pid, cpus->map[cpu],
+					     group_fd, flags);
 
 			FD(evsel, cpu, thread) = fd;
 
-- 
2.17.2

Re: [PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Andi Kleen]

On Fri, Mar 15, 2019 at 01:15:46PM +0100, Jiri Olsa wrote:
> On Thu, Mar 14, 2019 at 08:49:11AM -0700, Andi Kleen wrote:
> > > > Still seems like a hack. Even though I don't know of a case that
> > > > would break now. But it would be better to have the precise probing
> > > > in the open retry loop, instead of trying to reinvent it here.
> > > 
> > > how about something like below?
> > 
> > Yes looks reasonable.
> 
> full patch attached,

Acked-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Arnaldo Carvalho de Melo]

Em Fri, Mar 15, 2019 at 01:15:46PM +0100, Jiri Olsa escreveu:
> On Thu, Mar 14, 2019 at 08:49:11AM -0700, Andi Kleen wrote:
> > > > Still seems like a hack. Even though I don't know of a case that
> > > > would break now. But it would be better to have the precise probing
> > > > in the open retry loop, instead of trying to reinvent it here.
> > > 
> > > how about something like below?
> > 
> > Yes looks reasonable.
> 
> full patch attached,
> 
> jirka
> 
> 
> ---
> After discussion with Andi, moving the precise_ip detection
> for maximum precise config (via :P modifier or for default
> cycles event) into perf_evsel__open code.
> 
> The current detection code in perf_event_attr__set_max_precise_ip
> function is tricky, because precise_ip config is specific for
> given event and it currently checks only hw cycles.
> 
> We now check for valid precise_ip value right after failing
> sys_perf_event_open for specific event, before any of the
> perf_event_attr fallback code gets executed. This way we get
> the proper config in perf_event_attr together with allowed
> precise_ip settings.
> 
> We can see that code activity with -vv, like:
> 
>   $ perf record -vv ls
>   ...
>   ------------------------------------------------------------
>   perf_event_attr:
>     size                             112
>     { sample_period, sample_freq }   4000
>     ...
>     precise_ip                       3
>     sample_id_all                    1
>     exclude_guest                    1
>     mmap2                            1
>     comm_exec                        1
>     ksymbol                          1
>   ------------------------------------------------------------
>   sys_perf_event_open: pid 9926  cpu 0  group_fd -1  flags 0x8
>   sys_perf_event_open failed, error -95
>   decreasing precise_ip by one (2)
>   ------------------------------------------------------------
>   perf_event_attr:
>     size                             112
>     { sample_period, sample_freq }   4000
>     ...
>     precise_ip                       2
>     sample_id_all                    1
>     exclude_guest                    1
>     mmap2                            1
>     comm_exec                        1
>     ksymbol                          1
>   ------------------------------------------------------------
>   sys_perf_event_open: pid 9926  cpu 0  group_fd -1  flags 0x8 = 4
>   ...
> 
> Suggested-by: Andi Kleen <ak@linux.intel.com>
> Link: http://lkml.kernel.org/n/tip-dkvxxbeg7lu74155d4jhlmc9@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/evlist.c | 29 ----------------
>  tools/perf/util/evlist.h |  2 --
>  tools/perf/util/evsel.c  | 72 ++++++++++++++++++++++++++++++++--------
>  3 files changed, 59 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index ed20f4379956..3a243c249c29 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -230,35 +230,6 @@ void perf_evlist__set_leader(struct perf_evlist *evlist)
>  	}
>  }
>  
> -void perf_event_attr__set_max_precise_ip(struct perf_event_attr *pattr)
> -{
> -	struct perf_event_attr attr = {
> -		.type		= PERF_TYPE_HARDWARE,
> -		.config		= PERF_COUNT_HW_CPU_CYCLES,
> -		.exclude_kernel	= 1,
> -		.precise_ip	= 3,
> -	};
> -
> -	event_attr_init(&attr);
> -
> -	/*
> -	 * Unnamed union member, not supported as struct member named
> -	 * initializer in older compilers such as gcc 4.4.7
> -	 */
> -	attr.sample_period = 1;
> -
> -	while (attr.precise_ip != 0) {
> -		int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> -		if (fd != -1) {
> -			close(fd);
> -			break;
> -		}
> -		--attr.precise_ip;
> -	}
> -
> -	pattr->precise_ip = attr.precise_ip;
> -}
> -
>  int __perf_evlist__add_default(struct perf_evlist *evlist, bool precise)
>  {
>  	struct perf_evsel *evsel = perf_evsel__new_cycles(precise);
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 744906dd4887..cb15e3366eba 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -303,8 +303,6 @@ void perf_evlist__to_front(struct perf_evlist *evlist,
>  void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
>  				     struct perf_evsel *tracking_evsel);
>  
> -void perf_event_attr__set_max_precise_ip(struct perf_event_attr *attr);
> -
>  struct perf_evsel *
>  perf_evlist__find_evsel_by_str(struct perf_evlist *evlist, const char *str);
>  
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 3bbf73e979c0..f606e8a81f0e 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -295,7 +295,6 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
>  	if (!precise)
>  		goto new_event;
>  
> -	perf_event_attr__set_max_precise_ip(&attr);
>  	/*
>  	 * Now let the usual logic to set up the perf_event_attr defaults
>  	 * to kick in when we return and before perf_evsel__open() is called.
> @@ -305,6 +304,8 @@ struct perf_evsel *perf_evsel__new_cycles(bool precise)
>  	if (evsel == NULL)
>  		goto out;
>  
> +	evsel->precise_max = true;
> +
>  	/* use asprintf() because free(evsel) assumes name is allocated */
>  	if (asprintf(&evsel->name, "cycles%s%s%.*s",
>  		     (attr.precise_ip || attr.exclude_kernel) ? ":" : "",
> @@ -1083,7 +1084,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
>  	}
>  
>  	if (evsel->precise_max)
> -		perf_event_attr__set_max_precise_ip(attr);
> +		attr->precise_ip = 3;
>  
>  	if (opts->all_user) {
>  		attr->exclude_kernel = 1;
> @@ -1749,6 +1750,59 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
>  	return true;
>  }
>  
> +static void display_attr(struct perf_event_attr *attr)
> +{
> +	if (verbose >= 2) {
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +		fprintf(stderr, "perf_event_attr:\n");
> +		perf_event_attr__fprintf(stderr, attr, __open_attr__fprintf, NULL);
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +	}
> +}
> +
> +static int perf_event_open(struct perf_evsel *evsel,
> +			   pid_t pid, int cpu, int group_fd,
> +			   unsigned long flags)


The patch is ok, but I think the naming of this function is too generic,
so I'm renaming it to:

static int perf_evsel__open_adjust_precise_ip(struct perf_evsel *evsel,
					      pid_t pid, int cpu, int group_fd,
					      unsigned long flags)

Ok?

The perf_evsel__open() code is already complex with that fallback
mechanism, this is just one more way of fallbacking when asking the
kernel for something that may fail.

In fact what happens if the precise_ip that is being asked _is_
supported but sys_perf_event_open() fails because some other
perf_event_attr attribute that is set is not supported? 

I see, it gets it back restored to what the user asked so that the
standard fallback is tried, ok, I'll apply with just the rename for this
function,

Thanks,

- Arnaldo

> +{
> +	int precise_ip = evsel->attr.precise_ip;
> +	int fd;
> +
> +	while (1) {
> +		pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
> +			  pid, cpu, group_fd, flags);
> +
> +		fd = sys_perf_event_open(&evsel->attr, pid, cpu, group_fd, flags);
> +		if (fd >= 0)
> +			break;
> +
> +		/*
> +		 * Do quick precise_ip fallback if:
> +		 *  - there is precise_ip set in perf_event_attr
> +		 *  - maximum precise is requested
> +		 *  - sys_perf_event_open failed with ENOTSUP error,
> +		 *    which is associated with wrong precise_ip
> +		 */
> +		if (!precise_ip || !evsel->precise_max || (errno != ENOTSUP))
> +			break;
> +
> +		/*
> +		 * We tried all the precise_ip values, and it's
> +		 * still failing, so leave it to standard fallback.
> +		 */
> +		if (!evsel->attr.precise_ip) {
> +			evsel->attr.precise_ip = precise_ip;
> +			break;
> +		}
> +
> +		pr_debug2("\nsys_perf_event_open failed, error %d\n", -ENOTSUP);
> +		evsel->attr.precise_ip--;
> +		pr_debug2("decreasing precise_ip by one (%d)\n", evsel->attr.precise_ip);
> +		display_attr(&evsel->attr);
> +	}
> +
> +	return fd;
> +}
> +
>  int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  		     struct thread_map *threads)
>  {
> @@ -1824,12 +1878,7 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  	if (perf_missing_features.sample_id_all)
>  		evsel->attr.sample_id_all = 0;
>  
> -	if (verbose >= 2) {
> -		fprintf(stderr, "%.60s\n", graph_dotted_line);
> -		fprintf(stderr, "perf_event_attr:\n");
> -		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
> -		fprintf(stderr, "%.60s\n", graph_dotted_line);
> -	}
> +	display_attr(&evsel->attr);
>  
>  	for (cpu = 0; cpu < cpus->nr; cpu++) {
>  
> @@ -1841,13 +1890,10 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  
>  			group_fd = get_group_fd(evsel, cpu, thread);
>  retry_open:
> -			pr_debug2("sys_perf_event_open: pid %d  cpu %d  group_fd %d  flags %#lx",
> -				  pid, cpus->map[cpu], group_fd, flags);
> -
>  			test_attr__ready();
>  
> -			fd = sys_perf_event_open(&evsel->attr, pid, cpus->map[cpu],
> -						 group_fd, flags);
> +			fd = perf_event_open(evsel, pid, cpus->map[cpu],
> +					     group_fd, flags);
>  
>  			FD(evsel, cpu, thread) = fd;
>  
> -- 
> 2.17.2

-- 

- Arnaldo

Re: [PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Jiri Olsa]

On Fri, Mar 15, 2019 at 11:35:04AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > +static void display_attr(struct perf_event_attr *attr)
> > +{
> > +	if (verbose >= 2) {
> > +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> > +		fprintf(stderr, "perf_event_attr:\n");
> > +		perf_event_attr__fprintf(stderr, attr, __open_attr__fprintf, NULL);
> > +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> > +	}
> > +}
> > +
> > +static int perf_event_open(struct perf_evsel *evsel,
> > +			   pid_t pid, int cpu, int group_fd,
> > +			   unsigned long flags)
> 
> 
> The patch is ok, but I think the naming of this function is too generic,
> so I'm renaming it to:
> 
> static int perf_evsel__open_adjust_precise_ip(struct perf_evsel *evsel,
> 					      pid_t pid, int cpu, int group_fd,
> 					      unsigned long flags)
> 
> Ok?

ok

> 
> The perf_evsel__open() code is already complex with that fallback
> mechanism, this is just one more way of fallbacking when asking the
> kernel for something that may fail.
> 
> In fact what happens if the precise_ip that is being asked _is_
> supported but sys_perf_event_open() fails because some other
> perf_event_attr attribute that is set is not supported? 

it's outside the scope of standard feature fallback code,
so we will try it for any possible fallback variant, so:

we will try all possible precise_ip (3,2,1,0) and they will
all fail because of the unsupported attribute - so we will
restore the precise_ip back and continue in standard fallback
code that will eventualy switch that attribute off

> 
> I see, it gets it back restored to what the user asked so that the
> standard fallback is tried, ok, I'll apply with just the rename for this
> function,

thanks,
jirka

Re: [PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Jiri Olsa]

On Fri, Mar 15, 2019 at 03:52:25PM +0100, Jiri Olsa wrote:
> On Fri, Mar 15, 2019 at 11:35:04AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > +static void display_attr(struct perf_event_attr *attr)
> > > +{
> > > +	if (verbose >= 2) {
> > > +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> > > +		fprintf(stderr, "perf_event_attr:\n");
> > > +		perf_event_attr__fprintf(stderr, attr, __open_attr__fprintf, NULL);
> > > +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> > > +	}
> > > +}
> > > +
> > > +static int perf_event_open(struct perf_evsel *evsel,
> > > +			   pid_t pid, int cpu, int group_fd,
> > > +			   unsigned long flags)
> > 
> > 
> > The patch is ok, but I think the naming of this function is too generic,
> > so I'm renaming it to:
> > 
> > static int perf_evsel__open_adjust_precise_ip(struct perf_evsel *evsel,
> > 					      pid_t pid, int cpu, int group_fd,
> > 					      unsigned long flags)
> > 
> > Ok?
> 
> ok
> 
> > 
> > The perf_evsel__open() code is already complex with that fallback
> > mechanism, this is just one more way of fallbacking when asking the
> > kernel for something that may fail.
> > 
> > In fact what happens if the precise_ip that is being asked _is_
> > supported but sys_perf_event_open() fails because some other
> > perf_event_attr attribute that is set is not supported? 
> 
> it's outside the scope of standard feature fallback code,
> so we will try it for any possible fallback variant, so:
> 
> we will try all possible precise_ip (3,2,1,0) and they will
> all fail because of the unsupported attribute - so we will
> restore the precise_ip back and continue in standard fallback
> code that will eventualy switch that attribute off
> 
> > 
> > I see, it gets it back restored to what the user asked so that the
> > standard fallback is tried, ok, I'll apply with just the rename for this
> > function,
> 
> thanks,
> jirka

ping, there's rebased version in my perf/fixes branch

jirka

Re: [PATCH] perf tools: Move precise_ip detection into perf_evsel__open

[Arnaldo Carvalho de Melo]

Em Sat, Mar 23, 2019 at 04:04:22PM +0100, Jiri Olsa escreveu:
> On Fri, Mar 15, 2019 at 03:52:25PM +0100, Jiri Olsa wrote:
> > On Fri, Mar 15, 2019 at 11:35:04AM -0300, Arnaldo Carvalho de Melo wrote:
> > > The perf_evsel__open() code is already complex with that fallback
> > > mechanism, this is just one more way of fallbacking when asking the
> > > kernel for something that may fail.

> > > In fact what happens if the precise_ip that is being asked _is_
> > > supported but sys_perf_event_open() fails because some other
> > > perf_event_attr attribute that is set is not supported? 

> > it's outside the scope of standard feature fallback code,
> > so we will try it for any possible fallback variant, so:

> > we will try all possible precise_ip (3,2,1,0) and they will
> > all fail because of the unsupported attribute - so we will
> > restore the precise_ip back and continue in standard fallback
> > code that will eventualy switch that attribute off
 
> > > I see, it gets it back restored to what the user asked so that the
> > > standard fallback is tried, ok, I'll apply with just the rename for this
> > > function,
 
> ping, there's rebased version in my perf/fixes branch

Thanks, applied.

- Arnaldo