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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140423114529.GB11124@krava.brq.redhat.com>
Date:	Wed, 23 Apr 2014 13:45:29 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Alexander Yarygin <yarygin@...ux.vnet.ibm.com>
Cc:	Christian Borntraeger <borntraeger@...ibm.com>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	KVM <kvm@...r.kernel.org>,
	linux-s390 <linux-s390@...r.kernel.org>,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf/tool: Fix usage of trace events with '-' in trace
 system name.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/