Skip to contentSkip to conversation

perf tools: some fixes and tweaks

PATCH Superseded Adrian Hunter 3 participants 23 messages Jun 28, 2013 – Jul 2, 2013 perf events

v3 of 13 Superseded Latest is v13 →

lore
Share & tools
lore

Adrian Hunter

author reviewer
#0 lore raw

v3 cover perf tools: some fixes and tweaks

Hi

Here are some fixes and tweaks to perf tools (version 3).

Changes in V3:
perf tools: add pid to struct thread
Split into 2 patches
perf tools: fix ppid in thread__fork()
Dropped for now

Changes in V2:
perf tools: fix missing tool parameter
Fixed one extra occurrence
perf tools: fix parse_events_terms() freeing local variable on error path
Made "freeing" code into a new function
perf tools: validate perf event header size
Corrected byte-swapping
perf tools: allow non-matching sample types
Added comments
Fixed id_pos calculation
id_pos/is_pos updated whenever sample_type changes Removed perf_evlist__sample_type()
Added __perf_evlist__combined_sample_type()
Added perf_evlist__combined_sample_type()
Added perf_evlist__make_sample_types_compatible()
Added ack's to patches acked by Jiri Olsa

Adrian Hunter (15):
perf tools: remove unused parameter
perf tools: fix missing tool parameter
perf tools: fix missing 'finished_round'
perf tools: fix parse_events_terms() segfault on error path perf tools: fix new_term() missing free on error path
perf tools: fix parse_events_terms() freeing local variable on error path perf tools: add const specifier to perf_pmu__find name parameter perf tools: tidy duplicated munmap code
perf tools: validate perf event header size
perf tools: add debug prints
perf tools: fix symbol_conf.nr_events
perf tools: allow non-matching sample types
perf tools: struct thread has a tid not a pid
perf tools: add pid to struct thread
perf tools: change "machine" functions to set thread pid

 tools/perf/builtin-inject.c    |  40 +++++-----
 tools/perf/builtin-kmem.c      |   2 +-
 tools/perf/builtin-report.c    |   2 +-
 tools/perf/builtin-sched.c     |  12 +--
 tools/perf/builtin-trace.c     |   4 +-
 tools/perf/ui/browsers/hists.c |   6 +-
 tools/perf/util/event.c        |   2 +-
 tools/perf/util/event.h        |  14 ++++
 tools/perf/util/evlist.c       | 170 +++++++++++++++++++++++++++++++++++------
 tools/perf/util/evlist.h       |   8 +-
 tools/perf/util/evsel.c        | 116 +++++++++++++++++++++++++++-
 tools/perf/util/evsel.h        |  10 +++
 tools/perf/util/header.c       |   8 +-
 tools/perf/util/header.h       |   6 +-
 tools/perf/util/machine.c      |  57 ++++++++++----
 tools/perf/util/machine.h      |   8 +-
 tools/perf/util/parse-events.c |  11 ++-
 tools/perf/util/parse-events.h |   1 +
 tools/perf/util/pmu.c          |  16 ++--
 tools/perf/util/pmu.h          |   2 +-
 tools/perf/util/session.c      |  33 +++++---
 tools/perf/util/sort.c         |   6 +-
 tools/perf/util/thread.c       |  11 +--
 tools/perf/util/thread.h       |   5 +-
 tools/perf/util/tool.h         |   9 +--
 25 files changed, 441 insertions(+), 118 deletions(-)

Regards
Adrian

author reviewer
#1 lore raw

PATCH v3 1/15 perf tools: remove unused parameter

'machine' is unused in 'perf_event__repipe_synth()' and some callers pass NULL anyway. So remove it.

---
 tools/perf/builtin-inject.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
tools/perf/builtin-inject.c +9 -10
index 84ad6ab..f299ddf 100644
@@ -38,8 +38,7 @@ struct event_entry {
 };
 
 static int perf_event__repipe_synth(struct perf_tool *tool,
-				    union perf_event *event,
-				    struct machine *machine __maybe_unused)
+				    union perf_event *event)
 {
 	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
 	uint32_t size;
@@ -65,20 +64,20 @@ static int perf_event__repipe_op2_synth(struct perf_tool *tool,
 					struct perf_session *session
 					__maybe_unused)
 {
-	return perf_event__repipe_synth(tool, event, NULL);
+	return perf_event__repipe_synth(tool, event);
 }
 
 static int perf_event__repipe_event_type_synth(struct perf_tool *tool,
 					       union perf_event *event)
 {
-	return perf_event__repipe_synth(tool, event, NULL);
+	return perf_event__repipe_synth(tool, event);
 }
 
 static int perf_event__repipe_tracing_data_synth(union perf_event *event,
 						 struct perf_session *session
 						 __maybe_unused)
 {
-	return perf_event__repipe_synth(NULL, event, NULL);
+	return perf_event__repipe_synth(NULL, event);
 }
 
 static int perf_event__repipe_attr(union perf_event *event,
@@ -89,15 +88,15 @@ static int perf_event__repipe_attr(union perf_event *event,
 	if (ret)
 		return ret;
 
-	return perf_event__repipe_synth(NULL, event, NULL);
+	return perf_event__repipe_synth(NULL, event);
 }
 
 static int perf_event__repipe(struct perf_tool *tool,
 			      union perf_event *event,
 			      struct perf_sample *sample __maybe_unused,
-			      struct machine *machine)
+			      struct machine *machine __maybe_unused)
 {
-	return perf_event__repipe_synth(tool, event, machine);
+	return perf_event__repipe_synth(tool, event);
 }
 
 typedef int (*inject_handler)(struct perf_tool *tool,
@@ -119,7 +118,7 @@ static int perf_event__repipe_sample(struct perf_tool *tool,
 
 	build_id__mark_dso_hit(tool, event, sample, evsel, machine);
 
-	return perf_event__repipe_synth(tool, event, machine);
+	return perf_event__repipe_synth(tool, event);
 }
 
 static int perf_event__repipe_mmap(struct perf_tool *tool,
@@ -153,7 +152,7 @@ static int perf_event__repipe_tracing_data(union perf_event *event,
 {
 	int err;
 
-	perf_event__repipe_synth(NULL, event, NULL);
+	perf_event__repipe_synth(NULL, event);
 	err = perf_event__process_tracing_data(event, session);
 
 	return err;
Signature · 1 line
1.7.11.7
author reviewer
#2 lore raw

PATCH v3 2/15 perf tools: fix missing tool parameter

In reply to Adrian Hunter

perf inject expects to get a reference to 'struct perf_inject' from its 'tool' member. For that to work, 'tool' needs to be a parameter of all tool callbacks. Make it so.

---
 tools/perf/builtin-inject.c | 26 +++++++++++---------------
 tools/perf/util/header.c    |  6 ++++--
 tools/perf/util/header.h    |  6 ++++--
 tools/perf/util/session.c   | 11 +++++++----
 tools/perf/util/tool.h      |  9 ++++-----
 5 files changed, 30 insertions(+), 28 deletions(-)
tools/perf/builtin-inject.c +11 -15
index f299ddf..c943513 100644
@@ -73,22 +73,17 @@ static int perf_event__repipe_event_type_synth(struct perf_tool *tool,
 	return perf_event__repipe_synth(tool, event);
 }
 
-static int perf_event__repipe_tracing_data_synth(union perf_event *event,
-						 struct perf_session *session
-						 __maybe_unused)
-{
-	return perf_event__repipe_synth(NULL, event);
-}
-
-static int perf_event__repipe_attr(union perf_event *event,
-				   struct perf_evlist **pevlist __maybe_unused)
+static int perf_event__repipe_attr(struct perf_tool *tool,
+				   union perf_event *event,
+				   struct perf_evlist **pevlist)
 {
 	int ret;
-	ret = perf_event__process_attr(event, pevlist);
+
+	ret = perf_event__process_attr(tool, event, pevlist);
 	if (ret)
 		return ret;
 
-	return perf_event__repipe_synth(NULL, event);
+	return perf_event__repipe_synth(tool, event);
 }
 
 static int perf_event__repipe(struct perf_tool *tool,
@@ -147,13 +142,14 @@ static int perf_event__repipe_fork(struct perf_tool *tool,
 	return err;
 }
 
-static int perf_event__repipe_tracing_data(union perf_event *event,
+static int perf_event__repipe_tracing_data(struct perf_tool *tool,
+					   union perf_event *event,
 					   struct perf_session *session)
 {
 	int err;
 
-	perf_event__repipe_synth(NULL, event);
-	err = perf_event__process_tracing_data(event, session);
+	perf_event__repipe_synth(tool, event);
+	err = perf_event__process_tracing_data(tool, event, session);
 
 	return err;
 }
@@ -407,7 +403,7 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 			.unthrottle	= perf_event__repipe,
 			.attr		= perf_event__repipe_attr,
 			.event_type	= perf_event__repipe_event_type_synth,
-			.tracing_data	= perf_event__repipe_tracing_data_synth,
+			.tracing_data	= perf_event__repipe_op2_synth,
 			.build_id	= perf_event__repipe_op2_synth,
 		},
 		.input_name  = "-",
tools/perf/util/header.c +4 -2
index 738d3b8..4037b0a 100644
@@ -2933,7 +2933,8 @@ int perf_event__synthesize_attrs(struct perf_tool *tool,
 	return err;
 }
 
-int perf_event__process_attr(union perf_event *event,
+int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
+			     union perf_event *event,
 			     struct perf_evlist **pevlist)
 {
 	u32 i, ids, n_ids;
@@ -3074,7 +3075,8 @@ int perf_event__synthesize_tracing_data(struct perf_tool *tool, int fd,
 	return aligned_size;
 }
 
-int perf_event__process_tracing_data(union perf_event *event,
+int perf_event__process_tracing_data(struct perf_tool *tool __maybe_unused,
+				     union perf_event *event,
 				     struct perf_session *session)
 {
 	ssize_t size_read, padding, size = event->tracing_data.size;
tools/perf/util/header.h +4 -2
index 16a3e83..2d1ca7d 100644
@@ -130,7 +130,8 @@ int perf_event__synthesize_attr(struct perf_tool *tool,
 int perf_event__synthesize_attrs(struct perf_tool *tool,
 				 struct perf_session *session,
 				 perf_event__handler_t process);
-int perf_event__process_attr(union perf_event *event, struct perf_evlist **pevlist);
+int perf_event__process_attr(struct perf_tool *tool, union perf_event *event,
+			     struct perf_evlist **pevlist);
 
 int perf_event__synthesize_event_type(struct perf_tool *tool,
 				      u64 event_id, char *name,
@@ -145,7 +146,8 @@ int perf_event__process_event_type(struct perf_tool *tool,
 int perf_event__synthesize_tracing_data(struct perf_tool *tool,
 					int fd, struct perf_evlist *evlist,
 					perf_event__handler_t process);
-int perf_event__process_tracing_data(union perf_event *event,
+int perf_event__process_tracing_data(struct perf_tool *tool,
+				     union perf_event *event,
 				     struct perf_session *session);
 
 int perf_event__synthesize_build_id(struct perf_tool *tool,
tools/perf/util/session.c +7 -4
index cf1fe01..19ea888 100644
@@ -193,7 +193,9 @@ void perf_session__delete(struct perf_session *self)
 	vdso__exit();
 }
 
-static int process_event_synth_tracing_data_stub(union perf_event *event
+static int process_event_synth_tracing_data_stub(struct perf_tool *tool
+						 __maybe_unused,
+						 union perf_event *event
 						 __maybe_unused,
 						 struct perf_session *session
 						__maybe_unused)
@@ -202,7 +204,8 @@ static int process_event_synth_tracing_data_stub(union perf_event *event
 	return 0;
 }
 
-static int process_event_synth_attr_stub(union perf_event *event __maybe_unused,
+static int process_event_synth_attr_stub(struct perf_tool *tool __maybe_unused,
+					 union perf_event *event __maybe_unused,
 					 struct perf_evlist **pevlist
 					 __maybe_unused)
 {
@@ -921,7 +924,7 @@ static int perf_session__process_user_event(struct perf_session *session, union
 	/* These events are processed right away */
 	switch (event->header.type) {
 	case PERF_RECORD_HEADER_ATTR:
-		err = tool->attr(event, &session->evlist);
+		err = tool->attr(tool, event, &session->evlist);
 		if (err == 0)
 			perf_session__set_id_hdr_size(session);
 		return err;
@@ -930,7 +933,7 @@ static int perf_session__process_user_event(struct perf_session *session, union
 	case PERF_RECORD_HEADER_TRACING_DATA:
 		/* setup for reading amidst mmap */
 		lseek(session->fd, file_offset, SEEK_SET);
-		return tool->tracing_data(event, session);
+		return tool->tracing_data(tool, event, session);
 	case PERF_RECORD_HEADER_BUILD_ID:
 		return tool->build_id(tool, event, session);
 	case PERF_RECORD_FINISHED_ROUND:
tools/perf/util/tool.h +4 -5
index b0e1aad..88f8cbd 100644
@@ -18,12 +18,11 @@ typedef int (*event_sample)(struct perf_tool *tool, union perf_event *event,
 typedef int (*event_op)(struct perf_tool *tool, union perf_event *event,
 			struct perf_sample *sample, struct machine *machine);
 
-typedef int (*event_attr_op)(union perf_event *event,
+typedef int (*event_attr_op)(struct perf_tool *tool,
+			     union perf_event *event,
 			     struct perf_evlist **pevlist);
-typedef int (*event_simple_op)(struct perf_tool *tool, union perf_event *event);
 
-typedef int (*event_synth_op)(union perf_event *event,
-			      struct perf_session *session);
+typedef int (*event_simple_op)(struct perf_tool *tool, union perf_event *event);
 
 typedef int (*event_op2)(struct perf_tool *tool, union perf_event *event,
 			 struct perf_session *session);
@@ -39,7 +38,7 @@ struct perf_tool {
 			throttle,
 			unthrottle;
 	event_attr_op	attr;
-	event_synth_op	tracing_data;
+	event_op2	tracing_data;
 	event_simple_op	event_type;
 	event_op2	finished_round,
 			build_id;
Signature · 1 line
1.7.11.7
author reviewer
#3 lore raw

PATCH v3 3/15 perf tools: fix missing 'finished_round'

In reply to Adrian Hunter

By default, perf inject should "repipe" all events including 'finished_round'.

---
 tools/perf/builtin-inject.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/builtin-inject.c +1 -0
index c943513..ad1296c 100644
@@ -404,6 +404,7 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 			.attr		= perf_event__repipe_attr,
 			.event_type	= perf_event__repipe_event_type_synth,
 			.tracing_data	= perf_event__repipe_op2_synth,
+			.finished_round	= perf_event__repipe_op2_synth,
 			.build_id	= perf_event__repipe_op2_synth,
 		},
 		.input_name  = "-",
Signature · 1 line
1.7.11.7
author reviewer
#4 lore raw

PATCH v3 4/15 perf tools: fix parse_events_terms() segfault on error path

In reply to Adrian Hunter

On the error path, 'data.terms' may not have been
initialised.

---
 tools/perf/util/parse-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tools/perf/util/parse-events.c +2 -1
index 6c8bb0f..d8dcb8d 100644
@@ -860,7 +860,8 @@ int parse_events_terms(struct list_head *terms, const char *str)
 		return 0;
 	}
 
-	parse_events__free_terms(data.terms);
+	if (data.terms)
+		parse_events__free_terms(data.terms);
 	return ret;
 }
 
Signature · 1 line
1.7.11.7
author reviewer
#5 lore raw

PATCH v3 5/15 perf tools: fix new_term() missing free on error path

In reply to Adrian Hunter

On the error path, newly allocated 'term' must be freed.

---
 tools/perf/util/parse-events.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/util/parse-events.c +1 -0
index d8dcb8d..995fc25 100644
@@ -1184,6 +1184,7 @@ static int new_term(struct parse_events_term **_term, int type_val,
 		term->val.str = str;
 		break;
 	default:
+		free(term);
 		return -EINVAL;
 	}
 
Signature · 1 line
1.7.11.7
author reviewer
#6 lore raw

PATCH v3 6/15 perf tools: fix parse_events_terms() freeing local variable on error path

In reply to Adrian Hunter

The list_head is on the stack, so just free the rest of the list.

---
 tools/perf/util/parse-events.c | 7 ++++++-
 tools/perf/util/parse-events.h | 1 +
 tools/perf/util/pmu.c          | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)
tools/perf/util/parse-events.c +6 -1
index 995fc25..d9cb055 100644
@@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct parse_events_term **new,
 			term->val.str, term->val.num);
 }
 
-void parse_events__free_terms(struct list_head *terms)
+void parse_events__free_terms_only(struct list_head *terms)
 {
 	struct parse_events_term *term, *h;
 
 	list_for_each_entry_safe(term, h, terms, list)
 		free(term);
+}
+
+void parse_events__free_terms(struct list_head *terms)
+{
+	parse_events__free_terms_only(terms);
 
 	free(terms);
 }
tools/perf/util/parse-events.h +1 -0
index 8a48593..6cb6f32 100644
@@ -80,6 +80,7 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx);
 int parse_events_term__clone(struct parse_events_term **new,
 			     struct parse_events_term *term);
+void parse_events__free_terms_only(struct list_head *terms);
 void parse_events__free_terms(struct list_head *terms);
 int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
tools/perf/util/pmu.c +1 -1
index 4c6f9c4..5dd9e37 100644
@@ -194,7 +194,7 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
 	list_for_each_entry(term, &alias->terms, list) {
 		ret = parse_events_term__clone(&clone, term);
 		if (ret) {
-			parse_events__free_terms(&list);
+			parse_events__free_terms_only(&list);
 			return ret;
 		}
 		list_add_tail(&clone->list, &list);
Signature · 1 line
1.7.11.7
#8 lore raw

Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeing local variable on error path

In reply to Adrian Hunter
Quoted 22 lines · Adrian Hunter

The list_head is on the stack, so just free the rest of the list.

---
  tools/perf/util/parse-events.c | 7 ++++++-
  tools/perf/util/parse-events.h | 1 +
  tools/perf/util/pmu.c          | 2 +-
  3 files changed, 8 insertions(+), 2 deletions(-)
tools/perf/util/parse-events.c +1 -1
index 995fc25..d9cb055 100644
@@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct parse_events_term **new,
  			term->val.str, term->val.num);
  }
 
-void parse_events__free_terms(struct list_head *terms)
+void parse_events__free_terms_only(struct list_head *terms)
  {
  	struct parse_events_term *term, *h;
 

tools/perf/util/parse-events.c int parse_events_term__clone(struct parse_events_term **new, · line 1247

Quoted 10 lines
tools/perf/util/parse-events.c +5 -0
  	list_for_each_entry_safe(term, h, terms, list)
  		free(term);
+}
+
+void parse_events__free_terms(struct list_head *terms)
+{
+	parse_events__free_terms_only(terms);
 
  	free(terms);
  }

I still don't understand the reasoning for an _only function. There is only 1 place that mallocs the list_head and that 1 user should free its own memory. All of the other users pass a stack variable.

David

author reviewer
#9 lore raw

Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeing local variable on error path

Quoted 32 lines · David Ahern
Quoted 31 lines · Adrian Hunter

The list_head is on the stack, so just free the rest of the list.

---
  tools/perf/util/parse-events.c | 7 ++++++-
  tools/perf/util/parse-events.h | 1 +
  tools/perf/util/pmu.c          | 2 +-
  3 files changed, 8 insertions(+), 2 deletions(-)
tools/perf/util/parse-events.c +6 -1
index 995fc25..d9cb055 100644
@@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct
 parse_events_term **new,
              term->val.str, term->val.num);
  }
 
-void parse_events__free_terms(struct list_head *terms)
+void parse_events__free_terms_only(struct list_head *terms)
  {
      struct parse_events_term *term, *h;
 
      list_for_each_entry_safe(term, h, terms, list)
          free(term);
+}
+
+void parse_events__free_terms(struct list_head *terms)
+{
+    parse_events__free_terms_only(terms);
 
Quoted 6 lines
Quoted 2 lines
tools/perf/util/parse-events.c +0 -0
      free(terms);
  }

I still don't understand the reasoning for an _only function. There is only 1 place that mallocs the list_head and that 1 user should free its own memory. All of the other users pass a stack variable.

No. See parse-events.y

The list head is defined as a pointer in the YYTYPE stack element:

%union

{
	char *str;
	u64 num;
	struct list_head *head;
	struct parse_events_term *term;
}

It is malloc'ed when terms are created:

event_term

{
	struct list_head *head = malloc(sizeof(*head));
	struct parse_events_term *term = $1;

	ABORT_ON(!head);
	INIT_LIST_HEAD(head);
	list_add_tail(&term->list, head);
	$$ = head;
}
#10 lore raw

Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeing local variable on error path

Quoted 33 lines · Adrian Hunter
Quoted 32 lines · David Ahern
Quoted 31 lines · Adrian Hunter

The list_head is on the stack, so just free the rest of the list.

---
   tools/perf/util/parse-events.c | 7 ++++++-
   tools/perf/util/parse-events.h | 1 +
   tools/perf/util/pmu.c          | 2 +-
   3 files changed, 8 insertions(+), 2 deletions(-)
tools/perf/util/parse-events.c +6 -1
index 995fc25..d9cb055 100644
@@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct
 parse_events_term **new,
               term->val.str, term->val.num);
   }
 
-void parse_events__free_terms(struct list_head *terms)
+void parse_events__free_terms_only(struct list_head *terms)
   {
       struct parse_events_term *term, *h;
 
       list_for_each_entry_safe(term, h, terms, list)
           free(term);
+}
+
+void parse_events__free_terms(struct list_head *terms)
+{
+    parse_events__free_terms_only(terms);
 
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
tools/perf/util/parse-events.c +0 -0
       free(terms);
   }

I still don't understand the reasoning for an _only function. There is only 1 place that mallocs the list_head and that 1 user should free its own memory. All of the other users pass a stack variable.

No. See parse-events.y

Fine. Fix both then. My point is that parse-events.c code should not be freeing memory it does not allocate.

David

Quoted 24 lines

The list head is defined as a pointer in the YYTYPE stack element:

%union

{
	char *str;
	u64 num;
	struct list_head *head;
	struct parse_events_term *term;
}

It is malloc'ed when terms are created:

event_term

{
	struct list_head *head = malloc(sizeof(*head));
	struct parse_events_term *term = $1;

	ABORT_ON(!head);
	INIT_LIST_HEAD(head);
	list_add_tail(&term->list, head);
	$$ = head;
}
author reviewer
#11 lore raw

Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeing local variable on error path

Quoted 42 lines · David Ahern
Quoted 41 lines · Adrian Hunter
Quoted 39 lines · David Ahern
Quoted 33 lines · Adrian Hunter

The list_head is on the stack, so just free the rest of the list.

---
   tools/perf/util/parse-events.c | 7 ++++++-
   tools/perf/util/parse-events.h | 1 +
   tools/perf/util/pmu.c          | 2 +-
   3 files changed, 8 insertions(+), 2 deletions(-)
tools/perf/util/parse-events.c +6 -1
index 995fc25..d9cb055 100644
@@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct
 parse_events_term **new,
               term->val.str, term->val.num);
   }
 
-void parse_events__free_terms(struct list_head *terms)
+void parse_events__free_terms_only(struct list_head *terms)
   {
       struct parse_events_term *term, *h;
 
       list_for_each_entry_safe(term, h, terms, list)
           free(term);
+}
+
+void parse_events__free_terms(struct list_head *terms)
+{
+    parse_events__free_terms_only(terms);
 
       free(terms);
Quoted 1 line
   }

I still don't understand the reasoning for an _only function. There is only 1 place that mallocs the list_head and that 1 user should free its own memory. All of the other users pass a stack variable.

Quoted 4 lines
Quoted 1 line

No. See parse-events.y

Fine. Fix both then. My point is that parse-events.c code should not be freeing memory it does not allocate.

No. Read the code. The 'head' member is shared with other lists. It does not make sense to turn a tiny bug-fix into such a lot of re-work.

Quoted 27 lines
Quoted 24 lines

The list head is defined as a pointer in the YYTYPE stack element:

%union

{
    char *str;
    u64 num;
    struct list_head *head;
    struct parse_events_term *term;
}

It is malloc'ed when terms are created:

event_term

{
    struct list_head *head = malloc(sizeof(*head));
    struct parse_events_term *term = $1;

    ABORT_ON(!head);
    INIT_LIST_HEAD(head);
    list_add_tail(&term->list, head);
    $$ = head;
}
author reviewer
#12 lore raw

PATCH v3 7/15 perf tools: add const specifier to perf_pmu__find name parameter

In reply to Adrian Hunter

The name parameter is constant, declare it so.

---
 tools/perf/util/pmu.c | 14 +++++++-------
 tools/perf/util/pmu.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)
tools/perf/util/pmu.c +7 -7
index 5dd9e37..7360f9b 100644
@@ -73,7 +73,7 @@ int perf_pmu__format_parse(char *dir, struct list_head *head)
  * located at:
  * /sys/bus/event_source/devices/<dev>/format as sysfs group attributes.
  */
-static int pmu_format(char *name, struct list_head *format)
+static int pmu_format(const char *name, struct list_head *format)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -162,7 +162,7 @@ static int pmu_aliases_parse(char *dir, struct list_head *head)
  * Reading the pmu event aliases definition, which should be located at:
  * /sys/bus/event_source/devices/<dev>/events as sysfs group attributes.
  */
-static int pmu_aliases(char *name, struct list_head *head)
+static int pmu_aliases(const char *name, struct list_head *head)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -208,7 +208,7 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
  * located at:
  * /sys/bus/event_source/devices/<dev>/type as sysfs attribute.
  */
-static int pmu_type(char *name, __u32 *type)
+static int pmu_type(const char *name, __u32 *type)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -266,7 +266,7 @@ static void pmu_read_sysfs(void)
 	closedir(dir);
 }
 
-static struct cpu_map *pmu_cpumask(char *name)
+static struct cpu_map *pmu_cpumask(const char *name)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -293,7 +293,7 @@ static struct cpu_map *pmu_cpumask(char *name)
 	return cpus;
 }
 
-static struct perf_pmu *pmu_lookup(char *name)
+static struct perf_pmu *pmu_lookup(const char *name)
 {
 	struct perf_pmu *pmu;
 	LIST_HEAD(format);
@@ -330,7 +330,7 @@ static struct perf_pmu *pmu_lookup(char *name)
 	return pmu;
 }
 
-static struct perf_pmu *pmu_find(char *name)
+static struct perf_pmu *pmu_find(const char *name)
 {
 	struct perf_pmu *pmu;
 
@@ -356,7 +356,7 @@ struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu)
 	return NULL;
 }
 
-struct perf_pmu *perf_pmu__find(char *name)
+struct perf_pmu *perf_pmu__find(const char *name)
 {
 	struct perf_pmu *pmu;
 
tools/perf/util/pmu.h +1 -1
index 32fe55b..d17b565 100644
@@ -21,7 +21,7 @@ struct perf_pmu {
 	struct list_head list;
 };
 
-struct perf_pmu *perf_pmu__find(char *name);
+struct perf_pmu *perf_pmu__find(const char *name);
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
 		     struct list_head *head_terms);
 int perf_pmu__config_terms(struct list_head *formats,
Signature · 1 line
1.7.11.7
author reviewer
#13 lore raw

PATCH v3 8/15 perf tools: tidy duplicated munmap code

In reply to Adrian Hunter

The same lines of code are used in three places.
Make it a new function '__perf_evlist__munmap()'.

---
 tools/perf/util/evlist.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)
tools/perf/util/evlist.c +14 -18
index 99b43dd..3c8b569 100644
@@ -403,16 +403,20 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
 	return event;
 }
 
+static void __perf_evlist__munmap(struct perf_evlist *evlist, int idx)
+{
+	if (evlist->mmap[idx].base != NULL) {
+		munmap(evlist->mmap[idx].base, evlist->mmap_len);
+		evlist->mmap[idx].base = NULL;
+	}
+}
+
 void perf_evlist__munmap(struct perf_evlist *evlist)
 {
 	int i;
 
-	for (i = 0; i < evlist->nr_mmaps; i++) {
-		if (evlist->mmap[i].base != NULL) {
-			munmap(evlist->mmap[i].base, evlist->mmap_len);
-			evlist->mmap[i].base = NULL;
-		}
-	}
+	for (i = 0; i < evlist->nr_mmaps; i++)
+		__perf_evlist__munmap(evlist, i);
 
 	free(evlist->mmap);
 	evlist->mmap = NULL;
@@ -477,12 +481,8 @@ static int perf_evlist__mmap_per_cpu(struct perf_evlist *evlist, int prot, int m
 	return 0;
 
 out_unmap:
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		if (evlist->mmap[cpu].base != NULL) {
-			munmap(evlist->mmap[cpu].base, evlist->mmap_len);
-			evlist->mmap[cpu].base = NULL;
-		}
-	}
+	for (cpu = 0; cpu < nr_cpus; cpu++)
+		__perf_evlist__munmap(evlist, cpu);
 	return -1;
 }
 
@@ -517,12 +517,8 @@ static int perf_evlist__mmap_per_thread(struct perf_evlist *evlist, int prot, in
 	return 0;
 
 out_unmap:
-	for (thread = 0; thread < nr_threads; thread++) {
-		if (evlist->mmap[thread].base != NULL) {
-			munmap(evlist->mmap[thread].base, evlist->mmap_len);
-			evlist->mmap[thread].base = NULL;
-		}
-	}
+	for (thread = 0; thread < nr_threads; thread++)
+		__perf_evlist__munmap(evlist, thread);
 	return -1;
 }
 
Signature · 1 line
1.7.11.7
author reviewer
#14 lore raw

PATCH v3 9/15 perf tools: validate perf event header size

In reply to Adrian Hunter

'size' includes the header so must be at least 'sizeof(struct perf_event_header)'. Error out immediately if that is not the case. Also don't byte-swap the header until it is actually "fetched" from the mmap region.

---
 tools/perf/util/session.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
tools/perf/util/session.c +10 -4
index 19ea888..7a12886 100644
@@ -1094,8 +1094,10 @@ more:
 		perf_event_header__bswap(&event->header);
 
 	size = event->header.size;
-	if (size == 0)
-		size = 8;
+	if (size < sizeof(struct perf_event_header)) {
+		pr_err("bad event header size\n");
+		goto out_err;
+	}
 
 	if (size > cur_size) {
 		void *new = realloc(buf, size);
@@ -1164,8 +1166,12 @@ fetch_mmaped_event(struct perf_session *session,
 	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	if (head + event->header.size > mmap_size)
+	if (head + event->header.size > mmap_size) {
+		/* We're not fetching the event so swap back again */
+		if (session->header.needs_swap)
+			perf_event_header__bswap(&event->header);
 		return NULL;
+	}
 
 	return event;
 }
@@ -1245,7 +1251,7 @@ more:
 
 	size = event->header.size;
 
-	if (size == 0 ||
+	if (size < sizeof(struct perf_event_header) ||
 	    perf_session__process_event(session, event, tool, file_pos) < 0) {
 		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
 		       file_offset + head, event->header.size,
Signature · 1 line
1.7.11.7
author reviewer
#15 lore raw

PATCH v3 10/15 perf tools: add debug prints

In reply to Adrian Hunter

It is useful to see the arguments to perf_event_open and whether the perf events ring buffer was mmapped per-cpu or per-thread. That information will now be displayed when verbose is 2 i.e option -vv

---
 tools/perf/util/evlist.c |  3 +++
 tools/perf/util/evsel.c  | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)
tools/perf/util/evlist.c +3 -0
index 3c8b569..a660f56 100644
@@ -14,6 +14,7 @@
 #include "target.h"
 #include "evlist.h"
 #include "evsel.h"
+#include "debug.h"
 #include <unistd.h>
 
 #include "parse-events.h"
@@ -454,6 +455,7 @@ static int perf_evlist__mmap_per_cpu(struct perf_evlist *evlist, int prot, int m
 	int nr_cpus = cpu_map__nr(evlist->cpus);
 	int nr_threads = thread_map__nr(evlist->threads);
 
+	pr_debug2("perf event ring buffer mmapped per cpu\n");
 	for (cpu = 0; cpu < nr_cpus; cpu++) {
 		int output = -1;
 
@@ -492,6 +494,7 @@ static int perf_evlist__mmap_per_thread(struct perf_evlist *evlist, int prot, in
 	int thread;
 	int nr_threads = thread_map__nr(evlist->threads);
 
+	pr_debug2("perf event ring buffer mmapped per thread\n");
 	for (thread = 0; thread < nr_threads; thread++) {
 		int output = -1;
 
tools/perf/util/evsel.c +52 -0

76 lines not on this page · raw message

Signature · 1 line
1.7.11.7
author reviewer
#16 lore raw

PATCH v3 11/15 perf tools: fix symbol_conf.nr_events

In reply to Adrian Hunter

symbol_conf.nr_events must be updated when processing attribute events.

---
 tools/perf/util/header.c | 2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/header.c +2 -0
index 4037b0a..81a3a0d 100644
@@ -2968,6 +2968,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
 		perf_evlist__id_add(evlist, evsel, 0, i, event->attr.id[i]);
 	}
 
+	symbol_conf.nr_events = evlist->nr_entries;
+
 	return 0;
 }
 
Signature · 1 line
1.7.11.7
author reviewer
#17 lore raw

PATCH v3 12/15 perf tools: allow non-matching sample types

In reply to Adrian Hunter

Sample types need not be identical to determine the sample id from the event. Only the position of the sample id needs to be the same.

Compatible sample types are ones in which the bits defined by PERF_COMPAT_MASK are the same. 'perf_evlist__config()' forces sample types to be compatible on that basis.

---
 tools/perf/builtin-report.c |   2 +-
 tools/perf/util/event.h     |  14 +++++
 tools/perf/util/evlist.c    | 135 ++++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/evlist.h    |   8 ++-
 tools/perf/util/evsel.c     |  64 ++++++++++++++++++++-
 tools/perf/util/evsel.h     |  10 ++++
 tools/perf/util/session.c   |   8 ++-
 7 files changed, 230 insertions(+), 11 deletions(-)
tools/perf/builtin-report.c +1 -1
index ca98d34..58572ac 100644
@@ -367,7 +367,7 @@ static int process_read_event(struct perf_tool *tool,
 static int perf_report__setup_sample_type(struct perf_report *rep)
 {
 	struct perf_session *self = rep->session;
-	u64 sample_type = perf_evlist__sample_type(self->evlist);
+	u64 sample_type = perf_evlist__combined_sample_type(self->evlist);
 
 	if (!self->fd_pipe && !(sample_type & PERF_SAMPLE_CALLCHAIN)) {
 		if (sort__has_parent) {
tools/perf/util/event.h +14 -0
index 1813895..3aef78c 100644
@@ -65,6 +65,20 @@ struct read_event {
 	PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID |	\
 	 PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD)
 
+/*
+ * Events have compatible sample types if the following bits all have the same
+ * value.  This is because the order of sample members is fixed.  For sample
+ * events the order is: PERF_SAMPLE_IP, PERF_SAMPLE_TID, PERF_SAMPLE_TIME,
+ * PERF_SAMPLE_ADDR, PERF_SAMPLE_ID.  For non-sample events the sample members
+ * are accessed in reverse order.  The order is: PERF_SAMPLE_ID,
+ * PERF_SAMPLE_STREAM_ID, PERF_SAMPLE_CPU.
+ */
+#define PERF_COMPAT_MASK				\
+	(PERF_SAMPLE_IP   | PERF_SAMPLE_TID       |	\
+	 PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR      |	\
+	 PERF_SAMPLE_ID   | PERF_SAMPLE_STREAM_ID |	\
+	 PERF_SAMPLE_CPU)
+
 struct sample_event {
 	struct perf_event_header        header;
 	u64 array[];
tools/perf/util/evlist.c +130 -5

186 lines not on this page · raw message

tools/perf/util/evlist.h +7 -1
index 0583d36..b1be475 100644
@@ -32,11 +32,15 @@ struct perf_evlist {
 	int		 nr_fds;
 	int		 nr_mmaps;
 	int		 mmap_len;
+	int		 id_pos;
+	int		 is_pos;
+	u64		 combined_sample_type;
 	struct {
 		int	cork_fd;
 		pid_t	pid;
 	} workload;
 	bool		 overwrite;
+	bool		 matching_sample_types;
 	struct perf_mmap *mmap;
 	struct pollfd	 *pollfd;
 	struct thread_map *threads;
@@ -83,6 +87,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *self, int idx);
 int perf_evlist__open(struct perf_evlist *evlist);
 void perf_evlist__close(struct perf_evlist *evlist);
 
+void perf_evlist__make_sample_types_compatible(struct perf_evlist *evlist);
 void perf_evlist__config(struct perf_evlist *evlist,
 			 struct perf_record_opts *opts);
 
@@ -118,7 +123,8 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist);
 void __perf_evlist__set_leader(struct list_head *list);
 void perf_evlist__set_leader(struct perf_evlist *evlist);
 
-u64 perf_evlist__sample_type(struct perf_evlist *evlist);
+u64 __perf_evlist__combined_sample_type(struct perf_evlist *evlist);
+u64 perf_evlist__combined_sample_type(struct perf_evlist *evlist);
 bool perf_evlist__sample_id_all(struct perf_evlist *evlist);
 u16 perf_evlist__id_hdr_size(struct perf_evlist *evlist);
 
tools/perf/util/evsel.c +63 -1

94 lines not on this page · raw message

tools/perf/util/evsel.h +10 -0
index 3f156cc..c6d616c 100644
@@ -45,6 +45,11 @@ struct perf_sample_id {
  * @name - Can be set to retain the original event name passed by the user,
  *         so that when showing results in tools such as 'perf stat', we
  *         show the name used, not some alias.
+ * @id_pos: the position of the event id (PERF_SAMPLE_ID) in a sample event
+ *          i.e. in the array of struct sample_event
+ * @is_pos: the position (counting backwards) of the event id (PERF_SAMPLE_ID)
+ *          in a non-sample event i.e. if sample_id_all is used there is an id
+ *          sample appended to non-sample events
  */
 struct perf_evsel {
 	struct list_head	node;
@@ -71,6 +76,8 @@ struct perf_evsel {
 	} handler;
 	struct cpu_map		*cpus;
 	unsigned int		sample_size;
+	int			id_pos;
+	int			is_pos;
 	bool 			supported;
 	bool 			needs_swap;
 	/* parse modifier helper */
@@ -100,6 +107,9 @@ void perf_evsel__delete(struct perf_evsel *evsel);
 void perf_evsel__config(struct perf_evsel *evsel,
 			struct perf_record_opts *opts);
 
+int __perf_evsel__sample_size(u64 sample_type);
+void perf_evsel__calc_id_pos(struct perf_evsel *evsel);
+
 bool perf_evsel__is_cache_op_valid(u8 type, u8 op);
 
 #define PERF_EVSEL__MAX_ALIASES 8
tools/perf/util/session.c +5 -3
index 7a12886..e0cc110 100644
@@ -743,7 +743,7 @@ static void perf_session__print_tstamp(struct perf_session *session,
 				       union perf_event *event,
 				       struct perf_sample *sample)
 {
-	u64 sample_type = perf_evlist__sample_type(session->evlist);
+	u64 sample_type = __perf_evlist__combined_sample_type(session->evlist);
 
 	if (event->header.type != PERF_RECORD_SAMPLE &&
 	    !perf_evlist__sample_id_all(session->evlist)) {
@@ -902,7 +902,7 @@ static int perf_session__preprocess_sample(struct perf_session *session,
 					   union perf_event *event, struct perf_sample *sample)
 {
 	if (event->header.type != PERF_RECORD_SAMPLE ||
-	    !(perf_evlist__sample_type(session->evlist) & PERF_SAMPLE_CALLCHAIN))
+	    !sample->callchain)
 		return 0;
 
 	if (!ip_callchain__valid(sample->callchain, event)) {
@@ -1304,7 +1304,9 @@ int perf_session__process_events(struct perf_session *self,
 
 bool perf_session__has_traces(struct perf_session *session, const char *msg)
 {
-	if (!(perf_evlist__sample_type(session->evlist) & PERF_SAMPLE_RAW)) {
+	u64 sample_type = perf_evlist__combined_sample_type(session->evlist);
+
+	if (!(sample_type & PERF_SAMPLE_RAW)) {
 		pr_err("No trace sample to read. Did you call 'perf %s'?\n", msg);
 		return false;
 	}
Signature · 1 line
1.7.11.7
author reviewer
#18 lore raw

PATCH v3 13/15 perf tools: struct thread has a tid not a pid

In reply to Adrian Hunter

As evident from 'machine__process_fork_event()' and 'machine__process_exit_event()' the 'pid' member of struct thread is actually the tid.

Rename 'pid' to 'tid' in struct thread accordingly.

---
 tools/perf/builtin-kmem.c      |  2 +-
 tools/perf/builtin-sched.c     | 12 ++++++------
 tools/perf/builtin-trace.c     |  4 ++--
 tools/perf/ui/browsers/hists.c |  6 +++---
 tools/perf/util/event.c        |  2 +-
 tools/perf/util/machine.c      | 20 ++++++++++----------
 tools/perf/util/machine.h      |  4 ++--
 tools/perf/util/sort.c         |  6 +++---
 tools/perf/util/thread.c       | 10 +++++-----
 tools/perf/util/thread.h       |  4 ++--
 10 files changed, 35 insertions(+), 35 deletions(-)
tools/perf/builtin-kmem.c +1 -1
index 46878da..34647fa 100644
@@ -313,7 +313,7 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 		return -1;
 	}
 
-	dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);
+	dump_printf(" ... thread: %s:%d\n", thread->comm, thread->tid);
 
 	if (evsel->handler.func != NULL) {
 		tracepoint_handler f = evsel->handler.func;
tools/perf/builtin-sched.c +6 -6
index 2da2a6c..b1715f4 100644
@@ -1075,7 +1075,7 @@ static int latency_migrate_task_event(struct perf_sched *sched,
 	if (!atoms) {
 		if (thread_atoms_insert(sched, migrant))
 			return -1;
-		register_pid(sched, migrant->pid, migrant->comm);
+		register_pid(sched, migrant->tid, migrant->comm);
 		atoms = thread_atoms_search(&sched->atom_root, migrant, &sched->cmp_pid);
 		if (!atoms) {
 			pr_err("migration-event: Internal tree error");
@@ -1115,7 +1115,7 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
 	sched->all_runtime += work_list->total_runtime;
 	sched->all_count   += work_list->nb_atoms;
 
-	ret = printf("  %s:%d ", work_list->thread->comm, work_list->thread->pid);
+	ret = printf("  %s:%d ", work_list->thread->comm, work_list->thread->tid);
 
 	for (i = 0; i < 24 - ret; i++)
 		printf(" ");
@@ -1131,9 +1131,9 @@ static void output_lat_thread(struct perf_sched *sched, struct work_atoms *work_
 
 static int pid_cmp(struct work_atoms *l, struct work_atoms *r)
 {
-	if (l->thread->pid < r->thread->pid)
+	if (l->thread->tid < r->thread->tid)
 		return -1;
-	if (l->thread->pid > r->thread->pid)
+	if (l->thread->tid > r->thread->tid)
 		return 1;
 
 	return 0;
@@ -1321,7 +1321,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
 			printf("*");
 
 		if (sched->curr_thread[cpu]) {
-			if (sched->curr_thread[cpu]->pid)
+			if (sched->curr_thread[cpu]->tid)
 				printf("%2s ", sched->curr_thread[cpu]->shortname);
 			else
 				printf(".  ");
@@ -1332,7 +1332,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
 	printf("  %12.6f secs ", (double)timestamp/1e9);
 	if (new_shortname) {
 		printf("%s => %s:%d\n",
-			sched_in->shortname, sched_in->comm, sched_in->pid);
+			sched_in->shortname, sched_in->comm, sched_in->tid);
 	} else {
 		printf("\n");
 	}
tools/perf/builtin-trace.c +2 -2
index ab3ed4a..6a36a95 100644
@@ -142,7 +142,7 @@ static size_t trace__fprintf_entry_head(struct trace *trace, struct thread *thre
 	printed += fprintf_duration(duration, fp);
 
 	if (trace->multiple_threads)
-		printed += fprintf(fp, "%d ", thread->pid);
+		printed += fprintf(fp, "%d ", thread->tid);
 
 	return printed;
 }
@@ -593,7 +593,7 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp)
 			color = PERF_COLOR_YELLOW;
 
 		printed += color_fprintf(fp, color, "%20s", thread->comm);
-		printed += fprintf(fp, " - %-5d :%11lu   [", thread->pid, ttrace->nr_events);
+		printed += fprintf(fp, " - %-5d :%11lu   [", thread->tid, ttrace->nr_events);
 		printed += color_fprintf(fp, color, "%5.1f%%", ratio);
 		printed += fprintf(fp, " ] %10.3f ms\n", ttrace->runtime_ms);
 	}
tools/perf/ui/browsers/hists.c +3 -3
index fc0bd38..06e892f 100644
@@ -1256,7 +1256,7 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size,
 		printed += scnprintf(bf + printed, size - printed,
 				    ", Thread: %s(%d)",
 				    (thread->comm_set ? thread->comm : ""),
-				    thread->pid);
+				    thread->tid);
 	if (dso)
 		printed += scnprintf(bf + printed, size - printed,
 				    ", DSO: %s", dso->short_name);
@@ -1579,7 +1579,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		    asprintf(&options[nr_options], "Zoom %s %s(%d) thread",
 			     (browser->hists->thread_filter ? "out of" : "into"),
 			     (thread->comm_set ? thread->comm : ""),
-			     thread->pid) > 0)
+			     thread->tid) > 0)
 			zoom_thread = nr_options++;
 
 		if (dso != NULL &&
@@ -1702,7 +1702,7 @@ zoom_out_thread:
 			} else {
 				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
 						   thread->comm_set ? thread->comm : "",
-						   thread->pid);
+						   thread->tid);
 				browser->hists->thread_filter = thread;
 				sort_thread.elide = true;
 				pstack__push(fstack, &browser->hists->thread_filter);
tools/perf/util/event.c +1 -1
index 5cd13d7..9541270 100644
@@ -686,7 +686,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
 	    !strlist__has_entry(symbol_conf.comm_list, thread->comm))
 		goto out_filtered;
 
-	dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);
+	dump_printf(" ... thread: %s:%d\n", thread->comm, thread->tid);
 	/*
 	 * Have we already created the kernel maps for this machine?
 	 *
tools/perf/util/machine.c +10 -10
index b2ecad6..ef0be97 100644
@@ -233,7 +233,7 @@ void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size)
 	return;
 }
 
-static struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid,
+static struct thread *__machine__findnew_thread(struct machine *machine, pid_t tid,
 						bool create)
 {
 	struct rb_node **p = &machine->threads.rb_node;
@@ -241,23 +241,23 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t p
 	struct thread *th;
 
 	/*
-	 * Font-end cache - PID lookups come in blocks,
+	 * Front-end cache - TID lookups come in blocks,
 	 * so most of the time we dont have to look up
 	 * the full rbtree:
 	 */
-	if (machine->last_match && machine->last_match->pid == pid)
+	if (machine->last_match && machine->last_match->tid == tid)
 		return machine->last_match;
 
 	while (*p != NULL) {
 		parent = *p;
 		th = rb_entry(parent, struct thread, rb_node);
 
-		if (th->pid == pid) {
+		if (th->tid == tid) {
 			machine->last_match = th;
 			return th;
 		}
 
-		if (pid < th->pid)
+		if (tid < th->tid)
 			p = &(*p)->rb_left;
 		else
 			p = &(*p)->rb_right;
@@ -266,7 +266,7 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t p
 	if (!create)
 		return NULL;
 
-	th = thread__new(pid);
+	th = thread__new(tid);
 	if (th != NULL) {
 		rb_link_node(&th->rb_node, parent, p);
 		rb_insert_color(&th->rb_node, &machine->threads);
@@ -276,14 +276,14 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t p
 	return th;
 }
 
-struct thread *machine__findnew_thread(struct machine *machine, pid_t pid)
+struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
 {
-	return __machine__findnew_thread(machine, pid, true);
+	return __machine__findnew_thread(machine, tid, true);
 }
 
-struct thread *machine__find_thread(struct machine *machine, pid_t pid)
+struct thread *machine__find_thread(struct machine *machine, pid_t tid)
 {
-	return __machine__findnew_thread(machine, pid, false);
+	return __machine__findnew_thread(machine, tid, false);
 }
 
 int machine__process_comm_event(struct machine *machine, union perf_event *event)
tools/perf/util/machine.h +2 -2
index 7794068..e49ba01 100644
@@ -36,7 +36,7 @@ struct map *machine__kernel_map(struct machine *machine, enum map_type type)
 	return machine->vmlinux_maps[type];
 }
 
-struct thread *machine__find_thread(struct machine *machine, pid_t pid);
+struct thread *machine__find_thread(struct machine *machine, pid_t tid);
 
 int machine__process_comm_event(struct machine *machine, union perf_event *event);
 int machine__process_exit_event(struct machine *machine, union perf_event *event);
@@ -99,7 +99,7 @@ static inline bool machine__is_host(struct machine *machine)
 	return machine ? machine->pid == HOST_KERNEL_ID : false;
 }
 
-struct thread *machine__findnew_thread(struct machine *machine, pid_t pid);
+struct thread *machine__findnew_thread(struct machine *machine, pid_t tid);
 
 size_t machine__fprintf(struct machine *machine, FILE *fp);
 
tools/perf/util/sort.c +3 -3
index 313a5a7..8deee19 100644
@@ -55,14 +55,14 @@ static int64_t cmp_null(void *l, void *r)
 static int64_t
 sort__thread_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	return right->thread->pid - left->thread->pid;
+	return right->thread->tid - left->thread->tid;
 }
 
 static int hist_entry__thread_snprintf(struct hist_entry *self, char *bf,
 				       size_t size, unsigned int width)
 {
 	return repsep_snprintf(bf, size, "%*s:%5d", width - 6,
-			      self->thread->comm ?: "", self->thread->pid);
+			      self->thread->comm ?: "", self->thread->tid);
 }
 
 struct sort_entry sort_thread = {
@@ -77,7 +77,7 @@ struct sort_entry sort_thread = {
 static int64_t
 sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	return right->thread->pid - left->thread->pid;
+	return right->thread->tid - left->thread->tid;
 }
 
 static int64_t
tools/perf/util/thread.c +5 -5
index 40399cb..6feeb88 100644
@@ -7,17 +7,17 @@
 #include "util.h"
 #include "debug.h"
 
-struct thread *thread__new(pid_t pid)
+struct thread *thread__new(pid_t tid)
 {
 	struct thread *self = zalloc(sizeof(*self));
 
 	if (self != NULL) {
 		map_groups__init(&self->mg);
-		self->pid = pid;
+		self->tid = tid;
 		self->ppid = -1;
 		self->comm = malloc(32);
 		if (self->comm)
-			snprintf(self->comm, 32, ":%d", self->pid);
+			snprintf(self->comm, 32, ":%d", self->tid);
 	}
 
 	return self;
@@ -57,7 +57,7 @@ int thread__comm_len(struct thread *self)
 
 size_t thread__fprintf(struct thread *thread, FILE *fp)
 {
-	return fprintf(fp, "Thread %d %s\n", thread->pid, thread->comm) +
+	return fprintf(fp, "Thread %d %s\n", thread->tid, thread->comm) +
 	       map_groups__fprintf(&thread->mg, verbose, fp);
 }
 
@@ -84,7 +84,7 @@ int thread__fork(struct thread *self, struct thread *parent)
 		if (map_groups__clone(&self->mg, &parent->mg, i) < 0)
 			return -ENOMEM;
 
-	self->ppid = parent->pid;
+	self->ppid = parent->tid;
 
 	return 0;
 }
tools/perf/util/thread.h +2 -2
index eeb7ac6..37a86a3 100644
@@ -12,7 +12,7 @@ struct thread {
 		struct list_head node;
 	};
 	struct map_groups	mg;
-	pid_t			pid;
+	pid_t			tid;
 	pid_t			ppid;
 	char			shortname[3];
 	bool			comm_set;
@@ -24,7 +24,7 @@ struct thread {
 
 struct machine;
 
-struct thread *thread__new(pid_t pid);
+struct thread *thread__new(pid_t tid);
 void thread__delete(struct thread *self);
 
 int thread__set_comm(struct thread *self, const char *comm);
Signature · 1 line
1.7.11.7
Acked-by on v3 13/15
#19 lore raw

Re: [PATCH V3 13/15] perf tools: struct thread has a tid not a pid

Quoted 7 lines · Adrian Hunter

As evident from 'machine__process_fork_event()' and 'machine__process_exit_event()' the 'pid' member of struct thread is actually the tid.

Rename 'pid' to 'tid' in struct thread accordingly.

author reviewer
#20 lore raw

PATCH v3 14/15 perf tools: add pid to struct thread

In reply to Adrian Hunter

Record pid on struct thread. The member is named 'pid_' to avoid confusion with the 'tid' member which was previously named 'pid'.

---
 tools/perf/util/machine.c | 26 +++++++++++++++++++++-----
 tools/perf/util/machine.h |  2 ++
 tools/perf/util/thread.c  |  3 ++-
 tools/perf/util/thread.h  |  3 ++-
 4 files changed, 27 insertions(+), 7 deletions(-)
tools/perf/util/machine.c +21 -5
index ef0be97..722281a 100644
@@ -233,7 +233,8 @@ void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size)
 	return;
 }
 
-static struct thread *__machine__findnew_thread(struct machine *machine, pid_t tid,
+static struct thread *__machine__findnew_thread(struct machine *machine,
+						pid_t pid, pid_t tid,
 						bool create)
 {
 	struct rb_node **p = &machine->threads.rb_node;
@@ -245,8 +246,11 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 	 * so most of the time we dont have to look up
 	 * the full rbtree:
 	 */
-	if (machine->last_match && machine->last_match->tid == tid)
+	if (machine->last_match && machine->last_match->tid == tid) {
+		if (pid && !machine->last_match->pid_)
+			machine->last_match->pid_ = pid;
 		return machine->last_match;
+	}
 
 	while (*p != NULL) {
 		parent = *p;
@@ -254,6 +258,8 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 
 		if (th->tid == tid) {
 			machine->last_match = th;
+			if (pid && !th->pid_)
+				th->pid_ = pid;
 			return th;
 		}
 
@@ -266,7 +272,7 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 	if (!create)
 		return NULL;
 
-	th = thread__new(tid);
+	th = thread__new(pid, tid);
 	if (th != NULL) {
 		rb_link_node(&th->rb_node, parent, p);
 		rb_insert_color(&th->rb_node, &machine->threads);
@@ -278,12 +284,12 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 
 struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
 {
-	return __machine__findnew_thread(machine, tid, true);
+	return __machine__findnew_thread(machine, 0, tid, true);
 }
 
 struct thread *machine__find_thread(struct machine *machine, pid_t tid)
 {
-	return __machine__findnew_thread(machine, tid, false);
+	return __machine__findnew_thread(machine, 0, tid, false);
 }
 
 int machine__process_comm_event(struct machine *machine, union perf_event *event)
@@ -1264,3 +1270,13 @@ int machine__resolve_callchain(struct machine *machine,
 				   sample);
 
 }
+
+pid_t machine__get_thread_pid(struct machine *machine, pid_t tid)
+{
+	struct thread *thread = machine__find_thread(machine, tid);
+
+	if (!thread)
+		return 0;
+
+	return thread->pid_;
+}
tools/perf/util/machine.h +2 -0
index e49ba01..221a0a9 100644
@@ -156,4 +156,6 @@ void machines__destroy_kernel_maps(struct machines *machines);
 
 size_t machine__fprintf_vmlinux_path(struct machine *machine, FILE *fp);
 
+pid_t machine__get_thread_pid(struct machine *machine, pid_t tid);
+
 #endif /* __PERF_MACHINE_H */
tools/perf/util/thread.c +2 -1
index 6feeb88..e3d4a55 100644
@@ -7,12 +7,13 @@
 #include "util.h"
 #include "debug.h"
 
-struct thread *thread__new(pid_t tid)
+struct thread *thread__new(pid_t pid, pid_t tid)
 {
 	struct thread *self = zalloc(sizeof(*self));
 
 	if (self != NULL) {
 		map_groups__init(&self->mg);
+		self->pid_ = pid;
 		self->tid = tid;
 		self->ppid = -1;
 		self->comm = malloc(32);
tools/perf/util/thread.h +2 -1
index 37a86a3..bfa68e7 100644
@@ -12,6 +12,7 @@ struct thread {
 		struct list_head node;
 	};
 	struct map_groups	mg;
+	pid_t			pid_; /* Not all tools update this */
 	pid_t			tid;
 	pid_t			ppid;
 	char			shortname[3];
@@ -24,7 +25,7 @@ struct thread {
 
 struct machine;
 
-struct thread *thread__new(pid_t tid);
+struct thread *thread__new(pid_t pid, pid_t tid);
 void thread__delete(struct thread *self);
 
 int thread__set_comm(struct thread *self, const char *comm);
Signature · 1 line
1.7.11.7
#21 lore raw

Re: [PATCH V3 14/15] perf tools: add pid to struct thread

Quoted 76 lines · Adrian Hunter

Record pid on struct thread. The member is named 'pid_' to avoid confusion with the 'tid' member which was previously named 'pid'.

---
  tools/perf/util/machine.c | 26 +++++++++++++++++++++-----
  tools/perf/util/machine.h |  2 ++
  tools/perf/util/thread.c  |  3 ++-
  tools/perf/util/thread.h  |  3 ++-
  4 files changed, 27 insertions(+), 7 deletions(-)
tools/perf/util/machine.c +11 -5
index ef0be97..722281a 100644
@@ -233,7 +233,8 @@ void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size)
  	return;
  }
 
-static struct thread *__machine__findnew_thread(struct machine *machine, pid_t tid,
+static struct thread *__machine__findnew_thread(struct machine *machine,
+						pid_t pid, pid_t tid,
  						bool create)
  {
  	struct rb_node **p = &machine->threads.rb_node;
@@ -245,8 +246,11 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
  	 * so most of the time we dont have to look up
  	 * the full rbtree:
  	 */
-	if (machine->last_match && machine->last_match->tid == tid)
+	if (machine->last_match && machine->last_match->tid == tid) {
+		if (pid && !machine->last_match->pid_)
+			machine->last_match->pid_ = pid;
  		return machine->last_match;
+	}
 
  	while (*p != NULL) {
  		parent = *p;
@@ -254,6 +258,8 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 
  		if (th->tid == tid) {
  			machine->last_match = th;
+			if (pid && !th->pid_)
+				th->pid_ = pid;
  			return th;
  		}
 
@@ -266,7 +272,7 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
  	if (!create)
  		return NULL;
 
-	th = thread__new(tid);
+	th = thread__new(pid, tid);
  	if (th != NULL) {
  		rb_link_node(&th->rb_node, parent, p);
  		rb_insert_color(&th->rb_node, &machine->threads);
@@ -278,12 +284,12 @@ static struct thread *__machine__findnew_thread(struct machine *machine, pid_t t
 
  struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
  {
-	return __machine__findnew_thread(machine, tid, true);
+	return __machine__findnew_thread(machine, 0, tid, true);
  }
 
  struct thread *machine__find_thread(struct machine *machine, pid_t tid)
  {
-	return __machine__findnew_thread(machine, tid, false);
+	return __machine__findnew_thread(machine, 0, tid, false);
  }
 
  int machine__process_comm_event(struct machine *machine, union perf_event *event)
@@ -1264,3 +1270,13 @@ int machine__resolve_callchain(struct machine *machine,
  				   sample);
 
  }

tools/perf/util/machine.c int machine__resolve_callchain(struct machine *machine, · line 1282

Quoted 10 lines
tools/perf/util/machine.c +10 -0
+
+pid_t machine__get_thread_pid(struct machine *machine, pid_t tid)
+{
+	struct thread *thread = machine__find_thread(machine, tid);
+
+	if (!thread)
+		return 0;
+
+	return thread->pid_;
+}

The APIs should be machine__find_thread and then given a thread have something like thread__get_pid(thread) but to go from machine returning a thread's pid seems odd.

Also, there are no users of this function, so no need to create it anyways.

David

Quoted 52 lines
tools/perf/util/machine.h +2 -0
index e49ba01..221a0a9 100644
@@ -156,4 +156,6 @@ void machines__destroy_kernel_maps(struct machines *machines);
 
  size_t machine__fprintf_vmlinux_path(struct machine *machine, FILE *fp);
 
+pid_t machine__get_thread_pid(struct machine *machine, pid_t tid);
+
  #endif /* __PERF_MACHINE_H */
tools/perf/util/thread.c +2 -1
index 6feeb88..e3d4a55 100644
@@ -7,12 +7,13 @@
  #include "util.h"
  #include "debug.h"
 
-struct thread *thread__new(pid_t tid)
+struct thread *thread__new(pid_t pid, pid_t tid)
  {
  	struct thread *self = zalloc(sizeof(*self));
 
  	if (self != NULL) {
  		map_groups__init(&self->mg);
+		self->pid_ = pid;
  		self->tid = tid;
  		self->ppid = -1;
  		self->comm = malloc(32);
tools/perf/util/thread.h +2 -1
index 37a86a3..bfa68e7 100644
@@ -12,6 +12,7 @@ struct thread {
  		struct list_head node;
  	};
  	struct map_groups	mg;
+	pid_t			pid_; /* Not all tools update this */
  	pid_t			tid;
  	pid_t			ppid;
  	char			shortname[3];
@@ -24,7 +25,7 @@ struct thread {
 
  struct machine;
 
-struct thread *thread__new(pid_t tid);
+struct thread *thread__new(pid_t pid, pid_t tid);
  void thread__delete(struct thread *self);
 
  int thread__set_comm(struct thread *self, const char *comm);
author reviewer
#22 lore raw

PATCH v3 15/15 perf tools: change "machine" functions to set thread pid

In reply to Adrian Hunter

Typically tracking events such as mmap events, comm events, fork events and exit events, are processed by "machine" functions. Change these functions to put pid in the new pid_ member of struct thread.

---
 tools/perf/util/machine.c | 21 +++++++++++++++++----
 tools/perf/util/machine.h |  2 ++
 2 files changed, 19 insertions(+), 4 deletions(-)
tools/perf/util/machine.c +17 -4
index 722281a..d11ca3b 100644
@@ -287,6 +287,12 @@ struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
 	return __machine__findnew_thread(machine, 0, tid, true);
 }
 
+struct thread *machine__findnew_thread_ex(struct machine *machine, pid_t pid,
+					  pid_t tid)
+{
+	return __machine__findnew_thread(machine, pid, tid, true);
+}
+
 struct thread *machine__find_thread(struct machine *machine, pid_t tid)
 {
 	return __machine__findnew_thread(machine, 0, tid, false);
@@ -294,7 +300,9 @@ struct thread *machine__find_thread(struct machine *machine, pid_t tid)
 
 int machine__process_comm_event(struct machine *machine, union perf_event *event)
 {
-	struct thread *thread = machine__findnew_thread(machine, event->comm.tid);
+	struct thread *thread = machine__findnew_thread_ex(machine,
+							   event->comm.pid,
+							   event->comm.tid);
 
 	if (dump_trace)
 		perf_event__fprintf_comm(event, stdout);
@@ -975,7 +983,8 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
 		return 0;
 	}
 
-	thread = machine__findnew_thread(machine, event->mmap.pid);
+	thread = machine__findnew_thread_ex(machine, event->mmap.pid,
+					    event->mmap.pid);
 	if (thread == NULL)
 		goto out_problem;
 
@@ -1002,8 +1011,12 @@ out_problem:
 
 int machine__process_fork_event(struct machine *machine, union perf_event *event)
 {
-	struct thread *thread = machine__findnew_thread(machine, event->fork.tid);
-	struct thread *parent = machine__findnew_thread(machine, event->fork.ptid);
+	struct thread *thread = machine__findnew_thread_ex(machine,
+							   event->fork.pid,
+							   event->fork.tid);
+	struct thread *parent = machine__findnew_thread_ex(machine,
+							   event->fork.ppid,
+							   event->fork.ptid);
 
 	if (dump_trace)
 		perf_event__fprintf_task(event, stdout);
tools/perf/util/machine.h +2 -0
index 221a0a9..11a53d5 100644
@@ -100,6 +100,8 @@ static inline bool machine__is_host(struct machine *machine)
 }
 
 struct thread *machine__findnew_thread(struct machine *machine, pid_t tid);
+struct thread *machine__findnew_thread_ex(struct machine *machine, pid_t pid,
+					  pid_t tid);
 
 size_t machine__fprintf(struct machine *machine, FILE *fp);
 
Signature · 1 line
1.7.11.7