perf tools: some fixes and tweaks
Adrian Hunter
reviewerv5 cover perf tools: some fixes and tweaks
Hi
Here are some fixes and tweaks to perf tools (version 5).
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 (12):
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: add support for PERF_SAMPLE_IDENTFIER perf tools: expand perf_event__synthesize_sample() perf tools: add a sample parsing test
include/uapi/linux/perf_event.h | 3 +- kernel/events/core.c | 11 +- tools/perf/Makefile | 1 + 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 | 10 +- 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 | 231 ++++++++++++++++++++++++++ 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 | 31 ++-- tools/perf/util/evlist.c | 230 +++++++++++++++++++++++++- tools/perf/util/evlist.h | 9 +- tools/perf/util/evsel.c | 330 ++++++++++++++++++++++++++++++++------ tools/perf/util/evsel.h | 13 +- tools/perf/util/machine.c | 37 +++-- tools/perf/util/machine.h | 3 +- tools/perf/util/session.c | 35 ++-- tools/perf/util/thread.c | 3 +- tools/perf/util/thread.h | 3 +- 32 files changed, 909 insertions(+), 159 deletions(-) create mode 100644 tools/perf/tests/sample-parsing.c
Regards
Adrian
Adrian Hunter
reviewerPATCH v5 1/12 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 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+)
tools/perf/util/evlist.c +3 -0
@@ -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 mint 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, inint 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
Signature · 1 line
1.7.11.7
Adrian Hunter
reviewerPATCH v5 2/12 perf tools: allow non-matching sample types
In reply to Adrian HunterSample types need not be identical to determine the sample id from the event. Only the position of the sample id needs to be the same.
Compatible sample types are ones in which the bits defined by PERF_COMPAT_MASK are the same. 'perf_evlist__config()' forces sample types to be compatible on that basis.
--- tools/perf/builtin-report.c | 2 +- tools/perf/util/event.h | 14 +++++ tools/perf/util/evlist.c | 135 ++++++++++++++++++++++++++++++++++++++++++-- tools/perf/util/evlist.h | 8 ++- tools/perf/util/evsel.c | 64 ++++++++++++++++++++- tools/perf/util/evsel.h | 10 ++++ tools/perf/util/session.c | 8 ++- 7 files changed, 230 insertions(+), 11 deletions(-)
tools/perf/builtin-report.c +1 -1
@@ -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
@@ -65,6 +65,20 @@ struct read_event { PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID | \ PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD) +/* +* Events have compatible sample types if the following bits all have the same +* value. This is because the order of sample members is fixed. For sample +* events the order is: PERF_SAMPLE_IP, PERF_SAMPLE_TID, PERF_SAMPLE_TIME, +* PERF_SAMPLE_ADDR, PERF_SAMPLE_ID. For non-sample events the sample members +* are accessed in reverse order. The order is: PERF_SAMPLE_ID, +* PERF_SAMPLE_STREAM_ID, PERF_SAMPLE_CPU. +*/ +#define PERF_COMPAT_MASK \ + (PERF_SAMPLE_IP | PERF_SAMPLE_TID | \ + PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR | \ + PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID | \ + PERF_SAMPLE_CPU) +struct sample_event {struct perf_event_header header;u64 array[];
tools/perf/util/evlist.c +130 -5
186 lines not on this page · raw message
tools/perf/util/evlist.h +7 -1
@@ -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
@@ -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
@@ -743,7 +743,7 @@ static void perf_session__print_tstamp(struct perf_session *session,union perf_event *event, struct perf_sample *sample) { - u64 sample_type = perf_evlist__sample_type(session->evlist); + u64 sample_type = __perf_evlist__combined_sample_type(session->evlist);if (event->header.type !=PERF_RECORD_SAMPLE && !perf_evlist__sample_id_all (session->evlist)) {
@@ -902,7 +902,7 @@ static int perf_session__preprocess_sample(struct perf_session *session, union perf_event *event, struct perf_sample *sample) {if (event->header.type !=PERF_RECORD_SAMPLE || - !(perf_evlist__sample_type (session->evlist) &PERF_SAMPLE_CALLCHAIN )) + !sample->callchain)return 0 ;if (!ip_callchain__valid (sample->callchain, event)) {
@@ -1304,7 +1304,9 @@ int perf_session__process_events(struct perf_session *self,bool perf_session__has_traces (struct perf_session *session,const char *msg) { -if (!(perf_evlist__sample_type (session->evlist) &PERF_SAMPLE_RAW )) { +u64 sample_type =perf_evlist__combined_sample_type (session->evlist); + +if (!(sample_type &PERF_SAMPLE_RAW )) {pr_err ("No trace sample to read. Did you call 'perf %s'?\n" , msg);return false; }
Signature · 1 line
1.7.11.7
Adrian Hunter
reviewerPATCH v5 3/12 perf tools: add pid to struct thread
In reply to Adrian HunterRecord 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
@@ -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 tif (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 tif (!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 tstruct 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
@@ -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
@@ -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
Adrian Hunter
reviewerPATCH v5 4/12 perf tools: change machine__findnew_thread() to set thread pid
In reply to Adrian HunterAdd 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
@@ -204,7 +204,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 *eventreturn 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
@@ -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
@@ -1002,7 +1002,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
Adrian Hunter
reviewerPATCH v5 5/12 perf tools: tidy up sample parsing overflow checking
In reply to Adrian HunterThe 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 | 101 ++++++++++++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 38 deletions(-)
tools/perf/util/evsel.c +63 -38
@@ -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,72 @@ 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
Adrian Hunter
reviewerPATCH v5 6/12 perf tools: remove unnecessary callchain validation
In reply to Adrian HunterNow 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
@@ -20,14 +20,6 @@ __threadstruct 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
@@ -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
@@ -898,22 +898,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) {
@@ -978,10 +962,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
Adrian Hunter
reviewerPATCH v5 7/12 perf tools: remove references to struct ip_event
In reply to Adrian Hunterstruct 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 | 10 +++++----- 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 | 4 ++-- tools/perf/util/session.c | 7 ++++--- 10 files changed, 29 insertions(+), 24 deletions(-)
tools/perf/builtin-inject.c +2 -2
@@ -204,7 +204,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);
@@ -212,7 +212,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
@@ -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
@@ -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
@@ -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 +5 -5
@@ -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,7 @@ 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
@@ -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
@@ -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
@@ -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 +2 -2
@@ -1176,7 +1176,7 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,return -EFAULT ;if (type &PERF_SAMPLE_IP ) { - data->ip = event->ip.ip; + data->ip = *array; array++; }
@@ -1349,7 +1349,7 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, array = event->sample.array;if (type &PERF_SAMPLE_IP ) { - event->ip.ip = sample->ip; + *array = sample->ip; array++; }
tools/perf/util/session.c +4 -3
@@ -811,7 +811,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 ;
@@ -823,7 +824,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); }
@@ -860,7 +861,7 @@ 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
Adrian Hunter
reviewerPATCH v5 8/12 perf tools: move struct ip_event
In reply to Adrian Hunterstruct 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
@@ -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
@@ -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
Adrian Hunter
reviewerPATCH v5 9/12 perf: make events stream always parsable
In reply to Adrian HunterThe 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 | 3 ++- kernel/events/core.c | 11 ++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-)
include/uapi/linux/perf_event.h +2 -1
@@ -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 */ };/*
kernel/events/core.c +10 -1
@@ -1203,6 +1203,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);
@@ -4229,7 +4232,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 )
@@ -4268,6 +4271,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,
@@ -4380,6 +4386,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
Re: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 6 lines · Adrian Hunter
The event stream is not always parsable because the format of a sample is dependent on the
sample_typeof the selected event. When there is more than one selected event and thesample_typesare 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.
Here's an alternative suggestion -- one that does not involve changing the kernel API or requiring a common denominator in sample_type options.
perf handles event streams through an mmap which can be directly tied to an evsel (a single event) when the mmap is created. ie., when events are read we know exactly which evsel they correspond to. (See perf_evlist__mmap_per_cpu and perf_evlist__mmap_per_thread and add struct perf_evsel *evsel entry to struct perf_mmap).
Commands like perf-record can inject a user event into the stream and hence the data file every time the evsel changes while walking all of the mmap's reading events -- very similar to the way finished round is done. The event would only contain a perf_event_header which is 8 bytes so this does not add a lot to a data file. As an optimization the evsel event could only be injected if the sample_types differ.
Live commands would just use the evsel connected to the mmap -- no lookups needed which would simplify things a bit processing the events.
In short, the information to associate event streams to an event (evsel) is currently available -- it's just being discarded in the many layers.
I'll try to whip up some code that implements this in the next few days.
David
Re: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 8 lines · David Ahern
Quoted 6 lines · Adrian Hunter
The event stream is not always parsable because the format of a sample is dependent on the
sample_typeof the selected event. When there is more than one selected event and thesample_typesare 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 8 lines
Here's an alternative suggestion -- one that does not involve changing the kernel API or requiring a common denominator in
sample_typeoptions.perf handles event streams through an mmap which can be directly tied to an evsel (a single event) when the mmap is created. ie., when events are read we know exactly which evsel they correspond to. (See
perf_evlist__mmap_per_cpuandperf_evlist__mmap_per_threadand add structperf_evsel*evsel entry to structperf_mmap).
Read that code a bit too quickly. All events for a thread are dropped into the same buffer so as currently organized there is not an easy correlation.
David
Quoted 18 lines
Commands like perf-record can inject a user event into the stream and hence the data file every time the evsel changes while walking all of the mmap's reading events -- very similar to the way finished round is done. The event would only contain a
perf_event_headerwhich is 8 bytes so this does not add a lot to a data file. As an optimization the evsel event could only be injected if thesample_typesdiffer.Live commands would just use the evsel connected to the mmap -- no lookups needed which would simplify things a bit processing the events.
In short, the information to associate event streams to an event (evsel) is currently available -- it's just being discarded in the many layers.
I'll try to whip up some code that implements this in the next few days.
David
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
In reply to David AhernQuoted 10 lines · David Ahern
Quoted 6 lines · Adrian Hunter
The event stream is not always parsable because the format of a sample is dependent on the
sample_typeof the selected event. When there is more than one selected event and thesample_typesare 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.Here's an alternative suggestion -- one that does not involve changing the kernel API or requiring a common denominator in
sample_typeoptions.
The kernel API is designed to be extensible. Extending it in a way that is perhaps unexpected but nevertheless backward compatible, is an appropriate solution.
Quoted 26 lines
perf handles event streams through an mmap which can be directly tied to an evsel (a single event) when the mmap is created. ie., when events are read we know exactly which evsel they correspond to. (See
perf_evlist__mmap_per_cpuandperf_evlist__mmap_per_threadand add structperf_evsel*evsel entry to structperf_mmap).Commands like perf-record can inject a user event into the stream and hence the data file every time the evsel changes while walking all of the mmap's reading events -- very similar to the way finished round is done. The event would only contain a
perf_event_headerwhich is 8 bytes so this does not add a lot to a data file. As an optimization the evsel event could only be injected if thesample_typesdiffer.Live commands would just use the evsel connected to the mmap -- no lookups needed which would simplify things a bit processing the events.
In short, the information to associate event streams to an event (evsel) is currently available -- it's just being discarded in the many layers.
I'll try to whip up some code that implements this in the next few days.
David
Peter Zijlstra
maintainerRe: [PATCH V5 09/12] perf: make events stream always parsable
In reply to Adrian HunterQuoted 7 lines · Adrian Hunter
The event stream is not always parsable because the format of a sample is dependent on the
sample_typeof the selected event. When there is more than one selected event and thesample_typesare 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 5 lines
This patch adds a new sample format bit
PERF_SAMPLE_IDENTIFERthat 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.
There's events where this isn't a possible location; take PERF_RECORD_MMAP for instance; the tail is the complete filename.
Why not always insert right after the header?
Also; you forgot to update include/uapi/linux/perf_event.h:enum perf_event_type That's the format documentation.
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 8 lines · Peter Zijlstra
Quoted 7 lines · Adrian Hunter
The event stream is not always parsable because the format of a sample is dependent on the
sample_typeof the selected event. When there is more than one selected event and thesample_typesare 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 8 lines
Quoted 5 lines
This patch adds a new sample format bit
PERF_SAMPLE_IDENTIFERthat 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.There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAP falls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.size
Quoted 2 lines
Why not always insert right after the header?
It is for sample events i.e. PERF_RECORD_SAMPLE
Quoted 4 lines
Also; you forgot to update include/uapi/linux/perf_event.h:enum
perf_event_typeThat's the format documentation.
OK
Peter Zijlstra
maintainerRe: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 7 lines · Adrian Hunter
Quoted 2 lines
There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAPfalls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.size
But why? Why make it different per PERF_RECORD type?
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines · Adrian Hunter
Quoted 2 lines
There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAPfalls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.sizeBut why? Why make it different per
PERF_RECORDtype?
There have always been two formats:
1. PERF_RECORD_SAMPLE as defined by perf_output_sample()
2. everything else as defined by __perf_event__output_id_sample()
The two formats are not the same, and there is no reason for them to be.
PERF_RECORD_SAMPLE is parsed forwards, so the ID is at the first position.
ID samples are parsed backwards, so the ID is at the last position (i.e. the first position parsed).
Re: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 16 lines · Adrian Hunter
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines · Adrian Hunter
Quoted 2 lines
There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAPfalls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.sizeBut why? Why make it different per
PERF_RECORDtype?There have always been two formats:
1.
PERF_RECORD_SAMPLEas defined byperf_output_sample()
Quoted 9 lines
2. everything else as defined by
__perf_event__output_id_sample()The two formats are not the same, and there is no reason for them to be.
PERF_RECORD_SAMPLEis parsed forwards, so the ID is at the first position.ID samples are parsed backwards, so the ID is at the last position (i.e. the first position parsed).
I am missing something here.
Why do we need an event ID for RECORD_MMAP records?
I understand those are requested by events, but do we care which one? The information is global to the monitored process and not specific to an event.
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 23 lines · Stephane Eranian
Quoted 22 lines · Adrian Hunter
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines · Adrian Hunter
Quoted 2 lines
There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAPfalls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.sizeBut why? Why make it different per
PERF_RECORDtype?There have always been two formats:
1.
PERF_RECORD_SAMPLEas defined byperf_output_sample()2. everything else as defined by
__perf_event__output_id_sample()The two formats are not the same, and there is no reason for them to be.
PERF_RECORD_SAMPLEis parsed forwards, so the ID is at the first position.
Quoted 7 lines
Quoted 3 lines
ID samples are parsed backwards, so the ID is at the last position (i.e. the first position parsed).
I am missing something here.
Why do we need an event ID forRECORD_MMAPrecords?
I understand those are requested by events, but do we care which one? The information is global to the monitored process and not specific to an event.
The ID sample has, for example, the time, so you still have to parse it - which means you need the sample_type which means you need the id.
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
In reply to Adrian HunterQuoted 16 lines · Adrian Hunter
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines · Adrian Hunter
Quoted 2 lines
There's events where this isn't a possible location; take
PERF_RECORD_MMAPfor instance; the tail is the complete filename.
PERF_RECORD_MMAPfalls in the category I have called non-sample events. Those events are appended with an ID sample. perf tools parses the ID sample backwards from header.size. So the ID is at the last position relative to header.sizeBut why? Why make it different per
PERF_RECORDtype?There have always been two formats:
1.
PERF_RECORD_SAMPLEas defined byperf_output_sample()
Quoted 8 lines
2. everything else as defined by
__perf_event__output_id_sample()The two formats are not the same, and there is no reason for them to be.
PERF_RECORD_SAMPLEis parsed forwards, so the ID is at the first position.ID samples are parsed backwards, so the ID is at the last position (i.e. the first position parsed).
If you want the ID at the first position in the ID sample, it is do-able. It means perf tools will have to be changed to calculate the variable start position of the ID sample, and then parse the ID sample forwards from there. Please advise.
Peter Zijlstra
maintainerRe: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 4 lines · Adrian Hunter
If you want the ID at the first position in the ID sample, it is do-able. It means perf tools will have to be changed to calculate the variable start position of the ID sample, and then parse the ID sample forwards from there. Please advise.
Oh urgh I see.. how about I merge something like the below first?
Re-reading your patch; you add something to struct sample_id; which means that events which 'forget' to set perf_event_attr::sample_id_all while setting PERF_SAMPLE_IDENTIFIER are 'funny'. Should we refuse them?
---
Subject: perf: Update perf_event_type documentation
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 | 21 +++++++++++++++++++-- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 35 insertions(+), 17 deletions(-)
include/uapi/linux/perf_event.h +18 -1
@@ -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: *
@@ -488,7 +498,8 @@ enum perf_event_type {* u64 addr; * u64 len; * u64 pgoff; +* 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
@@ -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,
Re: [PATCH V5 09/12] perf: make events stream always parsable
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Adrian Hunter
If you want the ID at the first position in the ID sample, it is do-able. It means perf tools will have to be changed to calculate the variable start position of the ID sample, and then parse the ID sample forwards from there. Please advise.
This is complicated and leads me to believe that a better way of doing this is to leverage some bits in the header. This is what my proposal was. I did not yet have a chance to post it. But you can use a bit in the header to indicate
the presence of the eventID. That way, you do not have to relate to the event attr->sample_type to determine how to parse the meta-data records such as MMAP. When the bit is present in the header, the eventID is to be found at -8 bytes form the end of the record.
Quoted 165 lines
Oh urgh I see.. how about I merge something like the below first?
Re-reading your patch; you add something to struct
sample_id; which means that events which 'forget' to setperf_event_attr::sample_id_allwhile settingPERF_SAMPLE_IDENTIFIERare 'funny'. Should we refuse them?---
Subject: perf: Updateperf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.Cc Adrian Hunter <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>--- include/uapi/linux/perf_event.h | 21 +++++++++++++++++++-- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 35 insertions(+), 17 deletions(-)include/uapi/linux/perf_event.h +18
-1@@ -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: * @@ -488,7 +498,8 @@ enum perf_event_type {* u64 addr; * u64 len; * u64 pgoff; +* 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@@ -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,
Adrian Hunter
reviewerRe: [PATCH V5 09/12] perf: make events stream always parsable
In reply to Peter ZijlstraQuoted 8 lines · Peter Zijlstra
Quoted 4 lines · Adrian Hunter
If you want the ID at the first position in the ID sample, it is do-able. It means perf tools will have to be changed to calculate the variable start position of the ID sample, and then parse the ID sample forwards from there. Please advise.
Oh urgh I see.. how about I merge something like the below first?
I put in my V7 series.
Quoted 4 lines
Re-reading your patch; you add something to struct
sample_id; which means that events which 'forget' to setperf_event_attr::sample_id_allwhile settingPERF_SAMPLE_IDENTIFIERare 'funny'. Should we refuse them?
No, that is OK.
Quoted 162 lines
---
Subject: perf: Updateperf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.Cc Adrian Hunter <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>--- include/uapi/linux/perf_event.h | 21 +++++++++++++++++++-- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 35 insertions(+), 17 deletions(-)include/uapi/linux/perf_event.h +18
-1@@ -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: * @@ -488,7 +498,8 @@ enum perf_event_type {* u64 addr; * u64 len; * u64 pgoff; +* 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@@ -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,
tip-bot for Peter Zijlstra
merge bot[tip:perf/core] perf: Update perf_event_type documentation
In reply to Peter Zijlstraperf: Update perf_event_type documentation
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200
perf: Update perf_event_type documentation
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 | 18 +++++++++++++++++- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 33 insertions(+), 16 deletions(-)
include/uapi/linux/perf_event.h +17 -1
@@ -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: *
@@ -498,6 +508,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 +519,7 @@ enum perf_event_type {* * u32 pid, tid; * char comm[]; +* struct sample_id sample_id; * }; */ PERF_RECORD_COMM = 3,
@@ -518,6 +530,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_EXIT = 4,
@@ -528,6 +541,7 @@ enum perf_event_type {* u64 time; * u64 id; * u64 stream_id; +* struct sample_id sample_id; * }; */ PERF_RECORD_THROTTLE = 5,
@@ -539,6 +553,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_FORK = 7,
@@ -549,6 +564,7 @@ enum perf_event_type {* u32 pid, tid; * * struct read_format values; +* struct sample_id sample_id; * }; */ PERF_RECORD_READ = 8,
@@ -596,7 +612,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
@@ -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,
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 11 lines · tip-bot for Peter Zijlstra
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200perf: Update
perf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.
This code makes a lot of code changes considering it is "updating documentation".
Also, will code following this "documentation" be backward compatible?
Meaning, if you have code that depends on the new fields you've added and run on older kernels, will you get sane results? Or will the code get garbage and/or segfault in the sample_id fields because you read past the end of valid data?
Quoted 145 lines
Cc Adrian Hunter <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>--- include/uapi/linux/perf_event.h | 18 +++++++++++++++++- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 33 insertions(+), 16 deletions(-)include/uapi/linux/perf_event.h +17
-1@@ -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: * @@ -498,6 +508,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 +519,7 @@ enum perf_event_type {* * u32 pid, tid; * char comm[]; +* struct sample_id sample_id; * }; */ PERF_RECORD_COMM = 3,@@ -518,6 +530,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_EXIT = 4,@@ -528,6 +541,7 @@ enum perf_event_type {* u64 time; * u64 id; * u64 stream_id; +* struct sample_id sample_id; * }; */ PERF_RECORD_THROTTLE = 5,@@ -539,6 +553,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_FORK = 7,@@ -549,6 +564,7 @@ enum perf_event_type {* u32 pid, tid; * * struct read_format values; +* struct sample_id sample_id; * }; */ PERF_RECORD_READ = 8,@@ -596,7 +612,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@@ -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,
Adrian Hunter
reviewerRe: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 17 lines · Vince Weaver
Quoted 11 lines · tip-bot for Peter Zijlstra
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200perf: Update
perf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.This code makes a lot of code changes considering it is "updating documentation".
Quoted 6 lines
Also, will code following this "documentation" be backward compatible?
Meaning, if you have code that depends on the new fields you've added and run on older kernels, will you get sane results? Or will the code get garbage and/or segfault in the
sample_idfields because you read past the end of valid data?
This is not the patch that adds new sample format bits.
New sample format bits are always backward compatible in the sense that the kernel perf_event_open system call will return an error if you try to use sample format bits that it does not know about.
Applications will not spontaneously start using sample format bits they have not been programmed for. If an application is enhanced to use a new sample format bit then it must be ready to cope with errors on older kernels. With respect to the sample format bit I tried to propose (PERF_SAMPLE_IDENTIFIER), I modified perf tools to check if the kernel supported it. Other applications that want to use PERF_SAMPLE_IDENTIFIER should take the same approach.
New sample format bits are added from time to time such as PERF_SAMPLE_WEIGHT and PERF_SAMPLE_DATA_SRC which were added in 3.10.
Quoted 149 lines
Quoted 147 lines
Cc Adrian Hunter <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>--- include/uapi/linux/perf_event.h | 18 +++++++++++++++++- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 33 insertions(+), 16 deletions(-)include/uapi/linux/perf_event.h +17
-1@@ -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: * @@ -498,6 +508,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 +519,7 @@ enum perf_event_type {* * u32 pid, tid; * char comm[]; +* struct sample_id sample_id; * }; */ PERF_RECORD_COMM = 3,@@ -518,6 +530,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_EXIT = 4,@@ -528,6 +541,7 @@ enum perf_event_type {* u64 time; * u64 id; * u64 stream_id; +* struct sample_id sample_id; * }; */ PERF_RECORD_THROTTLE = 5,@@ -539,6 +553,7 @@ enum perf_event_type {* u32 pid, ppid; * u32 tid, ptid; * u64 time; +* struct sample_id sample_id; * }; */ PERF_RECORD_FORK = 7,@@ -549,6 +564,7 @@ enum perf_event_type {* u32 pid, tid; * * struct read_format values; +* struct sample_id sample_id; * }; */ PERF_RECORD_READ = 8,@@ -596,7 +612,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@@ -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,
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 15 lines · Adrian Hunter
Quoted 9 lines · Vince Weaver
Quoted 7 lines · tip-bot for Peter Zijlstra
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200Quoted 4 lines
Quoted 4 lines
perf: Update
perf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.
Quoted 7 lines
...
This is not the patch that adds new sample format bits.
New sample format bits are always backward compatible in the sense that the kernel
perf_event_opensystem call will return an error if you try to use sample format bits that it does not know about.
Yes, I meant the addition of the sample_id fields to the existing sample values. Though if Peter is right and those have always been there since the beginning and just not documented then that won't break anything. The commit message wasn't really as clear as it could be.
Vince
Peter Zijlstra
maintainerRe: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 4 lines · Vince Weaver
Yes, I meant the addition of the
sample_idfields to the existing sample values. Though if Peter is right and those have always been there since the beginning and just not documented then that won't break anything. The commit message wasn't really as clear as it could be.
/me writes "I shall write better Changelogs." a hundred times.
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Vince Weaver
Yes, I meant the addition of the
sample_idfields to the existing sample values. Though if Peter is right and those have always been there since the beginning and just not documented then that won't break anything. The commit message wasn't really as clear as it could be./me writes "I shall write better Changelogs." a hundred times.
yes, sorry, I shouldn't have been so grumpy about it.
After spending 45 minutes digging through the kernel source and other documentation it all makes sense now. The existing manpage even does a mildly accurate job of explaining the situation. It's just I have enough trouble keeping up with the large number of recent perf related patches without having to spend a lot of time decoding cryptic Changelogs.
Vince
Peter Zijlstra
maintainerRe: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 11 lines · Vince Weaver
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Vince Weaver
Yes, I meant the addition of the
sample_idfields to the existing sample values. Though if Peter is right and those have always been there since the beginning and just not documented then that won't break anything. The commit message wasn't really as clear as it could be./me writes "I shall write better Changelogs." a hundred times.
yes, sorry, I shouldn't have been so grumpy about it.
I don't mind; I wish people would tell me my changelogs suck before they get committed though; I can still fix them at that point.
It not only helps others, but also myself. I've on many occasion kicked myself for not writing clearer changelogs when investigating something months (or years) after I wrote the initial code.
The problem is that I mostly know what I mean when I write it and I'm not very good at writing in the first place -- real changelogs often take longer to write than the actual code. But I guess that's true for many people.
Anyway.. I'm ever trying to write better changelogs, but alas there's also always far more room for improvement :-)
Re: [tip:perf/core] perf: Update perf_event_type documentation
In reply to Adrian HunterQuoted 9 lines · Adrian Hunter
Quoted 9 lines
Quoted 9 lines
include/uapi/linux/perf_event.h +1
-0@@ -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 {
Quoted 8 lines
Quoted 8 lines
include/uapi/linux/perf_event.h enum perf_event_type { · line 489
Quoted 8 lines
include/uapi/linux/perf_event.h +8
-0+* { 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 +*/ +
a thing that personally bothers me are these imaginary struct definitions added as part of the documentation that aren't actually available in the public perf_event.h
I can see why it's done, but it can be confusing picking out in later definitions which struct fields are real and which ones are conceptual.
Quoted 6 lines
Quoted 6 lines
include/uapi/linux/perf_event.h enum perf_event_type { · line 512
Quoted 6 lines
include/uapi/linux/perf_event.h +1
-0@@ -498,6 +508,7 @@ enum perf_event_type {* struct perf_event_header header; * u64 id; * u64 lost; +* struct sample_id sample_id; * };
This is what confused me; this documentation shows the sample_id as always being there, but in reality that struct is only there if perf_event_attr::sample_id_all is set.
It might be clearer
if you stuck the perf_event_attr::sample_id_all qualifier each place you add the sample_id field.
And it's nice that the header holds a size field which ensures backward compatability, I somehow had forgotten that with my initial complaint.
Vince
Peter Zijlstra
maintainerRe: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 25 lines · Vince Weaver
Quoted 9 lines · Adrian Hunter
Quoted 9 lines
Quoted 9 lines
include/uapi/linux/perf_event.h +1
-0@@ -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 { Quoted 8 lines
Quoted 8 lines
include/uapi/linux/perf_event.h enum perf_event_type { · line 489
Quoted 8 lines
include/uapi/linux/perf_event.h +8
-0+* { 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 +*/ +a thing that personally bothers me are these imaginary struct definitions added as part of the documentation that aren't actually available in the public
perf_event.h
Quoted 2 lines
I can see why it's done, but it can be confusing picking out in later definitions which struct fields are real and which ones are conceptual.
Would it help if we changed the syntax to not look as much as real C would?
Quoted 11 lines
Quoted 6 lines
Quoted 6 lines
include/uapi/linux/perf_event.h enum perf_event_type { · line 512
Quoted 6 lines
include/uapi/linux/perf_event.h +1
-0@@ -498,6 +508,7 @@ enum perf_event_type {* struct perf_event_header header; * u64 id; * u64 lost; +* struct sample_id sample_id; * }; This is what confused me; this documentation shows the
sample_idas always being there, but in reality that struct is only there ifperf_event_attr::sample_id_allis set.
Quoted 3 lines
It might be clearer
if you stuck theperf_event_attr::sample_id_allqualifier each place you add thesample_idfield.
Ah, I actually considered that but then got lazy and used the 0 sized struct idea :/
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines · Vince Weaver
a thing that personally bothers me are these imaginary struct definitions added as part of the documentation that aren't actually available in the public
perf_event.hI can see why it's done, but it can be confusing picking out in later definitions which struct fields are real and which ones are conceptual.
Would it help if we changed the syntax to not look as much as real C would?
I've been thinking and I can't really think of a clearer way to present
the layout. So I guess it's fine the way it is. Hopefully not many
people are stuck having to implement code based on header file comments anyway.
Quoted 6 lines
Quoted 3 lines
It might be clearer
if you stuck theperf_event_attr::sample_id_allqualifier each place you add thesample_idfield.Ah, I actually considered that but then got lazy and used the 0 sized struct idea :/
It might just be me. For whatever reason the C parser in my head doesn't handle GNU extensions like 0-sized structs.
Vince
Peter Zijlstra
maintainerRe: [tip:perf/core] perf: Update perf_event_type documentation
In reply to Vince WeaverQuoted 17 lines · Vince Weaver
Quoted 11 lines · tip-bot for Peter Zijlstra
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200perf: Update
perf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.This code makes a lot of code changes considering it is "updating documentation".
Quoted 6 lines
Also, will code following this "documentation" be backward compatible?
Meaning, if you have code that depends on the new fields you've added and run on older kernels, will you get sane results? Or will the code get garbage and/or segfault in the
sample_idfields because you read past the end of valid data?
I didn't add any fields, this is simply catching up with what was already there. But yes, this is very much intended to be compatible.
Quoted 48 lines
Quoted 48 lines
kernel/events/core.c +9
-15@@ -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);
Quoted 8 lines
kernel/events/core.c void perf_output_sample(struct perf_output_handle *handle, · line 4525
Quoted 8 lines
kernel/events/core.c +7
-0+ +if (events >= wakeup_events) { +local_sub (wakeup_events, &rb->events); +local_inc (&rb->wakeup); + } + } + } }
I suppose you're 'complaining' about these changes? Yeah, maybe I should have split them up in a separate patch.
I moved the watermark code from somewhere in the middle of actually doing the output to the end (where it once was) and added a 'missing' pair of curlies.
I noticed both oddities when looking at the code to verify the output format which was updated in the other hunks.
It should be a NOP functionality wise.
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 2 lines · Peter Zijlstra
I didn't add any fields, this is simply catching up with what was already there. But yes, this is very much intended to be compatible.
So those id fields have been there since 2.6.31 and just no one noticed the header file comments were wrong? That's fine then, I just have a lot of code and other documentation that needs to be updated then.
Quoted 6 lines
I suppose you're 'complaining' about these changes? Yeah, maybe I should have split them up in a separate patch.
...
It should be a NOP functionality wise.
yes, it looks like a NOP. I was just surprised to see code changes (even trivial ones) when the commit message only said documentation update.
Vince
Re: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 8 lines · Vince Weaver
Quoted 2 lines · Peter Zijlstra
I didn't add any fields, this is simply catching up with what was already there. But yes, this is very much intended to be compatible.
So those id fields have been there since 2.6.31 and just no one noticed the header file comments were wrong? That's fine then, I just have a lot of code and other documentation that needs to be updated then.
I believe those fields came in with the sample_id_all patch - c980d1091810df13f21aabbce545fd98f545bbf7 - which is 2.6.37.
David
Re: [tip:perf/core] perf: Update perf_event_type documentation
In reply to tip-bot for Peter ZijlstraQuoted 43 lines · tip-bot for Peter Zijlstra
Commit-ID: a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Gitweb: http://git.kernel.org/tip/a5cdd40c9877e9aba704c020fd65d26b5cfecf18 Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 16 Jul 2013 17:09:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 23 Jul 2013 12:17:08 +0200perf: Update
perf_event_typedocumentationDue to a discussion with Adrian I had a good look at the
perf_event_typerecord layout and found the documentation to be somewhat unclear.Cc Adrian Hunter <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Signed-off-by Ingo Molnar <[email protected]>--- include/uapi/linux/perf_event.h | 18 +++++++++++++++++- kernel/events/core.c | 31 ++++++++++++++++--------------- 2 files changed, 33 insertions(+), 16 deletions(-)include/uapi/linux/perf_event.h +10
-0@@ -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: *
include/uapi/linux/perf_event.h enum perf_event_type { · line 514
Quoted 7 lines
include/uapi/linux/perf_event.h +1
-0@@ -498,6 +508,7 @@ enum perf_event_type {* struct perf_event_header header; * u64 id; * u64 lost; +* struct sample_id sample_id; * }; */ PERF_RECORD_LOST = 2,
So in Adrian Hunter's posted patches the PERF_RECORD_MMAP documentation is also patched like all the others, with
struct sample_id sample_id;
But in this tip message (and in the current linus-git kernel) somehow the PERF_RECORD_MMAP line of the patch was dropped.
Was that intentional?
I'm trying to document this mess in the manpage, not fun.
Vince
Peter Zijlstra
maintainerRe: [tip:perf/core] perf: Update perf_event_type documentation
Quoted 9 lines · Vince Weaver
So in Adrian Hunter's posted patches the
PERF_RECORD_MMAPdocumentation is also patched like all the others, withstruct sample_id sample_id;But in this tip message (and in the current linus-git kernel) somehow the
PERF_RECORD_MMAPline of the patch was dropped.Was that intentional?
No, no idea what happened..
---
Subject: perf: Update ABI comment
From: Peter Zijlstra <[email protected]>
Date: Fri Sep 13 23:39:17 CEST 2013
For some mysterious reason the sample_id field of PERF_RECORD_MMAP went AWOL.
--- include/uapi/linux/perf_event.h | 1 + 1 file changed, 1 insertion(+)
include/uapi/linux/perf_event.h +1 -0
@@ -528,6 +528,7 @@ enum perf_event_type {* u64 len; * u64 pgoff; * char filename[]; +* struct sample_id sample_id; * }; */ PERF_RECORD_MMAP = 1,
Adrian Hunter
reviewerPATCH v5 10/12 perf tools: add support for PERF_SAMPLE_IDENTFIER
In reply to Adrian HunterEnable 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 | 94 +++++++++++++++++++++++++++++++++++++++++-- tools/perf/util/evlist.h | 1 + tools/perf/util/evsel.c | 41 +++++++++++++++---- tools/perf/util/evsel.h | 3 +- 6 files changed, 134 insertions(+), 16 deletions(-)
tools/perf/tests/mmap-basic.c +1 -1
@@ -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
@@ -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 +90 -4
120 lines not on this page · raw message
tools/perf/util/evlist.h +1 -0
@@ -88,6 +88,7 @@ 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); +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
@@ -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);
Signature · 1 line
1.7.11.7
Adrian Hunter
reviewerPATCH v5 11/12 perf tools: expand perf_event__synthesize_sample()
In reply to Adrian HunterExpand 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
@@ -307,7 +307,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
@@ -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
Adrian Hunter
reviewerPATCH v5 12/12 perf tools: add a sample parsing test
In reply to Adrian HunterAdd 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 | 231 ++++++++++++++++++++++++++++++++++++++ tools/perf/tests/tests.h | 1 + 4 files changed, 237 insertions(+) create mode 100644 tools/perf/tests/sample-parsing.c
tools/perf/Makefile +1 -0
@@ -389,6 +389,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
@@ -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 +231 -0
231 lines not on this page · raw message
tools/perf/tests/tests.h +1 -0
@@ -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
Re: [PATCH V5 12/12] perf tools: add a sample parsing test
Quoted 2 lines · Adrian Hunter
Add a test that checks that sample parsing is correctly implemented.
nice!!! :)
Quoted 2 lines
Signed-off-by Adrian Hunter <[email protected]>
SNIP
Quoted 5 lines
(quoted diff) +5
-0+static int do_test(u64 sample_type, u64 sample_regs_user) +{ + struct perf_evsel evsel = { + .needs_swap = false, + .attr = {
Review
Quoted 10 lines
(quoted diff) +10
-0+ .sample_type = sample_type, + .sample_regs_user = sample_regs_user, + }, + }; + union perf_event event = { + .header = { + .type = PERF_RECORD_SAMPLE, + .size = sizeof(union perf_event), + }, + };
maybe I'm missing something but how does event.sample.array get allocated? It's used in perf_event__synthesize_sample func.
do we need to use something like in the patch below..
thanks,
jirka
---
tools/perf/tests/sample-parsing.c +9 -8
@@ -111,12 +111,8 @@ static int do_test(u64 sample_type, u64 sample_regs_user) .sample_regs_user = sample_regs_user, }, }; -union perf_event event = { - .header = { - .type =PERF_RECORD_SAMPLE , - .size =sizeof (union perf_event ), - }, - }; +u64 buf[100 ]; +union perf_event *event = (union perf_event *) buf;union {struct ip_callchain callchain;u64 data[64 ];
@@ -162,10 +158,15 @@ static int do_test(u64 sample_type, u64 sample_regs_user)size_t i;int err; + event->sample.header = (struct perf_event_header ) { + .type =PERF_RECORD_SAMPLE , + .size =sizeof (union perf_event ), + }; +for (i =0 ; i <sizeof (user_regs); i++) *(u8 *)user_regs = i; - err =perf_event__synthesize_sample (&event, sample_type, + err =perf_event__synthesize_sample (event, sample_type, sample_regs_user, &sample, false);if (err) { pr_debug("%s failed for sample_type %#"PRIx64", error %d\n",
@@ -175,7 +176,7 @@ static int do_test(u64 sample_type, u64 sample_regs_user) evsel.sample_size =__perf_evsel__sample_size (sample_type); - err =perf_evsel__parse_sample (&evsel, &event, &sample_out); + err =perf_evsel__parse_sample (&evsel, event, &sample_out);if (err) {pr_debug ("%s failed for sample_type %#" PRIx64", error %d\n" ,"perf_evsel__parse_sample" , sample_type, err);
Adrian Hunter
reviewerRe: [PATCH V5 12/12] perf tools: add a sample parsing test
Quoted 21 lines · Jiri Olsa
Quoted 2 lines · Adrian Hunter
Add a test that checks that sample parsing is correctly implemented.
nice!!! :)
Quoted 2 lines
Signed-off-by Adrian Hunter <[email protected]>SNIP
Quoted 10 lines
(quoted diff) +10
-0+static int do_test(u64 sample_type, u64 sample_regs_user) +{ + struct perf_evsel evsel = { + .needs_swap = false, + .attr = { + .sample_type = sample_type, + .sample_regs_user = sample_regs_user, + }, + }; + union perf_event event = {
Quoted 8 lines
tools/perf/tests/sample-parsing.c static int do_test(u64 sample_type, u64 sample_regs_user) · line 164
Quoted 5 lines
tools/perf/tests/sample-parsing.c +5
-0+ .header = { + .type = PERF_RECORD_SAMPLE, + .size = sizeof(union perf_event), + }, + };maybe I'm missing something but how does event.sample.array get allocated? It's used in
perf_event__synthesize_samplefunc.
'union perf_event' is very big because of 'struct mmap_event' which contains 'char filename[PATH_MAX]' and PATH_MAX is 4096.
Quoted 2 lines
do we need to use something like in the patch below..
Might as well calculate it correctly.
Quoted 52 lines
thanks,
jirka---
tools/perf/tests/sample-parsing.c +9
-8@@ -111,12 +111,8 @@ static int do_test(u64 sample_type, u64 sample_regs_user) .sample_regs_user = sample_regs_user, }, }; -union perf_event event = { - .header = { - .type =PERF_RECORD_SAMPLE , - .size =sizeof (union perf_event ), - }, - }; +u64 buf[100 ]; +union perf_event *event = (union perf_event *) buf;union {struct ip_callchain callchain;u64 data[64 ];@@ -162,10 +158,15 @@ static int do_test(u64 sample_type, u64 sample_regs_user)size_t i;int err; + event->sample.header = (struct perf_event_header ) { + .type =PERF_RECORD_SAMPLE , + .size =sizeof (union perf_event ), + }; +for (i =0 ; i <sizeof (user_regs); i++) *(u8 *)user_regs = i; - err =perf_event__synthesize_sample (&event, sample_type, + err =perf_event__synthesize_sample (event, sample_type, sample_regs_user, &sample, false);if (err) { pr_debug("%s failed for sample_type %#"PRIx64", error %d\n",@@ -175,7 +176,7 @@ static int do_test(u64 sample_type, u64 sample_regs_user) evsel.sample_size =__perf_evsel__sample_size (sample_type); - err =perf_evsel__parse_sample (&evsel, &event, &sample_out); + err =perf_evsel__parse_sample (&evsel, event, &sample_out);if (err) {pr_debug ("%s failed for sample_type %#" PRIx64", error %d\n" ,"perf_evsel__parse_sample" , sample_type, err);