Skip to contentSkip to conversation

perf tools: some fixes and tweaks

PATCH Superseded Adrian Hunter 4 participants 26 messages Jul 18, 2013 – Jul 29, 2013 perf events

v8 of 13 Superseded Latest is v13 →

lore
Share & tools
lore

Adrian Hunter

author reviewer
#0 lore raw

v8 cover perf tools: some fixes and tweaks

Hi

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

Changes in V8:
perf tools: add debug prints
Fixed Python link errors
perf tools: move perf_evlist__config() to a new source file
New Patch to avoid Python link errors
perf tools: add support for PERF_SAMPLE_IDENTFIER
Adjustments due to patch above

Changes in V7:
perf: Update perf_event_type documentation
Proposed new patch from Peter Zijlstra
perf: make events stream always parsable
Adjustments due to patch above
perf tools: tidy up sample parsing overflow checking
Change to a single overflow function Amend comment
perf tools: add a function to calculate sample event size
New patch
perf tools: add a sample parsing test
Amended to use sample event size calculation

Changes in V6:
Some checkpatch fixes

perf: make events stream always parsable
Add sample format comments

Changes in V5:
Re-based to Arnaldo's tree and dropped already applied patches:
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: 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
Changed to perf_event_attr__fprintf()
perf tools: add pid to struct thread
Always set the pid, even if a pid is already set
perf tools: change machine__findnew_thread() to set thread pid
Replaces: perf tools: change "machine" functions to set thread pid
perf tools: add support for PERF_SAMPLE_IDENTFIER
Only use PERF_SAMPLE_IDENTFIER if sample types are different
perf tools: expand perf_event__synthesize_sample()
New patch in preparation of a sample parsing test
perf tools: add a sample parsing test
New patch

Changes in V4:
I added kernel support for matching sample types via PERF_SAMPLE_IDENTIFIER. perf tools support for that required first fixing some other things.

perf tools: fix parse_events_terms() freeing local variable on error path
Dropped - covered by David Ahern
perf tools: struct thread has a tid not a pid
Added ack by David Ahern
perf tools: add pid to struct thread
Remove unused function
perf tools: fix missing increment in sample parsing
New patch
perf tools: tidy up sample parsing overflow checking
New patch
perf tools: remove unnecessary callchain validation
New patch
perf tools: remove references to struct ip_event
New patch
perf tools: move struct ip_event
New patch
perf: make events stream always parsable
New patch
perf tools: add support for PERF_SAMPLE_IDENTFIER
New patch

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 (14):
perf tools: add debug prints
perf tools: allow non-matching sample types
perf tools: add pid to struct thread
perf tools: change machine__findnew_thread() to set thread pid perf tools: tidy up sample parsing overflow checking perf tools: remove unnecessary callchain validation perf tools: remove references to struct ip_event perf tools: move struct ip_event
perf: make events stream always parsable
perf tools: move perf_evlist__config() to a new source file perf tools: add support for PERF_SAMPLE_IDENTFIER perf tools: expand perf_event__synthesize_sample() perf tools: add a function to calculate sample event size perf tools: add a sample parsing test

Peter Zijlstra (1):
perf: Update perf_event_type documentation

 include/uapi/linux/perf_event.h   |  46 ++++-
 kernel/events/core.c              |  42 ++--
 tools/perf/Makefile               |   2 +
 tools/perf/builtin-inject.c       |   7 +-
 tools/perf/builtin-kmem.c         |   3 +-
 tools/perf/builtin-kvm.c          |   3 +-
 tools/perf/builtin-lock.c         |   3 +-
 tools/perf/builtin-mem.c          |   2 +-
 tools/perf/builtin-report.c       |   2 +-
 tools/perf/builtin-sched.c        |  17 +-
 tools/perf/builtin-script.c       |   3 +-
 tools/perf/builtin-top.c          |  11 +-
 tools/perf/builtin-trace.c        |  12 +-
 tools/perf/tests/builtin-test.c   |   4 +
 tools/perf/tests/hists_link.c     |  30 ++-
 tools/perf/tests/mmap-basic.c     |   2 +-
 tools/perf/tests/sample-parsing.c | 263 +++++++++++++++++++++++++
 tools/perf/tests/tests.h          |   1 +
 tools/perf/util/build-id.c        |  11 +-
 tools/perf/util/callchain.c       |   8 -
 tools/perf/util/callchain.h       |   5 -
 tools/perf/util/event.c           |   5 +-
 tools/perf/util/event.h           |  33 ++--
 tools/perf/util/evlist.c          | 146 ++++++++++++--
 tools/perf/util/evlist.h          |  10 +-
 tools/perf/util/evsel.c           | 404 +++++++++++++++++++++++++++++++++-----
 tools/perf/util/evsel.h           |  13 +-
 tools/perf/util/machine.c         |  37 ++--
 tools/perf/util/machine.h         |   3 +-
 tools/perf/util/python.c          |  20 ++
 tools/perf/util/record.c          | 111 +++++++++++
 tools/perf/util/session.c         |  36 +---
 tools/perf/util/thread.c          |   3 +-
 tools/perf/util/thread.h          |   3 +-
 34 files changed, 1109 insertions(+), 192 deletions(-)
 create mode 100644 tools/perf/tests/sample-parsing.c
 create mode 100644 tools/perf/util/record.c

Regards
Adrian

author reviewer
#1 lore raw

PATCH v8 1/15 perf tools: add debug prints

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  | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/python.c | 20 +++++++++++++++++
 3 files changed, 81 insertions(+)
tools/perf/util/evlist.c +3 -0
index 42ea4e9..2b77b33 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 +58 -0

82 lines not on this page · raw message

tools/perf/util/python.c +20 -0
index 925e0c3..381f4fd 100644
@@ -8,6 +8,26 @@
 #include "cpumap.h"
 #include "thread_map.h"
 
+/*
+ * Support debug printing even though util/debug.c is not linked.  That means
+ * implementing 'verbose' and 'eprintf'.
+ */
+int verbose;
+
+int eprintf(int level, const char *fmt, ...)
+{
+	va_list args;
+	int ret = 0;
+
+	if (verbose >= level) {
+		va_start(args, fmt);
+		ret = vfprintf(stderr, fmt, args);
+		va_end(args);
+	}
+
+	return ret;
+}
+
 /* Define PyVarObject_HEAD_INIT for python 2.5 */
 #ifndef PyVarObject_HEAD_INIT
 # define PyVarObject_HEAD_INIT(type, size) PyObject_HEAD_INIT(type) size,
Signature · 1 line
1.7.11.7
author reviewer
#2 lore raw

PATCH v8 2/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    | 136 ++++++++++++++++++++++++++++++++++++++++++--
 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, 231 insertions(+), 11 deletions(-)
tools/perf/builtin-report.c +1 -1
index a34c587..7b6a2bb 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 1ebb8fb..e60585e 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 +131 -5

187 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 d0d9f94..d4c9a26 100644
@@ -734,7 +734,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)) {
@@ -893,7 +893,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)) {
@@ -1293,7 +1293,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
#3 lore raw

PATCH v8 3/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 | 16 +++++++++++-----
 tools/perf/util/thread.c  |  3 ++-
 tools/perf/util/thread.h  |  3 ++-
 3 files changed, 15 insertions(+), 7 deletions(-)
tools/perf/util/machine.c +11 -5
index f9f9d63..67ba572 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 && 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 && 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)
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 0fe1f9c..fc464bc 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
author reviewer
#5 lore raw

PATCH v8 4/15 perf tools: change machine__findnew_thread() to set thread pid

In reply to Adrian Hunter

Add a new parameter for 'pid' to machine__findnew_thread(). Change callers to pass 'pid' when it is known.

---
 tools/perf/builtin-inject.c   |  2 +-
 tools/perf/builtin-kmem.c     |  3 ++-
 tools/perf/builtin-kvm.c      |  3 ++-
 tools/perf/builtin-lock.c     |  3 ++-
 tools/perf/builtin-sched.c    | 17 +++++++++--------
 tools/perf/builtin-script.c   |  3 ++-
 tools/perf/builtin-trace.c    | 12 +++++++++---
 tools/perf/tests/hists_link.c |  3 ++-
 tools/perf/util/build-id.c    |  7 +++++--
 tools/perf/util/event.c       |  3 ++-
 tools/perf/util/machine.c     | 23 ++++++++++++++++-------
 tools/perf/util/machine.h     |  3 ++-
 tools/perf/util/session.c     |  2 +-
 13 files changed, 55 insertions(+), 29 deletions(-)
tools/perf/builtin-inject.c +1 -1
index 1d8de2e..0d4ae1d 100644
@@ -198,7 +198,7 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
 
 	cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
-	thread = machine__findnew_thread(machine, event->ip.pid);
+	thread = machine__findnew_thread(machine, event->ip.pid, event->ip.pid);
 	if (thread == NULL) {
 		pr_err("problem processing %d event, skipping it.\n",
 		       event->header.type);
tools/perf/builtin-kmem.c +2 -1
index b49f5c5..c324778 100644
@@ -305,7 +305,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				struct perf_evsel *evsel,
 				struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);
 
 	if (thread == NULL) {
 		pr_debug("problem processing %d event, skipping it.\n",
tools/perf/builtin-kvm.c +2 -1
index 24b78ae..ecbbec8 100644
@@ -691,7 +691,8 @@ static int process_sample_event(struct perf_tool *tool,
 				struct perf_evsel *evsel,
 				struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, sample->tid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->tid);
 	struct perf_kvm_stat *kvm = container_of(tool, struct perf_kvm_stat,
 						 tool);
 
tools/perf/builtin-lock.c +2 -1
index 76543a4..ee33ba2 100644
@@ -805,7 +805,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				struct perf_evsel *evsel,
 				struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, sample->tid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->tid);
 
 	if (thread == NULL) {
 		pr_debug("problem processing %d event, skipping it.\n",
tools/perf/builtin-sched.c +9 -8
index 948183a..49593a0 100644
@@ -934,8 +934,8 @@ static int latency_switch_event(struct perf_sched *sched,
 		return -1;
 	}
 
-	sched_out = machine__findnew_thread(machine, prev_pid);
-	sched_in = machine__findnew_thread(machine, next_pid);
+	sched_out = machine__findnew_thread(machine, 0, prev_pid);
+	sched_in = machine__findnew_thread(machine, 0, next_pid);
 
 	out_events = thread_atoms_search(&sched->atom_root, sched_out, &sched->cmp_pid);
 	if (!out_events) {
@@ -978,7 +978,7 @@ static int latency_runtime_event(struct perf_sched *sched,
 {
 	const u32 pid	   = perf_evsel__intval(evsel, sample, "pid");
 	const u64 runtime  = perf_evsel__intval(evsel, sample, "runtime");
-	struct thread *thread = machine__findnew_thread(machine, pid);
+	struct thread *thread = machine__findnew_thread(machine, 0, pid);
 	struct work_atoms *atoms = thread_atoms_search(&sched->atom_root, thread, &sched->cmp_pid);
 	u64 timestamp = sample->time;
 	int cpu = sample->cpu;
@@ -1016,7 +1016,7 @@ static int latency_wakeup_event(struct perf_sched *sched,
 	if (!success)
 		return 0;
 
-	wakee = machine__findnew_thread(machine, pid);
+	wakee = machine__findnew_thread(machine, 0, pid);
 	atoms = thread_atoms_search(&sched->atom_root, wakee, &sched->cmp_pid);
 	if (!atoms) {
 		if (thread_atoms_insert(sched, wakee))
@@ -1070,7 +1070,7 @@ static int latency_migrate_task_event(struct perf_sched *sched,
 	if (sched->profile_cpu == -1)
 		return 0;
 
-	migrant = machine__findnew_thread(machine, pid);
+	migrant = machine__findnew_thread(machine, 0, pid);
 	atoms = thread_atoms_search(&sched->atom_root, migrant, &sched->cmp_pid);
 	if (!atoms) {
 		if (thread_atoms_insert(sched, migrant))
@@ -1289,8 +1289,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,
 		return -1;
 	}
 
-	sched_out = machine__findnew_thread(machine, prev_pid);
-	sched_in = machine__findnew_thread(machine, next_pid);
+	sched_out = machine__findnew_thread(machine, 0, prev_pid);
+	sched_in = machine__findnew_thread(machine, 0, next_pid);
 
 	sched->curr_thread[this_cpu] = sched_in;
 
@@ -1425,7 +1425,8 @@ static int perf_sched__process_tracepoint_sample(struct perf_tool *tool __maybe_
 						 struct perf_evsel *evsel,
 						 struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, sample->tid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->tid);
 	int err = 0;
 
 	if (thread == NULL) {
tools/perf/builtin-script.c +2 -1
index ecb6979..b094b33 100644
@@ -480,7 +480,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				struct machine *machine)
 {
 	struct addr_location al;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.tid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.tid);
 
 	if (thread == NULL) {
 		pr_debug("problem processing %d event, skipping it.\n",
tools/perf/builtin-trace.c +9 -3
index 0e4b67f..f8bc748 100644
@@ -301,7 +301,9 @@ static int trace__sys_enter(struct trace *trace, struct perf_evsel *evsel,
 	char *msg;
 	void *args;
 	size_t printed = 0;
-	struct thread *thread = machine__findnew_thread(&trace->host, sample->tid);
+	struct thread *thread = machine__findnew_thread(&trace->host,
+							sample->pid,
+							sample->tid);
 	struct syscall *sc = trace__syscall_info(trace, evsel, sample);
 	struct thread_trace *ttrace = thread__trace(thread);
 
@@ -344,7 +346,9 @@ static int trace__sys_exit(struct trace *trace, struct perf_evsel *evsel,
 {
 	int ret;
 	u64 duration = 0;
-	struct thread *thread = machine__findnew_thread(&trace->host, sample->tid);
+	struct thread *thread = machine__findnew_thread(&trace->host,
+							sample->pid,
+							sample->tid);
 	struct thread_trace *ttrace = thread__trace(thread);
 	struct syscall *sc = trace__syscall_info(trace, evsel, sample);
 
@@ -397,7 +401,9 @@ static int trace__sched_stat_runtime(struct trace *trace, struct perf_evsel *evs
 {
         u64 runtime = perf_evsel__intval(evsel, sample, "runtime");
 	double runtime_ms = (double)runtime / NSEC_PER_MSEC;
-	struct thread *thread = machine__findnew_thread(&trace->host, sample->tid);
+	struct thread *thread = machine__findnew_thread(&trace->host,
+							sample->pid,
+							sample->tid);
 	struct thread_trace *ttrace = thread__trace(thread);
 
 	if (ttrace == NULL)
tools/perf/tests/hists_link.c +2 -1
index 89085a9..5a178d5 100644
@@ -88,7 +88,8 @@ static struct machine *setup_fake_machine(struct machines *machines)
 	for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
 		struct thread *thread;
 
-		thread = machine__findnew_thread(machine, fake_threads[i].pid);
+		thread = machine__findnew_thread(machine, fake_threads[i].pid,
+						 fake_threads[i].pid);
 		if (thread == NULL)
 			goto out;
 
tools/perf/util/build-id.c +5 -2
index 5295625..0f9d27a 100644
@@ -24,7 +24,8 @@ int build_id__mark_dso_hit(struct perf_tool *tool __maybe_unused,
 {
 	struct addr_location al;
 	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);
 
 	if (thread == NULL) {
 		pr_err("problem processing %d event, skipping it.\n",
@@ -47,7 +48,9 @@ static int perf_event__exit_del_thread(struct perf_tool *tool __maybe_unused,
 				       __maybe_unused,
 				       struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, event->fork.tid);
+	struct thread *thread = machine__findnew_thread(machine,
+							event->fork.pid,
+							event->fork.tid);
 
 	dump_printf("(%d:%d):(%d:%d)\n", event->fork.pid, event->fork.tid,
 		    event->fork.ppid, event->fork.ptid);
tools/perf/util/event.c +2 -1
index 9541270..ec494a3 100644
@@ -677,7 +677,8 @@ int perf_event__preprocess_sample(const union perf_event *event,
 				  symbol_filter_t filter)
 {
 	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);
 
 	if (thread == NULL)
 		return -1;
tools/perf/util/machine.c +16 -7
index 67ba572..16b84ac 100644
@@ -30,7 +30,8 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 		return -ENOMEM;
 
 	if (pid != HOST_KERNEL_ID) {
-		struct thread *thread = machine__findnew_thread(machine, pid);
+		struct thread *thread = machine__findnew_thread(machine, 0,
+								pid);
 		char comm[64];
 
 		if (thread == NULL)
@@ -282,9 +283,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
 	return th;
 }
 
-struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
+struct thread *machine__findnew_thread(struct machine *machine, pid_t pid,
+				       pid_t tid)
 {
-	return __machine__findnew_thread(machine, 0, tid, true);
+	return __machine__findnew_thread(machine, pid, tid, true);
 }
 
 struct thread *machine__find_thread(struct machine *machine, pid_t tid)
@@ -294,7 +296,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(machine,
+							event->comm.pid,
+							event->comm.tid);
 
 	if (dump_trace)
 		perf_event__fprintf_comm(event, stdout);
@@ -975,7 +979,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(machine, event->mmap.pid,
+					 event->mmap.pid);
 	if (thread == NULL)
 		goto out_problem;
 
@@ -1002,8 +1007,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(machine,
+							event->fork.pid,
+							event->fork.tid);
+	struct thread *parent = machine__findnew_thread(machine,
+							event->fork.ppid,
+							event->fork.ptid);
 
 	if (dump_trace)
 		perf_event__fprintf_task(event, stdout);
tools/perf/util/machine.h +2 -1
index 5bb6244..604be6b 100644
@@ -101,7 +101,8 @@ 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 tid);
+struct thread *machine__findnew_thread(struct machine *machine, pid_t pid,
+				       pid_t tid);
 
 size_t machine__fprintf(struct machine *machine, FILE *fp);
 
tools/perf/util/session.c +1 -1
index d4c9a26..708c72b 100644
@@ -991,7 +991,7 @@ void perf_event_header__bswap(struct perf_event_header *self)
 
 struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
 {
-	return machine__findnew_thread(&session->machines.host, pid);
+	return machine__findnew_thread(&session->machines.host, 0, pid);
 }
 
 static struct thread *perf_session__register_idle_thread(struct perf_session *self)
Signature · 1 line
1.7.11.7
#6 lore raw

Re: [PATCH V8 04/15] perf tools: change machine__findnew_thread() to set thread pid

Quoted 25 lines · Adrian Hunter

Add a new parameter for 'pid' to machine__findnew_thread(). Change callers to pass 'pid' when it is known.

---
  tools/perf/builtin-inject.c   |  2 +-
  tools/perf/builtin-kmem.c     |  3 ++-
  tools/perf/builtin-kvm.c      |  3 ++-
  tools/perf/builtin-lock.c     |  3 ++-
  tools/perf/builtin-sched.c    | 17 +++++++++--------
  tools/perf/builtin-script.c   |  3 ++-
  tools/perf/builtin-trace.c    | 12 +++++++++---
  tools/perf/tests/hists_link.c |  3 ++-
  tools/perf/util/build-id.c    |  7 +++++--
  tools/perf/util/event.c       |  3 ++-
  tools/perf/util/machine.c     | 23 ++++++++++++++++-------
  tools/perf/util/machine.h     |  3 ++-
  tools/perf/util/session.c     |  2 +-
  13 files changed, 55 insertions(+), 29 deletions(-)
tools/perf/builtin-inject.c +0 -0
index 1d8de2e..0d4ae1d 100644

tools/perf/builtin-inject.c static int perf_event__inject_buildid(struct perf_tool *tool, · line 201

Quoted 5 lines
tools/perf/builtin-inject.c +1 -1
@@ -198,7 +198,7 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
 
  	cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
-	thread = machine__findnew_thread(machine, event->ip.pid);
+	thread = machine__findnew_thread(machine, event->ip.pid, event->ip.pid);

event->ip.tid? at a minimum current functionality is retained with machine__findnew_thread(machine, 0, event->ip.pid)

Quoted 8 lines
tools/perf/builtin-inject.c +0 -0
  	if (thread == NULL) {
  		pr_err("problem processing %d event, skipping it.\n",
  		       event->header.type);
tools/perf/builtin-kmem.c +0 -0
index b49f5c5..c324778 100644

tools/perf/builtin-kmem.c static int process_sample_event(struct perf_tool *tool __maybe_unused, · line 309

Quoted 6 lines
tools/perf/builtin-kmem.c +2 -1
@@ -305,7 +305,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
  				struct perf_evsel *evsel,
  				struct machine *machine)
  {
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);

ditto.

tools/perf/tests/hists_link.c static struct machine *setup_fake_machine(struct machines *machines) · line 92

Quoted 11 lines
tools/perf/tests/hists_link.c +2 -1
index 89085a9..5a178d5 100644
@@ -88,7 +88,8 @@ static struct machine *setup_fake_machine(struct machines *machines)
  	for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
  		struct thread *thread;
 
-		thread = machine__findnew_thread(machine, fake_threads[i].pid);
+		thread = machine__findnew_thread(machine, fake_threads[i].pid,
+						 fake_threads[i].pid);

And here

Quoted 8 lines
tools/perf/tests/hists_link.c +0 -0
  		if (thread == NULL)
  			goto out;
 
tools/perf/util/build-id.c +0 -0
index 5295625..0f9d27a 100644

tools/perf/util/build-id.c int build_id__mark_dso_hit(struct perf_tool *tool __maybe_unused, · line 28

Quoted 6 lines
tools/perf/util/build-id.c +2 -1
@@ -24,7 +24,8 @@ int build_id__mark_dso_hit(struct perf_tool *tool __maybe_unused,
  {
  	struct addr_location al;
  	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);

and here.

tools/perf/util/event.c int perf_event__preprocess_sample(const union perf_event *event, · line 681

Quoted 11 lines
tools/perf/util/event.c +2 -1
index 9541270..ec494a3 100644
@@ -677,7 +677,8 @@ int perf_event__preprocess_sample(const union perf_event *event,
  				  symbol_filter_t filter)
  {
  	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+							event->ip.pid);

and here

Quoted 42 lines
tools/perf/util/event.c +0 -0
  	if (thread == NULL)
  		return -1;
tools/perf/util/machine.c +8 -4
index 67ba572..16b84ac 100644
@@ -30,7 +30,8 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
  		return -ENOMEM;
 
  	if (pid != HOST_KERNEL_ID) {
-		struct thread *thread = machine__findnew_thread(machine, pid);
+		struct thread *thread = machine__findnew_thread(machine, 0,
+								pid);
  		char comm[64];
 
  		if (thread == NULL)
@@ -282,9 +283,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
  	return th;
  }
 
-struct thread *machine__findnew_thread(struct machine *machine, pid_t tid)
+struct thread *machine__findnew_thread(struct machine *machine, pid_t pid,
+				       pid_t tid)
  {
-	return __machine__findnew_thread(machine, 0, tid, true);
+	return __machine__findnew_thread(machine, pid, tid, true);
  }
 
  struct thread *machine__find_thread(struct machine *machine, pid_t tid)
@@ -294,7 +296,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(machine,
+							event->comm.pid,
+							event->comm.tid);
 
  	if (dump_trace)
  		perf_event__fprintf_comm(event, stdout);

tools/perf/util/machine.c int machine__process_mmap_event(struct machine *machine, union perf_event *event · line 983

Quoted 6 lines
tools/perf/util/machine.c +2 -1
@@ -975,7 +979,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(machine, event->mmap.pid,
+					 event->mmap.pid);

and here.

David

author reviewer
#7 lore raw

Re: [PATCH V8 04/15] perf tools: change machine__findnew_thread() to set thread pid

Quoted 28 lines · David Ahern
Quoted 27 lines · Adrian Hunter

Add a new parameter for 'pid' to machine__findnew_thread(). Change callers to pass 'pid' when it is known.

---
tools/perf/builtin-inject.c | 2 +-
tools/perf/builtin-kmem.c | 3 ++-
tools/perf/builtin-kvm.c | 3 ++-
tools/perf/builtin-lock.c | 3 ++-
tools/perf/builtin-sched.c | 17 +++++++++--------
tools/perf/builtin-script.c | 3 ++-
tools/perf/builtin-trace.c | 12 +++++++++---
tools/perf/tests/hists_link.c | 3 ++-
tools/perf/util/build-id.c | 7 +++++--
tools/perf/util/event.c | 3 ++-

tools/perf/util/machine.c | 23 ++++++++++++++++-------
tools/perf/util/machine.h | 3 ++-
tools/perf/util/session.c | 2 +-
13 files changed, 55 insertions(+), 29 deletions(-)
tools/perf/builtin-inject.c +0 -0
index 1d8de2e..0d4ae1d 100644
@@ -198,7 +198,7 @@ static int perf_event__inject_buildid(struct
 perf_tool *tool,
 
Quoted 8 lines

tools/perf/builtin-inject.c static int perf_event__inject_buildid(struct perf_tool *tool,

Quoted 5 lines
tools/perf/builtin-inject.c +1 -1
 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
- thread = machine__findnew_thread(machine, event->ip.pid);
+ thread = machine__findnew_thread(machine, event->ip.pid,
 event->ip.pid);

event->ip.tid? at a minimum current functionality is retained with machine__findnew_thread(machine, 0, event->ip.pid)

perf tools records mmap information against the process's main thread for which pid == tid. So the correct values are being passed here.

Quoted 133 lines
Quoted 10 lines
tools/perf/builtin-inject.c +0 -0
 if (thread == NULL) {

pr_err("problem processing %d event, skipping it.\n", event->header.type);

tools/perf/builtin-kmem.c +0 -0
index b49f5c5..c324778 100644

tools/perf/builtin-kmem.c static int process_sample_event(struct perf_tool · line 311

Quoted 8 lines
tools/perf/builtin-kmem.c +2 -1
@@ -305,7 +305,8 @@ static int process_sample_event(struct perf_tool
 *tool __maybe_unused,
 struct perf_evsel *evsel,
 struct machine *machine)
 {
- struct thread *thread = machine__findnew_thread(machine,
 event->ip.pid);
+ struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+ event->ip.pid);

ditto.

tools/perf/tests/hists_link.c static struct machine *setup_fake_machine(struct · line 93

Quoted 13 lines
tools/perf/tests/hists_link.c +2 -1
index 89085a9..5a178d5 100644
@@ -88,7 +88,8 @@ static struct machine *setup_fake_machine(struct
 machines *machines)
 for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
 struct thread *thread;
 
- thread = machine__findnew_thread(machine, fake_threads[i].pid);
+ thread = machine__findnew_thread(machine, fake_threads[i].pid,
+ fake_threads[i].pid);

And here

Quoted 8 lines
tools/perf/tests/hists_link.c +0 -0
 if (thread == NULL)
 goto out;
tools/perf/util/build-id.c +0 -0
index 5295625..0f9d27a 100644

tools/perf/util/build-id.c int build_id__mark_dso_hit(struct perf_tool *tool · line 30

Quoted 8 lines
tools/perf/util/build-id.c +2 -1
@@ -24,7 +24,8 @@ int build_id__mark_dso_hit(struct perf_tool *tool
 __maybe_unused,
 {
 struct addr_location al;
 u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
- struct thread *thread = machine__findnew_thread(machine,
 event->ip.pid);
+ struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+ event->ip.pid);

and here.

tools/perf/util/event.c int perf_event__preprocess_sample(const union · line 683

Quoted 13 lines
tools/perf/util/event.c +2 -1
index 9541270..ec494a3 100644
@@ -677,7 +677,8 @@ int perf_event__preprocess_sample(const union
 perf_event *event,
 symbol_filter_t filter)
 {
 u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
- struct thread *thread = machine__findnew_thread(machine,
 event->ip.pid);
+ struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
+ event->ip.pid);

and here

Quoted 49 lines
tools/perf/util/event.c +0 -0
 if (thread == NULL)

return -1;

tools/perf/util/machine.c +2 -1
index 67ba572..16b84ac 100644
@@ -30,7 +30,8 @@ int machine__init(struct machine *machine, const
 char *root_dir, pid_t pid)
 return -ENOMEM;
 
 if (pid != HOST_KERNEL_ID) {
- struct thread *thread = machine__findnew_thread(machine, pid);
+ struct thread *thread = machine__findnew_thread(machine, 0,
+ pid);
 char comm[64];
 

if (thread == NULL)

tools/perf/util/machine.c +3 -2
@@ -282,9 +283,10 @@ static struct thread
 *__machine__findnew_thread(struct machine *machine,
 return th;
 }
 
-struct thread *machine__findnew_thread(struct machine *machine, pid_t
 tid)
+struct thread *machine__findnew_thread(struct machine *machine, pid_t
 pid,
+ pid_t tid)
 {
- return __machine__findnew_thread(machine, 0, tid, true);
+ return __machine__findnew_thread(machine, pid, tid, true);

}

struct thread *machine__find_thread(struct machine *machine, pid_t tid)

tools/perf/util/machine.c +0 -0
@@ -294,7 +296,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(machine,
+ event->comm.pid,
+ event->comm.tid);

if (dump_trace)
perf_event__fprintf_comm(event, stdout);

tools/perf/util/machine.c int machine__process_mmap_event(struct machine · line 984

Quoted 7 lines
tools/perf/util/machine.c +2 -1
@@ -975,7 +979,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(machine, event->mmap.pid,
+ event->mmap.pid);

and here.

David

Acked-by
#8 lore raw

Re: [PATCH V8 04/15] perf tools: change machine__findnew_thread() to set thread pid

Quoted 9 lines · Adrian Hunter
Quoted 6 lines
Quoted 3 lines

- thread = machine__findnew_thread(machine, event->ip.pid);
+ thread = machine__findnew_thread(machine, event->ip.pid, event->ip.pid);

event->ip.tid? at a minimum current functionality is retained with machine__findnew_thread(machine, 0, event->ip.pid)

perf tools records mmap information against the process's main thread for which pid == tid. So the correct values are being passed here.

Yes. It's a case by case -- if you want syms use pid, otherwise use tid. It would be less confusing if the API was improved - always take pid and tid along with an arg that says 'want syms' or a separate function name. But I digress.

Looking at your patch does maintain the tid element so it should be fine. I ran a few tests with perf-script and the output is the same after the patch, so..

author reviewer
#9 lore raw

PATCH v8 5/15 perf tools: tidy up sample parsing overflow checking

In reply to Adrian Hunter

The size of data retrieved from a sample event must be validated to ensure it does not go past the end of the event. That was being done sporadically and without considering integer overflows.

---
 tools/perf/util/evsel.c | 102 ++++++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 38 deletions(-)
tools/perf/util/evsel.c +64 -38
index 724b75a..20e2ed9 100644
@@ -1114,24 +1114,38 @@ static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel,
 	return 0;
 }
 
-static bool sample_overlap(const union perf_event *event,
-			   const void *offset, u64 size)
+static inline bool overflow_one(const void *endp, const void *offset)
 {
-	const void *base = event;
-
-	if (offset + size > base + event->header.size)
-		return true;
+	return offset + sizeof(u64) > endp;
+}
 
-	return false;
+static inline bool overflow(const void *endp, u16 max_size, const void *offset,
+			    u64 size)
+{
+	return size > max_size || offset + size > endp;
 }
 
+#define OVERFLOW_CHECK_ONE(offset)				\
+	do {							\
+		if (overflow_one(endp, (offset)))		\
+			return -EFAULT;				\
+	} while (0)
+
+#define OVERFLOW_CHECK(offset, size)				\
+	do {							\
+		if (overflow(endp, max_size, (offset), (size)))	\
+			return -EFAULT;				\
+	} while (0)
+
 int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 			     struct perf_sample *data)
 {
 	u64 type = evsel->attr.sample_type;
-	u64 regs_user = evsel->attr.sample_regs_user;
 	bool swapped = evsel->needs_swap;
 	const u64 *array;
+	u16 max_size = event->header.size;
+	const void *endp = (void *)event + max_size;
+	u64 sz;
 
 	/*
 	 * used for cross-endian analysis. See git commit 65014ab3
@@ -1153,6 +1167,11 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 
 	array = event->sample.array;
 
+	/*
+	 * sample_size is based on PERF_SAMPLE_MASK which includes up to
+	 * PERF_SAMPLE_PERIOD.  After that overflow_one() or overflow() must be
+	 * used to check the format does not go past the end of the event.
+	 */
 	if (evsel->sample_size + sizeof(event->header) > event->header.size)
 		return -EFAULT;
 
@@ -1221,20 +1240,19 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 	}
 
 	if (type & PERF_SAMPLE_CALLCHAIN) {
-		if (sample_overlap(event, array, sizeof(data->callchain->nr)))
-			return -EFAULT;
-
-		data->callchain = (struct ip_callchain *)array;
+		const u64 max_callchain_nr = UINT64_MAX / sizeof(u64);
 
-		if (sample_overlap(event, array, data->callchain->nr))
+		OVERFLOW_CHECK_ONE(array);
+		data->callchain = (struct ip_callchain *)array++;
+		if (data->callchain->nr > max_callchain_nr)
 			return -EFAULT;
-
-		array += 1 + data->callchain->nr;
+		sz = data->callchain->nr * sizeof(u64);
+		OVERFLOW_CHECK(array, sz);
+		array = (void *)array + sz;
 	}
 
 	if (type & PERF_SAMPLE_RAW) {
-		const u64 *pdata;
-
+		OVERFLOW_CHECK_ONE(array);
 		u.val64 = *array;
 		if (WARN_ONCE(swapped,
 			      "Endianness of raw data not corrected!\n")) {
@@ -1243,65 +1261,73 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 			u.val32[0] = bswap_32(u.val32[0]);
 			u.val32[1] = bswap_32(u.val32[1]);
 		}
-
-		if (sample_overlap(event, array, sizeof(u32)))
-			return -EFAULT;
-
 		data->raw_size = u.val32[0];
-		pdata = (void *) array + sizeof(u32);
+		array = (void *)array + sizeof(u32);
 
-		if (sample_overlap(event, pdata, data->raw_size))
-			return -EFAULT;
-
-		data->raw_data = (void *) pdata;
-
-		array = (void *)array + data->raw_size + sizeof(u32);
+		OVERFLOW_CHECK(array, data->raw_size);
+		data->raw_data = (void *)array;
+		array = (void *)array + data->raw_size;
 	}
 
 	if (type & PERF_SAMPLE_BRANCH_STACK) {
-		u64 sz;
+		const u64 max_branch_nr = UINT64_MAX /
+					  sizeof(struct branch_entry);
 
-		data->branch_stack = (struct branch_stack *)array;
-		array++; /* nr */
+		OVERFLOW_CHECK_ONE(array);
+		data->branch_stack = (struct branch_stack *)array++;
 
+		if (data->branch_stack->nr > max_branch_nr)
+			return -EFAULT;
 		sz = data->branch_stack->nr * sizeof(struct branch_entry);
-		sz /= sizeof(u64);
-		array += sz;
+		OVERFLOW_CHECK(array, sz);
+		array = (void *)array + sz;
 	}
 
 	if (type & PERF_SAMPLE_REGS_USER) {
+		u64 avail;
+
 		/* First u64 tells us if we have any regs in sample. */
-		u64 avail = *array++;
+		OVERFLOW_CHECK_ONE(array);
+		avail = *array++;
 
 		if (avail) {
+			u64 regs_user = evsel->attr.sample_regs_user;
+
+			sz = hweight_long(regs_user) * sizeof(u64);
+			OVERFLOW_CHECK(array, sz);
 			data->user_regs.regs = (u64 *)array;
-			array += hweight_long(regs_user);
+			array = (void *)array + sz;
 		}
 	}
 
 	if (type & PERF_SAMPLE_STACK_USER) {
-		u64 size = *array++;
+		OVERFLOW_CHECK_ONE(array);
+		sz = *array++;
 
 		data->user_stack.offset = ((char *)(array - 1)
 					  - (char *) event);
 
-		if (!size) {
+		if (!sz) {
 			data->user_stack.size = 0;
 		} else {
+			OVERFLOW_CHECK(array, sz);
 			data->user_stack.data = (char *)array;
-			array += size / sizeof(*array);
+			array = (void *)array + sz;
+			OVERFLOW_CHECK_ONE(array);
 			data->user_stack.size = *array++;
 		}
 	}
 
 	data->weight = 0;
 	if (type & PERF_SAMPLE_WEIGHT) {
+		OVERFLOW_CHECK_ONE(array);
 		data->weight = *array;
 		array++;
 	}
 
 	data->data_src = PERF_MEM_DATA_SRC_NONE;
 	if (type & PERF_SAMPLE_DATA_SRC) {
+		OVERFLOW_CHECK_ONE(array);
 		data->data_src = *array;
 		array++;
 	}
Signature · 1 line
1.7.11.7
author reviewer
#10 lore raw

PATCH v8 6/15 perf tools: remove unnecessary callchain validation

In reply to Adrian Hunter

Now that the sample parsing correctly checks data sizes there is no reason for it to be done again for callchains.

---
 tools/perf/util/callchain.c |  8 --------
 tools/perf/util/callchain.h |  5 -----
 tools/perf/util/session.c   | 20 --------------------
 3 files changed, 33 deletions(-)
tools/perf/util/callchain.c +0 -8
index 42b6a63..024162a 100644
@@ -20,14 +20,6 @@
 
 __thread struct callchain_cursor callchain_cursor;
 
-bool ip_callchain__valid(struct ip_callchain *chain,
-			 const union perf_event *event)
-{
-	unsigned int chain_size = event->header.size;
-	chain_size -= (unsigned long)&event->ip.__more_data - (unsigned long)event;
-	return chain->nr * sizeof(u64) <= chain_size;
-}
-
 #define chain_for_each_child(child, parent)	\
 	list_for_each_entry(child, &parent->children, siblings)
 
tools/perf/util/callchain.h +0 -5
index 3ee9f67..988c1aa 100644
@@ -103,11 +103,6 @@ int callchain_append(struct callchain_root *root,
 int callchain_merge(struct callchain_cursor *cursor,
 		    struct callchain_root *dst, struct callchain_root *src);
 
-struct ip_callchain;
-union perf_event;
-
-bool ip_callchain__valid(struct ip_callchain *chain,
-			 const union perf_event *event);
 /*
  * Initialize a cursor before adding entries inside, but keep
  * the previously allocated entries as a cache.
tools/perf/util/session.c +0 -20
index 708c72b..1b07d7a 100644
@@ -889,22 +889,6 @@ static int perf_session_deliver_event(struct perf_session *session,
 	}
 }
 
-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 ||
-	    !sample->callchain)
-		return 0;
-
-	if (!ip_callchain__valid(sample->callchain, event)) {
-		pr_debug("call-chain problem with event, skipping it.\n");
-		++session->stats.nr_invalid_chains;
-		session->stats.total_invalid_chains += sample->period;
-		return -EINVAL;
-	}
-	return 0;
-}
-
 static int perf_session__process_user_event(struct perf_session *session, union perf_event *event,
 					    struct perf_tool *tool, u64 file_offset)
 {
@@ -967,10 +951,6 @@ static int perf_session__process_event(struct perf_session *session,
 	if (ret)
 		return ret;
 
-	/* Preprocess sample records - precheck callchains */
-	if (perf_session__preprocess_sample(session, event, &sample))
-		return 0;
-
 	if (tool->ordered_samples) {
 		ret = perf_session_queue_event(session, event, &sample,
 					       file_offset);
Signature · 1 line
1.7.11.7
author reviewer
#11 lore raw

PATCH v8 7/15 perf tools: remove references to struct ip_event

In reply to Adrian Hunter

struct ip_event assumes fixeed positions for ip, pid and tid. That is no longer true with the addition of PERF_SAMPLE_IDENTIFIER. The information is anyway in struct sample, so use that instead.

---
 tools/perf/builtin-inject.c   |  4 ++--
 tools/perf/builtin-kmem.c     |  4 ++--
 tools/perf/builtin-mem.c      |  2 +-
 tools/perf/builtin-script.c   |  4 ++--
 tools/perf/builtin-top.c      | 11 +++++----
 tools/perf/tests/hists_link.c |  4 ++++
 tools/perf/util/build-id.c    |  8 +++----
 tools/perf/util/event.c       |  6 ++---
 tools/perf/util/evsel.c       | 56 +++++++++++++++++++------------------------
 tools/perf/util/session.c     |  8 ++++---
 10 files changed, 53 insertions(+), 54 deletions(-)
tools/perf/builtin-inject.c +2 -2
index 0d4ae1d..ffacd46 100644
@@ -198,7 +198,7 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
 
 	cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
-	thread = machine__findnew_thread(machine, event->ip.pid, event->ip.pid);
+	thread = machine__findnew_thread(machine, sample->pid, sample->pid);
 	if (thread == NULL) {
 		pr_err("problem processing %d event, skipping it.\n",
 		       event->header.type);
@@ -206,7 +206,7 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
 	}
 
 	thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
-			      event->ip.ip, &al);
+			      sample->ip, &al);
 
 	if (al.map != NULL) {
 		if (!al.map->dso->hit) {
tools/perf/builtin-kmem.c +2 -2
index c324778..c2dff9c 100644
@@ -305,8 +305,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				struct perf_evsel *evsel,
 				struct machine *machine)
 {
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
-							event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->pid);
 
 	if (thread == NULL) {
 		pr_debug("problem processing %d event, skipping it.\n",
tools/perf/builtin-mem.c +1 -1
index a8ff6d2..4274680 100644
@@ -96,7 +96,7 @@ dump_raw_samples(struct perf_tool *tool,
 		symbol_conf.field_sep,
 		sample->tid,
 		symbol_conf.field_sep,
-		event->ip.ip,
+		sample->ip,
 		symbol_conf.field_sep,
 		sample->addr,
 		symbol_conf.field_sep,
tools/perf/builtin-script.c +2 -2
index b094b33..05e1db2 100644
@@ -480,8 +480,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				struct machine *machine)
 {
 	struct addr_location al;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
-							event->ip.tid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->tid);
 
 	if (thread == NULL) {
 		pr_debug("problem processing %d event, skipping it.\n",
tools/perf/builtin-top.c +6 -5
index bbf4635..0ecec5f 100644
@@ -690,7 +690,7 @@ static void perf_event__process_sample(struct perf_tool *tool,
 {
 	struct perf_top *top = container_of(tool, struct perf_top, tool);
 	struct symbol *parent = NULL;
-	u64 ip = event->ip.ip;
+	u64 ip = sample->ip;
 	struct addr_location al;
 	int err;
 
@@ -700,10 +700,10 @@ static void perf_event__process_sample(struct perf_tool *tool,
 		if (!seen)
 			seen = intlist__new(NULL);
 
-		if (!intlist__has_entry(seen, event->ip.pid)) {
+		if (!intlist__has_entry(seen, sample->pid)) {
 			pr_err("Can't find guest [%d]'s kernel information\n",
-				event->ip.pid);
-			intlist__add(seen, event->ip.pid);
+				sample->pid);
+			intlist__add(seen, sample->pid);
 		}
 		return;
 	}
@@ -838,7 +838,8 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
 			break;
 		case PERF_RECORD_MISC_GUEST_KERNEL:
 			++top->guest_kernel_samples;
-			machine = perf_session__find_machine(session, event->ip.pid);
+			machine = perf_session__find_machine(session,
+							     sample.pid);
 			break;
 		case PERF_RECORD_MISC_GUEST_USER:
 			++top->guest_us_samples;
tools/perf/tests/hists_link.c +4 -0
index 5a178d5..7e0ca15 100644
@@ -220,6 +220,8 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 				},
 			};
 
+			sample.pid = ip_event.ip.pid;
+			sample.ip = ip_event.ip.ip;
 			if (perf_event__preprocess_sample(&event, machine, &al,
 							  &sample, 0) < 0)
 				goto out;
@@ -244,6 +246,8 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 				},
 			};
 
+			sample.pid = ip_event.ip.pid;
+			sample.ip = ip_event.ip.ip;
 			if (perf_event__preprocess_sample(&event, machine, &al,
 							  &sample, 0) < 0)
 				goto out;
tools/perf/util/build-id.c +4 -4
index 0f9d27a..fb58409 100644
@@ -18,14 +18,14 @@
 
 int build_id__mark_dso_hit(struct perf_tool *tool __maybe_unused,
 			   union perf_event *event,
-			   struct perf_sample *sample __maybe_unused,
+			   struct perf_sample *sample,
 			   struct perf_evsel *evsel __maybe_unused,
 			   struct machine *machine)
 {
 	struct addr_location al;
 	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
-							event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->pid);
 
 	if (thread == NULL) {
 		pr_err("problem processing %d event, skipping it.\n",
@@ -34,7 +34,7 @@ int build_id__mark_dso_hit(struct perf_tool *tool __maybe_unused,
 	}
 
 	thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
-			      event->ip.ip, &al);
+			      sample->ip, &al);
 
 	if (al.map != NULL)
 		al.map->dso->hit = 1;
tools/perf/util/event.c +3 -3
index ec494a3..6861d19 100644
@@ -677,8 +677,8 @@ int perf_event__preprocess_sample(const union perf_event *event,
 				  symbol_filter_t filter)
 {
 	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
-	struct thread *thread = machine__findnew_thread(machine, event->ip.pid,
-							event->ip.pid);
+	struct thread *thread = machine__findnew_thread(machine, sample->pid,
+							sample->pid);
 
 	if (thread == NULL)
 		return -1;
@@ -700,7 +700,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
 		machine__create_kernel_maps(machine);
 
 	thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
-			      event->ip.ip, al);
+			      sample->ip, al);
 	dump_printf(" ...... dso: %s\n",
 		    al->map ? al->map->dso->long_name :
 			al->level == 'H' ? "[hypervisor]" : "<not found>");
tools/perf/util/evsel.c +24 -32

156 lines not on this page · raw message

tools/perf/util/session.c +5 -3
index 1b07d7a..eea5eef 100644
@@ -802,7 +802,8 @@ static void dump_sample(struct perf_evsel *evsel, union perf_event *event,
 
 static struct machine *
 	perf_session__find_machine_for_cpumode(struct perf_session *session,
-					       union perf_event *event)
+					       union perf_event *event,
+					       struct perf_sample *sample)
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
@@ -814,7 +815,7 @@ static struct machine *
 		if (event->header.type == PERF_RECORD_MMAP)
 			pid = event->mmap.pid;
 		else
-			pid = event->ip.pid;
+			pid = sample->pid;
 
 		return perf_session__findnew_machine(session, pid);
 	}
@@ -851,7 +852,8 @@ static int perf_session_deliver_event(struct perf_session *session,
 		hists__inc_nr_events(&evsel->hists, event->header.type);
 	}
 
-	machine = perf_session__find_machine_for_cpumode(session, event);
+	machine = perf_session__find_machine_for_cpumode(session, event,
+							 sample);
 
 	switch (event->header.type) {
 	case PERF_RECORD_SAMPLE:
Signature · 1 line
1.7.11.7
author reviewer
#12 lore raw

PATCH v8 8/15 perf tools: move struct ip_event

In reply to Adrian Hunter

struct ip_event assumes fixed positions for ip, pid and tid. That is no longer true with the addition of PERF_SAMPLE_IDENTIFIER.

struct ip_event is no longer used except by hists_link.c. Move it there.

---
 tools/perf/tests/hists_link.c | 23 +++++++++++++++++++----
 tools/perf/util/event.h       | 11 -----------
 2 files changed, 19 insertions(+), 15 deletions(-)
tools/perf/tests/hists_link.c +19 -4
index 7e0ca15..cfdbfd9 100644
@@ -195,6 +195,19 @@ static struct sample fake_samples[][5] = {
 	},
 };
 
+/* PERF_SAMPLE_IP | PERF_SAMPLE_TID | * but not PERF_SAMPLE_IDENTIFIER */
+struct ip_event {
+	struct perf_event_header header;
+	u64 ip;
+	u32 pid, tid;
+	unsigned char __more_data[];
+};
+
+union perf_ip_event {
+	struct ip_event ip;
+	union perf_event event;
+};
+
 static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 {
 	struct perf_evsel *evsel;
@@ -210,7 +223,7 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 	 */
 	list_for_each_entry(evsel, &evlist->entries, node) {
 		for (k = 0; k < ARRAY_SIZE(fake_common_samples); k++) {
-			const union perf_event event = {
+			const union perf_ip_event ip_event = {
 				.ip = {
 					.header = {
 						.misc = PERF_RECORD_MISC_USER,
@@ -219,10 +232,11 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 					.ip  = fake_common_samples[k].ip,
 				},
 			};
+			const union perf_event *event = &ip_event.event;
 
 			sample.pid = ip_event.ip.pid;
 			sample.ip = ip_event.ip.ip;
-			if (perf_event__preprocess_sample(&event, machine, &al,
+			if (perf_event__preprocess_sample(event, machine, &al,
 							  &sample, 0) < 0)
 				goto out;
 
@@ -236,7 +250,7 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 		}
 
 		for (k = 0; k < ARRAY_SIZE(fake_samples[i]); k++) {
-			const union perf_event event = {
+			const union perf_ip_event ip_event = {
 				.ip = {
 					.header = {
 						.misc = PERF_RECORD_MISC_USER,
@@ -245,10 +259,11 @@ static int add_hist_entries(struct perf_evlist *evlist, struct machine *machine)
 					.ip  = fake_samples[i][k].ip,
 				},
 			};
+			const union perf_event *event = &ip_event.event;
 
 			sample.pid = ip_event.ip.pid;
 			sample.ip = ip_event.ip.ip;
-			if (perf_event__preprocess_sample(&event, machine, &al,
+			if (perf_event__preprocess_sample(event, machine, &al,
 							  &sample, 0) < 0)
 				goto out;
 
tools/perf/util/event.h +0 -11
index e60585e..94f4503 100644
@@ -8,16 +8,6 @@
 #include "map.h"
 #include "build-id.h"
 
-/*
- * PERF_SAMPLE_IP | PERF_SAMPLE_TID | *
- */
-struct ip_event {
-	struct perf_event_header header;
-	u64 ip;
-	u32 pid, tid;
-	unsigned char __more_data[];
-};
-
 struct mmap_event {
 	struct perf_event_header header;
 	u32 pid, tid;
@@ -162,7 +152,6 @@ struct tracing_data_event {
 
 union perf_event {
 	struct perf_event_header	header;
-	struct ip_event			ip;
 	struct mmap_event		mmap;
 	struct comm_event		comm;
 	struct fork_event		fork;
Signature · 1 line
1.7.11.7
author reviewer
#13 lore raw

PATCH v8 9/15 perf: Update perf_event_type documentation

In reply to Adrian Hunter

From: Peter Zijlstra <[email protected]>

Due to a discussion with Adrian I had a good look at the perf_event_type record layout and found the documentation to be somewhat unclear.

---
 include/uapi/linux/perf_event.h | 19 ++++++++++++++++++-
 kernel/events/core.c            | 31 ++++++++++++++++---------------
 2 files changed, 34 insertions(+), 16 deletions(-)
include/uapi/linux/perf_event.h +18 -1
index 0b1df41..876a18c 100644
@@ -478,6 +478,16 @@ enum perf_event_type {
 	 * file will be supported by older perf tools, with these new optional
 	 * fields being ignored.
 	 *
+	 * struct sample_id {
+	 *	{ u32			pid, tid; } && PERF_SAMPLE_TID
+	 *	{ u64			time;     } && PERF_SAMPLE_TIME
+	 *	{ u64			id;       } && PERF_SAMPLE_ID
+	 *	{ u64			stream_id;} && PERF_SAMPLE_STREAM_ID
+	 *	{ u32			cpu, res; } && PERF_SAMPLE_CPU
+	 * } && perf_event_attr.sample_id_all
+	 */
+
+	/*
 	 * The MMAP events record the PROT_EXEC mappings so that we can
 	 * correlate userspace IPs to code. They have the following structure:
 	 *
@@ -489,6 +499,7 @@ enum perf_event_type {
 	 *	u64				len;
 	 *	u64				pgoff;
 	 *	char				filename[];
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_MMAP			= 1,
@@ -498,6 +509,7 @@ enum perf_event_type {
 	 *	struct perf_event_header	header;
 	 *	u64				id;
 	 *	u64				lost;
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_LOST			= 2,
@@ -508,6 +520,7 @@ enum perf_event_type {
 	 *
 	 *	u32				pid, tid;
 	 *	char				comm[];
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_COMM			= 3,
@@ -518,6 +531,7 @@ enum perf_event_type {
 	 *	u32				pid, ppid;
 	 *	u32				tid, ptid;
 	 *	u64				time;
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_EXIT			= 4,
@@ -528,6 +542,7 @@ enum perf_event_type {
 	 *	u64				time;
 	 *	u64				id;
 	 *	u64				stream_id;
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_THROTTLE			= 5,
@@ -539,6 +554,7 @@ enum perf_event_type {
 	 *	u32				pid, ppid;
 	 *	u32				tid, ptid;
 	 *	u64				time;
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_FORK			= 7,
@@ -549,6 +565,7 @@ enum perf_event_type {
 	 *	u32				pid, tid;
 	 *
 	 *	struct read_format		values;
+	 *	struct sample_id		sample_id;
 	 * };
 	 */
 	PERF_RECORD_READ			= 8,
@@ -596,7 +613,7 @@ enum perf_event_type {
 	 * 	  u64			dyn_size; } && PERF_SAMPLE_STACK_USER
 	 *
 	 *	{ u64			weight;   } && PERF_SAMPLE_WEIGHT
-	 *	{ u64			data_src;     } && PERF_SAMPLE_DATA_SRC
+	 *	{ u64			data_src; } && PERF_SAMPLE_DATA_SRC
 	 * };
 	 */
 	PERF_RECORD_SAMPLE			= 9,
kernel/events/core.c +16 -15
index 708ab70..c9ef899 100644
@@ -4462,20 +4462,6 @@ void perf_output_sample(struct perf_output_handle *handle,
 		}
 	}
 
-	if (!event->attr.watermark) {
-		int wakeup_events = event->attr.wakeup_events;
-
-		if (wakeup_events) {
-			struct ring_buffer *rb = handle->rb;
-			int events = local_inc_return(&rb->events);
-
-			if (events >= wakeup_events) {
-				local_sub(wakeup_events, &rb->events);
-				local_inc(&rb->wakeup);
-			}
-		}
-	}
-
 	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
 		if (data->br_stack) {
 			size_t size;
@@ -4511,16 +4497,31 @@ void perf_output_sample(struct perf_output_handle *handle,
 		}
 	}
 
-	if (sample_type & PERF_SAMPLE_STACK_USER)
+	if (sample_type & PERF_SAMPLE_STACK_USER) {
 		perf_output_sample_ustack(handle,
 					  data->stack_user_size,
 					  data->regs_user.regs);
+	}
 
 	if (sample_type & PERF_SAMPLE_WEIGHT)
 		perf_output_put(handle, data->weight);
 
 	if (sample_type & PERF_SAMPLE_DATA_SRC)
 		perf_output_put(handle, data->data_src.val);
+
+	if (!event->attr.watermark) {
+		int wakeup_events = event->attr.wakeup_events;
+
+		if (wakeup_events) {
+			struct ring_buffer *rb = handle->rb;
+			int events = local_inc_return(&rb->events);
+
+			if (events >= wakeup_events) {
+				local_sub(wakeup_events, &rb->events);
+				local_inc(&rb->wakeup);
+			}
+		}
+	}
 }
 
 void perf_prepare_sample(struct perf_event_header *header,
Signature · 1 line
1.7.11.7
author reviewer
#14 lore raw

PATCH v8 10/15 perf: make events stream always parsable

In reply to Adrian Hunter

The event stream is not always parsable because the format of a sample is dependent on the sample_type of the selected event. When there is more than one selected event and the sample_types are not the same then parsing becomes problematic. A sample can be matched to its selected event using the ID that is allocated when the event is opened. Unfortunately, to get the ID from the sample means first parsing it.

This patch adds a new sample format bit PERF_SAMPLE_IDENTIFER that puts the ID at a fixed position so that the ID can be retrieved without parsing the sample. For sample events, that is the first position immediately after the header. For non-sample events, that is the last position.

---
 include/uapi/linux/perf_event.h | 27 ++++++++++++++++++++-------
 kernel/events/core.c            | 11 ++++++++++-
 2 files changed, 30 insertions(+), 8 deletions(-)
include/uapi/linux/perf_event.h +20 -7
index 876a18c..71f1ad3 100644
@@ -134,8 +134,9 @@ enum perf_event_sample_format {
 	PERF_SAMPLE_STACK_USER			= 1U << 13,
 	PERF_SAMPLE_WEIGHT			= 1U << 14,
 	PERF_SAMPLE_DATA_SRC			= 1U << 15,
+	PERF_SAMPLE_IDENTIFIER			= 1U << 16,
 
-	PERF_SAMPLE_MAX = 1U << 16,		/* non-ABI */
+	PERF_SAMPLE_MAX = 1U << 17,		/* non-ABI */
 };
 
 /*
@@ -471,12 +472,12 @@ enum perf_event_type {
 	/*
 	 * If perf_event_attr.sample_id_all is set then all event types will
 	 * have the sample_type selected fields related to where/when
-	 * (identity) an event took place (TID, TIME, ID, CPU, STREAM_ID)
-	 * described in PERF_RECORD_SAMPLE below, it will be stashed just after
-	 * the perf_event_header and the fields already present for the existing
-	 * fields, i.e. at the end of the payload. That way a newer perf.data
-	 * file will be supported by older perf tools, with these new optional
-	 * fields being ignored.
+	 * (identity) an event took place (TID, TIME, ID, STREAM_ID, CPU,
+	 * IDENTIFIER) described in PERF_RECORD_SAMPLE below, it will be stashed
+	 * just after the perf_event_header and the fields already present for
+	 * the existing fields, i.e. at the end of the payload. That way a newer
+	 * perf.data file will be supported by older perf tools, with these new
+	 * optional fields being ignored.
 	 *
 	 * struct sample_id {
 	 *	{ u32			pid, tid; } && PERF_SAMPLE_TID
@@ -484,7 +485,12 @@ enum perf_event_type {
 	 *	{ u64			id;       } && PERF_SAMPLE_ID
 	 *	{ u64			stream_id;} && PERF_SAMPLE_STREAM_ID
 	 *	{ u32			cpu, res; } && PERF_SAMPLE_CPU
+	 *	{ u64			id;	  } && PERF_SAMPLE_IDENTIFIER
 	 * } && perf_event_attr.sample_id_all
+	 *
+	 * Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID.  The
+	 * advantage of PERF_SAMPLE_IDENTIFIER is that its position is fixed
+	 * relative to header.size.
 	 */
 
 	/*
@@ -574,6 +580,13 @@ enum perf_event_type {
 	 * struct {
 	 *	struct perf_event_header	header;
 	 *
+	 *	#
+	 *	# Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID.
+	 *	# The advantage of PERF_SAMPLE_IDENTIFIER is that its position
+	 *	# is fixed relative to header.
+	 *	#
+	 *
+	 *	{ u64			id;	  } && PERF_SAMPLE_IDENTIFIER
 	 *	{ u64			ip;	  } && PERF_SAMPLE_IP
 	 *	{ u32			pid, tid; } && PERF_SAMPLE_TID
 	 *	{ u64			time;     } && PERF_SAMPLE_TIME
kernel/events/core.c +10 -1
index c9ef899..91c241d 100644
@@ -1216,6 +1216,9 @@ static void perf_event__id_header_size(struct perf_event *event)
 	if (sample_type & PERF_SAMPLE_TIME)
 		size += sizeof(data->time);
 
+	if (sample_type & PERF_SAMPLE_IDENTIFIER)
+		size += sizeof(data->id);
+
 	if (sample_type & PERF_SAMPLE_ID)
 		size += sizeof(data->id);
 
@@ -4251,7 +4254,7 @@ static void __perf_event_header__init_id(struct perf_event_header *header,
 	if (sample_type & PERF_SAMPLE_TIME)
 		data->time = perf_clock();
 
-	if (sample_type & PERF_SAMPLE_ID)
+	if (sample_type & (PERF_SAMPLE_ID | PERF_SAMPLE_IDENTIFIER))
 		data->id = primary_event_id(event);
 
 	if (sample_type & PERF_SAMPLE_STREAM_ID)
@@ -4290,6 +4293,9 @@ static void __perf_event__output_id_sample(struct perf_output_handle *handle,
 
 	if (sample_type & PERF_SAMPLE_CPU)
 		perf_output_put(handle, data->cpu_entry);
+
+	if (sample_type & PERF_SAMPLE_IDENTIFIER)
+		perf_output_put(handle, data->id);
 }
 
 void perf_event__output_id_sample(struct perf_event *event,
@@ -4402,6 +4408,9 @@ void perf_output_sample(struct perf_output_handle *handle,
 
 	perf_output_put(handle, *header);
 
+	if (sample_type & PERF_SAMPLE_IDENTIFIER)
+		perf_output_put(handle, data->id);
+
 	if (sample_type & PERF_SAMPLE_IP)
 		perf_output_put(handle, data->ip);
 
Signature · 1 line
1.7.11.7
#15 lore raw

Re: [PATCH V8 10/15] perf: make events stream always parsable

Quoted 7 lines · Adrian Hunter

The event stream is not always parsable because the format of a sample is dependent on the sample_type of the selected event. When there is more than one selected event and the sample_types are not the same then parsing becomes problematic. A sample can be matched to its selected event using the ID that is allocated when the event is opened. Unfortunately, to get the ID from the sample means first parsing it.

Quoted 6 lines

This patch adds a new sample format bit PERF_SAMPLE_IDENTIFER that puts the ID at a fixed position so that the ID can be retrieved without parsing the sample. For sample events, that is the first position immediately after the header. For non-sample events, that is the last position.

I think you should add to your description a paragraph explaining that the reason
this solves the problem is because you always get the event description BEFORE you get any sample records. Otherwise, how would you know you're dealing with a perf.data file that is using PERF_SAMPLE_IDENTIFIER? And that holds for both file and pipe modes.

Quoted 113 lines
---
 include/uapi/linux/perf_event.h | 27 ++++++++++++++++++++-------
 kernel/events/core.c            | 11 ++++++++++-
 2 files changed, 30 insertions(+), 8 deletions(-)
include/uapi/linux/perf_event.h +20 -7
index 876a18c..71f1ad3 100644
@@ -134,8 +134,9 @@ enum perf_event_sample_format {
        PERF_SAMPLE_STACK_USER                  = 1U << 13,
        PERF_SAMPLE_WEIGHT                      = 1U << 14,
        PERF_SAMPLE_DATA_SRC                    = 1U << 15,
+       PERF_SAMPLE_IDENTIFIER                  = 1U << 16,
 
-       PERF_SAMPLE_MAX = 1U << 16,             /* non-ABI */
+       PERF_SAMPLE_MAX = 1U << 17,             /* non-ABI */
 };
 
 /*
@@ -471,12 +472,12 @@ enum perf_event_type {
        /*
         * If perf_event_attr.sample_id_all is set then all event types will
         * have the sample_type selected fields related to where/when
-        * (identity) an event took place (TID, TIME, ID, CPU, STREAM_ID)
-        * described in PERF_RECORD_SAMPLE below, it will be stashed just after
-        * the perf_event_header and the fields already present for the existing
-        * fields, i.e. at the end of the payload. That way a newer perf.data
-        * file will be supported by older perf tools, with these new optional
-        * fields being ignored.
+        * (identity) an event took place (TID, TIME, ID, STREAM_ID, CPU,
+        * IDENTIFIER) described in PERF_RECORD_SAMPLE below, it will be stashed
+        * just after the perf_event_header and the fields already present for
+        * the existing fields, i.e. at the end of the payload. That way a newer
+        * perf.data file will be supported by older perf tools, with these new
+        * optional fields being ignored.
         *
         * struct sample_id {
         *      { u32                   pid, tid; } && PERF_SAMPLE_TID
@@ -484,7 +485,12 @@ enum perf_event_type {
         *      { u64                   id;       } && PERF_SAMPLE_ID
         *      { u64                   stream_id;} && PERF_SAMPLE_STREAM_ID
         *      { u32                   cpu, res; } && PERF_SAMPLE_CPU
+        *      { u64                   id;       } && PERF_SAMPLE_IDENTIFIER
         * } && perf_event_attr.sample_id_all
+        *
+        * Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID.  The
+        * advantage of PERF_SAMPLE_IDENTIFIER is that its position is fixed
+        * relative to header.size.
         */
 
        /*
@@ -574,6 +580,13 @@ enum perf_event_type {
         * struct {
         *      struct perf_event_header        header;
         *
+        *      #
+        *      # Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID.
+        *      # The advantage of PERF_SAMPLE_IDENTIFIER is that its position
+        *      # is fixed relative to header.
+        *      #
+        *
+        *      { u64                   id;       } && PERF_SAMPLE_IDENTIFIER
         *      { u64                   ip;       } && PERF_SAMPLE_IP
         *      { u32                   pid, tid; } && PERF_SAMPLE_TID
         *      { u64                   time;     } && PERF_SAMPLE_TIME
kernel/events/core.c +10 -2
index c9ef899..91c241d 100644
@@ -1216,6 +1216,9 @@ static void perf_event__id_header_size(struct perf_event *event)
        if (sample_type & PERF_SAMPLE_TIME)
                size += sizeof(data->time);
 
+       if (sample_type & PERF_SAMPLE_IDENTIFIER)
+               size += sizeof(data->id);
+
        if (sample_type & PERF_SAMPLE_ID)
                size += sizeof(data->id);
 
@@ -4251,7 +4254,7 @@ static void __perf_event_header__init_id(struct perf_event_header *header,
        if (sample_type & PERF_SAMPLE_TIME)
                data->time = perf_clock();
 
-       if (sample_type & PERF_SAMPLE_ID)
+       if (sample_type & (PERF_SAMPLE_ID | PERF_SAMPLE_IDENTIFIER))
                data->id = primary_event_id(event);
 
        if (sample_type & PERF_SAMPLE_STREAM_ID)
@@ -4290,6 +4293,9 @@ static void __perf_event__output_id_sample(struct perf_output_handle *handle,
 
        if (sample_type & PERF_SAMPLE_CPU)
                perf_output_put(handle, data->cpu_entry);
+
+       if (sample_type & PERF_SAMPLE_IDENTIFIER)
+               perf_output_put(handle, data->id);
 }
 
 void perf_event__output_id_sample(struct perf_event *event,
@@ -4402,6 +4408,9 @@ void perf_output_sample(struct perf_output_handle *handle,
 
        perf_output_put(handle, *header);
 
+       if (sample_type & PERF_SAMPLE_IDENTIFIER)
+               perf_output_put(handle, data->id);
+
        if (sample_type & PERF_SAMPLE_IP)
                perf_output_put(handle, data->ip);
 
--

1.7.11.7

author reviewer
#16 lore raw

PATCH v8 11/15 perf tools: move perf_evlist__config() to a new source file

In reply to Adrian Hunter

perf_evlist__config() must be moved to a separate source file to avoid Python link errors when adding support for PERF_SAMPLE_IDENTIFIER.

It is appropriate to do this because perf_evlist__config() is a helper function for event recording. It is used by tools to apply recording options to perf_evlist. It is not used by the Python API.

---
 tools/perf/Makefile      |  1 +
 tools/perf/util/evlist.c | 24 ------------------------
 tools/perf/util/record.c | 27 +++++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 24 deletions(-)
 create mode 100644 tools/perf/util/record.c
tools/perf/Makefile +1 -0
index 5b7c6db..5798cb9 100644
@@ -360,6 +360,7 @@ LIB_OBJS += $(OUTPUT)util/rblist.o
 LIB_OBJS += $(OUTPUT)util/intlist.o
 LIB_OBJS += $(OUTPUT)util/vdso.o
 LIB_OBJS += $(OUTPUT)util/stat.o
+LIB_OBJS += $(OUTPUT)util/record.o
 
 LIB_OBJS += $(OUTPUT)ui/setup.o
 LIB_OBJS += $(OUTPUT)ui/helpline.o
tools/perf/util/evlist.c +0 -24
index b8a5a75..ee6a8f1 100644
@@ -89,30 +89,6 @@ void perf_evlist__make_sample_types_compatible(struct perf_evlist *evlist)
 	perf_evlist__set_id_pos(evlist);
 }
 
-void perf_evlist__config(struct perf_evlist *evlist,
-			struct perf_record_opts *opts)
-{
-	struct perf_evsel *evsel;
-	/*
-	 * Set the evsel leader links before we configure attributes,
-	 * since some might depend on this info.
-	 */
-	if (opts->group)
-		perf_evlist__set_leader(evlist);
-
-	if (evlist->cpus->map[0] < 0)
-		opts->no_inherit = true;
-
-	list_for_each_entry(evsel, &evlist->entries, node) {
-		perf_evsel__config(evsel, opts);
-
-		if (evlist->nr_entries > 1)
-			perf_evsel__set_sample_id(evsel);
-	}
-
-	perf_evlist__make_sample_types_compatible(evlist);
-}
-
 static void perf_evlist__purge(struct perf_evlist *evlist)
 {
 	struct perf_evsel *pos, *n;
tools/perf/util/record.c new file +27 -0
new file mode 100644
index 0000000..67291e4
@@ -0,0 +1,27 @@
+#include "evlist.h"
+#include "evsel.h"
+#include "cpumap.h"
+
+void perf_evlist__config(struct perf_evlist *evlist,
+			 struct perf_record_opts *opts)
+{
+	struct perf_evsel *evsel;
+	/*
+	 * Set the evsel leader links before we configure attributes,
+	 * since some might depend on this info.
+	 */
+	if (opts->group)
+		perf_evlist__set_leader(evlist);
+
+	if (evlist->cpus->map[0] < 0)
+		opts->no_inherit = true;
+
+	list_for_each_entry(evsel, &evlist->entries, node) {
+		perf_evsel__config(evsel, opts);
+
+		if (evlist->nr_entries > 1)
+			perf_evsel__set_sample_id(evsel);
+	}
+
+	perf_evlist__make_sample_types_compatible(evlist);
+}
Signature · 1 line
1.7.11.7
author reviewer
#17 lore raw

PATCH v8 12/15 perf tools: add support for PERF_SAMPLE_IDENTFIER

In reply to Adrian Hunter

Enable parsing of samples with sample format bit PERF_SAMPLE_IDENTFIER. In addition, if the kernel supports it, prefer it to selecting PERF_SAMPLE_ID thereby avoiding the need to force compatible sample types.

---
 tools/perf/tests/mmap-basic.c |  2 +-
 tools/perf/util/event.h       |  9 +++--
 tools/perf/util/evlist.c      |  5 ++-
 tools/perf/util/evlist.h      |  2 +
 tools/perf/util/evsel.c       | 41 +++++++++++++++----
 tools/perf/util/evsel.h       |  3 +-
 tools/perf/util/record.c      | 94 ++++++++++++++++++++++++++++++++++++++++---
 7 files changed, 138 insertions(+), 18 deletions(-)
tools/perf/tests/mmap-basic.c +1 -1
index 5b1b5ab..c4185b9 100644
@@ -72,7 +72,7 @@ int test__basic_mmap(void)
 		}
 
 		evsels[i]->attr.wakeup_events = 1;
-		perf_evsel__set_sample_id(evsels[i]);
+		perf_evsel__set_sample_id(evsels[i], false);
 
 		perf_evlist__add(evlist, evsels[i]);
 
tools/perf/util/event.h +6 -3
index 94f4503..82bad0d 100644
@@ -53,7 +53,8 @@ struct read_event {
 	(PERF_SAMPLE_IP | PERF_SAMPLE_TID |		\
 	 PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR |		\
 	PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID |	\
-	 PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD)
+	 PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD |		\
+	 PERF_SAMPLE_IDENTIFIER)
 
 /*
  * Events have compatible sample types if the following bits all have the same
@@ -61,13 +62,15 @@ struct read_event {
  * 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.
+ * PERF_SAMPLE_STREAM_ID, PERF_SAMPLE_CPU.  PERF_SAMPLE_IDENTIFIER is added for
+ * completeness but it should not be used with PERF_SAMPLE_ID.  Sample types
+ * that include PERF_SAMPLE_IDENTIFIER are always compatible.
  */
 #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)
+	 PERF_SAMPLE_CPU  | PERF_SAMPLE_IDENTIFIER)
 
 struct sample_event {
 	struct perf_event_header        header;
tools/perf/util/evlist.c +4 -1
index ee6a8f1..49ea13c 100644
@@ -56,7 +56,7 @@ struct perf_evlist *perf_evlist__new(void)
  * Events with compatible sample types all have the same id_pos
  * and is_pos.  For convenience, put a copy on evlist.
  */
-static void perf_evlist__set_id_pos(struct perf_evlist *evlist)
+void perf_evlist__set_id_pos(struct perf_evlist *evlist)
 {
 	struct perf_evsel *first = perf_evlist__first(evlist);
 
@@ -790,6 +790,9 @@ u16 perf_evlist__id_hdr_size(struct perf_evlist *evlist)
 
 	if (sample_type & PERF_SAMPLE_CPU)
 		size += sizeof(data->cpu) * 2;
+
+	if (sample_type & PERF_SAMPLE_IDENTIFIER)
+		size += sizeof(data->id);
 out:
 	return size;
 }
tools/perf/util/evlist.h +2 -0
index b1be475..18b968c3 100644
@@ -87,7 +87,9 @@ 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__set_id_pos(struct perf_evlist *evlist);
 void perf_evlist__make_sample_types_compatible(struct perf_evlist *evlist);
+bool perf_can_sample_identifier(void);
 void perf_evlist__config(struct perf_evlist *evlist,
 			 struct perf_record_opts *opts);
 
tools/perf/util/evsel.c +34 -7

100 lines not on this page · raw message

tools/perf/util/evsel.h +2 -1
index c6d616c..bca8e5f 100644
@@ -148,7 +148,8 @@ void __perf_evsel__reset_sample_bit(struct perf_evsel *evsel,
 #define perf_evsel__reset_sample_bit(evsel, bit) \
 	__perf_evsel__reset_sample_bit(evsel, PERF_SAMPLE_##bit)
 
-void perf_evsel__set_sample_id(struct perf_evsel *evsel);
+void perf_evsel__set_sample_id(struct perf_evsel *evsel,
+			       bool use_sample_identifier);
 
 int perf_evsel__set_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			   const char *filter);
tools/perf/util/record.c +89 -5

112 lines not on this page · raw message

Signature · 1 line
1.7.11.7
author reviewer
#18 lore raw

PATCH v8 13/15 perf tools: expand perf_event__synthesize_sample()

In reply to Adrian Hunter

Expand perf_event__synthesize_sample() to handle all sample format bits.

---
 tools/perf/builtin-inject.c |  3 +-
 tools/perf/util/event.h     |  1 +
 tools/perf/util/evsel.c     | 70 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 2 deletions(-)
tools/perf/builtin-inject.c +2 -1
index ffacd46..eb33da6 100644
@@ -301,7 +301,8 @@ found:
 	sample_sw.period = sample->period;
 	sample_sw.time	 = sample->time;
 	perf_event__synthesize_sample(event_sw, evsel->attr.sample_type,
-				      &sample_sw, false);
+				      evsel->attr.sample_regs_user, &sample_sw,
+				      false);
 	build_id__mark_dso_hit(tool, event_sw, &sample_sw, evsel, machine);
 	return perf_event__repipe(tool, event_sw, &sample_sw, machine);
 }
tools/perf/util/event.h +1 -0
index 82bad0d..7f5c36b 100644
@@ -228,6 +228,7 @@ int perf_event__preprocess_sample(const union perf_event *self,
 const char *perf_event__name(unsigned int id);
 
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
+				  u64 sample_regs_user,
 				  const struct perf_sample *sample,
 				  bool swapped);
 
tools/perf/util/evsel.c +69 -1

86 lines not on this page · raw message

Signature · 1 line
1.7.11.7
#19 lore raw

Re: [PATCH V8 13/15] perf tools: expand perf_event__synthesize_sample()

On Thu, Jul 18, 2013 at 01:42:55PM +0300, Adrian Hunter wrote:

SNIP

Quoted 47 lines
tools/perf/util/evsel.c +31 -1
@@ -1349,11 +1349,12 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 }
 
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
+				  u64 sample_regs_user,
 				  const struct perf_sample *sample,
 				  bool swapped)
 {
 	u64 *array;
-
+	size_t sz;
 	/*
 	 * used for cross-endian analysis. See git commit 65014ab3
 	 * for why this goofiness is needed.
@@ -1426,6 +1427,73 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type,
 		array++;
 	}
 
+	if (type & PERF_SAMPLE_READ) {
+		fprintf(stderr, "PERF_SAMPLE_READ is unsupported for now\n");
+		return -1;
+	}
+
+	if (type & PERF_SAMPLE_CALLCHAIN) {
+		sz = (sample->callchain->nr + 1) * sizeof(u64);
+		memcpy(array, sample->callchain, sz);
+		array = (void *)array + sz;
+	}
+
+	if (type & PERF_SAMPLE_RAW) {
+		u.val32[0] = sample->raw_size;
+		if (WARN_ONCE(swapped,
+			      "Endianness of raw data not corrected!\n")) {
+			/*
+			 * Inverse of what is done in perf_evsel__parse_sample
+			 */
+			u.val32[0] = bswap_32(u.val32[0]);
+			u.val32[1] = bswap_32(u.val32[1]);
+			u.val64 = bswap_64(u.val64);
+		}
+		*array = u.val64;
+		array = (void *)array + sizeof(u32);
+
+		memcpy(array, sample->raw_data, sample->raw_size);
+		array = (void *)array + sample->raw_size;
+	}
+

tools/perf/util/evsel.c int perf_event__synthesize_sample(union perf_event *event, u64 type, · line 1468

Quoted 10 lines
tools/perf/util/evsel.c +10 -0
+	if (type & PERF_SAMPLE_BRANCH_STACK) {
+		sz = sample->branch_stack->nr * sizeof(struct branch_entry);
+		sz += sizeof(u64);
+		memcpy(array, sample->branch_stack, sz);
+		array = (void *)array + sz;
+	}
+
+	if (type & PERF_SAMPLE_REGS_USER) {
+		if (sample->user_regs.regs && sample_regs_user) {
+			*array++ = sample_regs_user;

it's 'enum perf_sample_regs_abi' value that goes here ^^^

I guess for now it could be hardcoded like:

#if defined(__x86_64__)
abi = PERF_SAMPLE_REGS_ABI_64
#else
#if defined(__i386__)
abi = PERF_SAMPLE_REGS_ABI_64
#endif
WARN()
#endif

jirka

author reviewer
#20 lore raw

Re: [PATCH V8 13/15] perf tools: expand perf_event__synthesize_sample()

Quoted 66 lines · Jiri Olsa

On Thu, Jul 18, 2013 at 01:42:55PM +0300, Adrian Hunter wrote:

SNIP

Quoted 47 lines
tools/perf/util/evsel.c +31 -1
@@ -1349,11 +1349,12 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 }
 
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
+				  u64 sample_regs_user,
 				  const struct perf_sample *sample,
 				  bool swapped)
 {
 	u64 *array;
-
+	size_t sz;
 	/*
 	 * used for cross-endian analysis. See git commit 65014ab3
 	 * for why this goofiness is needed.
@@ -1426,6 +1427,73 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type,
 		array++;
 	}
 
+	if (type & PERF_SAMPLE_READ) {
+		fprintf(stderr, "PERF_SAMPLE_READ is unsupported for now\n");
+		return -1;
+	}
+
+	if (type & PERF_SAMPLE_CALLCHAIN) {
+		sz = (sample->callchain->nr + 1) * sizeof(u64);
+		memcpy(array, sample->callchain, sz);
+		array = (void *)array + sz;
+	}
+
+	if (type & PERF_SAMPLE_RAW) {
+		u.val32[0] = sample->raw_size;
+		if (WARN_ONCE(swapped,
+			      "Endianness of raw data not corrected!\n")) {
+			/*
+			 * Inverse of what is done in perf_evsel__parse_sample
+			 */
+			u.val32[0] = bswap_32(u.val32[0]);
+			u.val32[1] = bswap_32(u.val32[1]);
+			u.val64 = bswap_64(u.val64);
+		}
+		*array = u.val64;
+		array = (void *)array + sizeof(u32);
+
+		memcpy(array, sample->raw_data, sample->raw_size);
+		array = (void *)array + sample->raw_size;
+	}
+

tools/perf/util/evsel.c int perf_event__synthesize_sample(union perf_event *event, u64 type, · line 1468

Quoted 10 lines
tools/perf/util/evsel.c +10 -0
+	if (type & PERF_SAMPLE_BRANCH_STACK) {
+		sz = sample->branch_stack->nr * sizeof(struct branch_entry);
+		sz += sizeof(u64);
+		memcpy(array, sample->branch_stack, sz);
+		array = (void *)array + sz;
+	}
+
+	if (type & PERF_SAMPLE_REGS_USER) {
+		if (sample->user_regs.regs && sample_regs_user) {
+			*array++ = sample_regs_user;

it's 'enum perf_sample_regs_abi' value that goes here ^^^

I guess for now it could be hardcoded like:

Quoted 8 lines

#if defined(__x86_64__)
abi = PERF_SAMPLE_REGS_ABI_64
#else
#if defined(__i386__)
abi = PERF_SAMPLE_REGS_ABI_64
#endif
WARN()
#endif

Thanks for reviewing this.

Why isn't it better to leave it as a parameter?

#21 lore raw

Re: [PATCH V8 13/15] perf tools: expand perf_event__synthesize_sample()

On Wed, Jul 24, 2013 at 04:15:20PM +0300, Adrian Hunter wrote:

SNIP

Quoted 8 lines
Quoted 8 lines

tools/perf/util/evsel.c int perf_event__synthesize_sample(union perf_event *event, u64 type, · line 1468

Quoted 5 lines
tools/perf/util/evsel.c +5 -0
+	}
+
+	if (type & PERF_SAMPLE_REGS_USER) {
+		if (sample->user_regs.regs && sample_regs_user) {
+			*array++ = sample_regs_user;

it's 'enum perf_sample_regs_abi' value that goes here ^^^

Quoted 7 lines
Quoted 7 lines

I guess for now it could be hardcoded like:

#if defined(__x86_64__)
abi = PERF_SAMPLE_REGS_ABI_64
#else
#if defined(__i386__)
abi = PERF_SAMPLE_REGS_ABI_64

ugh, PERF_SAMPLE_REGS_ABI_32 ^^^ of course ;-)

Quoted 7 lines
Quoted 3 lines

#endif
WARN()
#endif

Thanks for reviewing this.

Why isn't it better to leave it as a parameter?

np, you could have it as parameter.. but I guess for now you'll endup with above code in the upper layer anyway

jirka

author reviewer
#22 lore raw

PATCH v8 14/15 perf tools: add a function to calculate sample event size

In reply to Adrian Hunter

Add perf_event__sample_event_size() which can be used when synthesizing sample events to determine how big the resulting event will be, and therefore how much memory to allocate.

---
 tools/perf/util/event.h |  2 ++
 tools/perf/util/evsel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)
tools/perf/util/event.h +2 -0
index 7f5c36b..c49cb4c 100644
@@ -227,6 +227,8 @@ int perf_event__preprocess_sample(const union perf_event *self,
 
 const char *perf_event__name(unsigned int id);
 
+size_t perf_event__sample_event_size(const struct perf_sample *sample, u64 type,
+				     u64 sample_regs_user);
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
 				  u64 sample_regs_user,
 				  const struct perf_sample *sample,
tools/perf/util/evsel.c +81 -0

87 lines not on this page · raw message

Signature · 1 line
1.7.11.7
Acked-by on v8 14/15
#23 lore raw

Re: [PATCH V8 14/15] perf tools: add a function to calculate sample event size

On Thu, Jul 18, 2013 at 01:42:56PM +0300, Adrian Hunter wrote:

SNIP

Quoted 43 lines
tools/perf/util/event.h +2 -0
@@ -227,6 +227,8 @@ int perf_event__preprocess_sample(const union perf_event *self,
 
 const char *perf_event__name(unsigned int id);
 
+size_t perf_event__sample_event_size(const struct perf_sample *sample, u64 type,
+				     u64 sample_regs_user);
 int perf_event__synthesize_sample(union perf_event *event, u64 type,
 				  u64 sample_regs_user,
 				  const struct perf_sample *sample,
tools/perf/util/evsel.c +26 -0
index 047ab02..7a5ff06 100644
@@ -1348,6 +1348,87 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 	return 0;
 }
 
+size_t perf_event__sample_event_size(const struct perf_sample *sample, u64 type,
+				     u64 sample_regs_user)
+{
+	size_t sz, result = sizeof(struct sample_event);
+
+	if (type & PERF_SAMPLE_IDENTIFIER)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_IP)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_TID)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_TIME)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_ADDR)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_ID)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_STREAM_ID)
+		result += sizeof(u64);
+

tools/perf/util/evsel.c int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event, · line 1386

Quoted 10 lines
tools/perf/util/evsel.c +10 -0
+	if (type & PERF_SAMPLE_CPU)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_PERIOD)
+		result += sizeof(u64);
+
+	if (type & PERF_SAMPLE_READ) {
+		fprintf(stderr, "PERF_SAMPLE_READ is unsupported for now\n");
+		result += 8192; /* To be on the safe side */
+	}

There' a patchset pending to be merged that implements PERF_SAMPLE_READ. I wonder which one makes it in first.. looser will have to update this code ;-)

thanks,
jirka

author reviewer
#24 lore raw

PATCH v8 15/15 perf tools: add a sample parsing test

In reply to Adrian Hunter

Add a test that checks that sample parsing is correctly implemented.

---
 tools/perf/Makefile               |   1 +
 tools/perf/tests/builtin-test.c   |   4 +
 tools/perf/tests/sample-parsing.c | 263 ++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h          |   1 +
 4 files changed, 269 insertions(+)
 create mode 100644 tools/perf/tests/sample-parsing.c
tools/perf/Makefile +1 -0
index 5798cb9..cc81445 100644
@@ -390,6 +390,7 @@ LIB_OBJS += $(OUTPUT)tests/bp_signal.o
 LIB_OBJS += $(OUTPUT)tests/bp_signal_overflow.o
 LIB_OBJS += $(OUTPUT)tests/task-exit.o
 LIB_OBJS += $(OUTPUT)tests/sw-clock.o
+LIB_OBJS += $(OUTPUT)tests/sample-parsing.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o
tools/perf/tests/builtin-test.c +4 -0
index 35b45f1466..5ee3933 100644
@@ -94,6 +94,10 @@ static struct test {
 		.func = test__sw_clock_freq,
 	},
 	{
+		.desc = "Test sample parsing",
+		.func = test__sample_parsing,
+	},
+	{
 		.func = NULL,
 	},
 };
tools/perf/tests/sample-parsing.c new file +263 -0

263 lines not on this page · raw message

tools/perf/tests/tests.h +1 -0
index 07a92f9..90e3056 100644
@@ -35,5 +35,6 @@ int test__bp_signal(void);
 int test__bp_signal_overflow(void);
 int test__task_exit(void);
 int test__sw_clock_freq(void);
+int test__sample_parsing(void);
 
 #endif /* TESTS_H */
Signature · 1 line
1.7.11.7