perf/urgent fixes

Hi Ingo,

Please consider pulling,

- Arnaldo

The following changes since commit 2059fc7a5a9e667797b8ec503bfb4685afee48d8:

perf symbols: Allow forcing reading of non-root owned files by root (2015-11-12 18:58:18 -0300)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo

for you to fetch changes up to 092b1f0b5f9f797812da0de927c3aa26acbe8762:

perf probe: Clear probe_trace_event when add_probe_trace_event() fails (2015-11-13 12:28:09 -0300)

----------------------------------------------------------------
perf/urgent fixes:

- Do not change the key of an object in a rbtree, this time it was the one for DSOs lookup by its long_name, and the noticed symptom was with 'perf buildid-list --with-hits' (Adrian Hunter)

- 'perf inject' is a pipe, events it doesn't touch should be passed on, PERF_RECORD_LOST wasn't, fix it (Adrian Hunter)

- Make 'perf buildid-list' request event ordering, as it needs to first get the mmap events to be able to mark wich DSOs had hits (Adrian Hunter)

- Fix memory leaks on failure in 'perf probe' (Masami Hiramatsu, Wang Nan)

----------------------------------------------------------------
Adrian Hunter (3):
perf symbols: Fix dso lookup by long name and missing buildids perf buildid-list: Requires ordered events
perf inject: Also re-pipe lost_samples event

Masami Hiramatsu (1):
perf probe: Fix memory leaking on failure by clearing all probe_trace_events

Wang Nan (1):
perf probe: Clear probe_trace_event when add_probe_trace_event() fails

 tools/perf/builtin-inject.c    |  1 +
 tools/perf/util/build-id.c     |  1 +
 tools/perf/util/dso.c          | 17 +++++++++++++++++
 tools/perf/util/dso.h          |  1 +
 tools/perf/util/machine.c      |  1 +
 tools/perf/util/probe-finder.c | 24 +++++++++++++++++-------
 6 files changed, 38 insertions(+), 7 deletions(-)

From: Adrian Hunter <[email protected]>

Commit 4598a0a6d22f ("perf symbols: Improve DSO long names lookup speed with rbtree") Added a tree to lookup dsos by long name. That tree gets corrupted whenever a dso long name is changed because the tree is not updated.

One effect of that is buildid-list does not work with the 'with-hits' option because dso lookup fails and results in two structs for the same dso. The first has the buildid but no hits, the second has hits but no buildid. e.g.

Before:

  $ tools/perf/perf record ls
  arch     certs    CREDITS  Documentation  firmware  include
  ipc      Kconfig  lib      Makefile       net       REPORTING-BUGS
  scripts  sound    usr      block          COPYING   crypto
  drivers  fs       init     Kbuild         kernel    MAINTAINERS
  mm       README   samples  security       tools     virt
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.012 MB perf.data (11 samples) ]
  $ tools/perf/perf buildid-list
  574da826c66538a8d9060d393a8866289bd06005 [kernel.kallsyms]
  30c94dc66a1fe95180c3d68d2b89e576d5ae213c /lib/x86_64-linux-gnu/libc-2.19.so
  $ tools/perf/perf buildid-list -H
  574da826c66538a8d9060d393a8866289bd06005 [kernel.kallsyms]
  0000000000000000000000000000000000000000 /lib/x86_64-linux-gnu/libc-2.19.so

After:

  $ tools/perf/perf buildid-list -H
  574da826c66538a8d9060d393a8866289bd06005 [kernel.kallsyms]
  30c94dc66a1fe95180c3d68d2b89e576d5ae213c /lib/x86_64-linux-gnu/libc-2.19.so

The fix is to record the root of the tree on the dso so that dso__set_long_name() can update the tree when the long name changes.

---
 tools/perf/util/dso.c     | 17 +++++++++++++++++
 tools/perf/util/dso.h     |  1 +
 tools/perf/util/machine.c |  1 +
 3 files changed, 19 insertions(+)

index 7c0c08386a1d..425df5c86c9c 100644
@@ -933,6 +933,7 @@ static struct dso *__dso__findlink_by_longname(struct rb_root *root,
 		/* Add new node and rebalance tree */
 		rb_link_node(&dso->rb_node, parent, p);
 		rb_insert_color(&dso->rb_node, root);
+		dso->root = root;
 	}
 	return NULL;
 }

@@ -945,15 +946,30 @@ static inline struct dso *__dso__find_by_longname(struct rb_root *root,
 
 void dso__set_long_name(struct dso *dso, const char *name, bool name_allocated)
 {
+	struct rb_root *root = dso->root;
+
 	if (name == NULL)
 		return;
 
 	if (dso->long_name_allocated)
 		free((char *)dso->long_name);
 
+	if (root) {
+		rb_erase(&dso->rb_node, root);
+		/*
+		 * __dso__findlink_by_longname() isn't guaranteed to add it
+		 * back, so a clean removal is required here.
+		 */
+		RB_CLEAR_NODE(&dso->rb_node);
+		dso->root = NULL;
+	}
+
 	dso->long_name		 = name;
 	dso->long_name_len	 = strlen(name);
 	dso->long_name_allocated = name_allocated;
+
+	if (root)
+		__dso__findlink_by_longname(root, dso, NULL);
 }
 
 void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated)

@@ -1046,6 +1062,7 @@ struct dso *dso__new(const char *name)
 		dso->kernel = DSO_TYPE_USER;
 		dso->needs_swap = DSO_SWAP__UNSET;
 		RB_CLEAR_NODE(&dso->rb_node);
+		dso->root = NULL;
 		INIT_LIST_HEAD(&dso->node);
 		INIT_LIST_HEAD(&dso->data.open_entry);
 		pthread_mutex_init(&dso->lock, NULL);

index fc8db9c764ac..45ec4d0a50ed 100644
@@ -135,6 +135,7 @@ struct dso {
 	pthread_mutex_t	 lock;
 	struct list_head node;
 	struct rb_node	 rb_node;	/* rbtree node sorted by long name */
+	struct rb_root	 *root;		/* root of rbtree that rb_node is in */
 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
 	struct {

index 5ef90be2a249..8b303ff20289 100644
@@ -91,6 +91,7 @@ static void dsos__purge(struct dsos *dsos)
 
 	list_for_each_entry_safe(pos, n, &dsos->head, node) {
 		RB_CLEAR_NODE(&pos->rb_node);
+		pos->root = NULL;
 		list_del_init(&pos->node);
 		dso__put(pos);
 	}

2.1.0

From: Adrian Hunter <[email protected]>

'perf buildid-list' processes events to determine hits (i.e. with-hits option). That may not work if events are not sorted in order. i.e. MMAP events must be processed before the samples that depend on them so that sample processing can 'hit' the DSO to which the MMAP refers.

---
 tools/perf/util/build-id.c | 1 +
 1 file changed, 1 insertion(+)

index d909459fb54c..217b5a60e2ab 100644
@@ -76,6 +76,7 @@ struct perf_tool build_id__mark_dso_hit_ops = {
 	.exit	= perf_event__exit_del_thread,
 	.attr		 = perf_event__process_attr,
 	.build_id	 = perf_event__process_build_id,
+	.ordered_events	 = true,
 };
 
 int build_id__sprintf(const u8 *build_id, int len, char *bf)

2.1.0

From: Adrian Hunter <[email protected]>

perf inject must re-pipe all events otherwise they get dropped from the output file.

---
 tools/perf/builtin-inject.c | 1 +
 1 file changed, 1 insertion(+)

index 0a945d2e8ca5..99d127fe9c35 100644
@@ -675,6 +675,7 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 			.fork		= perf_event__repipe,
 			.exit		= perf_event__repipe,
 			.lost		= perf_event__repipe,
+			.lost_samples	= perf_event__repipe,
 			.aux		= perf_event__repipe,
 			.itrace_start	= perf_event__repipe,
 			.context_switch	= perf_event__repipe,

2.1.0

From: Masami Hiramatsu <[email protected]>

Fix memory leaking on the debuginfo__find_trace_events() failure path which frees an array of probe_trace_events but doesn't clears all the allocated sub-structures and strings.

So, before doing zfree(tevs), clear all the array elements which may have allocated resources.

---
 tools/perf/util/probe-finder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index bd8f03de5e40..63993d7e0fac 100644
@@ -1246,7 +1246,7 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
 	struct trace_event_finder tf = {
 			.pf = {.pev = pev, .callback = add_probe_trace_event},
 			.max_tevs = probe_conf.max_probes, .mod = dbg->mod};
-	int ret;
+	int ret, i;
 
 	/* Allocate result tevs array */
 	*tevs = zalloc(sizeof(struct probe_trace_event) * tf.max_tevs);

@@ -1258,6 +1258,8 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
 
 	ret = debuginfo__find_probes(dbg, &tf.pf);
 	if (ret < 0) {
+		for (i = 0; i < tf.ntevs; i++)
+			clear_probe_trace_event(&tf.tevs[i]);
 		zfree(tevs);
 		return ret;
 	}

2.1.0

From: Wang Nan <[email protected]>

When probing with a glob, errors in add_probe_trace_event() won't be passed to debuginfo__find_trace_events() because it would be modified by probe_point_search_cb(). It causes a segfault if perf fails to find an argument for a probe point matched by the glob. For example:

  # ./perf probe -v -n 'SyS_dup? oldfd'
  probe-definition(0): SyS_dup? oldfd
  symbol:SyS_dup? file:(null) line:0 offset:0 return:0 lazy:(null)
  parsing arg: oldfd into oldfd
  1 arguments
  Looking at the vmlinux_path (7 entries long)
  Using /lib/modules/4.3.0-rc4+/build/vmlinux for symbols
  Open Debuginfo file: /lib/modules/4.3.0-rc4+/build/vmlinux
  Try to find probe point from debuginfo.
  Matched function: SyS_dup3
  found inline addr: 0xffffffff812095c0
  Probe point found: SyS_dup3+0
  Searching 'oldfd' variable in context.
  Converting variable oldfd into trace event.
  oldfd type is long int.
  found inline addr: 0xffffffff812096d4
  Probe point found: SyS_dup2+36
  Searching 'oldfd' variable in context.
  Failed to find 'oldfd' in this function.
  Matched function: SyS_dup3
  Probe point found: SyS_dup3+0
  Searching 'oldfd' variable in context.
  Converting variable oldfd into trace event.
  oldfd type is long int.
  Matched function: SyS_dup2
  Probe point found: SyS_dup2+0
  Searching 'oldfd' variable in context.
  Converting variable oldfd into trace event.
  oldfd type is long int.
  Found 4 probe_trace_events.
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Writing event: p:probe/SyS_dup3 _text+2135488 oldfd=%di:s64
  Segmentation fault (core dumped)
  #

This patch ensures that add_probe_trace_event() doesn't touches tf->ntevs and tf->tevs if those functions fail.

After the patch:

  # perf probe  'SyS_dup? oldfd'
  Failed to find 'oldfd' in this function.
  Added new events:
    probe:SyS_dup3       (on SyS_dup? with oldfd)
    probe:SyS_dup3_1     (on SyS_dup? with oldfd)
    probe:SyS_dup2       (on SyS_dup? with oldfd)

You can now use it in all perf tools, such as:

perf record -e probe:SyS_dup2 -aR sleep 1

---
 tools/perf/util/probe-finder.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

index 63993d7e0fac..05012bb178d7 100644
@@ -1183,7 +1183,7 @@ static int add_probe_trace_event(Dwarf_Die *sc_die, struct probe_finder *pf)
 			container_of(pf, struct trace_event_finder, pf);
 	struct perf_probe_point *pp = &pf->pev->point;
 	struct probe_trace_event *tev;
-	struct perf_probe_arg *args;
+	struct perf_probe_arg *args = NULL;
 	int ret, i;
 
 	/* Check number of tevs */

@@ -1198,19 +1198,23 @@ static int add_probe_trace_event(Dwarf_Die *sc_die, struct probe_finder *pf)
 	ret = convert_to_trace_point(&pf->sp_die, tf->mod, pf->addr,
 				     pp->retprobe, pp->function, &tev->point);
 	if (ret < 0)
-		return ret;
+		goto end;
 
 	tev->point.realname = strdup(dwarf_diename(sc_die));
-	if (!tev->point.realname)
-		return -ENOMEM;
+	if (!tev->point.realname) {
+		ret = -ENOMEM;
+		goto end;
+	}
 
 	pr_debug("Probe point found: %s+%lu\n", tev->point.symbol,
 		 tev->point.offset);
 
 	/* Expand special probe argument if exist */
 	args = zalloc(sizeof(struct perf_probe_arg) * MAX_PROBE_ARGS);
-	if (args == NULL)
-		return -ENOMEM;
+	if (args == NULL) {
+		ret = -ENOMEM;
+		goto end;
+	}
 
 	ret = expand_probe_args(sc_die, pf, args);
 	if (ret < 0)

@@ -1234,6 +1238,10 @@ static int add_probe_trace_event(Dwarf_Die *sc_die, struct probe_finder *pf)
 	}
 
 end:
+	if (ret) {
+		clear_probe_trace_event(tev);
+		tf->ntevs--;
+	}
 	free(args);
 	return ret;
 }

2.1.0

Pulled, thanks a lot Arnaldo!

Ingo