Skip to contentSkip to conversation

perf tools: 3 fixes

PATCH In review Adrian Hunter 3 participants 8 messages Nov 13, 2015 – Nov 18, 2015 perf events

lore
Share & tools
lore

Adrian Hunter

author reviewer
#0 lore raw

cover perf tools: 3 fixes

Hi

Here are a 3 fixes. The patches apply cleanly to 4.3, although the problems go back further than that.

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

 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 +
 5 files changed, 21 insertions(+)

Regards
Adrian

author reviewer
#1 lore raw

PATCH 1/3 perf tools: Fix dso lookup by long name and missing buildids

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(+)
tools/perf/util/dso.c +17 -0
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);
tools/perf/util/dso.h +1 -0
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 {
tools/perf/util/machine.c +1 -0
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);
 	}
Signature · 1 line
1.9.1
#2 lore raw

[tip:perf/urgent] perf symbols: Fix dso lookup by long name and missing buildids

perf symbols: Fix dso lookup by long name and missing buildids
Commit-ID:  e266a753bf51b2c3b46d0d230349662c35ac5629
Gitweb:     http://git.kernel.org/tip/e266a753bf51b2c3b46d0d230349662c35ac5629
Author:     Adrian Hunter <[email protected]>
AuthorDate: Fri, 13 Nov 2015 11:48:30 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 13 Nov 2015 11:14:36 -0300

perf symbols: Fix dso lookup by long name and missing buildids

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.

Cc Adrian Hunter <[email protected]>
Cc Don Zickus <[email protected]>
Cc Douglas Hatch <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Scott J Norton <[email protected]>
Cc Waiman Long <[email protected]>
Fixes 4598a0a6d22f ("perf symbols: Improve DSO long names lookup speed with rbtree")
---
 tools/perf/util/dso.c     | 17 +++++++++++++++++
 tools/perf/util/dso.h     |  1 +
 tools/perf/util/machine.c |  1 +
 3 files changed, 19 insertions(+)
tools/perf/util/dso.c +17 -0
index 7c0c083..425df5c 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);
tools/perf/util/dso.h +1 -0
index fc8db9c..45ec4d0 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 {
tools/perf/util/machine.c +1 -0
index 5ef90be..8b303ff 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);
 	}
author reviewer
#3 lore raw

PATCH 2/3 perf buildid-list: Requires ordered events

In reply to Adrian Hunter

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(+)
tools/perf/util/build-id.c +1 -0
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)
Signature · 1 line
1.9.1
#4 lore raw

[tip:perf/urgent] perf buildid-list: Requires ordered events

perf buildid-list: Requires ordered events
Commit-ID:  1216b65c502e0f130cc9984dfd5f9e1968c1eb46
Gitweb:     http://git.kernel.org/tip/1216b65c502e0f130cc9984dfd5f9e1968c1eb46
Author:     Adrian Hunter <[email protected]>
AuthorDate: Fri, 13 Nov 2015 11:48:31 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 13 Nov 2015 12:22:04 -0300

perf buildid-list: Requires ordered events

'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(+)
tools/perf/util/build-id.c +1 -0
index d909459..217b5a6 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)
author reviewer
#5 lore raw

PATCH 3/3 perf inject: Also re-pipe lost_samples event

In reply to Adrian Hunter

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(+)
tools/perf/builtin-inject.c +1 -0
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,
Signature · 1 line
1.9.1
#6 lore raw

[tip:perf/urgent] perf inject: Also re-pipe lost_samples event

perf inject: Also re-pipe lost_samples event
Commit-ID:  d8145b3e30a24280c396d88c8703c50a1ea0aa3a
Gitweb:     http://git.kernel.org/tip/d8145b3e30a24280c396d88c8703c50a1ea0aa3a
Author:     Adrian Hunter <[email protected]>
AuthorDate: Fri, 13 Nov 2015 11:48:32 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 13 Nov 2015 12:23:12 -0300

perf inject: Also re-pipe lost_samples event

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(+)
tools/perf/builtin-inject.c +1 -0
index 0a945d2..99d127f 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,
#7 lore raw
In reply to Adrian Hunter

Em Fri, Nov 13, 2015 at 11:48:29AM +0200, Adrian Hunter escreveu:

Quoted 4 lines

Hi

Here are a 3 fixes. The patches apply cleanly to 4.3, although the problems go back further than that.

Thanks for the patches and for the before/after for the rbtree problem, I reproduced it here, fixes the problem!

- Arnaldo

Quoted 16 lines

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

 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 +
 5 files changed, 21 insertions(+)

Regards
Adrian