[PULL 0/2] small perf fixes

[PULL 0/2] small perf fixes

[Christian Borntraeger]

Ingo, Arnaldo,

two fixes indentified during kvm on s390 development are available 

since commit 538592ff0b008237ae88f5ce5fb1247127dc3ce5:

  Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core (2014-03-19 08:05:47 +0100)

in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/perf-fixes

for you to fetch changes up to 5e032c6c3eaf499460e3ca6b9586c7605c16004e:

  perf-kvm: fix of 'Min time' counting in report command (2014-04-09 16:06:15 +0200)

Please consider to apply.

----------------------------------------------------------------
Provide two fixes for perf:
- allow "-" in trace names. This enables perf to also handle kvm-s390
- fix min accounting in for perf kvm

----------------------------------------------------------------
Alexander Yarygin (2):
      perf/tool: Fix usage of trace events with '-' in trace system name.
      perf-kvm: fix of 'Min time' counting in report command

 tools/perf/builtin-kvm.c       |    1 +
 tools/perf/util/parse-events.l |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

[PULL 1/2] perf/tool: Fix usage of trace events with '-' in trace system name.

[Christian Borntraeger]

From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

Trace events potentially can have a '-' in their trace system name,
e.g. kvm on s390 defines kvm-s390:* tracepoints.
tools/perf could not parse them, because there was no rule for this:
$ sudo ./perf top -e "kvm-s390:*"
invalid or unsupported event: 'kvm-s390:*'

This patch allows '-' to be a part of PE_NAME token, so tracepoints
with '-' can be parsed by the event_legacy_tracepoint rule.
Without the patch, perf will not accept such tracepoints in the -e
option.

Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 tools/perf/util/parse-events.l |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 3432995..ca20da7 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -199,7 +199,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 {num_hex}		{ return value(yyscanner, 16); }
 
 {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
-{name}			{ return str(yyscanner, PE_NAME); }
+{name_minus}			{ return str(yyscanner, PE_NAME); }
 "/"			{ BEGIN(config); return '/'; }
 -			{ return '-'; }
 ,			{ BEGIN(event); return ','; }
-- 
1.7.9.5

[PULL 2/2] perf-kvm: fix of 'Min time' counting in report command

[Christian Borntraeger]

From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

Every event in the perf-kvm has a 'stats' structure, which contains
max/min/average/etc times of handling this event.
The problem is that the 'perf-kvm stat report' command always shows
that 'min time' is 0us for every event. Example:

 # perf kvm stat report
Analyze events for all VCPUs:

    VM-EXIT    Samples  Samples%     Time%   Min Time   Max Time
    Avg time
[..]
0xB2 MSCH         12     0.07%     0.00%        0us        8us
    7.31us ( +-   2.11% )
0xB2 CHSC         12     0.07%     0.00%        0us       18us
    9.39us ( +-   9.49% )
0xB2 STPX          8     0.05%     0.00%        0us        2us
    1.88us ( +-   7.18% )
0xB2 STSI          7     0.04%     0.00%        0us       44us
    16.49us ( +-  38.20% )
[..]

This happens because 'stats' structure is not initialized and
stats->min equals to 0. Lets initialize the structure for every
event after it's allocation using init_stats() function. This initialize
stats->min to -1 and makes 'Min time' statistics counting work:

 # perf kvm stat report

Analyze events for all VCPUs:

    VM-EXIT    Samples  Samples%     Time%   Min Time   Max Time
    Avg time
[..]
0xB2 MSCH         12     0.07%     0.00%        6us        8us
    7.31us ( +-   2.11% )
0xB2 CHSC         12     0.07%     0.00%        7us       18us
    9.39us ( +-   9.49% )
0xB2 STPX          8     0.05%     0.00%        1us        2us
    1.88us ( +-   7.18% )
0xB2 STSI          7     0.04%     0.00%        1us       44us
    16.49us ( +-  38.20% )
[..]

Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 tools/perf/builtin-kvm.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 21c164b..0f1e5a2 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -404,6 +404,7 @@ static struct kvm_event *kvm_alloc_init_event(struct event_key *key)
 	}
 
 	event->key = *key;
+	init_stats(&event->total.stats);
 	return event;
 }
 
-- 
1.7.9.5

Re: [PULL 2/2] perf-kvm: fix of 'Min time' counting in report command

[David Ahern]

On 4/9/14, 7:21 AM, Christian Borntraeger wrote:
> From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
>
> Every event in the perf-kvm has a 'stats' structure, which contains
> max/min/average/etc times of handling this event.
> The problem is that the 'perf-kvm stat report' command always shows
> that 'min time' is 0us for every event. Example:
>
>   # perf kvm stat report
> Analyze events for all VCPUs:
>
>      VM-EXIT    Samples  Samples%     Time%   Min Time   Max Time
>      Avg time
> [..]
> 0xB2 MSCH         12     0.07%     0.00%        0us        8us
>      7.31us ( +-   2.11% )
> 0xB2 CHSC         12     0.07%     0.00%        0us       18us
>      9.39us ( +-   9.49% )
> 0xB2 STPX          8     0.05%     0.00%        0us        2us
>      1.88us ( +-   7.18% )
> 0xB2 STSI          7     0.04%     0.00%        0us       44us
>      16.49us ( +-  38.20% )
> [..]
>
> This happens because 'stats' structure is not initialized and
> stats->min equals to 0. Lets initialize the structure for every
> event after it's allocation using init_stats() function. This initialize
> stats->min to -1 and makes 'Min time' statistics counting work:
>
>   # perf kvm stat report
>
> Analyze events for all VCPUs:
>
>      VM-EXIT    Samples  Samples%     Time%   Min Time   Max Time
>      Avg time
> [..]
> 0xB2 MSCH         12     0.07%     0.00%        6us        8us
>      7.31us ( +-   2.11% )
> 0xB2 CHSC         12     0.07%     0.00%        7us       18us
>      9.39us ( +-   9.49% )
> 0xB2 STPX          8     0.05%     0.00%        1us        2us
>      1.88us ( +-   7.18% )
> 0xB2 STSI          7     0.04%     0.00%        1us       44us
>      16.49us ( +-  38.20% )
> [..]
>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>   tools/perf/builtin-kvm.c |    1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 21c164b..0f1e5a2 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -404,6 +404,7 @@ static struct kvm_event *kvm_alloc_init_event(struct event_key *key)
>   	}
>
>   	event->key = *key;
> +	init_stats(&event->total.stats);
>   	return event;
>   }

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PULL 0/2] small perf fixes

[Christian Borntraeger]

On 09/04/14 16:21, Christian Borntraeger wrote:
> Ingo, Arnaldo,
> 
> two fixes indentified during kvm on s390 development are available 
> 
> since commit 538592ff0b008237ae88f5ce5fb1247127dc3ce5:
> 
>   Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core (2014-03-19 08:05:47 +0100)
> 
> in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/perf-fixes
> 
> for you to fetch changes up to 5e032c6c3eaf499460e3ca6b9586c7605c16004e:
> 
>   perf-kvm: fix of 'Min time' counting in report command (2014-04-09 16:06:15 +0200)
> 
> Please consider to apply.
> 
> ----------------------------------------------------------------
> Provide two fixes for perf:
> - allow "-" in trace names. This enables perf to also handle kvm-s390
> - fix min accounting in for perf kvm
> 
> ----------------------------------------------------------------
> Alexander Yarygin (2):
>       perf/tool: Fix usage of trace events with '-' in trace system name.
>       perf-kvm: fix of 'Min time' counting in report command
> 
>  tools/perf/builtin-kvm.c       |    1 +
>  tools/perf/util/parse-events.l |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 

Ping.

Re: [PULL 0/2] small perf fixes

[Jiri Olsa]

On Tue, Apr 15, 2014 at 03:05:02PM +0200, Christian Borntraeger wrote:
> On 09/04/14 16:21, Christian Borntraeger wrote:
> > Ingo, Arnaldo,
> > 
> > two fixes indentified during kvm on s390 development are available 
> > 
> > since commit 538592ff0b008237ae88f5ce5fb1247127dc3ce5:
> > 
> >   Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core (2014-03-19 08:05:47 +0100)
> > 
> > in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/perf-fixes
> > 
> > for you to fetch changes up to 5e032c6c3eaf499460e3ca6b9586c7605c16004e:
> > 
> >   perf-kvm: fix of 'Min time' counting in report command (2014-04-09 16:06:15 +0200)
> > 
> > Please consider to apply.
> > 
> > ----------------------------------------------------------------
> > Provide two fixes for perf:
> > - allow "-" in trace names. This enables perf to also handle kvm-s390
> > - fix min accounting in for perf kvm
> > 
> > ----------------------------------------------------------------
> > Alexander Yarygin (2):
> >       perf/tool: Fix usage of trace events with '-' in trace system name.
> >       perf-kvm: fix of 'Min time' counting in report command
> > 
> >  tools/perf/builtin-kvm.c       |    1 +
> >  tools/perf/util/parse-events.l |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> 
> Ping.

hi,
I'll take care of this.. tomorrow ;-)

thanks,
jirka

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

[Jiri Olsa]

On Wed, Apr 09, 2014 at 04:21:58PM +0200, Christian Borntraeger wrote:
> From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> 
> Trace events potentially can have a '-' in their trace system name,
> e.g. kvm on s390 defines kvm-s390:* tracepoints.
> tools/perf could not parse them, because there was no rule for this:
> $ sudo ./perf top -e "kvm-s390:*"
> invalid or unsupported event: 'kvm-s390:*'
> 
> This patch allows '-' to be a part of PE_NAME token, so tracepoints
> with '-' can be parsed by the event_legacy_tracepoint rule.
> Without the patch, perf will not accept such tracepoints in the -e
> option.
> 
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  tools/perf/util/parse-events.l |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 3432995..ca20da7 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -199,7 +199,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
>  {num_hex}		{ return value(yyscanner, 16); }
>  
>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
> -{name}			{ return str(yyscanner, PE_NAME); }
> +{name_minus}			{ return str(yyscanner, PE_NAME); }
>  "/"			{ BEGIN(config); return '/'; }
>  -			{ return '-'; }
>  ,			{ BEGIN(event); return ','; }

this breaks parsing of cache events like:

  $ perf record -e 'L1-dcache-loads' ls

also test 10 (same issue):
  $ ./perf test 10
  10: roundtrip evsel->name check                            : FAILED!


it might be little tricky to fix, let me know if you
have any troubles with that, I could look on it

thanks,
jirka

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

[Christian Borntraeger]

On 16/04/14 15:00, Jiri Olsa wrote:
> On Wed, Apr 09, 2014 at 04:21:58PM +0200, Christian Borntraeger wrote:
>> From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
>>
>> Trace events potentially can have a '-' in their trace system name,
>> e.g. kvm on s390 defines kvm-s390:* tracepoints.
>> tools/perf could not parse them, because there was no rule for this:
>> $ sudo ./perf top -e "kvm-s390:*"
>> invalid or unsupported event: 'kvm-s390:*'
>>
>> This patch allows '-' to be a part of PE_NAME token, so tracepoints
>> with '-' can be parsed by the event_legacy_tracepoint rule.
>> Without the patch, perf will not accept such tracepoints in the -e
>> option.
>>
>> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  tools/perf/util/parse-events.l |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> index 3432995..ca20da7 100644
>> --- a/tools/perf/util/parse-events.l
>> +++ b/tools/perf/util/parse-events.l
>> @@ -199,7 +199,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
>>  {num_hex}		{ return value(yyscanner, 16); }
>>  
>>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
>> -{name}			{ return str(yyscanner, PE_NAME); }
>> +{name_minus}			{ return str(yyscanner, PE_NAME); }
>>  "/"			{ BEGIN(config); return '/'; }
>>  -			{ return '-'; }
>>  ,			{ BEGIN(event); return ','; }
> 
> this breaks parsing of cache events like:
> 
>   $ perf record -e 'L1-dcache-loads' ls
> 
> also test 10 (same issue):
>   $ ./perf test 10
>   10: roundtrip evsel->name check                            : FAILED!
> 
> 
> it might be little tricky to fix, let me know if you
> have any troubles with that, I could look on it

Hmm, so do you prefer tackling this problem directly at  event_legacy_tracepoint, e.g.
like in
https://lkml.org/lkml/2014/3/24/364

Christian

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

[Christian Borntraeger]

On 16/04/14 15:28, Christian Borntraeger wrote:
> On 16/04/14 15:00, Jiri Olsa wrote:
>> On Wed, Apr 09, 2014 at 04:21:58PM +0200, Christian Borntraeger wrote:
>>> From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
>>>
>>> Trace events potentially can have a '-' in their trace system name,
>>> e.g. kvm on s390 defines kvm-s390:* tracepoints.
>>> tools/perf could not parse them, because there was no rule for this:
>>> $ sudo ./perf top -e "kvm-s390:*"
>>> invalid or unsupported event: 'kvm-s390:*'
>>>
>>> This patch allows '-' to be a part of PE_NAME token, so tracepoints
>>> with '-' can be parsed by the event_legacy_tracepoint rule.
>>> Without the patch, perf will not accept such tracepoints in the -e
>>> option.
>>>
>>> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  tools/perf/util/parse-events.l |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>>> index 3432995..ca20da7 100644
>>> --- a/tools/perf/util/parse-events.l
>>> +++ b/tools/perf/util/parse-events.l
>>> @@ -199,7 +199,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
>>>  {num_hex}		{ return value(yyscanner, 16); }
>>>  
>>>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
>>> -{name}			{ return str(yyscanner, PE_NAME); }
>>> +{name_minus}			{ return str(yyscanner, PE_NAME); }
>>>  "/"			{ BEGIN(config); return '/'; }
>>>  -			{ return '-'; }
>>>  ,			{ BEGIN(event); return ','; }
>>
>> this breaks parsing of cache events like:
>>
>>   $ perf record -e 'L1-dcache-loads' ls
>>
>> also test 10 (same issue):
>>   $ ./perf test 10
>>   10: roundtrip evsel->name check                            : FAILED!
>>
>>
>> it might be little tricky to fix, let me know if you
>> have any troubles with that, I could look on it
> 
> Hmm, so do you prefer tackling this problem directly at  event_legacy_tracepoint, e.g.
> like in
> https://lkml.org/lkml/2014/3/24/364

A totally different approach would be to rename the kvm-s390 trace events to kvm_s390.

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

[Jiri Olsa]

On Wed, Apr 16, 2014 at 03:31:15PM +0200, Christian Borntraeger wrote:
> On 16/04/14 15:28, Christian Borntraeger wrote:
> > On 16/04/14 15:00, Jiri Olsa wrote:
> >> On Wed, Apr 09, 2014 at 04:21:58PM +0200, Christian Borntraeger wrote:
> >>> From: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> >>>
> >>> Trace events potentially can have a '-' in their trace system name,
> >>> e.g. kvm on s390 defines kvm-s390:* tracepoints.
> >>> tools/perf could not parse them, because there was no rule for this:
> >>> $ sudo ./perf top -e "kvm-s390:*"
> >>> invalid or unsupported event: 'kvm-s390:*'
> >>>
> >>> This patch allows '-' to be a part of PE_NAME token, so tracepoints
> >>> with '-' can be parsed by the event_legacy_tracepoint rule.
> >>> Without the patch, perf will not accept such tracepoints in the -e
> >>> option.
> >>>
> >>> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>> ---
> >>>  tools/perf/util/parse-events.l |    2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> >>> index 3432995..ca20da7 100644
> >>> --- a/tools/perf/util/parse-events.l
> >>> +++ b/tools/perf/util/parse-events.l
> >>> @@ -199,7 +199,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
> >>>  {num_hex}		{ return value(yyscanner, 16); }
> >>>  
> >>>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
> >>> -{name}			{ return str(yyscanner, PE_NAME); }
> >>> +{name_minus}			{ return str(yyscanner, PE_NAME); }
> >>>  "/"			{ BEGIN(config); return '/'; }
> >>>  -			{ return '-'; }
> >>>  ,			{ BEGIN(event); return ','; }
> >>
> >> this breaks parsing of cache events like:
> >>
> >>   $ perf record -e 'L1-dcache-loads' ls
> >>
> >> also test 10 (same issue):
> >>   $ ./perf test 10
> >>   10: roundtrip evsel->name check                            : FAILED!
> >>
> >>
> >> it might be little tricky to fix, let me know if you
> >> have any troubles with that, I could look on it
> > 
> > Hmm, so do you prefer tackling this problem directly at  event_legacy_tracepoint, e.g.
> > like in
> > https://lkml.org/lkml/2014/3/24/364
> 
> A totally different approach would be to rename the kvm-s390 trace events to kvm_s390.
> 

hopefully that wouldn't be necessary ;-)

I checked the other approach and it works.. I'll see if I can find
some other way, if not I'll take that patch

thanks,
jirka