Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752901Ab3FYLXi (ORCPT ); Tue, 25 Jun 2013 07:23:38 -0400 Received: from mail-ob0-f180.google.com ([209.85.214.180]:46829 "EHLO mail-ob0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751831Ab3FYLXg (ORCPT ); Tue, 25 Jun 2013 07:23:36 -0400 MIME-Version: 1.0 In-Reply-To: <1372079772-20803-13-git-send-email-adrian.hunter@intel.com> References: <1372079772-20803-1-git-send-email-adrian.hunter@intel.com> <1372079772-20803-13-git-send-email-adrian.hunter@intel.com> Date: Tue, 25 Jun 2013 13:23:36 +0200 Message-ID: Subject: Re: [PATCH 12/15] perf tools: allow non-matching sample types From: Stephane Eranian To: Adrian Hunter Cc: Arnaldo Carvalho de Melo , LKML , David Ahern , Frederic Weisbecker , Jiri Olsa , Mike Galbraith , Namhyung Kim , Paul Mackerras , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11127 Lines: 316 On Mon, Jun 24, 2013 at 3:16 PM, Adrian Hunter wrote: > Sample types need not be identical to determine > the sample id from the event. Only the position > of the sample id needs to be the same. > > Compatible sample types are ones in which the bits > defined by PERF_COMPAT_MASK are the same. > 'perf_evlist__config()' forces sample types to be > compatible on that basis. > This is indeed a major flaw of the current sampling buffer format. I have a patch coming to address this from the kernel side. I am trying to understand this patch and I am confused by the description and especially the structure of COMPAT_MASK. I agree that if the SAMPLE_ID position remains constant then it can be extracted from the body of the sample uniformly. The only way to guarantee a fixed position is by ensuring that all the sample_types before SAMPLE_ID and either set or unset. By before I don't mean in the enum order but in the order in which the kernel lays them various sample_types in the buffer. And that's determined by perf_output_sample(). So I don't understand why PERF_SAMPLE_CPU and PERF_SAMPLE_STREAM_ID are here. Any explanation? > Signed-off-by: Adrian Hunter > --- > tools/perf/util/event.h | 6 ++++ > tools/perf/util/evlist.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++-- > tools/perf/util/evlist.h | 3 ++ > tools/perf/util/evsel.c | 41 +++++++++++++++++++++ > tools/perf/util/evsel.h | 4 +++ > 5 files changed, 145 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > index 1813895..858572f 100644 > --- a/tools/perf/util/event.h > +++ b/tools/perf/util/event.h > @@ -65,6 +65,12 @@ struct read_event { > PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID | \ > PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD) > > +#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[]; > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index a660f56..85c4d91 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -49,10 +49,20 @@ struct perf_evlist *perf_evlist__new(void) > return evlist; > } > > +static void perf_evlist__set_id_pos(struct perf_evlist *evlist) > +{ > + struct perf_evsel *first = perf_evlist__first(evlist); > + > + evlist->id_pos = first->id_pos; > + evlist->is_pos = first->is_pos; > +} > + > void perf_evlist__config(struct perf_evlist *evlist, > struct perf_record_opts *opts) > { > struct perf_evsel *evsel; > + u64 compat = 0; > + > /* > * Set the evsel leader links before we configure attributes, > * since some might depend on this info. > @@ -68,7 +78,15 @@ void perf_evlist__config(struct perf_evlist *evlist, > > if (evlist->nr_entries > 1) > perf_evsel__set_sample_id(evsel); > + compat |= evsel->attr.sample_type & PERF_COMPAT_MASK; > } > + > + list_for_each_entry(evsel, &evlist->entries, node) { > + evsel->attr.sample_type |= compat; > + perf_evsel__calc_id_pos(evsel); > + } > + > + perf_evlist__set_id_pos(evlist); > } > > static void perf_evlist__purge(struct perf_evlist *evlist) > @@ -102,6 +120,7 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) > { > list_add_tail(&entry->node, &evlist->entries); > ++evlist->nr_entries; > + perf_evlist__set_id_pos(evlist); > } > > void perf_evlist__splice_list_tail(struct perf_evlist *evlist, > @@ -110,6 +129,7 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist, > { > list_splice_tail(list, &evlist->entries); > evlist->nr_entries += nr_entries; > + perf_evlist__set_id_pos(evlist); > } > > void __perf_evlist__set_leader(struct list_head *list) > @@ -339,6 +359,55 @@ struct perf_evsel *perf_evlist__id2evsel(struct perf_evlist *evlist, u64 id) > return NULL; > } > > +static int perf_evlist__event2id(struct perf_evlist *evlist, > + union perf_event *event, u64 *id) > +{ > + const u64 *array = event->sample.array; > + ssize_t n; > + > + n = (event->header.size - sizeof(event->header)) >> 3; > + > + if (event->header.type == PERF_RECORD_SAMPLE) { > + if (evlist->id_pos >= n) > + return -1; > + *id = array[evlist->id_pos]; > + } else { > + if (evlist->is_pos >= n) > + return -1; > + n -= evlist->is_pos; > + *id = array[n]; > + } > + return 0; > +} > + > +static struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist, > + union perf_event *event) > +{ > + struct hlist_head *head; > + struct perf_sample_id *sid; > + int hash; > + u64 id; > + > + if (evlist->nr_entries == 1 || evlist->matching_sample_types) > + return perf_evlist__first(evlist); > + > + if (perf_evlist__event2id(evlist, event, &id)) > + return NULL; > + > + /* Synthesized events have an id of zero */ > + if (!id) > + return perf_evlist__first(evlist); > + > + hash = hash_64(id, PERF_EVLIST__HLIST_BITS); > + head = &evlist->heads[hash]; > + > + hlist_for_each_entry(sid, head, node) { > + if (sid->id == id) > + return sid->evsel; > + } > + return NULL; > +} > + > union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx) > { > struct perf_mmap *md = &evlist->mmap[idx]; > @@ -650,9 +719,26 @@ int perf_evlist__set_filter(struct perf_evlist *evlist, const char *filter) > bool perf_evlist__valid_sample_type(struct perf_evlist *evlist) > { > struct perf_evsel *first = perf_evlist__first(evlist), *pos = first; > + bool ok = true; > > list_for_each_entry_continue(pos, &evlist->entries, node) { > - if (first->attr.sample_type != pos->attr.sample_type) > + if (first->attr.sample_type != pos->attr.sample_type) { > + ok = false; > + break; > + } > + } > + > + if (ok) { > + evlist->matching_sample_types = true; > + return true; > + } > + > + if (evlist->id_pos < 0 || evlist->is_pos < 0) > + return false; > + > + list_for_each_entry(pos, &evlist->entries, node) { > + if (pos->id_pos != evlist->id_pos || > + pos->is_pos != evlist->is_pos) > return false; > } > > @@ -848,7 +934,10 @@ int perf_evlist__start_workload(struct perf_evlist *evlist) > int perf_evlist__parse_sample(struct perf_evlist *evlist, union perf_event *event, > struct perf_sample *sample) > { > - struct perf_evsel *evsel = perf_evlist__first(evlist); > + struct perf_evsel *evsel = perf_evlist__event2evsel(evlist, event); > + > + if (!evsel) > + return -EFAULT; > return perf_evsel__parse_sample(evsel, event, sample); > } > > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h > index 0583d36..bfcbf67 100644 > --- a/tools/perf/util/evlist.h > +++ b/tools/perf/util/evlist.h > @@ -32,11 +32,14 @@ struct perf_evlist { > int nr_fds; > int nr_mmaps; > int mmap_len; > + int id_pos; > + int is_pos; > 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; > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index d01d2cd..ee0f894 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -46,6 +46,46 @@ static int __perf_evsel__sample_size(u64 sample_type) > return size; > } > > +static int __perf_evsel__calc_id_pos(u64 sample_type) > +{ > + u64 mask; > + int i, idx; > + > + if (!(sample_type & PERF_SAMPLE_ID)) > + return -1; > + > + mask = sample_type & (PERF_SAMPLE_ID - 1); > + > + for (i = 0, idx = 0; i < 64; i++) { > + if (mask & (1ULL << i)) > + idx++; > + } > + > + return idx; > +} > + > +static int __perf_evsel__calc_is_pos(u64 sample_type) > +{ > + int idx = 1; > + > + if (!(sample_type & PERF_SAMPLE_ID)) > + return -1; > + > + if (sample_type & PERF_SAMPLE_CPU) > + idx += 1; > + > + if (sample_type & PERF_SAMPLE_STREAM_ID) > + idx += 1; > + > + return idx; > +} > + > +void perf_evsel__calc_id_pos(struct perf_evsel *evsel) > +{ > + evsel->id_pos = __perf_evsel__calc_id_pos(evsel->attr.sample_type); > + evsel->is_pos = __perf_evsel__calc_is_pos(evsel->attr.sample_type); > +} > + > void hists__init(struct hists *hists) > { > memset(hists, 0, sizeof(*hists)); > @@ -89,6 +129,7 @@ void perf_evsel__init(struct perf_evsel *evsel, > INIT_LIST_HEAD(&evsel->node); > hists__init(&evsel->hists); > evsel->sample_size = __perf_evsel__sample_size(attr->sample_type); > + perf_evsel__calc_id_pos(evsel); > } > > struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr, int idx) > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 3f156cc..88b4319 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -71,6 +71,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 +102,8 @@ void perf_evsel__delete(struct perf_evsel *evsel); > void perf_evsel__config(struct perf_evsel *evsel, > struct perf_record_opts *opts); > > +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 > -- > 1.7.11.7 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/