perf/core improvements and fixes

From: Arnaldo Carvalho de Melo <[email protected]>

Hi Ingo,

Please consider pulling,

- Arnaldo

The following changes since commit d18929e9fde30c4d57ae57eb9a7f6f10b5808ca1:

Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core (2015-12-10 09:10:40 +0100)

are available in the git repository at:

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

for you to fetch changes up to e7a7865cc0da306542db0b9205cb0a467f59e33d:

perf symbols: Fix dso__load_sym to put dso (2015-12-10 16:29:32 -0300)

----------------------------------------------------------------
perf/core refactorings and fixes:

Infrastructure:

- Revert "perf tools: Improve setting of gcc debug option", -Og is broken, GCC PR created (Jiri Olsa)

- More reference count fixes (Masami Hiramatsu)

- Untangle browser setup (--stdio, --tui, etc) from argument checking, prep work to move the usage() code out of tools/perf for use by other tools/ living utilities (Namhyung Kim)

- Delete half-processed hist entries when exiting 'perf top' (Namhyung Kim)

----------------------------------------------------------------
Arnaldo Carvalho de Melo (2):
perf top: Do show usage message when failing to create cpu/thread maps Revert "perf tools: Improve setting of gcc debug option"

Masami Hiramatsu (2):
perf tools: Make perf_session__register_idle_thread drop the refcount perf symbols: Fix dso__load_sym to put dso

Namhyung Kim (7):
perf annotate: Check argument before calling setup_browser() perf annotate: Delay UI browser setup after initialization is done perf kvm: Remove invocation of setup/exit_browser() perf report: Check argument before calling setup_browser() perf thread_map: Free strlist on constructor error path perf tools: Get rid of exit_browser() from usage_with_options() perf top: Delete half-processed hist entries when exit

 tools/perf/builtin-annotate.c   | 33 ++++++++++++++++-----------------
 tools/perf/builtin-kvm.c        |  3 ---
 tools/perf/builtin-report.c     | 21 ++++++++++-----------
 tools/perf/builtin-top.c        |  9 ++++++---
 tools/perf/config/Makefile      |  2 --
 tools/perf/config/utilities.mak | 19 -------------------
 tools/perf/util/hist.c          | 26 +++++++++++++++++++++++++-
 tools/perf/util/parse-options.c |  3 ---
 tools/perf/util/session.c       | 11 +++++++----
 tools/perf/util/session.h       |  2 +-
 tools/perf/util/symbol-elf.c    |  9 +++++++--
 tools/perf/util/thread_map.c    |  1 +
 12 files changed, 73 insertions(+), 66 deletions(-)

From: Namhyung Kim <[email protected]>

This is necessary to get rid of the browser dependency from usage_with_options() and its friends. Because there's no code changing the argc and argv, it'd be ok to check it early.

---
 tools/perf/builtin-annotate.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

index 2bf9b3fd9e61..55f6f8dab5d4 100644
@@ -343,6 +343,16 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		return ret;
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(annotate_usage, options);
+
+		annotate.sym_hist_filter = argv[0];
+	}
 
 	if (annotate.use_stdio)
 		use_browser = 0;

@@ -369,17 +379,6 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(annotate_usage, options);
-
-		annotate.sym_hist_filter = argv[0];
-	}
-
 	ret = __cmd_annotate(&annotate);
 
 out_delete:

2.1.0

From: Namhyung Kim <[email protected]>

Move setup_browser after all necessary initialization is done. This is to remove the browser dependency from usage_with_options and friends.

---
 tools/perf/builtin-annotate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 55f6f8dab5d4..1f00dc7cecba 100644
@@ -354,17 +354,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		annotate.sym_hist_filter = argv[0];
 	}
 
-	if (annotate.use_stdio)
-		use_browser = 0;
-	else if (annotate.use_tui)
-		use_browser = 1;
-	else if (annotate.use_gtk)
-		use_browser = 2;
-
 	file.path  = input_name;
 
-	setup_browser(true);
-
 	annotate.session = perf_session__new(&file, false, &annotate.tool);
 	if (annotate.session == NULL)
 		return -1;

@@ -379,6 +370,15 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
+	if (annotate.use_stdio)
+		use_browser = 0;
+	else if (annotate.use_tui)
+		use_browser = 1;
+	else if (annotate.use_gtk)
+		use_browser = 2;
+
+	setup_browser(true);
+
 	ret = __cmd_annotate(&annotate);
 
 out_delete:

2.1.0

From: Namhyung Kim <[email protected]>

Calling setup_browser(false) with use_browser = 0 is meaningless. Just get rid of it. This is necessary to remove the browser dependency from usage_with_options() and friends.

---
 tools/perf/builtin-kvm.c | 3 ---
 1 file changed, 3 deletions(-)

index dd94b4ca2213..031f9f55c281 100644
@@ -1351,7 +1351,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	disable_buildid_cache();
 
 	use_browser = 0;
-	setup_browser(false);
 
 	if (argc) {
 		argc = parse_options(argc, argv, live_options,

@@ -1409,8 +1408,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	err = kvm_events_live_report(kvm);
 
 out:
-	exit_browser(0);
-
 	if (kvm->session)
 		perf_session__delete(kvm->session);
 	kvm->session = NULL;

2.1.0

From: Namhyung Kim <[email protected]>

This is necessary to get rid of the browser dependency from usage_with_options() and its friends. Because there's no code changing the argc and argv, it'd be ok to check it early.

---
 tools/perf/builtin-report.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

index af5db885ea9c..5a454669d075 100644
@@ -801,6 +801,16 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_config(report__config, &report);
 
 	argc = parse_options(argc, argv, options, report_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(report_usage, options);
+
+		report.symbol_filter_str = argv[0];
+	}
 
 	if (symbol_conf.vmlinux_name &&
 	    access(symbol_conf.vmlinux_name, R_OK)) {

@@ -946,17 +956,6 @@ repeat:
 	if (symbol__init(&session->header.env) < 0)
 		goto error;
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(report_usage, options);
-
-		report.symbol_filter_str = argv[0];
-	}
-
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);

2.1.0

From: Arnaldo Carvalho de Melo <[email protected]>

This is necessary to get rid of the browser dependency from usage_with_options() and its friends. Because we validate the targets which are used to create the cpu/thread maps and inform the user about any override performed via the chosen UI, we don't need to call the usage routine for that.

---
 tools/perf/builtin-top.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 7e2e72e6d9d1..84fd6368ed6d 100644
@@ -1279,8 +1279,11 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (target__none(target))
 		target->system_wide = true;
 
-	if (perf_evlist__create_maps(top.evlist, target) < 0)
-		usage_with_options(top_usage, options);
+	if (perf_evlist__create_maps(top.evlist, target) < 0) {
+		ui__error("Couldn't create thread/CPU maps: %s\n",
+			  errno == ENOENT ? "No such process" : strerror_r(errno, errbuf, sizeof(errbuf)));
+		goto out_delete_evlist;
+	}
 
 	if (!top.evlist->nr_entries &&
 	    perf_evlist__add_default(top.evlist) < 0) {

2.1.0

From: Namhyung Kim <[email protected]>

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

index 6ec3c5ca438f..371fb28fe5b1 100644
@@ -304,6 +304,7 @@ out:
 
 out_free_threads:
 	zfree(&threads);
+	strlist__delete(slist);
 	goto out;
 }
 

2.1.0

From: Namhyung Kim <[email protected]>

Since all of its users call before setup_browser(), there's no need to call exit_browser() inside of the function.

---
 tools/perf/util/parse-options.c | 3 ---
 1 file changed, 3 deletions(-)

index d09aff983581..de3290b47db1 100644
@@ -766,7 +766,6 @@ int usage_with_options_internal(const char * const *usagestr,
 void usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	exit_browser(false);
 	usage_with_options_internal(usagestr, opts, 0, NULL);
 	exit(129);
 }

@@ -776,8 +775,6 @@ void usage_with_options_msg(const char * const *usagestr,
 {
 	va_list ap;
 
-	exit_browser(false);
-
 	va_start(ap, fmt);
 	strbuf_addv(&error_buf, fmt, ap);
 	va_end(ap);

2.1.0

From: Namhyung Kim <[email protected]>

After sample processing is done, hist entries are in both of hists->entries and hists->entries_in (or hists->entries_collapsed). So I guess perf report does not have leaks on hists.

But for perf top, it's possible to have half-processed entries which are only in hists->entries_in. Eventually they will go to the hists->entries and get freed but they cannot be deleted by current hists__delete_entries(). This patch adds hists__delete_all_entries function to delete those entries.

---
 tools/perf/util/hist.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

index 565ea3549894..56e97f5af598 100644
@@ -270,6 +270,8 @@ static void hists__delete_entry(struct hists *hists, struct hist_entry *he)
 
 	if (sort__need_collapse)
 		rb_erase(&he->rb_node_in, &hists->entries_collapsed);
+	else
+		rb_erase(&he->rb_node_in, hists->entries_in);
 
 	--hists->nr_entries;
 	if (!he->filtered)

@@ -1567,11 +1569,33 @@ static int hists_evsel__init(struct perf_evsel *evsel)
 	return 0;
 }
 
+static void hists__delete_remaining_entries(struct rb_root *root)
+{
+	struct rb_node *node;
+	struct hist_entry *he;
+
+	while (!RB_EMPTY_ROOT(root)) {
+		node = rb_first(root);
+		rb_erase(node, root);
+
+		he = rb_entry(node, struct hist_entry, rb_node_in);
+		hist_entry__delete(he);
+	}
+}
+
+static void hists__delete_all_entries(struct hists *hists)
+{
+	hists__delete_entries(hists);
+	hists__delete_remaining_entries(&hists->entries_in_array[0]);
+	hists__delete_remaining_entries(&hists->entries_in_array[1]);
+	hists__delete_remaining_entries(&hists->entries_collapsed);
+}
+
 static void hists_evsel__exit(struct perf_evsel *evsel)
 {
 	struct hists *hists = evsel__hists(evsel);
 
-	hists__delete_entries(hists);
+	hists__delete_all_entries(hists);
 }
 
 /*

2.1.0

From: Arnaldo Carvalho de Melo <[email protected]>

This reverts commit e8b7ea4356fdd3c4de5478f3418eb84f8dce2b61.

Martin created a gcc PR:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68836

---
 tools/perf/config/Makefile      |  2 --
 tools/perf/config/utilities.mak | 19 -------------------
 2 files changed, 21 deletions(-)

index 6eb9a956a408..a5524179d26e 100644
@@ -135,8 +135,6 @@ endif
 
 ifeq ($(DEBUG),0)
   CFLAGS += -O6
-else
-  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG

index 0ebef09c0842..c16ce833079c 100644
@@ -177,22 +177,3 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
-
-# try-run
-# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
-# Exit code chooses option. "$$TMP" is can be used as temporary file and
-# is automatically cleaned up.
-try-run = $(shell set -e;		\
-	TMP="$(TMPOUT).$$$$.tmp";	\
-	TMPO="$(TMPOUT).$$$$.o";	\
-	if ($(1)) >/dev/null 2>&1;	\
-	then echo "$(2)";		\
-	else echo "$(3)";		\
-	fi;				\
-	rm -f "$$TMP" "$$TMPO")
-
-# cc-option
-# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
-
-cc-option = $(call try-run,\
-	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))

2.1.0

From: Masami Hiramatsu <[email protected]>

Note that since the thread was already inserted to the session list, it will be released when the session is released. Also, in perf_session__register_idle_thread() failure path, the thread should be put before returning.

Refcnt debugger shows that the perf_session__register_idle_thread gets the returned thread, but the caller (__cmd_top) does not put the returned idle thread.

----
==== [0] ====
Unreclaimed thread@0x24e6240
Refcount +1 => 0 at

    ./perf(thread__new+0xe5) [0x4c8a75]
    ./perf(machine__findnew_thread+0x9a) [0x4bbdba]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]
  Refcount +1 => 1 at
    ./perf(thread__get+0x2c) [0x4c8bcc]
    ./perf(machine__findnew_thread+0xee) [0x4bbe0e]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]
  Refcount +1 => 2 at
    ./perf(thread__get+0x2c) [0x4c8bcc]
    ./perf(machine__findnew_thread+0x112) [0x4bbe32]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]

----

---
 tools/perf/builtin-top.c  |  2 +-
 tools/perf/util/session.c | 11 +++++++----
 tools/perf/util/session.h |  2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

index 84fd6368ed6d..785aa2dd8f0b 100644
@@ -964,7 +964,7 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	if (perf_session__register_idle_thread(top->session) == NULL)
+	if (perf_session__register_idle_thread(top->session) < 0)
 		goto out_delete;
 
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,

index c35ffdd360fe..9774686525b4 100644
@@ -1311,17 +1311,20 @@ struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
 	return machine__findnew_thread(&session->machines.host, -1, pid);
 }
 
-struct thread *perf_session__register_idle_thread(struct perf_session *session)
+int perf_session__register_idle_thread(struct perf_session *session)
 {
 	struct thread *thread;
+	int err = 0;
 
 	thread = machine__findnew_thread(&session->machines.host, 0, 0);
 	if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
 		pr_err("problem inserting idle task.\n");
-		thread = NULL;
+		err = -1;
 	}
 
-	return thread;
+	/* machine__findnew_thread() got the thread, so put it */
+	thread__put(thread);
+	return err;
 }
 
 static void perf_session__warn_about_errors(const struct perf_session *session)

@@ -1676,7 +1679,7 @@ int perf_session__process_events(struct perf_session *session)
 	u64 size = perf_data_file__size(session->file);
 	int err;
 
-	if (perf_session__register_idle_thread(session) == NULL)
+	if (perf_session__register_idle_thread(session) < 0)
 		return -ENOMEM;
 
 	if (!perf_data_file__is_pipe(session->file))

index 3e900c0efc73..5f792e35d4c1 100644
@@ -89,7 +89,7 @@ struct machine *perf_session__findnew_machine(struct perf_session *session, pid_
 }
 
 struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
-struct thread *perf_session__register_idle_thread(struct perf_session *session);
+int perf_session__register_idle_thread(struct perf_session *session);
 
 size_t perf_session__fprintf(struct perf_session *session, FILE *fp);
 

2.1.0

From: Masami Hiramatsu <[email protected]>

Fix dso__load_sym to put dso because dsos__add already got it.

Refcnt debugger explain the problem:
----
==== [0] ====
Unreclaimed dso: 0x19dd200
Refcount +1 => 1 at

    ./perf(dso__new+0x1ff) [0x4a62df]
    ./perf(dso__load_sym+0xe89) [0x503509]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount +1 => 2 at
    ./perf(dso__get+0x34) [0x4a65f4]
    ./perf(map__new2+0x76) [0x4be216]
    ./perf(dso__load_sym+0xee1) [0x503561]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount +1 => 3 at
    ./perf(dsos__add+0xf3) [0x4a6bc3]
    ./perf(dso__load_sym+0xfc1) [0x503641]
    ./perf(dso__load_vmlinux+0xbf) [0x4aa77f]
    ./perf(dso__load_vmlinux_path+0x8c) [0x4aa8dc]
    ./perf() [0x50539a]
    ./perf(convert_perf_probe_events+0xd79) [0x50ad39]
    ./perf() [0x45600f]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount -1 => 2 at
    ./perf(dso__put+0x2f) [0x4a664f]
    ./perf(map_groups__exit+0xb9) [0x4bee29]
    ./perf(machine__delete+0xb0) [0x4b93d0]
    ./perf(exit_probe_symbol_maps+0x28) [0x506718]
    ./perf() [0x45628a]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]
  Refcount -1 => 1 at
    ./perf(dso__put+0x2f) [0x4a664f]
    ./perf(machine__delete+0xfe) [0x4b941e]
    ./perf(exit_probe_symbol_maps+0x28) [0x506718]
    ./perf() [0x45628a]
    ./perf(cmd_probe+0x6c) [0x4566bc]
    ./perf() [0x47abc5]
    ./perf(main+0x610) [0x421f90]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f74dd0efaf5]
    ./perf() [0x4220a9]

----
So, in the dso__load_sym, dso is gotten 3 times, by dso__new, map__new2, and dsos__add. The last 2 is actually released by map_groups and machine__delete correspondingly. However, the first reference by dso__new, is never released.

Committer note:

Changed the place where the reference count is dropped to:

Fix it by dropping it right after creating curr_map, since we know that either that operation failed and we need to drop the dso refcount or that it succeed and we have it referenced via curr_map->dso.

Then only drop the curr_map refcount after we call dsos__add() to make sure we hold a reference to it via curr_map->dso.

---
 tools/perf/util/symbol-elf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 53f19968bfa2..562b8ebeae5b 100644
@@ -1026,8 +1026,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
 				curr_dso->long_name_len = dso->long_name_len;
 				curr_map = map__new2(start, curr_dso,
 						     map->type);
+				dso__put(curr_dso);
 				if (curr_map == NULL) {
-					dso__put(curr_dso);
 					goto out_elf_end;
 				}
 				if (adjust_kernel_syms) {

@@ -1042,9 +1042,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
 				}
 				curr_dso->symtab_type = dso->symtab_type;
 				map_groups__insert(kmaps, curr_map);
+				/*
+				 * Add it before we drop the referece to curr_map,
+				 * i.e. while we still are sure to have a reference
+				 * to this DSO via curr_map->dso.
+				 */
+				dsos__add(&map->groups->machine->dsos, curr_dso);
 				/* kmaps already got it */
 				map__put(curr_map);
-				dsos__add(&map->groups->machine->dsos, curr_dso);
 				dso__set_loaded(curr_dso, map->type);
 			} else
 				curr_dso = curr_map->dso;

2.1.0

Pulled, thanks a lot Arnaldo!

Ingo