Re: [RFC PATCH 2/2] tools/perf: Make group_fd static and move its place in __perf_evsel__open()

[Deng-Cheng Zhu <dczhu@mips.com>]

On 10/25/2011 11:23 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 24, 2011 at 12:03:28PM -0200, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Oct 24, 2011 at 06:57:00PM +0800, Deng-Cheng Zhu escreveu:
>>> __perf_evsel__open() is called per event, it does not work for all the
>>> grouped events at one time. So, currently group_fd will alway be -1 for
>>> the events in a group. This patch fixes it.
>>>
>>> Signed-off-by: Deng-Cheng Zhu<dczhu@mips.com>
>>> Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
>>> Cc: Paul Mackerras<paulus@samba.org>
>>> Cc: Ingo Molnar<mingo@elte.hu>
>>> Cc: Arnaldo Carvalho de Melo<acme@ghostprotocols.net>
>>> ---
>>>   tools/perf/util/evsel.c |    3 +--
>>>   1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index e389815..7bd0d9d 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -219,9 +219,8 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>>>   	}
>>>
>>>   	for (cpu = 0; cpu<  cpus->nr; cpu++) {
>>> -		int group_fd = -1;
>>> -
>>>   		for (thread = 0; thread<  threads->nr; thread++) {
>>> +			static int group_fd = -1;
>>>
>>>   			if (!evsel->cgrp)
>>>   				pid = threads->map[thread];
>>
>> Lets not do it that way, using statics for this is humm, ugly, IMHO.
>>
>> Just pass an integer pointer that is a member of perf_evlist, I'll work
>> on a patch now.
>>
>> Thanks for reporting the bug tho!
>
> Can you try this patch?
>
> I tested it with:
>
> [root@emilia ~]# perf top -e cycles -e instructions --group
>
> and
>
> [root@emilia ~]# perf record -e cycles -e instructions --group -a sleep 2
>

Your patch does fix the group fd issue. But to get event grouping
workable, the event state fix is still needed. Please see the discussion
here: http://www.spinics.net/lists/mips/msg42190.html

With _only_ your patch applied, I tested with the following commands on
MIPS 74K (4 counters available in total):

perf stat -g -e 
L1-dcache-load-misses,cycles,LLC-load-misses,iTLB-loads,instructions 
find / >/dev/null

I tried to group up to 5 events in the hope of seeing NOSPC error. But
the command didn't fail and output:

  Performance counter stats for 'find /':

            9300823 L1-dcache-load-misses
      <not counted> cycles
      <not counted> LLC-load-misses
      <not counted> iTLB-loads
      <not counted> instructions

        8.463207591 seconds time elapsed

This is due to the event state check in validate_group() filtering out
the grouped events in OFF state. They are in OFF state because we are
running the command with the perf tool as opposed to attaching to an
existing task:

builtin-stat.c:create_perf_stat_counter():

         if (target_pid == -1 && target_tid == -1) {
                 attr->disabled = 1;
                 attr->enable_on_exec = 1;
         }

I suppose X86 has this issue too -- collect_events() in validate_group()
won't do real work in the bottom half of the function.


Deng-Cheng