Re: [PATCH 05/16] perf tools: Add facility to export data in database-friendly way
From: Arnaldo Carvalho de Melo
Date: Fri Oct 24 2014 - 10:41:31 EST
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@xxxxxxxxx> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/