[PATCH 0/3] perf tools: 3 fixes

[PATCH 0/3] perf tools: 3 fixes

[Adrian Hunter]

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

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

[Adrian Hunter]

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.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/dso.c     | 17 +++++++++++++++++
 tools/perf/util/dso.h     |  1 +
 tools/perf/util/machine.c |  1 +
 3 files changed, 19 insertions(+)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 7c0c08386a1d..425df5c86c9c 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -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);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index fc8db9c764ac..45ec4d0a50ed 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -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 {
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 5ef90be2a249..8b303ff20289 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -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);
 	}
-- 
1.9.1

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

[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.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/build-id.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index d909459fb54c..217b5a60e2ab 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -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)
-- 
1.9.1

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

[Adrian Hunter]

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

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-inject.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 0a945d2e8ca5..99d127fe9c35 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -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,
-- 
1.9.1

Re: [PATCH 0/3] perf tools: 3 fixes

[Arnaldo Carvalho de Melo]

Em Fri, Nov 13, 2015 at 11:48:29AM +0200, Adrian Hunter escreveu:
> 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
 
> 
> 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

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

[tip-bot for Adrian Hunter]

Commit-ID:  e266a753bf51b2c3b46d0d230349662c35ac5629
Gitweb:     http://git.kernel.org/tip/e266a753bf51b2c3b46d0d230349662c35ac5629
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Fri, 13 Nov 2015 11:48:30 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Douglas Hatch <doug.hatch@hp.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Scott J Norton <scott.norton@hp.com>
Cc: Waiman Long <Waiman.Long@hp.com>
Fixes: 4598a0a6d22f ("perf symbols: Improve DSO long names lookup speed with rbtree")
Link: http://lkml.kernel.org/r/1447408112-1920-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c     | 17 +++++++++++++++++
 tools/perf/util/dso.h     |  1 +
 tools/perf/util/machine.c |  1 +
 3 files changed, 19 insertions(+)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 7c0c083..425df5c 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -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);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index fc8db9c..45ec4d0 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -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 {
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 5ef90be..8b303ff 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -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);
 	}

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

[tip-bot for Adrian Hunter]

Commit-ID:  1216b65c502e0f130cc9984dfd5f9e1968c1eb46
Gitweb:     http://git.kernel.org/tip/1216b65c502e0f130cc9984dfd5f9e1968c1eb46
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Fri, 13 Nov 2015 11:48:31 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/1447408112-1920-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/build-id.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index d909459..217b5a6 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -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)

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

[tip-bot for Adrian Hunter]

Commit-ID:  d8145b3e30a24280c396d88c8703c50a1ea0aa3a
Gitweb:     http://git.kernel.org/tip/d8145b3e30a24280c396d88c8703c50a1ea0aa3a
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Fri, 13 Nov 2015 11:48:32 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/1447408112-1920-4-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-inject.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 0a945d2..99d127f 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -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,