Re: [PATCH 03/13] perf annotate: Move session handling out of __cmd_annotate()

Re: [PATCH 03/13] perf annotate: Move session handling out of __cmd_annotate()

[taeung]

Hi,

I modified error code for the requirement as below.

Author: taeung <treeze.taeung@gmail.com>
Date:   Sat Sep 13 16:22:53 2014 +0900

      modified error code when perf_session__new() fail

         Because perf_session__new() could fail
         for more reasons than just ENOMEM,
         I modified error code(ENOMEM or EINVAL)
         into -1.


diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 1ec429f..81c9fda 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -199,7 +199,7 @@ static int __cmd_annotate(struct perf_annotate *ann)

      session = perf_session__new(&file, false, &ann->tool);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      machines__set_symbol_filter(&session->machines, 
symbol__annotate_init);

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 9a5a035..3363dce 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -683,7 +683,7 @@ static int __cmd_diff(void)
          d->session = perf_session__new(&d->file, false, &tool);
          if (!d->session) {
              pr_err("Failed to open %s\n", d->file.path);
-            ret = -ENOMEM;
+            ret = -1;
              goto out_delete;
          }

diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 66e12f5..0f93f85 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -28,7 +28,7 @@ static int __cmd_evlist(const char *file_name, struct 
perf_attr_details *details

      session = perf_session__new(&file, 0, NULL);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      evlist__for_each(session->evlist, pos)
          perf_evsel__fprintf(pos, details, stdout);
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 9a02807..8dbdd13 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -359,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)

      session = perf_session__new(&file, true, &inject->tool);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      if (inject->build_ids) {
          inject->tool.sample = perf_event__inject_buildid;
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index bef3376..b518f4b 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -422,7 +422,7 @@ static int __cmd_kmem(void)

      session = perf_session__new(&file, false, &perf_kmem);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      if (perf_session__create_kernel_maps(session) < 0)
          goto out_delete;
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 43367eb..828e706 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1060,7 +1060,7 @@ static int read_events(struct perf_kvm_stat *kvm)
          .comm            = perf_event__process_comm,
          .ordered_samples    = true,
      };
-    struct perf_data_file file = {
+    struct perf_data_file file = {
          .path = kvm->file_name,
          .mode = PERF_DATA_MODE_READ,
      };
@@ -1069,7 +1069,7 @@ static int read_events(struct perf_kvm_stat *kvm)
      kvm->session = perf_session__new(&file, false, &kvm->tool);
      if (!kvm->session) {
          pr_err("Initializing perf session failed\n");
-        return -EINVAL;
+        return -1;
      }

      if (!perf_session__has_traces(kvm->session, "kvm record"))
@@ -1369,7 +1369,7 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
       */
      kvm->session = perf_session__new(&file, false, &kvm->tool);
      if (kvm->session == NULL) {
-        err = -ENOMEM;
+        err = -1;
          goto out;
      }
      kvm->session->evlist = kvm->evlist;
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 6148afc..49f1e30 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -862,7 +862,7 @@ static int __cmd_report(bool display_info)
      session = perf_session__new(&file, false, &eops);
      if (!session) {
          pr_err("Initializing perf session failed\n");
-        return -ENOMEM;
+        return -1;
      }

      if (!perf_session__has_traces(session, "lock record"))
diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index 4a1a6c9..75976ce 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -124,7 +124,7 @@ static int report_raw_events(struct perf_mem *mem)
                               &mem->tool);

      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      if (mem->cpu_list) {
          ret = perf_session__cpu_bitmap(session, mem->cpu_list,
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 21d830b..1a7abac 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -712,7 +712,7 @@ int cmd_report(int argc, const char **argv, const 
char *prefix __maybe_unused)
  repeat:
      session = perf_session__new(&file, false, &report.tool);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      report.session = session;

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index f57035b..77cef60 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1725,7 +1725,7 @@ int cmd_script(int argc, const char **argv, const 
char *prefix __maybe_unused)

      session = perf_session__new(&file, false, &script.tool);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      if (header || header_only) {
          perf_session__fprintf_info(session, stdout, show_full_info);
diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 2f1a522..dc9a465 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -1605,7 +1605,7 @@ static int __cmd_timechart(struct timechart 
*tchart, const char *output_name)
      int ret = -EINVAL;

      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      (void)perf_header__process_sections(&session->header,
                          perf_data_file__fd(session->file),
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 377971d..8217b5d 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -911,7 +911,7 @@ static int __cmd_top(struct perf_top *top)

      top->session = perf_session__new(NULL, false, NULL);
      if (top->session == NULL)
-        return -ENOMEM;
+        return -1;

      machines__set_symbol_filter(&top->session->machines, symbol_filter);

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index a6c3752..9e23293 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2220,7 +2220,7 @@ static int trace__replay(struct trace *trace)

      session = perf_session__new(&file, false, &trace->tool);
      if (session == NULL)
-        return -ENOMEM;
+        return -1;

      trace->host = &session->machines.host;

Thanks,
--
taeung

> On Tue, Aug 12, 2014 at 03:40:35PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> >
> > @@ -301,6 +281,10 @@ int cmd_annotate(int argc, const char **argv, 
> const char *prefix __maybe_unused)
> > .ordering_requires_timestamps = true,
> > },
> > };
> > + struct perf_data_file file = {
> > + .path = input_name,
> > + .mode = PERF_DATA_MODE_READ,
> > + };
> > const struct option options[] = {
> > OPT_STRING('i', "input", &input_name, "file",
> > "input file name"),
> > @@ -308,7 +292,7 @@ int cmd_annotate(int argc, const char **argv, 
> const char *prefix __maybe_unused)
> > "only consider symbols in these dsos"),
> > OPT_STRING('s', "symbol", &annotate.sym_hist_filter, "symbol",
> > "symbol to annotate"),
> > - OPT_BOOLEAN('f', "force", &annotate.force, "don't complain, do it"),
> > + OPT_BOOLEAN('f', "force", &file.force, "don't complain, do it"),
> > OPT_INCR('v', "verbose", &verbose,
> > "be more verbose (show symbol address, etc)"),
> > OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
> > @@ -341,6 +325,7 @@ int cmd_annotate(int argc, const char **argv, 
> const char *prefix __maybe_unused)
> > "Show event group information together"),
> > OPT_END()
> > };
> > + int ret;
> >
> > argc = parse_options(argc, argv, options, annotate_usage, 0);
> >
> > @@ -353,11 +338,16 @@ int cmd_annotate(int argc, const char **argv, 
> const char *prefix __maybe_unused)
> >
> > setup_browser(true);
> >
> > + annotate.session = perf_session__new(&file, false, &annotate.tool);
> > + if (annotate.session == NULL)
> > + return -ENOMEM;
>
> I know you're just moving code, but perf_session__new could
> fail for more reasons than just ENOMEM, which is the most
> unlikely case ;-) -1 is probably better option here
>
> jirka

[PATCHSET 00/13] perf tools: Fix vmlinux search path initialization

[Namhyung Kim]

Hello,

Currently, when perf reports kernel symbols, it tries to look up the
sample ip from kallsyms/vmlinux in the build-id cache, and then a
running kernel.  This can be a problem if it's recorded on a different
kernel and failed to find it from the build-id cache for some reason.

We can fix it by using --symfs option but it's annoying for user to do
it always.  As we already have the kernel version info in the
perf.data file, it'd be better to change it to use the info to search
the correct file automatically.

Minchan Kim reported this during his kernel work.  And this patchset
fixes it by using the recorded kernel info.

The patch 1 and 2 are independent fixes so can be applied separately.
The patch 3 to 12 are preparation for the patch 13 which move
symbol__init() after creating a session.


Before:

  $ perf report
  ...
  # Samples: 4K of event 'cpu-clock'
  # Event count (approx.): 1067250000
  #
  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  ..............................
      71.87%     swapper  [kernel.kallsyms]  [k] recover_probed_instruction

After:

  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  ....................
      71.87%     swapper  [kernel.kallsyms]  [k] native_safe_halt


You can also get it from 'perf/vmlinux-v1' branch on my tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Any comments are welcome.

Thanks,
Namhyung


Namhyung Kim (13):
  perf script: Fix possible memory leaks
  perf tools: Fix a memory leak in vmlinux_path__init()
  perf annotate: Move session handling out of __cmd_annotate()
  perf buildid-cache: Move session handling into cmd_buildid_cache()
  perf inject: Move session handling out of __cmd_inject()
  perf kmem: Move session handling out of __cmd_kmem()
  perf kvm: Move call to symbol__init() after creating session
  perf lock: Move call to symbol__init() after creating session
  perf sched: Move call to symbol__init() after creating session
  perf script: Move call to symbol__init() after creating session
  perf timechart: Move call to symbol__init() after creating session
  perf trace: Move call to symbol__init() after creating session
  perf tools: Check recorded kernel version when finding vmlinux

 tools/perf/builtin-annotate.c      | 75 ++++++++++++++++++++------------------
 tools/perf/builtin-buildid-cache.c | 37 +++++++++++--------
 tools/perf/builtin-diff.c          |  2 +-
 tools/perf/builtin-inject.c        | 31 +++++++++-------
 tools/perf/builtin-kmem.c          | 49 ++++++++++++++-----------
 tools/perf/builtin-kvm.c           |  6 +--
 tools/perf/builtin-lock.c          |  3 +-
 tools/perf/builtin-mem.c           |  2 +-
 tools/perf/builtin-record.c        |  2 +-
 tools/perf/builtin-report.c        |  2 +-
 tools/perf/builtin-sched.c         |  3 +-
 tools/perf/builtin-script.c        | 40 ++++++++++++--------
 tools/perf/builtin-timechart.c     |  4 +-
 tools/perf/builtin-top.c           |  2 +-
 tools/perf/builtin-trace.c         |  8 ++--
 tools/perf/tests/builtin-test.c    |  2 +-
 tools/perf/util/probe-event.c      |  2 +-
 tools/perf/util/symbol.c           | 26 ++++++++-----
 tools/perf/util/symbol.h           |  3 +-
 19 files changed, 172 insertions(+), 127 deletions(-)

-- 
2.0.0

[PATCH 03/13] perf annotate: Move session handling out of __cmd_annotate()

[Namhyung Kim]

This is a preparation of fixing dso__load_kernel_sym().  It needs a
session info before calling symbol__init().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c | 75 +++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 35 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 952b5ece6740..c0464dc38057 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -36,7 +36,8 @@
 
 struct perf_annotate {
 	struct perf_tool tool;
-	bool	   force, use_tui, use_stdio, use_gtk;
+	struct perf_session *session;
+	bool	   use_tui, use_stdio, use_gtk;
 	bool	   full_paths;
 	bool	   print_line;
 	bool	   skip_missing;
@@ -188,18 +189,9 @@ find_next:
 static int __cmd_annotate(struct perf_annotate *ann)
 {
 	int ret;
-	struct perf_session *session;
+	struct perf_session *session = ann->session;
 	struct perf_evsel *pos;
 	u64 total_nr_samples;
-	struct perf_data_file file = {
-		.path  = input_name,
-		.mode  = PERF_DATA_MODE_READ,
-		.force = ann->force,
-	};
-
-	session = perf_session__new(&file, false, &ann->tool);
-	if (session == NULL)
-		return -ENOMEM;
 
 	machines__set_symbol_filter(&session->machines, symbol__annotate_init);
 
@@ -207,22 +199,22 @@ static int __cmd_annotate(struct perf_annotate *ann)
 		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
 					       ann->cpu_bitmap);
 		if (ret)
-			goto out_delete;
+			goto out;
 	}
 
 	if (!objdump_path) {
 		ret = perf_session_env__lookup_objdump(&session->header.env);
 		if (ret)
-			goto out_delete;
+			goto out;
 	}
 
 	ret = perf_session__process_events(session, &ann->tool);
 	if (ret)
-		goto out_delete;
+		goto out;
 
 	if (dump_trace) {
 		perf_session__fprintf_nr_events(session, stdout);
-		goto out_delete;
+		goto out;
 	}
 
 	if (verbose > 3)
@@ -250,8 +242,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
 	}
 
 	if (total_nr_samples == 0) {
-		ui__error("The %s file has no samples!\n", file.path);
-		goto out_delete;
+		ui__error("The %s file has no samples!\n", session->file->path);
+		goto out;
 	}
 
 	if (use_browser == 2) {
@@ -261,24 +253,12 @@ static int __cmd_annotate(struct perf_annotate *ann)
 					 "perf_gtk__show_annotations");
 		if (show_annotations == NULL) {
 			ui__error("GTK browser not found!\n");
-			goto out_delete;
+			goto out;
 		}
 		show_annotations();
 	}
 
-out_delete:
-	/*
-	 * Speed up the exit process, for large files this can
-	 * take quite a while.
-	 *
-	 * XXX Enable this when using valgrind or if we ever
-	 * librarize this command.
-	 *
-	 * Also experiment with obstacks to see how much speed
-	 * up we'll get here.
-	 *
-	 * perf_session__delete(session);
-	 */
+out:
 	return ret;
 }
 
@@ -301,6 +281,10 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 			.ordering_requires_timestamps = true,
 		},
 	};
+	struct perf_data_file file = {
+		.path  = input_name,
+		.mode  = PERF_DATA_MODE_READ,
+	};
 	const struct option options[] = {
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
@@ -308,7 +292,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		   "only consider symbols in these dsos"),
 	OPT_STRING('s', "symbol", &annotate.sym_hist_filter, "symbol",
 		    "symbol to annotate"),
-	OPT_BOOLEAN('f', "force", &annotate.force, "don't complain, do it"),
+	OPT_BOOLEAN('f', "force", &file.force, "don't complain, do it"),
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
@@ -341,6 +325,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Show event group information together"),
 	OPT_END()
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
 
@@ -353,11 +338,16 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	setup_browser(true);
 
+	annotate.session = perf_session__new(&file, false, &annotate.tool);
+	if (annotate.session == NULL)
+		return -ENOMEM;
+
 	symbol_conf.priv_size = sizeof(struct annotation);
 	symbol_conf.try_vmlinux_path = true;
 
-	if (symbol__init() < 0)
-		return -1;
+	ret = symbol__init();
+	if (ret < 0)
+		goto out_delete;
 
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
@@ -373,5 +363,20 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		annotate.sym_hist_filter = argv[0];
 	}
 
-	return __cmd_annotate(&annotate);
+	ret = __cmd_annotate(&annotate);
+
+out_delete:
+	/*
+	 * Speed up the exit process, for large files this can
+	 * take quite a while.
+	 *
+	 * XXX Enable this when using valgrind or if we ever
+	 * librarize this command.
+	 *
+	 * Also experiment with obstacks to see how much speed
+	 * up we'll get here.
+	 *
+	 * perf_session__delete(session);
+	 */
+	return ret;
 }
-- 
2.0.0

Re: [PATCH 03/13] perf annotate: Move session handling out of __cmd_annotate()

[Jiri Olsa]

On Tue, Aug 12, 2014 at 03:40:35PM +0900, Namhyung Kim wrote:

SNIP

>  
> @@ -301,6 +281,10 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  			.ordering_requires_timestamps = true,
>  		},
>  	};
> +	struct perf_data_file file = {
> +		.path  = input_name,
> +		.mode  = PERF_DATA_MODE_READ,
> +	};
>  	const struct option options[] = {
>  	OPT_STRING('i', "input", &input_name, "file",
>  		    "input file name"),
> @@ -308,7 +292,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  		   "only consider symbols in these dsos"),
>  	OPT_STRING('s', "symbol", &annotate.sym_hist_filter, "symbol",
>  		    "symbol to annotate"),
> -	OPT_BOOLEAN('f', "force", &annotate.force, "don't complain, do it"),
> +	OPT_BOOLEAN('f', "force", &file.force, "don't complain, do it"),
>  	OPT_INCR('v', "verbose", &verbose,
>  		    "be more verbose (show symbol address, etc)"),
>  	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
> @@ -341,6 +325,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  		    "Show event group information together"),
>  	OPT_END()
>  	};
> +	int ret;
>  
>  	argc = parse_options(argc, argv, options, annotate_usage, 0);
>  
> @@ -353,11 +338,16 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	setup_browser(true);
>  
> +	annotate.session = perf_session__new(&file, false, &annotate.tool);
> +	if (annotate.session == NULL)
> +		return -ENOMEM;

I know you're just moving code, but perf_session__new could
fail for more reasons than just ENOMEM, which is the most
unlikely case ;-) -1 is probably better option here

jirka

Re: [PATCH 03/13] perf annotate: Move session handling out of __cmd_annotate()

[Namhyung Kim]

On Wed, 13 Aug 2014 13:48:22 +0200, Jiri Olsa wrote:
> On Tue, Aug 12, 2014 at 03:40:35PM +0900, Namhyung Kim wrote:
>> +	annotate.session = perf_session__new(&file, false, &annotate.tool);
>> +	if (annotate.session == NULL)
>> +		return -ENOMEM;
>
> I know you're just moving code, but perf_session__new could
> fail for more reasons than just ENOMEM, which is the most
> unlikely case ;-) -1 is probably better option here

Okay, will change as a separate fix later.

Thanks,
Namhyung