[PATCH 0/3] perf: misc fixes

[PATCH 0/3] perf: misc fixes

[Don Zickus]

Just a small collection of fixes noticed while hacking up the c2c tool

Don Zickus (3):
  perf, machine: Use map as success in ip__resolve_ams
  perf, session: Change header.misc dump from decimal to hex
  perf: fix synthesizing mmaps for threads

 tools/perf/util/event.c   | 15 +++++++++++----
 tools/perf/util/machine.c |  6 +++---
 tools/perf/util/session.c |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

-- 
1.7.11.7

[PATCH 1/3] perf, machine: Use map as success in ip__resolve_ams

[Don Zickus]

When trying to map a bunch of instruction addresses to their respective
threads, I kept getting a lot of bogus entries [I forget the exact reason
as I patched my code months ago].

Looking through ip__resovle_ams, I noticed the check for

if (al.sym)

and realized, most times I have an al.map definition but sometimes an
al.sym is undefined.  In the cases where al.sym is undefined, the loop
keeps going even though a valid al.map exists.

Modify this check to use the more reliable al.map.  This fixed my bogus
entries.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index ac37d78..813e94e 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1213,7 +1213,7 @@ static void ip__resolve_ams(struct machine *machine, struct thread *thread,
 		 */
 		thread__find_addr_location(thread, machine, m, MAP__FUNCTION,
 				ip, &al);
-		if (al.sym)
+		if (al.map)
 			goto found;
 	}
 found:
-- 
1.7.11.7

[tip:perf/urgent] perf machine: Use map as success in ip__resolve_ams

[tip-bot for Don Zickus]

Commit-ID:  fdf57dd052d5cbd415533ae98f4d423286a85220
Gitweb:     http://git.kernel.org/tip/fdf57dd052d5cbd415533ae98f4d423286a85220
Author:     Don Zickus <dzickus@redhat.com>
AuthorDate: Tue, 25 Feb 2014 22:43:45 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 10 Mar 2014 11:19:36 -0300

perf machine: Use map as success in ip__resolve_ams

When trying to map a bunch of instruction addresses to their respective
threads, I kept getting a lot of bogus entries [I forget the exact
reason as I patched my code months ago].

Looking through ip__resolve_ams, I noticed the check for

  if (al.sym)

and realized, most times I have an al.map definition but sometimes an
al.sym is undefined.  In the cases where al.sym is undefined, the loop
keeps going even though a valid al.map exists.

Modify this check to use the more reliable al.map.  This fixed my bogus
entries.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1393386227-149412-2-git-send-email-dzickus@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index c872991..620a198 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1213,7 +1213,7 @@ static void ip__resolve_ams(struct machine *machine, struct thread *thread,
 		 */
 		thread__find_addr_location(thread, machine, m, MAP__FUNCTION,
 				ip, &al);
-		if (al.sym)
+		if (al.map)
 			goto found;
 	}
 found:

[PATCH 2/3] perf, session: Change header.misc dump from decimal to hex

[Don Zickus]

When printing the raw dump of a data file, the header.misc is
printed as a decimal.  Unfortunately, that field is a bit mask, so
it is hard to interpret as a decimal.

Print in hex, so the user can easily see what bits are set and more
importantly what type of info it is conveying.

V2: add 0x in front per Jiri Olsa

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/session.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 1d555d6..55960f2 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -794,7 +794,7 @@ static void dump_sample(struct perf_evsel *evsel, union perf_event *event,
 	if (!dump_trace)
 		return;
 
-	printf("(IP, %d): %d/%d: %#" PRIx64 " period: %" PRIu64 " addr: %#" PRIx64 "\n",
+	printf("(IP, 0x%x): %d/%d: %#" PRIx64 " period: %" PRIu64 " addr: %#" PRIx64 "\n",
 	       event->header.misc, sample->pid, sample->tid, sample->ip,
 	       sample->period, sample->addr);
 
-- 
1.7.11.7

[tip:perf/core] perf session: Change header.misc dump from decimal to hex

[tip-bot for Don Zickus]

Commit-ID:  0ea590ae8198547d5898c72b04fa9d8f23bd0b8f
Gitweb:     http://git.kernel.org/tip/0ea590ae8198547d5898c72b04fa9d8f23bd0b8f
Author:     Don Zickus <dzickus@redhat.com>
AuthorDate: Tue, 25 Feb 2014 22:43:46 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 14 Mar 2014 18:08:39 -0300

perf session: Change header.misc dump from decimal to hex

When printing the raw dump of a data file, the header.misc is
printed as a decimal.  Unfortunately, that field is a bit mask, so
it is hard to interpret as a decimal.

Print in hex, so the user can easily see what bits are set and more
importantly what type of info it is conveying.

V2: add 0x in front per Jiri Olsa

Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1393386227-149412-3-git-send-email-dzickus@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 1d555d6..55960f2 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -794,7 +794,7 @@ static void dump_sample(struct perf_evsel *evsel, union perf_event *event,
 	if (!dump_trace)
 		return;
 
-	printf("(IP, %d): %d/%d: %#" PRIx64 " period: %" PRIu64 " addr: %#" PRIx64 "\n",
+	printf("(IP, 0x%x): %d/%d: %#" PRIx64 " period: %" PRIu64 " addr: %#" PRIx64 "\n",
 	       event->header.misc, sample->pid, sample->tid, sample->ip,
 	       sample->period, sample->addr);
 

[PATCH 3/3] perf: fix synthesizing mmaps for threads

[Don Zickus]

Currently if a process creates a bunch of threads using pthread_create
and then perf is run in system_wide mode, the mmaps for those threads
are not captured with a synthesized mmap event.

The reason is those threads are not visible when walking the /proc/
directory looking for /proc/<pid>/maps files.  Instead they are discovered
using the /proc/<pid>/tasks file (which the synthesized comm event uses).

This causes problems when a program is trying to map a data address to a
tid.  Because the tid has no maps, the event is dropped.  Changing the program
to look up using the pid instead of the tid, finds the correct maps but creates
ugly hacks in the program to carry the correct tid around.

Fix this by synthesizing mmap events for each tid found in the /proc/<pid>/tasks
file.

This may not be entirely clean but it seems to work.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/event.c   | 15 +++++++++++----
 tools/perf/util/machine.c |  4 ++--
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 086c7c8..09c53bb 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -93,10 +93,13 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
 }
 
 static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
-					 union perf_event *event, pid_t pid,
+					 union perf_event *event,
+					 union perf_event *mmap_event,
+					 pid_t pid,
 					 int full,
 					 perf_event__handler_t process,
-					 struct machine *machine)
+					 struct machine *machine,
+					 bool mmap_data)
 {
 	char filename[PATH_MAX];
 	size_t size;
@@ -168,6 +171,10 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 			tgid = -1;
 			break;
 		}
+
+		/* process the thread's maps too */
+		perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
+						  process, machine, mmap_data);
 	}
 
 	closedir(tasks);
@@ -331,8 +338,8 @@ static int __event__synthesize_thread(union perf_event *comm_event,
 				      struct perf_tool *tool,
 				      struct machine *machine, bool mmap_data)
 {
-	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, pid, full,
-						 process, machine);
+	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, mmap_event, pid,
+						 full, process, machine, mmap_data);
 	if (tgid == -1)
 		return -1;
 	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 813e94e..eb26544 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1026,7 +1026,7 @@ int machine__process_mmap2_event(struct machine *machine,
 	}
 
 	thread = machine__findnew_thread(machine, event->mmap2.pid,
-					event->mmap2.pid);
+					event->mmap2.tid);
 	if (thread == NULL)
 		goto out_problem;
 
@@ -1074,7 +1074,7 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
 	}
 
 	thread = machine__findnew_thread(machine, event->mmap.pid,
-					 event->mmap.pid);
+					 event->mmap.tid);
 	if (thread == NULL)
 		goto out_problem;
 
-- 
1.7.11.7

Re: [PATCH 3/3] perf: fix synthesizing mmaps for threads

[Arnaldo Carvalho de Melo]

Em Tue, Feb 25, 2014 at 10:43:47PM -0500, Don Zickus escreveu:
> Currently if a process creates a bunch of threads using pthread_create
> and then perf is run in system_wide mode, the mmaps for those threads
> are not captured with a synthesized mmap event.
> 
> The reason is those threads are not visible when walking the /proc/
> directory looking for /proc/<pid>/maps files.  Instead they are discovered
> using the /proc/<pid>/tasks file (which the synthesized comm event uses).
> 
> This causes problems when a program is trying to map a data address to a
> tid.  Because the tid has no maps, the event is dropped.  Changing the program
> to look up using the pid instead of the tid, finds the correct maps but creates
> ugly hacks in the program to carry the correct tid around.
> 
> Fix this by synthesizing mmap events for each tid found in the /proc/<pid>/tasks
> file.

This seems to cover two problems, the first is for mmap/mmap2 event
processing to lookup pid/tid instead of pid/pid, the other one is to
iterate thru /proc/pid/tasks/, so this needes spliting up.

Now looking at the /tasks/ part...
 
> This may not be entirely clean but it seems to work.
> 
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  tools/perf/util/event.c   | 15 +++++++++++----
>  tools/perf/util/machine.c |  4 ++--
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 086c7c8..09c53bb 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -93,10 +93,13 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
>  }
>  
>  static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> -					 union perf_event *event, pid_t pid,
> +					 union perf_event *event,
> +					 union perf_event *mmap_event,
> +					 pid_t pid,
>  					 int full,
>  					 perf_event__handler_t process,
> -					 struct machine *machine)
> +					 struct machine *machine,
> +					 bool mmap_data)
>  {
>  	char filename[PATH_MAX];
>  	size_t size;
> @@ -168,6 +171,10 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
>  			tgid = -1;
>  			break;
>  		}
> +
> +		/* process the thread's maps too */
> +		perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> +						  process, machine, mmap_data);
>  	}
>  
>  	closedir(tasks);
> @@ -331,8 +338,8 @@ static int __event__synthesize_thread(union perf_event *comm_event,
>  				      struct perf_tool *tool,
>  				      struct machine *machine, bool mmap_data)
>  {
> -	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, pid, full,
> -						 process, machine);
> +	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, mmap_event, pid,
> +						 full, process, machine, mmap_data);
>  	if (tgid == -1)
>  		return -1;
>  	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 813e94e..eb26544 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1026,7 +1026,7 @@ int machine__process_mmap2_event(struct machine *machine,
>  	}
>  
>  	thread = machine__findnew_thread(machine, event->mmap2.pid,
> -					event->mmap2.pid);
> +					event->mmap2.tid);
>  	if (thread == NULL)
>  		goto out_problem;
>  
> @@ -1074,7 +1074,7 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
>  	}
>  
>  	thread = machine__findnew_thread(machine, event->mmap.pid,
> -					 event->mmap.pid);
> +					 event->mmap.tid);
>  	if (thread == NULL)
>  		goto out_problem;
>  
> -- 
> 1.7.11.7

Re: [PATCH 3/3] perf: fix synthesizing mmaps for threads

[Arnaldo Carvalho de Melo]

Em Wed, Feb 26, 2014 at 11:17:26AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 25, 2014 at 10:43:47PM -0500, Don Zickus escreveu:
> > Currently if a process creates a bunch of threads using pthread_create
> > and then perf is run in system_wide mode, the mmaps for those threads
> > are not captured with a synthesized mmap event.
> > 
> > The reason is those threads are not visible when walking the /proc/
> > directory looking for /proc/<pid>/maps files.  Instead they are discovered
> > using the /proc/<pid>/tasks file (which the synthesized comm event uses).
> > 
> > This causes problems when a program is trying to map a data address to a
> > tid.  Because the tid has no maps, the event is dropped.  Changing the program
> > to look up using the pid instead of the tid, finds the correct maps but creates
> > ugly hacks in the program to carry the correct tid around.
> > 
> > Fix this by synthesizing mmap events for each tid found in the /proc/<pid>/tasks
> > file.
> 
> This seems to cover two problems, the first is for mmap/mmap2 event
> processing to lookup pid/tid instead of pid/pid, the other one is to
> iterate thru /proc/pid/tasks/, so this needes spliting up.
> 
> Now looking at the /tasks/ part...

Agreed we need to synthesize the mmap events for the threads in
/proc/pid/task/, but that need to be done in
perf_event__synthesize_mmap_events, not in
perf_event__synthesize_comm_events, that should remain just for
synthesizing comm events .

I.e. we should move that /tasks/ traversal from synthesize_comm to its
caller and from there synthesize the mmaps too.

I can do that if you don't beat me to it. :-)

- Arnaldo

> > This may not be entirely clean but it seems to work.
> > 
> > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > ---
> >  tools/perf/util/event.c   | 15 +++++++++++----
> >  tools/perf/util/machine.c |  4 ++--
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> > index 086c7c8..09c53bb 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -93,10 +93,13 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
> >  }
> >  
> >  static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> > -					 union perf_event *event, pid_t pid,
> > +					 union perf_event *event,
> > +					 union perf_event *mmap_event,
> > +					 pid_t pid,
> >  					 int full,
> >  					 perf_event__handler_t process,
> > -					 struct machine *machine)
> > +					 struct machine *machine,
> > +					 bool mmap_data)
> >  {
> >  	char filename[PATH_MAX];
> >  	size_t size;
> > @@ -168,6 +171,10 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> >  			tgid = -1;
> >  			break;
> >  		}
> > +
> > +		/* process the thread's maps too */
> > +		perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> > +						  process, machine, mmap_data);
> >  	}
> >  
> >  	closedir(tasks);
> > @@ -331,8 +338,8 @@ static int __event__synthesize_thread(union perf_event *comm_event,
> >  				      struct perf_tool *tool,
> >  				      struct machine *machine, bool mmap_data)
> >  {
> > -	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, pid, full,
> > -						 process, machine);
> > +	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, mmap_event, pid,
> > +						 full, process, machine, mmap_data);
> >  	if (tgid == -1)
> >  		return -1;
> >  	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 813e94e..eb26544 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1026,7 +1026,7 @@ int machine__process_mmap2_event(struct machine *machine,
> >  	}
> >  
> >  	thread = machine__findnew_thread(machine, event->mmap2.pid,
> > -					event->mmap2.pid);
> > +					event->mmap2.tid);
> >  	if (thread == NULL)
> >  		goto out_problem;
> >  
> > @@ -1074,7 +1074,7 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
> >  	}
> >  
> >  	thread = machine__findnew_thread(machine, event->mmap.pid,
> > -					 event->mmap.pid);
> > +					 event->mmap.tid);
> >  	if (thread == NULL)
> >  		goto out_problem;
> >  
> > -- 
> > 1.7.11.7

Re: [PATCH 3/3] perf: fix synthesizing mmaps for threads

[Don Zickus]

On Wed, Feb 26, 2014 at 11:17:26AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 25, 2014 at 10:43:47PM -0500, Don Zickus escreveu:
> > Currently if a process creates a bunch of threads using pthread_create
> > and then perf is run in system_wide mode, the mmaps for those threads
> > are not captured with a synthesized mmap event.
> > 
> > The reason is those threads are not visible when walking the /proc/
> > directory looking for /proc/<pid>/maps files.  Instead they are discovered
> > using the /proc/<pid>/tasks file (which the synthesized comm event uses).
> > 
> > This causes problems when a program is trying to map a data address to a
> > tid.  Because the tid has no maps, the event is dropped.  Changing the program
> > to look up using the pid instead of the tid, finds the correct maps but creates
> > ugly hacks in the program to carry the correct tid around.
> > 
> > Fix this by synthesizing mmap events for each tid found in the /proc/<pid>/tasks
> > file.
> 
> This seems to cover two problems, the first is for mmap/mmap2 event
> processing to lookup pid/tid instead of pid/pid, the other one is to
> iterate thru /proc/pid/tasks/, so this needes spliting up.
> 
> Now looking at the /tasks/ part...

Fair enough. :-)

Cheers,
Don

>  
> > This may not be entirely clean but it seems to work.
> > 
> > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > ---
> >  tools/perf/util/event.c   | 15 +++++++++++----
> >  tools/perf/util/machine.c |  4 ++--
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> > index 086c7c8..09c53bb 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -93,10 +93,13 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
> >  }
> >  
> >  static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> > -					 union perf_event *event, pid_t pid,
> > +					 union perf_event *event,
> > +					 union perf_event *mmap_event,
> > +					 pid_t pid,
> >  					 int full,
> >  					 perf_event__handler_t process,
> > -					 struct machine *machine)
> > +					 struct machine *machine,
> > +					 bool mmap_data)
> >  {
> >  	char filename[PATH_MAX];
> >  	size_t size;
> > @@ -168,6 +171,10 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> >  			tgid = -1;
> >  			break;
> >  		}
> > +
> > +		/* process the thread's maps too */
> > +		perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> > +						  process, machine, mmap_data);
> >  	}
> >  
> >  	closedir(tasks);
> > @@ -331,8 +338,8 @@ static int __event__synthesize_thread(union perf_event *comm_event,
> >  				      struct perf_tool *tool,
> >  				      struct machine *machine, bool mmap_data)
> >  {
> > -	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, pid, full,
> > -						 process, machine);
> > +	pid_t tgid = perf_event__synthesize_comm(tool, comm_event, mmap_event, pid,
> > +						 full, process, machine, mmap_data);
> >  	if (tgid == -1)
> >  		return -1;
> >  	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 813e94e..eb26544 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1026,7 +1026,7 @@ int machine__process_mmap2_event(struct machine *machine,
> >  	}
> >  
> >  	thread = machine__findnew_thread(machine, event->mmap2.pid,
> > -					event->mmap2.pid);
> > +					event->mmap2.tid);
> >  	if (thread == NULL)
> >  		goto out_problem;
> >  
> > @@ -1074,7 +1074,7 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
> >  	}
> >  
> >  	thread = machine__findnew_thread(machine, event->mmap.pid,
> > -					 event->mmap.pid);
> > +					 event->mmap.tid);
> >  	if (thread == NULL)
> >  		goto out_problem;
> >  
> > -- 
> > 1.7.11.7