Messages in this thread Patches in this message | | | Date | Mon, 21 Apr 2014 19:43:50 +0400 | From | Alexander Yarygin <> | Subject | Re: [PATCH] perf/tool: Fix usage of trace events with '-' in trace system name. |
| |
At Thu, 17 Apr 2014 13:32:21 +0200, Jiri Olsa wrote: > > On Tue, Mar 25, 2014 at 11:15:29AM +0100, Paolo Bonzini wrote: > > Il 24/03/2014 21:49, Christian Borntraeger ha scritto: > > > event_legacy_tracepoint: > > >+PE_NAME '-' PE_NAME ':' PE_NAME > > >+{ > > >+ struct parse_events_evlist *data = _data; > > >+ struct list_head *list; > > >+ char sys_name[strlen($1) + strlen($3) + 2]; > > >+ sprintf(&sys_name, "%s-%s", $1, $3); > > >+ > > >+ ALLOC_LIST(list); > > >+ ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5)); > > >+ $$ = list; > > >+} > > > > Why isn't '-' part of PE_NAME? > > hi Paolo ;-) > > because it screws cache events parsing.. we need some code factoring > in this part > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > it'd be nice to add test to tests/parse-events.c, probably s390 specific, > because the parsing code touches the tracepoint format file >
Hi,
Hmm, looks like we can't simply add arch-specific test:
--- a/tools/perf/tests/parse-events.c +++ b/tools/perf/tests/parse-events.c @@ -1346,6 +1346,12 @@ static struct evlist_test test__events[] = { .name = "{cycles,cache-misses,branch-misses}:D", .check = test__pinned_group, }, +#if defined(__s390x__) + [42] = { + .name = "kvm-s390:kvm_s390_create_vm", + .check = test__checkevent_tracepoint, + }, +#endif /* and what will be the next number: 42 or 43? */ };
static struct evlist_test test__events_pmu[] = {
Because it breaks explicit numbering of test__events[]. I can suggest to move numeration into evlist_test, i.e.
--- a/tools/perf/tests/parse-events.c +++ b/tools/perf/tests/parse-events.c @@ -1174,25 +1174,30 @@ static int test__all_tracepoints(struct perf_evlist *evlist) struct evlist_test { const char *name; __u32 type; + int index; int (*check)(struct perf_evlist *evlist); }; static struct evlist_test test__events[] = { - [0] = { + { .name = "syscalls:sys_enter_open", .check = test__checkevent_tracepoint, + .index = 0; }, ...
or just to remove it?
How do you think?
And a bit of offtopic :) Apparently, s390 doesn't have syscalls:*, so some of the tests don't work properly (or maybe I missed something? I set CONFIG_FTRACE_SYSCALLS to 'y' in my config: still no syscalls:*).
What do you think about this idea:
--- a/tools/perf/tests/parse-events.c +++ b/tools/perf/tests/parse-events.c @@ -1177,13 +1177,21 @@ struct evlist_test { int (*check)(struct perf_evlist *evlist); };
+#if !defined(__s390x__) +#define TP_SYS_NAME "syscalls" +#define TP_EVENT_NAME "sys_enter_open" +#else +#define TP_SYS_NAME "sched" +#define TP_EVENT_NAME "sched_wakeup" +#endif + static struct evlist_test test__events[] = { [0] = { - .name = "syscalls:sys_enter_open", + .name = TP_SYS_NAME ":" TP_EVENT_NAME, .check = test__checkevent_tracepoint, },
... and so on?
Also, test_pmu() looks at /sys/bus/event_source/devices/cpu/ but instead of "cpu/" on s390 there are "cpum_sf/" and "cpum_cf/", so pmu tests don't work either..
Thanks
| |