Re: [PATCH] perf/tool: Fix usage of trace events with '-' in trace system name.

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Apr 21, 2014 at 07:43:50PM +0400, Alexander Yarygin wrote:

SNIP

> 
> --- 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?

right, that numbering is there to ease the search for test,
and is printed for -v option

the 'index' field should be fine, and please print it
out for '-v' option


> 
> 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?

that looks fine.. also we could use just generic tracepoints
like the 'sched' ones

> 
> 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..

we could #ifdef the pmu name for s390 and other archs

or make it more fancy and detect it by using pmu.c code,
please check perf_pmu__scan,perf_pmu__find functions

thanks,
jirka