[PATCH 0/2] perf: thread fixes

[PATCH 0/2] perf: thread fixes

[Don Zickus]

Fixups for synthesizing thread maps and locating them later

Don Zickus (2):
  perf: fix synthesizing mmaps for threads
  perf: Use tid in mmap/mmap2 events to find maps

 tools/perf/util/event.c   | 111 +++++++++++++++++++++++-----------------------
 tools/perf/util/machine.c |   4 +-
 2 files changed, 58 insertions(+), 57 deletions(-)

-- 
1.7.11.7

[PATCH 1/2] 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 moving the walking of the /proc/<pid>/tasks up a level (out of the
comm function) based on Arnaldo's suggestion.

Tweaked things a bit to special case the 'full' bit and 'guest' check.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/event.c | 111 ++++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 55 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 086c7c8..82fb890 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -94,14 +94,10 @@ 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,
-					 int full,
 					 perf_event__handler_t process,
 					 struct machine *machine)
 {
-	char filename[PATH_MAX];
 	size_t size;
-	DIR *tasks;
-	struct dirent dirent, *next;
 	pid_t tgid;
 
 	memset(&event->comm, 0, sizeof(event->comm));
@@ -124,53 +120,11 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 	event->comm.header.size = (sizeof(event->comm) -
 				(sizeof(event->comm.comm) - size) +
 				machine->id_hdr_size);
-	if (!full) {
-		event->comm.tid = pid;
-
-		if (process(tool, event, &synth_sample, machine) != 0)
-			return -1;
+	event->comm.tid = pid;
 
-		goto out;
-	}
-
-	if (machine__is_default_guest(machine))
-		return 0;
-
-	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
-		 machine->root_dir, pid);
-
-	tasks = opendir(filename);
-	if (tasks == NULL) {
-		pr_debug("couldn't open %s\n", filename);
-		return 0;
-	}
-
-	while (!readdir_r(tasks, &dirent, &next) && next) {
-		char *end;
-		pid = strtol(dirent.d_name, &end, 10);
-		if (*end)
-			continue;
-
-		/* already have tgid; jut want to update the comm */
-		(void) perf_event__get_comm_tgid(pid, event->comm.comm,
-					 sizeof(event->comm.comm));
-
-		size = strlen(event->comm.comm) + 1;
-		size = PERF_ALIGN(size, sizeof(u64));
-		memset(event->comm.comm + size, 0, machine->id_hdr_size);
-		event->comm.header.size = (sizeof(event->comm) -
-					  (sizeof(event->comm.comm) - size) +
-					  machine->id_hdr_size);
-
-		event->comm.tid = pid;
-
-		if (process(tool, event, &synth_sample, machine) != 0) {
-			tgid = -1;
-			break;
-		}
-	}
+	if (process(tool, event, &synth_sample, machine) != 0)
+		return -1;
 
-	closedir(tasks);
 out:
 	return tgid;
 }
@@ -331,12 +285,59 @@ 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);
-	if (tgid == -1)
-		return -1;
-	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
-						  process, machine, mmap_data);
+	char filename[PATH_MAX];
+	DIR *tasks;
+	struct dirent dirent, *next;
+	pid_t tgid;
+
+	/* special case: only send one comm event using passed in pid */
+	if (!full) {
+		tgid = perf_event__synthesize_comm(tool, comm_event, pid,
+						   process, machine);
+
+		if (tgid == -1)
+			return -1;
+
+		return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
+							  process, machine, mmap_data);
+	}
+
+	if (machine__is_default_guest(machine))
+		return 0;
+
+	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
+		 machine->root_dir, pid);
+
+	tasks = opendir(filename);
+	if (tasks == NULL) {
+		pr_debug("couldn't open %s\n", filename);
+		return 0;
+	}
+
+	while (!readdir_r(tasks, &dirent, &next) && next) {
+		char *end;
+		int rc = 0;
+		pid_t _pid;
+
+		_pid = strtol(dirent.d_name, &end, 10);
+		if (*end)
+			continue;
+
+		tgid = perf_event__synthesize_comm(tool, comm_event, _pid,
+						   process, machine);
+		if (tgid == -1)
+			return -1;
+
+		/* process the thread's maps too */
+		rc = perf_event__synthesize_mmap_events(tool, mmap_event, _pid, tgid,
+							process, machine, mmap_data);
+
+		if (rc)
+			return rc;
+	}
+
+	closedir(tasks);
+	return 0;
 }
 
 int perf_event__synthesize_thread_map(struct perf_tool *tool,
-- 
1.7.11.7

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

[Jiri Olsa]

On Wed, Feb 26, 2014 at 10:45:26AM -0500, Don Zickus wrote:
> 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.

hm, 2 hacks comes to my mind ;-)

1) share 'struct thread::mg' among thread group (pid)

2) or lookup the thread group leader if we find out we are
   not the leader and dont have the map info (attached)

your change makes the process map info (same info) being duplicated
for all threads (eventhough it's probably not that much bytes wasted)

I think I'd prefer ad 1) ... the patch for ad 2) assumes there's
always thread group leader (which might not be the case always?)
also 'thread->pid_' handling seems troubled

I dont have code solution for 1), maybe you've already cover that
and considered it hacky.. just throwing ideas ;-)

jirka

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index b0f3ca8..c428186 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -654,9 +654,17 @@ void thread__find_addr_map(struct thread *thread,
 			   enum map_type type, u64 addr,
 			   struct addr_location *al)
 {
-	struct map_groups *mg = &thread->mg;
+	struct map_groups *mg;
 	bool load_map = false;
 
+	if (thread->tid != thread->pid_) {
+		thread = machine__findnew_thread(machine, thread->pid_, thread->pid_);
+		if (!thread)
+			return;
+	}
+
+	mg = &thread->mg;
+
 	al->machine = machine;
 	al->thread = thread;
 	al->addr = addr;

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

[Don Zickus]

On Wed, Mar 05, 2014 at 05:58:57PM +0100, Jiri Olsa wrote:
> On Wed, Feb 26, 2014 at 10:45:26AM -0500, Don Zickus wrote:
> > 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.
> 
> hm, 2 hacks comes to my mind ;-)
> 
> 1) share 'struct thread::mg' among thread group (pid)
> 
> 2) or lookup the thread group leader if we find out we are
>    not the leader and dont have the map info (attached)
> 
> your change makes the process map info (same info) being duplicated
> for all threads (eventhough it's probably not that much bytes wasted)
> 
> I think I'd prefer ad 1) ... the patch for ad 2) assumes there's
> always thread group leader (which might not be the case always?)
> also 'thread->pid_' handling seems troubled
> 
> I dont have code solution for 1), maybe you've already cover that
> and considered it hacky.. just throwing ideas ;-)

It doesn't matter to me. :-)  The c2c tool needs this to work correctly
otherwise the analysis is wrong when profiling the system with the app
already running with lots of threads (ie databases).

Well I shouldn't say the analysis is wrong, it just wrongly attributes the
pid being responsible for all the problems when it could be some garbage
collection thread running to frequently.

Cheers,
Don

> 
> jirka
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index b0f3ca8..c428186 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -654,9 +654,17 @@ void thread__find_addr_map(struct thread *thread,
>  			   enum map_type type, u64 addr,
>  			   struct addr_location *al)
>  {
> -	struct map_groups *mg = &thread->mg;
> +	struct map_groups *mg;
>  	bool load_map = false;
>  
> +	if (thread->tid != thread->pid_) {
> +		thread = machine__findnew_thread(machine, thread->pid_, thread->pid_);
> +		if (!thread)
> +			return;
> +	}
> +
> +	mg = &thread->mg;
> +
>  	al->machine = machine;
>  	al->thread = thread;
>  	al->addr = addr;

[tip:perf/core] perf tools: Fix synthesizing mmaps for threads

[tip-bot for Don Zickus]

Commit-ID:  bfd66cc71a3f831ba7c2116d79416cfb8883f6cf
Gitweb:     http://git.kernel.org/tip/bfd66cc71a3f831ba7c2116d79416cfb8883f6cf
Author:     Don Zickus <dzickus@redhat.com>
AuthorDate: Wed, 26 Feb 2014 10:45:26 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 14 Mar 2014 11:20:44 -0300

perf tools: Fix synthesizing mmaps for threads

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 moving the walking of the /proc/<pid>/tasks up a level (out
of the comm function) based on Arnaldo's suggestion.

Tweaked things a bit to special case the 'full' bit and 'guest' check.

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/1393429527-167840-2-git-send-email-dzickus@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/event.c | 111 ++++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 55 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index b0f3ca8..55eebe9 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -94,14 +94,10 @@ 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,
-					 int full,
 					 perf_event__handler_t process,
 					 struct machine *machine)
 {
-	char filename[PATH_MAX];
 	size_t size;
-	DIR *tasks;
-	struct dirent dirent, *next;
 	pid_t tgid;
 
 	memset(&event->comm, 0, sizeof(event->comm));
@@ -124,53 +120,11 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 	event->comm.header.size = (sizeof(event->comm) -
 				(sizeof(event->comm.comm) - size) +
 				machine->id_hdr_size);
-	if (!full) {
-		event->comm.tid = pid;
-
-		if (process(tool, event, &synth_sample, machine) != 0)
-			return -1;
+	event->comm.tid = pid;
 
-		goto out;
-	}
-
-	if (machine__is_default_guest(machine))
-		return 0;
-
-	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
-		 machine->root_dir, pid);
-
-	tasks = opendir(filename);
-	if (tasks == NULL) {
-		pr_debug("couldn't open %s\n", filename);
-		return 0;
-	}
-
-	while (!readdir_r(tasks, &dirent, &next) && next) {
-		char *end;
-		pid = strtol(dirent.d_name, &end, 10);
-		if (*end)
-			continue;
-
-		/* already have tgid; jut want to update the comm */
-		(void) perf_event__get_comm_tgid(pid, event->comm.comm,
-					 sizeof(event->comm.comm));
-
-		size = strlen(event->comm.comm) + 1;
-		size = PERF_ALIGN(size, sizeof(u64));
-		memset(event->comm.comm + size, 0, machine->id_hdr_size);
-		event->comm.header.size = (sizeof(event->comm) -
-					  (sizeof(event->comm.comm) - size) +
-					  machine->id_hdr_size);
-
-		event->comm.tid = pid;
-
-		if (process(tool, event, &synth_sample, machine) != 0) {
-			tgid = -1;
-			break;
-		}
-	}
+	if (process(tool, event, &synth_sample, machine) != 0)
+		return -1;
 
-	closedir(tasks);
 out:
 	return tgid;
 }
@@ -329,12 +283,59 @@ 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);
-	if (tgid == -1)
-		return -1;
-	return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
-						  process, machine, mmap_data);
+	char filename[PATH_MAX];
+	DIR *tasks;
+	struct dirent dirent, *next;
+	pid_t tgid;
+
+	/* special case: only send one comm event using passed in pid */
+	if (!full) {
+		tgid = perf_event__synthesize_comm(tool, comm_event, pid,
+						   process, machine);
+
+		if (tgid == -1)
+			return -1;
+
+		return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
+							  process, machine, mmap_data);
+	}
+
+	if (machine__is_default_guest(machine))
+		return 0;
+
+	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
+		 machine->root_dir, pid);
+
+	tasks = opendir(filename);
+	if (tasks == NULL) {
+		pr_debug("couldn't open %s\n", filename);
+		return 0;
+	}
+
+	while (!readdir_r(tasks, &dirent, &next) && next) {
+		char *end;
+		int rc = 0;
+		pid_t _pid;
+
+		_pid = strtol(dirent.d_name, &end, 10);
+		if (*end)
+			continue;
+
+		tgid = perf_event__synthesize_comm(tool, comm_event, _pid,
+						   process, machine);
+		if (tgid == -1)
+			return -1;
+
+		/* process the thread's maps too */
+		rc = perf_event__synthesize_mmap_events(tool, mmap_event, _pid, tgid,
+							process, machine, mmap_data);
+
+		if (rc)
+			return rc;
+	}
+
+	closedir(tasks);
+	return 0;
 }
 
 int perf_event__synthesize_thread_map(struct perf_tool *tool,

[PATCH 2/2] perf: Use tid in mmap/mmap2 events to find maps

[Don Zickus]

Now that we can properly synthesize threads system-wide, make sure
the mmap and mmap2 events use tids instead of pids to locate their
maps.

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

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

[tip:perf/core] perf tools: Use tid in mmap/ mmap2 events to find maps

[tip-bot for Don Zickus]

Commit-ID:  11c9abf2270793bd1c1b8828edb4223f8010e56c
Gitweb:     http://git.kernel.org/tip/11c9abf2270793bd1c1b8828edb4223f8010e56c
Author:     Don Zickus <dzickus@redhat.com>
AuthorDate: Wed, 26 Feb 2014 10:45:27 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 18 Mar 2014 18:17:00 -0300

perf tools: Use tid in mmap/mmap2 events to find maps

Now that we can properly synthesize threads system-wide, make sure the
mmap and mmap2 events use tids instead of pids to locate their maps.

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/1393429527-167840-3-git-send-email-dzickus@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/hists_link.c | 1 +
 tools/perf/util/machine.c     | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 2b6519e..7ccbc7b 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -101,6 +101,7 @@ static struct machine *setup_fake_machine(struct machines *machines)
 			.mmap = {
 				.header = { .misc = PERF_RECORD_MISC_USER, },
 				.pid = fake_mmap_info[i].pid,
+				.tid = fake_mmap_info[i].pid,
 				.start = fake_mmap_info[i].start,
 				.len = 0x1000ULL,
 				.pgoff = 0ULL,
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d280bf2..a53cd0b 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1027,7 +1027,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;
 
@@ -1075,7 +1075,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;