| Message ID | 1339232758-920-1-git-send-email-dmitry.antipov@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, On Sat, 9 Jun 2012 13:05:58 +0400, Dmitry Antipov wrote: > Use new function trace_find_event_by_name to lookup events before > looking through /sys files. This helps 'perf sched replay' to map > event names to IDs correctly when processing perf.data recorded > on another machine. > Basically the same approach with the previous reply, please put this into trace_event__id(). And minor nits below.. > Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org> > --- > tools/perf/util/evlist.c | 18 ++++++++++++++++-- > tools/perf/util/trace-event-parse.c | 4 ++++ > tools/perf/util/trace-event.h | 1 + > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 4ac5f5a..7ebb9c5 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -17,6 +17,7 @@ > #include <unistd.h> > > #include "parse-events.h" > +#include "trace-event.h" > > #include <sys/mman.h> > > @@ -231,12 +232,25 @@ int perf_evlist__set_tracepoints_handlers(struct perf_evlist *evlist, > const struct perf_evsel_str_handler *assocs, > size_t nr_assocs) > { > + struct event_format *event; > struct perf_evsel *evsel; > + char *p, *sys, *name; > int err; > - size_t i; > + size_t i, off; > > for (i = 0; i < nr_assocs; i++) { > - err = trace_event__id(assocs[i].name); > + err = -ENOENT; > + p = strchr(assocs[i].name, ':'); > + if (!p) > + goto out; Is this really needed? It looks original trace_event__id() require this. But because we'll use pevent_find_event_by_name, the 'sys' part can be omitted from now on? > + off = p - assocs[i].name; > + sys = malloc(off + 1); The malloc() can fail, please check the return value. Thanks, Namhyung > + memcpy(sys, assocs[i].name, off); > + sys[off] = '\0'; > + name = p + 1; > + event = trace_find_event_by_name(sys, name); > + err = event ? event->id : trace_event__id(assocs[i].name); > + free(sys); > if (err < 0) > goto out; > > diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c > index df2fddb..44cbb40 100644 > --- a/tools/perf/util/trace-event-parse.c > +++ b/tools/perf/util/trace-event-parse.c > @@ -176,6 +176,10 @@ struct event_format *trace_find_event(int type) > return pevent_find_event(pevent, type); > } > > +struct event_format *trace_find_event_by_name(const char *sys, const char *name) > +{ > + return pevent_find_event_by_name(pevent, sys, name); > +} > > void print_trace_event(int cpu, void *data, int size) > { > diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h > index 639852a..66f83a0 100644 > --- a/tools/perf/util/trace-event.h > +++ b/tools/perf/util/trace-event.h > @@ -40,6 +40,7 @@ int parse_event_file(char *buf, unsigned long size, char *sys); > > struct pevent_record *trace_peek_data(int cpu); > struct event_format *trace_find_event(int type); > +struct event_format *trace_find_event_by_name(const char *sys, const char *name); > > unsigned long long > raw_field_value(struct event_format *event, const char *name, void *data);
Em Mon, Jun 11, 2012 at 02:46:02PM +0900, Namhyung Kim escreveu: > On Sat, 9 Jun 2012 13:05:58 +0400, Dmitry Antipov wrote: > > Use new function trace_find_event_by_name to lookup events before > > looking through /sys files. This helps 'perf sched replay' to map > > event names to IDs correctly when processing perf.data recorded > > on another machine. > > Basically the same approach with the previous reply, please put this > into trace_event__id(). And minor nits below.. Well, trace_event__id() is private to evlist and evlist so far is a local thing, i.e. it doesn't know anything about perf.data files. So I think we should have a per perf.data (perf_session) method that knows that it shouldn't look _at all_ to /sys, but just at what came in the perf.data file. As well when we want something that is on the running machine, even if we're dealing somehow with a perf.data file, we shouldn't use what is in it. - Arnaldo
Hi, On Mon, 11 Jun 2012 11:08:52 -0300, Arnaldo Carvalho de Melo wrote: > Em Mon, Jun 11, 2012 at 02:46:02PM +0900, Namhyung Kim escreveu: >> On Sat, 9 Jun 2012 13:05:58 +0400, Dmitry Antipov wrote: >> > Use new function trace_find_event_by_name to lookup events before >> > looking through /sys files. This helps 'perf sched replay' to map >> > event names to IDs correctly when processing perf.data recorded >> > on another machine. >> >> Basically the same approach with the previous reply, please put this >> into trace_event__id(). And minor nits below.. > > Well, trace_event__id() is private to evlist and evlist so far is a > local thing, i.e. it doesn't know anything about perf.data files. > Really? I see that perf_session__open make up an evlist for the session and a tracepoint event in the evlist should look up the perf.data first. As this patch addressed, perf sched replay dealt with the session->evlist already. Am I missing something? > So I think we should have a per perf.data (perf_session) method that > knows that it shouldn't look _at all_ to /sys, but just at what came in > the perf.data file. > Fair enough. The method should be a simple wrapper to libtraceevent APIs like this patch. > As well when we want something that is on the running machine, even if > we're dealing somehow with a perf.data file, we shouldn't use what is in > it. > That's the current behavior of the trace_event__id(). Do you want to make it public? Thanks, Namhyung
Em Tue, Jun 12, 2012 at 03:01:26PM +0900, Namhyung Kim escreveu: > Hi, > > On Mon, 11 Jun 2012 11:08:52 -0300, Arnaldo Carvalho de Melo wrote: > > Em Mon, Jun 11, 2012 at 02:46:02PM +0900, Namhyung Kim escreveu: > >> On Sat, 9 Jun 2012 13:05:58 +0400, Dmitry Antipov wrote: > >> > Use new function trace_find_event_by_name to lookup events before > >> > looking through /sys files. This helps 'perf sched replay' to map > >> > event names to IDs correctly when processing perf.data recorded > >> > on another machine. > >> > >> Basically the same approach with the previous reply, please put this > >> into trace_event__id(). And minor nits below.. > > > > Well, trace_event__id() is private to evlist and evlist so far is a > > local thing, i.e. it doesn't know anything about perf.data files. > > > > Really? I see that perf_session__open make up an evlist for the session > and a tracepoint event in the evlist should look up the perf.data > first. As this patch addressed, perf sched replay dealt with the > session->evlist already. Am I missing something? I sent a patch fixing it, basically after creating the evlist in perf_session__open it will traverse it, looking up the pevents list created while processing the trace feature section in the header, setting up evsel->name properly. > > So I think we should have a per perf.data (perf_session) method that > > knows that it shouldn't look _at all_ to /sys, but just at what came in > > the perf.data file. > > > > Fair enough. The method should be a simple wrapper to libtraceevent APIs > like this patch. Right, that is what it does. > > > As well when we want something that is on the running machine, even if > > we're dealing somehow with a perf.data file, we shouldn't use what is in > > it. > > > > That's the current behavior of the trace_event__id(). Do you want to > make it public? No need for it, the only users should be inside evsel.c. - Arnaldo
Em Sat, Jun 09, 2012 at 01:05:58PM +0400, Dmitry Antipov escreveu: > Use new function trace_find_event_by_name to lookup events before > looking through /sys files. This helps 'perf sched replay' to map > event names to IDs correctly when processing perf.data recorded > on another machine. > > Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org> > --- > tools/perf/util/evlist.c | 18 ++++++++++++++++-- > tools/perf/util/trace-event-parse.c | 4 ++++ > tools/perf/util/trace-event.h | 1 + > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 4ac5f5a..7ebb9c5 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -17,6 +17,7 @@ > #include <unistd.h> > > #include "parse-events.h" > +#include "trace-event.h" > > #include <sys/mman.h> > > @@ -231,12 +232,25 @@ int perf_evlist__set_tracepoints_handlers(struct perf_evlist *evlist, > const struct perf_evsel_str_handler *assocs, > size_t nr_assocs) > { > + struct event_format *event; > struct perf_evsel *evsel; > + char *p, *sys, *name; > int err; > - size_t i; > + size_t i, off; > > for (i = 0; i < nr_assocs; i++) { > - err = trace_event__id(assocs[i].name); > + err = -ENOENT; > + p = strchr(assocs[i].name, ':'); > + if (!p) > + goto out; > + off = p - assocs[i].name; > + sys = malloc(off + 1); malloc can fail, so testing it is needed here. Also I think using just strdup(assocs[i].name), then looking at the separators and then inline setting them to '\0', etc would be simpler, no? > + memcpy(sys, assocs[i].name, off); > + sys[off] = '\0'; > + name = p + 1; > + event = trace_find_event_by_name(sys, name); > + err = event ? event->id : trace_event__id(assocs[i].name); I'll try and cook an alternate patch fixing this problem. > + free(sys); > if (err < 0) > goto out; > > diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c > index df2fddb..44cbb40 100644 > --- a/tools/perf/util/trace-event-parse.c > +++ b/tools/perf/util/trace-event-parse.c > @@ -176,6 +176,10 @@ struct event_format *trace_find_event(int type) > return pevent_find_event(pevent, type); > } > > +struct event_format *trace_find_event_by_name(const char *sys, const char *name) > +{ > + return pevent_find_event_by_name(pevent, sys, name); > +} > > void print_trace_event(int cpu, void *data, int size) > { > diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h > index 639852a..66f83a0 100644 > --- a/tools/perf/util/trace-event.h > +++ b/tools/perf/util/trace-event.h > @@ -40,6 +40,7 @@ int parse_event_file(char *buf, unsigned long size, char *sys); > > struct pevent_record *trace_peek_data(int cpu); > struct event_format *trace_find_event(int type); > +struct event_format *trace_find_event_by_name(const char *sys, const char *name); > > unsigned long long > raw_field_value(struct event_format *event, const char *name, void *data); > -- > 1.7.7.6
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 4ac5f5a..7ebb9c5 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -17,6 +17,7 @@ #include <unistd.h> #include "parse-events.h" +#include "trace-event.h" #include <sys/mman.h> @@ -231,12 +232,25 @@ int perf_evlist__set_tracepoints_handlers(struct perf_evlist *evlist, const struct perf_evsel_str_handler *assocs, size_t nr_assocs) { + struct event_format *event; struct perf_evsel *evsel; + char *p, *sys, *name; int err; - size_t i; + size_t i, off; for (i = 0; i < nr_assocs; i++) { - err = trace_event__id(assocs[i].name); + err = -ENOENT; + p = strchr(assocs[i].name, ':'); + if (!p) + goto out; + off = p - assocs[i].name; + sys = malloc(off + 1); + memcpy(sys, assocs[i].name, off); + sys[off] = '\0'; + name = p + 1; + event = trace_find_event_by_name(sys, name); + err = event ? event->id : trace_event__id(assocs[i].name); + free(sys); if (err < 0) goto out; diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c index df2fddb..44cbb40 100644 --- a/tools/perf/util/trace-event-parse.c +++ b/tools/perf/util/trace-event-parse.c @@ -176,6 +176,10 @@ struct event_format *trace_find_event(int type) return pevent_find_event(pevent, type); } +struct event_format *trace_find_event_by_name(const char *sys, const char *name) +{ + return pevent_find_event_by_name(pevent, sys, name); +} void print_trace_event(int cpu, void *data, int size) { diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h index 639852a..66f83a0 100644 --- a/tools/perf/util/trace-event.h +++ b/tools/perf/util/trace-event.h @@ -40,6 +40,7 @@ int parse_event_file(char *buf, unsigned long size, char *sys); struct pevent_record *trace_peek_data(int cpu); struct event_format *trace_find_event(int type); +struct event_format *trace_find_event_by_name(const char *sys, const char *name); unsigned long long raw_field_value(struct event_format *event, const char *name, void *data);
Use new function trace_find_event_by_name to lookup events before looking through /sys files. This helps 'perf sched replay' to map event names to IDs correctly when processing perf.data recorded on another machine. Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org> --- tools/perf/util/evlist.c | 18 ++++++++++++++++-- tools/perf/util/trace-event-parse.c | 4 ++++ tools/perf/util/trace-event.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)