[PATCH 1/2] perf trace: Beautify fifth argument of mmap() as fd

[PATCH 1/2] perf trace: Beautify fifth argument of mmap() as fd

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The fifth argument of mmap syscall is fd and it often contains -1 as a
value for anon mappings.  Without this patch it doesn't show the file
name as well as it shows -1 as 4294967295.

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index c3008b1c369c..aeb6296a76bd 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -951,7 +951,8 @@ static struct syscall_fmt {
 	{ .name	    = "mmap",	    .hexret = true,
 	  .arg_scnprintf = { [0] = SCA_HEX,	  /* addr */
 			     [2] = SCA_MMAP_PROT, /* prot */
-			     [3] = SCA_MMAP_FLAGS, /* flags */ }, },
+			     [3] = SCA_MMAP_FLAGS, /* flags */
+			     [4] = SCA_FD, 	  /* fd */ }, },
 	{ .name	    = "mprotect",   .errmsg = true,
 	  .arg_scnprintf = { [0] = SCA_HEX, /* start */
 			     [2] = SCA_MMAP_PROT, /* prot */ }, },
-- 
1.7.11.7

[PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

When replaying a previous record session, it'll get a segfault since
it doesn't initialize evsel->priv for finding syscall id.  So fix it
by initialize sys_enter/exit evsel manually.

While at it, factor out perf_evsel__init_syscall_tp() to init a
syscall evsel and remove unused perf_session__has_tp().

Cc: David Ahern <dsahern@gmail.com>
Cc: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-trace.c | 63 ++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index aeb6296a76bd..6a18ed0a3e2a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -149,20 +149,28 @@ static void perf_evsel__delete_priv(struct perf_evsel *evsel)
 	perf_evsel__delete(evsel);
 }
 
-static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void *handler)
+static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void *handler)
 {
-	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
+	evsel->priv = malloc(sizeof(struct syscall_tp));
 
-	if (evsel) {
-		evsel->priv = malloc(sizeof(struct syscall_tp));
+	if (evsel->priv == NULL)
+		return -ENOMEM;
 
-		if (evsel->priv == NULL)
-			goto out_delete;
+	if (perf_evsel__init_sc_tp_uint_field(evsel, id))
+		return -1;
 
-		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
-			goto out_delete;
+	evsel->handler = handler;
+	return 0;
+}
+
+static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction,
+						    void *handler)
+{
+	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
 
-		evsel->handler = handler;
+	if (evsel) {
+		if (perf_evsel__init_syscall_tp(evsel, handler) < 0)
+			goto out_delete;
 	}
 
 	return evsel;
@@ -1754,16 +1762,6 @@ static int trace__process_sample(struct perf_tool *tool,
 	return err;
 }
 
-static bool
-perf_session__has_tp(struct perf_session *session, const char *name)
-{
-	struct perf_evsel *evsel;
-
-	evsel = perf_evlist__find_tracepoint_by_name(session->evlist, name);
-
-	return evsel != NULL;
-}
-
 static int parse_target_str(struct trace *trace)
 {
 	if (trace->opts.target.pid) {
@@ -2000,8 +1998,6 @@ out_error:
 static int trace__replay(struct trace *trace)
 {
 	const struct perf_evsel_str_handler handlers[] = {
-		{ "raw_syscalls:sys_enter",  trace__sys_enter, },
-		{ "raw_syscalls:sys_exit",   trace__sys_exit, },
 		{ "probe:vfs_getname",	     trace__vfs_getname, },
 	};
 	struct perf_data_file file = {
@@ -2009,6 +2005,7 @@ static int trace__replay(struct trace *trace)
 		.mode  = PERF_DATA_MODE_READ,
 	};
 	struct perf_session *session;
+	struct perf_evsel *evsel;
 	int err = -1;
 
 	trace->tool.sample	  = trace__process_sample;
@@ -2040,13 +2037,29 @@ static int trace__replay(struct trace *trace)
 	if (err)
 		goto out;
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_enter")) {
-		pr_err("Data file does not have raw_syscalls:sys_enter events\n");
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_enter");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_enter) < 0 ||
+	    perf_evsel__init_sc_tp_ptr_field(evsel, args)) {
+		pr_err("Error during initialize raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_exit");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_exit")) {
-		pr_err("Data file does not have raw_syscalls:sys_exit events\n");
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_exit) < 0 ||
+	    perf_evsel__init_sc_tp_uint_field(evsel, ret)) {
+		pr_err("Error during initialize raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 
-- 
1.7.11.7

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Arnaldo Carvalho de Melo]

Em Tue, Nov 12, 2013 at 03:25:00PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> When replaying a previous record session, it'll get a segfault since
> it doesn't initialize evsel->priv for finding syscall id.  So fix it
> by initialize sys_enter/exit evsel manually.
> 
> While at it, factor out perf_evsel__init_syscall_tp() to init a
> syscall evsel and remove unused perf_session__has_tp().

Try to avoid these "while at it" to combine multiple patches into one
:-)

I'll try to do it for you this time as it fixes a real problem, thanks
for fixing it!

More comments below:

- Arnaldo
 
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-trace.c | 63 ++++++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> -static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void *handler)
> +static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void *handler)
>  {
> -	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
> +	evsel->priv = malloc(sizeof(struct syscall_tp));
>  
> -	if (evsel) {
> -		evsel->priv = malloc(sizeof(struct syscall_tp));
> +	if (evsel->priv == NULL)
> +		return -ENOMEM;
>  
> -		if (evsel->priv == NULL)
> -			goto out_delete;
> +	if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> +		return -1;

Try to be consistent here, if when allocating memory you return minus
ernno code, try to use that error reporting convention here as well.

Otherwise if perf_evsel__init_syscall_tp checks the error it will try to
figure out why it is returning -EPERM (-1) when
perf_evsel__init_sc_tp_uint_field fails :)

I think the right error to return here is -ENOENT, i.e. the field was
not found in this tracepoint.

>  
> -		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> -			goto out_delete;
> +	evsel->handler = handler;
> +	return 0;
> +}
> +
> +static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction,
> +						    void *handler)
> +{
> +	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
>  
> -		evsel->handler = handler;
> +	if (evsel) {
> +		if (perf_evsel__init_syscall_tp(evsel, handler) < 0)
> +			goto out_delete;
>  	}
>  
>  	return evsel;
> @@ -1754,16 +1762,6 @@ static int trace__process_sample(struct perf_tool *tool,
>  	return err;
>  }
>  
> -static bool
> -perf_session__has_tp(struct perf_session *session, const char *name)
> -{
> -	struct perf_evsel *evsel;
> -
> -	evsel = perf_evlist__find_tracepoint_by_name(session->evlist, name);
> -
> -	return evsel != NULL;
> -}
> -
>  static int parse_target_str(struct trace *trace)
>  {
>  	if (trace->opts.target.pid) {
> @@ -2000,8 +1998,6 @@ out_error:
>  static int trace__replay(struct trace *trace)
>  {
>  	const struct perf_evsel_str_handler handlers[] = {
> -		{ "raw_syscalls:sys_enter",  trace__sys_enter, },
> -		{ "raw_syscalls:sys_exit",   trace__sys_exit, },
>  		{ "probe:vfs_getname",	     trace__vfs_getname, },
>  	};
>  	struct perf_data_file file = {
> @@ -2009,6 +2005,7 @@ static int trace__replay(struct trace *trace)
>  		.mode  = PERF_DATA_MODE_READ,
>  	};
>  	struct perf_session *session;
> +	struct perf_evsel *evsel;
>  	int err = -1;
>  
>  	trace->tool.sample	  = trace__process_sample;
> @@ -2040,13 +2037,29 @@ static int trace__replay(struct trace *trace)
>  	if (err)
>  		goto out;
>  
> -	if (!perf_session__has_tp(session, "raw_syscalls:sys_enter")) {
> -		pr_err("Data file does not have raw_syscalls:sys_enter events\n");
> +	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
> +						     "raw_syscalls:sys_enter");
> +	if (evsel == NULL) {
> +		pr_err("Data file does not have raw_syscalls:sys_enter event\n");
> +		goto out;
> +	}
> +
> +	if (perf_evsel__init_syscall_tp(evsel, trace__sys_enter) < 0 ||
> +	    perf_evsel__init_sc_tp_ptr_field(evsel, args)) {
> +		pr_err("Error during initialize raw_syscalls:sys_enter event\n");
> +		goto out;
> +	}
> +
> +	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
> +						     "raw_syscalls:sys_exit");
> +	if (evsel == NULL) {
> +		pr_err("Data file does not have raw_syscalls:sys_exit event\n");
>  		goto out;
>  	}
>  
> -	if (!perf_session__has_tp(session, "raw_syscalls:sys_exit")) {
> -		pr_err("Data file does not have raw_syscalls:sys_exit events\n");
> +	if (perf_evsel__init_syscall_tp(evsel, trace__sys_exit) < 0 ||
> +	    perf_evsel__init_sc_tp_uint_field(evsel, ret)) {
> +		pr_err("Error during initialize raw_syscalls:sys_exit event\n");
>  		goto out;
>  	}
>  
> -- 
> 1.7.11.7

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Arnaldo Carvalho de Melo]

Em Tue, Nov 12, 2013 at 08:46:09AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Nov 12, 2013 at 03:25:00PM +0900, Namhyung Kim escreveu:
> > From: Namhyung Kim <namhyung.kim@lge.com>

> > When replaying a previous record session, it'll get a segfault since
> > it doesn't initialize evsel->priv for finding syscall id.  So fix it
> > by initialize sys_enter/exit evsel manually.

> > While at it, factor out perf_evsel__init_syscall_tp() to init a
> > syscall evsel and remove unused perf_session__has_tp().

> Try to avoid these "while at it" to combine multiple patches into one
> :-)

> I'll try to do it for you this time as it fixes a real problem, thanks
> for fixing it!

So this becomes the first part of this patch, split from yours and
massaged a bit so that by looking at the patch it becomes quickly clear
what it is doing, please let me now if I can keep this as-is (with your
authorship, etc).

I'll test this all out after finishing the next part of the split up.

commit 296f6ce34590099740bfe03ced37f6f53a0133f8
Author: Namhyung Kim <namhyung@kernel.org>
Date:   Tue Nov 12 08:51:45 2013 -0300

    perf trace: Separate tp syscall field caching into init routine to be reused
    
    We need to set this in evsels coming out of a perf.data file header, not
    just for new ones created for live sessions.

    So separate the code that caches the syscall entry/exit tracepoint
    format fields into a new function that will be used in the next
    changeset.
    
    Signed-off-by: Namhyung Kim <namhyung@kernel.org>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Mike Galbraith <efault@gmx.de>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Stephane Eranian <eranian@google.com>
    Link: http://lkml.kernel.org/n/tip-iv4vbx2064hc2drv38egqzee@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index aeb6296a76bd..3fa1dce6d43e 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -149,20 +149,32 @@ static void perf_evsel__delete_priv(struct perf_evsel *evsel)
 	perf_evsel__delete(evsel);
 }
 
+static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void *handler)
+{
+	evsel->priv = malloc(sizeof(struct syscall_tp));
+	if (evsel->priv != NULL) {
+		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
+			goto out_delete;
+
+		evsel->handler = handler;
+		return 0;
+	}
+
+	return -ENOMEM;
+
+out_delete:
+	free(evsel->priv);
+	evsel->priv = NULL;
+	return -ENOENT;
+}
+
 static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void *handler)
 {
 	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
 
 	if (evsel) {
-		evsel->priv = malloc(sizeof(struct syscall_tp));
-
-		if (evsel->priv == NULL)
+		if (perf_evsel__init_syscall_tp(evsel, handler))
 			goto out_delete;
-
-		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
-			goto out_delete;
-
-		evsel->handler = handler;
 	}
 
 	return evsel;

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Arnaldo Carvalho de Melo]

Em Tue, Nov 12, 2013 at 08:57:00AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Nov 12, 2013 at 08:46:09AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Nov 12, 2013 at 03:25:00PM +0900, Namhyung Kim escreveu:
> > > From: Namhyung Kim <namhyung.kim@lge.com>
> 
> > > When replaying a previous record session, it'll get a segfault since
> > > it doesn't initialize evsel->priv for finding syscall id.  So fix it
> > > by initialize sys_enter/exit evsel manually.
> 
> > > While at it, factor out perf_evsel__init_syscall_tp() to init a
> > > syscall evsel and remove unused perf_session__has_tp().
> 
> > Try to avoid these "while at it" to combine multiple patches into one
> > :-)
> 
> > I'll try to do it for you this time as it fixes a real problem, thanks
> > for fixing it!
> 
> So this becomes the first part of this patch, split from yours and
> massaged a bit so that by looking at the patch it becomes quickly clear
> what it is doing, please let me now if I can keep this as-is (with your
> authorship, etc).
> 
> I'll test this all out after finishing the next part of the split up.

Second part, just rewrote the changelog a bit:

commit 4e8a6e566fc611b9cbec2978a52266f29cb65785
Author: Namhyung Kim <namhyung.kim@lge.com>
Date:   Tue Nov 12 15:25:00 2013 +0900

    perf trace: Fix segfault on perf trace -i perf.data
    
    When replaying a previous record session, it'll get a segfault since it
    doesn't initialize raw_syscalls enter/exit tracepoint's evsel->priv for
    caching the format fields.
    
    So fix it by properly initializing sys_enter/exit evsels that comes from
    reading the perf.data file header.
    
    Signed-off-by: Namhyung Kim <namhyung@kernel.org>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Ingo Molnar <mingo@kernel.org>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Pekka Enberg <penberg@kernel.org>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Link: http://lkml.kernel.org/r/1384237500-22991-2-git-send-email-namhyung@kernel.org
    [ Split the syscall tp field caching part in the previous patch ]
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 3fa1dce6d43e..8d6ea8fbf476 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1766,16 +1766,6 @@ static int trace__process_sample(struct perf_tool *tool,
 	return err;
 }
 
-static bool
-perf_session__has_tp(struct perf_session *session, const char *name)
-{
-	struct perf_evsel *evsel;
-
-	evsel = perf_evlist__find_tracepoint_by_name(session->evlist, name);
-
-	return evsel != NULL;
-}
-
 static int parse_target_str(struct trace *trace)
 {
 	if (trace->opts.target.pid) {
@@ -2012,8 +2002,6 @@ out_error:
 static int trace__replay(struct trace *trace)
 {
 	const struct perf_evsel_str_handler handlers[] = {
-		{ "raw_syscalls:sys_enter",  trace__sys_enter, },
-		{ "raw_syscalls:sys_exit",   trace__sys_exit, },
 		{ "probe:vfs_getname",	     trace__vfs_getname, },
 	};
 	struct perf_data_file file = {
@@ -2021,6 +2009,7 @@ static int trace__replay(struct trace *trace)
 		.mode  = PERF_DATA_MODE_READ,
 	};
 	struct perf_session *session;
+	struct perf_evsel *evsel;
 	int err = -1;
 
 	trace->tool.sample	  = trace__process_sample;
@@ -2052,13 +2041,29 @@ static int trace__replay(struct trace *trace)
 	if (err)
 		goto out;
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_enter")) {
-		pr_err("Data file does not have raw_syscalls:sys_enter events\n");
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_enter");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_enter) < 0 ||
+	    perf_evsel__init_sc_tp_ptr_field(evsel, args)) {
+		pr_err("Error during initialize raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_exit");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_exit")) {
-		pr_err("Data file does not have raw_syscalls:sys_exit events\n");
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_exit) < 0 ||
+	    perf_evsel__init_sc_tp_uint_field(evsel, ret)) {
+		pr_err("Error during initialize raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Namhyung Kim]

On Tue, Nov 12, 2013 at 09:15:08AM -0300, Arnaldo Carvalho de Melo wrote:
> Second part, just rewrote the changelog a bit:
> 
> commit 4e8a6e566fc611b9cbec2978a52266f29cb65785
> Author: Namhyung Kim <namhyung.kim@lge.com>
> Date:   Tue Nov 12 15:25:00 2013 +0900
> 
>     perf trace: Fix segfault on perf trace -i perf.data
>     
>     When replaying a previous record session, it'll get a segfault since it
>     doesn't initialize raw_syscalls enter/exit tracepoint's evsel->priv for
>     caching the format fields.
>     
>     So fix it by properly initializing sys_enter/exit evsels that comes from
>     reading the perf.data file header.

Looks good to me!

Thanks for doing this, Arnaldo.  I'll try to avoid this kind of thing
again in the future.

Thanks,
Namhyung

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Namhyung Kim]

Hi Arnaldo,

On Tue, Nov 12, 2013 at 08:57:00AM -0300, Arnaldo Carvalho de Melo wrote:
> So this becomes the first part of this patch, split from yours and
> massaged a bit so that by looking at the patch it becomes quickly clear
> what it is doing, please let me now if I can keep this as-is (with your
> authorship, etc).

Looks good to me.

But I just have a nitpick, please see below.

> 
> I'll test this all out after finishing the next part of the split up.
> 
> commit 296f6ce34590099740bfe03ced37f6f53a0133f8
> Author: Namhyung Kim <namhyung@kernel.org>
> Date:   Tue Nov 12 08:51:45 2013 -0300
> 
>     perf trace: Separate tp syscall field caching into init routine to be reused
>     
>     We need to set this in evsels coming out of a perf.data file header, not
>     just for new ones created for live sessions.
> 
>     So separate the code that caches the syscall entry/exit tracepoint
>     format fields into a new function that will be used in the next
>     changeset.
>     
>     Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: David Ahern <dsahern@gmail.com>
>     Cc: Frederic Weisbecker <fweisbec@gmail.com>
>     Cc: Jiri Olsa <jolsa@redhat.com>
>     Cc: Mike Galbraith <efault@gmx.de>
>     Cc: Paul Mackerras <paulus@samba.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Stephane Eranian <eranian@google.com>
>     Link: http://lkml.kernel.org/n/tip-iv4vbx2064hc2drv38egqzee@git.kernel.org
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index aeb6296a76bd..3fa1dce6d43e 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -149,20 +149,32 @@ static void perf_evsel__delete_priv(struct perf_evsel *evsel)
>  	perf_evsel__delete(evsel);
>  }
>  
> +static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void *handler)
> +{
> +	evsel->priv = malloc(sizeof(struct syscall_tp));
> +	if (evsel->priv != NULL) {
> +		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> +			goto out_delete;
> +
> +		evsel->handler = handler;
> +		return 0;
> +	}
> +
> +	return -ENOMEM;
> +
> +out_delete:
> +	free(evsel->priv);
> +	evsel->priv = NULL;

Is this part needed?  I can see that perf_evsel__delete_priv() can do
it for you anyway.  Yes I know it's needed for my later change, but I
think we do it a bit differently.

And again, is perf_evsel__delete_priv() needed?  Isn't the ->priv is
not used for anything else?  Why not just letting perf_evsel__delete()
handle this transparently?

Looking at the source, evsel->priv is a member of union and the other
member ->id_offset is used in when dealing with the perf file header
and it doesn't allocate memory.

Hmm, how about adding a new field like ->needs_free_priv then?

Anyway, it should definitely be a different change, I just want to
raise an issue after seeing it.

Thanks,
Namhyung


> +	return -ENOENT;
> +}
> +
>  static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void *handler)
>  {
>  	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
>  
>  	if (evsel) {
> -		evsel->priv = malloc(sizeof(struct syscall_tp));
> -
> -		if (evsel->priv == NULL)
> +		if (perf_evsel__init_syscall_tp(evsel, handler))
>  			goto out_delete;
> -
> -		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> -			goto out_delete;
> -
> -		evsel->handler = handler;
>  	}
>  
>  	return evsel;

Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Arnaldo Carvalho de Melo]

Em Tue, Nov 12, 2013 at 09:27:23PM +0000, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Nov 12, 2013 at 08:57:00AM -0300, Arnaldo Carvalho de Melo wrote:
> > So this becomes the first part of this patch, split from yours and
> > massaged a bit so that by looking at the patch it becomes quickly clear
> > what it is doing, please let me now if I can keep this as-is (with your
> > authorship, etc).
> 
> Looks good to me.

Thanks for checking!
 
> But I just have a nitpick, please see below.
 
> > +{
> > +	evsel->priv = malloc(sizeof(struct syscall_tp));
> > +	if (evsel->priv != NULL) {
> > +		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> > +			goto out_delete;
> > +
> > +		evsel->handler = handler;
> > +		return 0;
> > +	}
> > +
> > +	return -ENOMEM;
> > +
> > +out_delete:
> > +	free(evsel->priv);
> > +	evsel->priv = NULL;
 
> Is this part needed?  I can see that perf_evsel__delete_priv() can do
> it for you anyway.  Yes I know it's needed for my later change, but I
> think we do it a bit differently.
 
> And again, is perf_evsel__delete_priv() needed?  Isn't the ->priv is
> not used for anything else?  Why not just letting perf_evsel__delete()
> handle this transparently?

Because it may point to something not allocated via malloc, so the one
who allocates it, frees it, furthermore, the one who allocates it, if
fails to complete the greater init sequence of which the allocation is
part of, frees it and leaves it as it was before the transaction
started.
 
- Arnaldo

[tip:perf/urgent] perf trace: Separate tp syscall field caching into init routine to be reused

[tip-bot for Namhyung Kim]

Commit-ID:  96695d440242aca871ef8d797bd98d9cbd7ad8a0
Gitweb:     http://git.kernel.org/tip/96695d440242aca871ef8d797bd98d9cbd7ad8a0
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 12 Nov 2013 08:51:45 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 12 Nov 2013 13:00:36 -0300

perf trace: Separate tp syscall field caching into init routine to be reused

We need to set this in evsels coming out of a perf.data file header, not
just for new ones created for live sessions.

So separate the code that caches the syscall entry/exit tracepoint
format fields into a new function that will be used in the next
changeset.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/20131112115700.GC4053@ghostprotocols.net
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index aeb6296..3fa1dce 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -149,20 +149,32 @@ static void perf_evsel__delete_priv(struct perf_evsel *evsel)
 	perf_evsel__delete(evsel);
 }
 
+static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void *handler)
+{
+	evsel->priv = malloc(sizeof(struct syscall_tp));
+	if (evsel->priv != NULL) {
+		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
+			goto out_delete;
+
+		evsel->handler = handler;
+		return 0;
+	}
+
+	return -ENOMEM;
+
+out_delete:
+	free(evsel->priv);
+	evsel->priv = NULL;
+	return -ENOENT;
+}
+
 static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void *handler)
 {
 	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
 
 	if (evsel) {
-		evsel->priv = malloc(sizeof(struct syscall_tp));
-
-		if (evsel->priv == NULL)
+		if (perf_evsel__init_syscall_tp(evsel, handler))
 			goto out_delete;
-
-		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
-			goto out_delete;
-
-		evsel->handler = handler;
 	}
 
 	return evsel;

[tip:perf/urgent] perf trace: Fix segfault on perf trace -i perf.data

[tip-bot for Namhyung Kim]

Commit-ID:  003824e8c27eeb8d3eec54cfab5845ec01ab532e
Gitweb:     http://git.kernel.org/tip/003824e8c27eeb8d3eec54cfab5845ec01ab532e
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Tue, 12 Nov 2013 15:25:00 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 12 Nov 2013 13:00:37 -0300

perf trace: Fix segfault on perf trace -i perf.data

When replaying a previous record session, it'll get a segfault since it
doesn't initialize raw_syscalls enter/exit tracepoint's evsel->priv for
caching the format fields.

So fix it by properly initializing sys_enter/exit evsels that comes from
reading the perf.data file header.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1384237500-22991-2-git-send-email-namhyung@kernel.org
[ Split the syscall tp field caching part in the previous patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 3fa1dce..8d6ea8f 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1766,16 +1766,6 @@ static int trace__process_sample(struct perf_tool *tool,
 	return err;
 }
 
-static bool
-perf_session__has_tp(struct perf_session *session, const char *name)
-{
-	struct perf_evsel *evsel;
-
-	evsel = perf_evlist__find_tracepoint_by_name(session->evlist, name);
-
-	return evsel != NULL;
-}
-
 static int parse_target_str(struct trace *trace)
 {
 	if (trace->opts.target.pid) {
@@ -2012,8 +2002,6 @@ out_error:
 static int trace__replay(struct trace *trace)
 {
 	const struct perf_evsel_str_handler handlers[] = {
-		{ "raw_syscalls:sys_enter",  trace__sys_enter, },
-		{ "raw_syscalls:sys_exit",   trace__sys_exit, },
 		{ "probe:vfs_getname",	     trace__vfs_getname, },
 	};
 	struct perf_data_file file = {
@@ -2021,6 +2009,7 @@ static int trace__replay(struct trace *trace)
 		.mode  = PERF_DATA_MODE_READ,
 	};
 	struct perf_session *session;
+	struct perf_evsel *evsel;
 	int err = -1;
 
 	trace->tool.sample	  = trace__process_sample;
@@ -2052,13 +2041,29 @@ static int trace__replay(struct trace *trace)
 	if (err)
 		goto out;
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_enter")) {
-		pr_err("Data file does not have raw_syscalls:sys_enter events\n");
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_enter");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_enter) < 0 ||
+	    perf_evsel__init_sc_tp_ptr_field(evsel, args)) {
+		pr_err("Error during initialize raw_syscalls:sys_enter event\n");
+		goto out;
+	}
+
+	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+						     "raw_syscalls:sys_exit");
+	if (evsel == NULL) {
+		pr_err("Data file does not have raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 
-	if (!perf_session__has_tp(session, "raw_syscalls:sys_exit")) {
-		pr_err("Data file does not have raw_syscalls:sys_exit events\n");
+	if (perf_evsel__init_syscall_tp(evsel, trace__sys_exit) < 0 ||
+	    perf_evsel__init_sc_tp_uint_field(evsel, ret)) {
+		pr_err("Error during initialize raw_syscalls:sys_exit event\n");
 		goto out;
 	}
 

Re: [PATCH 1/2] perf trace: Beautify fifth argument of mmap() as fd

[David Ahern]

On 11/11/13, 11:24 PM, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
>
> The fifth argument of mmap syscall is fd and it often contains -1 as a
> value for anon mappings.  Without this patch it doesn't show the file
> name as well as it shows -1 as 4294967295.
>
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>   tools/perf/builtin-trace.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index c3008b1c369c..aeb6296a76bd 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -951,7 +951,8 @@ static struct syscall_fmt {
>   	{ .name	    = "mmap",	    .hexret = true,
>   	  .arg_scnprintf = { [0] = SCA_HEX,	  /* addr */
>   			     [2] = SCA_MMAP_PROT, /* prot */
> -			     [3] = SCA_MMAP_FLAGS, /* flags */ }, },
> +			     [3] = SCA_MMAP_FLAGS, /* flags */
> +			     [4] = SCA_FD, 	  /* fd */ }, },
>   	{ .name	    = "mprotect",   .errmsg = true,
>   	  .arg_scnprintf = { [0] = SCA_HEX, /* start */
>   			     [2] = SCA_MMAP_PROT, /* prot */ }, },
>

Looks good to me.

Acked-by: David Ahern <dsahern@gmail.com>

[tip:perf/urgent] perf trace: Beautify fifth argument of mmap() as fd

[tip-bot for Namhyung Kim]

Commit-ID:  73faab3a421351c342b10b0177fec9eea1a1d809
Gitweb:     http://git.kernel.org/tip/73faab3a421351c342b10b0177fec9eea1a1d809
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Tue, 12 Nov 2013 15:24:59 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 12 Nov 2013 13:00:23 -0300

perf trace: Beautify fifth argument of mmap() as fd

The fifth argument of mmap syscall is fd and it often contains -1 as a
value for anon mappings.  Without this patch it doesn't show the file
name as well as it shows -1 as 4294967295.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: David Ahern <dsahern@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1384237500-22991-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index c3008b1..aeb6296 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -951,7 +951,8 @@ static struct syscall_fmt {
 	{ .name	    = "mmap",	    .hexret = true,
 	  .arg_scnprintf = { [0] = SCA_HEX,	  /* addr */
 			     [2] = SCA_MMAP_PROT, /* prot */
-			     [3] = SCA_MMAP_FLAGS, /* flags */ }, },
+			     [3] = SCA_MMAP_FLAGS, /* flags */
+			     [4] = SCA_FD, 	  /* fd */ }, },
 	{ .name	    = "mprotect",   .errmsg = true,
 	  .arg_scnprintf = { [0] = SCA_HEX, /* start */
 			     [2] = SCA_MMAP_PROT, /* prot */ }, },