perf record: Allocate area for sample_id_hdr in a synthesized comm event

PATCH Applied Namhyung Kim 3 participants 7 messages Sep 30, 2015 – Oct 3, 2015 perf events

Share & tools

Namhyung Kim

author maintainer

2015-09-30 01:59 UTC #0 lore raw

PATCH 1/3 perf record: Allocate area for sample_id_hdr in a synthesized comm event

A previous patch added a synthesized comm event for forked child process but it missed that the event should contain area for sample_id_hdr at the end. It worked by accident since the perf_event union contains bigger event structs like mmap_events.

This patch fixes it by dynamically allocating event struct including those area like in perf_event__synthesize_thread_map().

Reported-by Arnaldo Carvalho de Melo <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-record.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

tools/perf/builtin-record.c +10 -2

index a01c8ae1ee07..5e01c070dbf2 100644
@@ -637,17 +637,25 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	 * Let the child rip
 	 */
 	if (forks) {
-		union perf_event event;
+		union perf_event *event;
+
+		event = malloc(sizeof(event->comm) + machine->id_hdr_size);
+		if (event == NULL) {
+			err = -ENOMEM;
+			goto out_child;
+		}
+
 		/*
 		 * Some H/W events are generated before COMM event
 		 * which is emitted during exec(), so perf script
 		 * cannot see a correct process name for those events.
 		 * Synthesize COMM event to prevent it.
 		 */
-		perf_event__synthesize_comm(tool, &event,
+		perf_event__synthesize_comm(tool, event,
 					    rec->evlist->workload.pid,
 					    process_synthesized_event,
 					    machine);
+		free(event);
 
 		perf_evlist__start_workload(rec->evlist);
 	}

Signature · 1 line

2.5.0

Namhyung Kim

author maintainer

2015-09-30 01:59 UTC #1 lore raw

PATCH 2/3 perf top: Fix unresolved comm when -s comm is used

The perf top uses 'dso,symbol' sort keys by default so it overlooked a problem in task's comm resolving. When the sort key contains 'comm', some task's comm is not shown properly. This is because the perf_top__mmap_read_idx() checks the cpumode value improperly.

The cpumode value of non-sample events are 0 (PERF_RECORD_MISC_CPUMODE_ UNKNOWN) so the events will be ignored by the switch statement. This patch allows it for non-sample events.

Signed-off-by Namhyung Kim <[email protected]>

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

tools/perf/builtin-top.c +5 -2

index 38d4d6cac823..ae4c6420300b 100644
@@ -857,9 +857,12 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
 			 * TODO: we don't process guest user from host side
 			 * except simple counting.
 			 */
-			/* Fall thru */
-		default:
 			goto next_event;
+		default:
+			if (event->header.type == PERF_RECORD_SAMPLE)
+				goto next_event;
+			machine = &session->machines.host;
+			break;
 		}

Signature · 1 line

2.5.0

tip-bot for Namhyung Kim

merge bot

2015-10-03 07:49 UTC #2 lore raw

[tip:perf/core] perf top: Fix unresolved comm when -s comm is used

perf top: Fix unresolved comm when -s comm is used

Commit-ID:  4b37af595742977f1bdd8c0fd0f3e6e55b36e7b7
Gitweb:     http://git.kernel.org/tip/4b37af595742977f1bdd8c0fd0f3e6e55b36e7b7
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 30 Sep 2015 10:45:25 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 1 Oct 2015 09:54:33 -0300

perf top: Fix unresolved comm when -s comm is used

The cpumode value of non-sample events are 0 (PERF_RECORD_MISC_CPUMODE_ UNKNOWN) so the events will be ignored by the switch statement. This patch allows it for non-sample events.

Signed-off-by Namhyung Kim <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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

tools/perf/builtin-top.c +5 -2

index 38d4d6c..ae4c642 100644
@@ -857,9 +857,12 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
 			 * TODO: we don't process guest user from host side
 			 * except simple counting.
 			 */
-			/* Fall thru */
-		default:
 			goto next_event;
+		default:
+			if (event->header.type == PERF_RECORD_SAMPLE)
+				goto next_event;
+			machine = &session->machines.host;
+			break;
 		}

Namhyung Kim

author maintainer

2015-09-30 01:59 UTC #3 lore raw

PATCH 3/3 perf top: Register idle thread

In reply to Namhyung Kim

The perf top didn't add the idle/swapper thread to the machine's thread list and its comm was displayed as ':0'. Fix it.

Signed-off-by Namhyung Kim <[email protected]>

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

tools/perf/builtin-top.c +3 -0

index ae4c6420300b..6f641fd68296 100644
@@ -964,6 +964,9 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
+	if (perf_session__register_idle_thread(top->session) == NULL)
+		goto out_delete;
+
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
 				    top->evlist->threads, false, opts->proc_map_timeout);

tools/perf/util/session.c +1 -1

index 84a02eae4394..428149bc64d2 100644
@@ -1311,7 +1311,7 @@ struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
 	return machine__findnew_thread(&session->machines.host, -1, pid);
 }
 
-static struct thread *perf_session__register_idle_thread(struct perf_session *session)
+struct thread *perf_session__register_idle_thread(struct perf_session *session)
 {
 	struct thread *thread;

tools/perf/util/session.h +2 -0

index b44afc75d1cc..3e900c0efc73 100644
@@ -89,6 +89,8 @@ struct machine *perf_session__findnew_machine(struct perf_session *session, pid_
 }
 
 struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
+struct thread *perf_session__register_idle_thread(struct perf_session *session);
+
 size_t perf_session__fprintf(struct perf_session *session, FILE *fp);
 
 size_t perf_session__fprintf_dsos(struct perf_session *session, FILE *fp);

Signature · 1 line

2.5.0

tip-bot for Namhyung Kim

merge bot

2015-10-03 07:49 UTC #4 lore raw

[tip:perf/core] perf top: Register idle thread

perf top: Register idle thread

Commit-ID:  c53d138d41a7f33cf085762c64b4b61e8d223e1c
Gitweb:     http://git.kernel.org/tip/c53d138d41a7f33cf085762c64b4b61e8d223e1c
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 30 Sep 2015 10:45:26 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 1 Oct 2015 09:54:33 -0300

perf top: Register idle thread

The perf top didn't add the idle/swapper thread to the machine's thread list and its comm was displayed as ':0'. Fix it.

Signed-off-by Namhyung Kim <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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

tools/perf/builtin-top.c +3 -0

index ae4c642..6f641fd 100644
@@ -964,6 +964,9 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
+	if (perf_session__register_idle_thread(top->session) == NULL)
+		goto out_delete;
+
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
 				    top->evlist->threads, false, opts->proc_map_timeout);

tools/perf/util/session.c +1 -1

index 84a02eae..428149b 100644
@@ -1311,7 +1311,7 @@ struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
 	return machine__findnew_thread(&session->machines.host, -1, pid);
 }
 
-static struct thread *perf_session__register_idle_thread(struct perf_session *session)
+struct thread *perf_session__register_idle_thread(struct perf_session *session)
 {
 	struct thread *thread;

tools/perf/util/session.h +2 -0

index b44afc7..3e900c0 100644
@@ -89,6 +89,8 @@ struct machine *perf_session__findnew_machine(struct perf_session *session, pid_
 }
 
 struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
+struct thread *perf_session__register_idle_thread(struct perf_session *session);
+
 size_t perf_session__fprintf(struct perf_session *session, FILE *fp);
 
 size_t perf_session__fprintf_dsos(struct perf_session *session, FILE *fp);

Arnaldo Carvalho de Melo

maintainer Applied

2015-09-30 22:03 UTC #5 lore raw

In reply to Namhyung Kim

Em Wed, Sep 30, 2015 at 10:45:24AM +0900, Namhyung Kim escreveu:

Quoted 7 lines

A previous patch added a synthesized comm event for forked child process but it missed that the event should contain area for sample_id_hdr at the end. It worked by accident since the perf_event union contains bigger event structs like mmap_events.
This patch fixes it by dynamically allocating event struct including those area like in perf_event__synthesize_thread_map().

Thanks, applied the three patches in this series.

- Arnaldo

tip-bot for Namhyung Kim

merge bot

2015-10-03 07:49 UTC #6 lore raw

In reply to Namhyung Kim

perf record: Allocate area for sample_id_hdr in a synthesized comm event

Commit-ID:  e5bed564485b340d87f2d8945643a393e55b8225
Gitweb:     http://git.kernel.org/tip/e5bed564485b340d87f2d8945643a393e55b8225
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 30 Sep 2015 10:45:24 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 1 Oct 2015 09:54:33 -0300

perf record: Allocate area for sample_id_hdr in a synthesized comm event

This patch fixes it by dynamically allocating event struct including those area like in perf_event__synthesize_thread_map().

Reported-by Arnaldo Carvalho de Melo <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-record.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

tools/perf/builtin-record.c +10 -2

index a01c8ae..5e01c07 100644
@@ -637,17 +637,25 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	 * Let the child rip
 	 */
 	if (forks) {
-		union perf_event event;
+		union perf_event *event;
+
+		event = malloc(sizeof(event->comm) + machine->id_hdr_size);
+		if (event == NULL) {
+			err = -ENOMEM;
+			goto out_child;
+		}
+
 		/*
 		 * Some H/W events are generated before COMM event
 		 * which is emitted during exec(), so perf script
 		 * cannot see a correct process name for those events.
 		 * Synthesize COMM event to prevent it.
 		 */
-		perf_event__synthesize_comm(tool, &event,
+		perf_event__synthesize_comm(tool, event,
 					    rec->evlist->workload.pid,
 					    process_synthesized_event,
 					    machine);
+		free(event);
 
 		perf_evlist__start_workload(rec->evlist);
 	}