Messages in this thread Patch in this message | | | Date | Fri, 24 Oct 2014 11:41:16 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 05/16] perf tools: Add facility to export data in database-friendly way |
| |
Em Fri, Oct 24, 2014 at 11:40:02AM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Oct 24, 2014 at 04:13:00PM +0300, Adrian Hunter escreveu: > > On 24/10/14 15:26, Namhyung Kim wrote: > > > Hi Adrian, > > > > > > On Fri, Oct 24, 2014 at 7:47 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > > >> On 24/10/14 11:11, Adrian Hunter wrote: > > >>> On 24/10/14 09:02, Namhyung Kim wrote: > > >>>> Can we do it somewhere in a script not in the core code? I don't feel > > >>>> comfortable to add those bits into the core code. What if we export > > >>> > > >>> Please explain what you mean by "comfortable". > > >> > > >> Or rather: What about it is wrong for core code? > > > > > > Well, there's nothing "wrong" about it. :) > > > > > > But I think if some code is used only by a specific bits, it'd be > > > better hiding it from the rest as much as possible. > > > > It is pretty self-contained in db-export.[ch] and the scripting > > engines. > > So, what I meant was the patch below, on top of yours. > > In some data structures where there were no provision for tool specific > storage, I added an unnamed union with db_id and a void *priv pointer, > where there were, be it a priv pointer, like in perf_evsel, I moved > db_id to it, no extra costs for the other tools. > > And in struct symbol, I made it use symbol__priv(), that will access the > area reserved by setting symbol.priv_size at tool start time, like done > for the annotation case in top, report, annotate. I.e. on the tool that > will use symbol->db_id you will need to add this just before calling > symbol__init(): > > symbol_conf.priv_size = sizeof(u64); > > Since this is all specific to your tool, there should be no problems and > I'm now applying this change merged with your original patch + note > about that and then moving on to the other patches that make use of it.
Oops, now with the patch:
diff --git a/tools/perf/util/comm.h b/tools/perf/util/comm.h index 99e7021..71c9c39 100644 --- a/tools/perf/util/comm.h +++ b/tools/perf/util/comm.h @@ -10,9 +10,12 @@ struct comm_str; struct comm { struct comm_str *comm_str; u64 start; - u64 db_id; struct list_head list; bool exec; + union { /* Tool specific area */ + void *priv; + u64 db_id; + }; }; void comm__free(struct comm *comm); diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c index 53d0e75..be128b0 100644 --- a/tools/perf/util/db-export.c +++ b/tools/perf/util/db-export.c @@ -150,10 +150,12 @@ int db_export__dso(struct db_export *dbe, struct dso *dso, int db_export__symbol(struct db_export *dbe, struct symbol *sym, struct dso *dso) { - if (sym->db_id) + u64 *sym_db_id = symbol__priv(sym); + + if (*sym_db_id) return 0; - sym->db_id = ++dbe->symbol_last_db_id; + *sym_db_id = ++dbe->symbol_last_db_id; if (dbe->export_symbol) return dbe->export_symbol(dbe, sym, dso); @@ -161,8 +163,7 @@ int db_export__symbol(struct db_export *dbe, struct symbol *sym, return 0; } -static struct thread *get_main_thread(struct machine *machine, - struct thread *thread) +static struct thread *get_main_thread(struct machine *machine, struct thread *thread) { if (thread->pid_ == thread->tid) return thread; @@ -194,10 +195,12 @@ static int db_ids_from_al(struct db_export *dbe, struct addr_location *al, } if (al->sym) { + u64 *db_id = symbol__priv(al->sym); + err = db_export__symbol(dbe, al->sym, dso); if (err) return err; - *sym_db_id = al->sym->db_id; + *sym_db_id = *db_id; *offset = al->addr - al->sym->start; } } @@ -253,8 +256,7 @@ int db_export__sample(struct db_export *dbe, union perf_event *event, sample_addr_correlates_sym(&evsel->attr)) { struct addr_location addr_al; - perf_event__preprocess_sample_addr(event, sample, al->machine, - thread, &addr_al); + perf_event__preprocess_sample_addr(event, sample, thread, &addr_al); err = db_ids_from_al(dbe, &addr_al, &es.addr_dso_db_id, &es.addr_sym_db_id, &es.addr_offset); if (err) diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index 66c99cc..a316e4a 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -111,7 +111,6 @@ struct dso { enum dso_swap_type needs_swap; enum dso_binary_type symtab_type; enum dso_binary_type binary_type; - u64 db_id; u8 adjust_symbols:1; u8 has_build_id:1; u8 has_srcline:1; @@ -140,6 +139,11 @@ struct dso { struct list_head open_entry; } data; + union { /* Tool specific area */ + void *priv; + u64 db_id; + }; + char name[0]; }; diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 9459899..d3854c4 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -54,6 +54,7 @@ struct cgroup_sel; * @is_pos: the position (counting backwards) of the event id (PERF_SAMPLE_ID or * PERF_SAMPLE_IDENTIFIER) in a non-sample event i.e. if sample_id_all * is used there is an id sample appended to non-sample events + * @priv: And what is in its containing unnamed union are tool specific */ struct perf_evsel { struct list_head node; @@ -73,6 +74,7 @@ struct perf_evsel { union { void *priv; off_t id_offset; + u64 db_id; }; struct cgroup_sel *cgrp; void *handler; @@ -92,7 +94,6 @@ struct perf_evsel { int sample_read; struct perf_evsel *leader; char *group_name; - u64 db_id; }; union u64_swap { diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h index 2e150a2..e8b7779 100644 --- a/tools/perf/util/machine.h +++ b/tools/perf/util/machine.h @@ -40,7 +40,10 @@ struct machine { u64 kernel_start; symbol_filter_t symbol_filter; pid_t *current_tid; - u64 db_id; + union { /* Tool specific area */ + void *priv; + u64 db_id; + }; }; static inline diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h index 6f54ade..eb2c19b 100644 --- a/tools/perf/util/symbol.h +++ b/tools/perf/util/symbol.h @@ -73,7 +73,6 @@ struct symbol { struct rb_node rb_node; u64 start; u64 end; - u64 db_id; u16 namelen; u8 binding; bool ignore;
| |