perf event group siblings not counting in mainline

perf event group siblings not counting in mainline

[Will Deacon]

Hi Peter,

If I try using perf with event groups on a mainline kernel, only the group
leader counts:


linaro@mashed-potato:~$ perf stat --no-scale -g -e task-clock -e cpu-clock -e cs -- ls
hwb  linux

 Performance counter stats for 'ls':

         17.545541 task-clock                #    0.634 CPUs utilized
          0.000000 cpu-clock
                 0 cs                        #    0.000 M/sec

       0.027684984 seconds time elapsed


This is fixed by b79387ef ("perf: Fix enable_on_exec for sibling events")
in -next (20111222) but I can't see this queued anywhere for 3.2 (I thought
I might see it in -rc7).

Is this going in for the final cut, or will groups just be borked until 3.3?

Cheers,

Will

Re: perf event group siblings not counting in mainline

[Will Deacon]

On Sat, Dec 24, 2011 at 12:54:15PM +0000, Will Deacon wrote:
> If I try using perf with event groups on a mainline kernel, only the group
> leader counts:
> 
> 
> linaro@mashed-potato:~$ perf stat --no-scale -g -e task-clock -e cpu-clock -e cs -- ls
> hwb  linux
> 
>  Performance counter stats for 'ls':
> 
>          17.545541 task-clock                #    0.634 CPUs utilized
>           0.000000 cpu-clock
>                  0 cs                        #    0.000 M/sec
> 
>        0.027684984 seconds time elapsed
> 
> 
> This is fixed by b79387ef ("perf: Fix enable_on_exec for sibling events")
> in -next (20111222) but I can't see this queued anywhere for 3.2 (I thought
> I might see it in -rc7).
> 
> Is this going in for the final cut, or will groups just be borked until 3.3?

Now that 3.2 is out with this regression, would it be possible to get this
(b79387ef ("perf: Fix enable_on_exec for sibling events")) into -stable please?

Will

Re: perf event group siblings not counting in mainline

[Peter Zijlstra]

On Tue, 2012-01-10 at 11:09 +0000, Will Deacon wrote:
> On Sat, Dec 24, 2011 at 12:54:15PM +0000, Will Deacon wrote:
> > If I try using perf with event groups on a mainline kernel, only the group
> > leader counts:
> > 
> > 
> > linaro@mashed-potato:~$ perf stat --no-scale -g -e task-clock -e cpu-clock -e cs -- ls
> > hwb  linux
> > 
> >  Performance counter stats for 'ls':
> > 
> >          17.545541 task-clock                #    0.634 CPUs utilized
> >           0.000000 cpu-clock
> >                  0 cs                        #    0.000 M/sec
> > 
> >        0.027684984 seconds time elapsed
> > 
> > 
> > This is fixed by b79387ef ("perf: Fix enable_on_exec for sibling events")
> > in -next (20111222) but I can't see this queued anywhere for 3.2 (I thought
> > I might see it in -rc7).
> > 
> > Is this going in for the final cut, or will groups just be borked until 3.3?
> 
> Now that 3.2 is out with this regression, would it be possible to get this
> (b79387ef ("perf: Fix enable_on_exec for sibling events")) into -stable please?

How can this be a regression? Its always been like that. At worst its a
broken new feature.

Also, you really shouldn't set enable_on_exec on non group leaders,
that's just daft. But yes that patch sorts it. Something like the below
should too.

---
 tools/perf/builtin-stat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f5d2a63..623519f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -298,7 +298,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 						group, group_fd);
 	if (target_pid == -1 && target_tid == -1) {
 		attr->disabled = 1;
-		attr->enable_on_exec = 1;
+		attr->enable_on_exec = !group_fd;
 	}
 
 	return perf_evsel__open_per_thread(evsel, evsel_list->threads,

Re: perf event group siblings not counting in mainline

[Will Deacon]

On Tue, Jan 10, 2012 at 11:16:28AM +0000, Peter Zijlstra wrote:
> On Tue, 2012-01-10 at 11:09 +0000, Will Deacon wrote:
> > On Sat, Dec 24, 2011 at 12:54:15PM +0000, Will Deacon wrote:
> > > If I try using perf with event groups on a mainline kernel, only the group
> > > leader counts:

[...]

> > Now that 3.2 is out with this regression, would it be possible to get this
> > (b79387ef ("perf: Fix enable_on_exec for sibling events")) into -stable please?
> 
> How can this be a regression? Its always been like that. At worst its a
> broken new feature.

My mistake, for some reason I thought this had always had the new behaviour.

> Also, you really shouldn't set enable_on_exec on non group leaders,
> that's just daft. But yes that patch sorts it. Something like the below
> should too.

>From a perf stat perspective, it should probably do the right thing then.

> 
> ---
>  tools/perf/builtin-stat.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f5d2a63..623519f 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -298,7 +298,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
>  						group, group_fd);
>  	if (target_pid == -1 && target_tid == -1) {
>  		attr->disabled = 1;
> -		attr->enable_on_exec = 1;
> +		attr->enable_on_exec = !group_fd;
>  	}
>  
>  	return perf_evsel__open_per_thread(evsel, evsel_list->threads,

Had to make a small change so that that the siblings don't get stuck as
disabled:


diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 955930e..a0f4b26 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -296,10 +296,9 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
        if (system_wide)
                return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
                                                group, group_fd);
-       if (target_pid == -1 && target_tid == -1) {
-               attr->disabled = 1;
-               attr->enable_on_exec = 1;
-       }
+
+       if (target_pid == -1 && target_tid == -1)
+               attr->disabled = attr->enable_on_exec = !group_fd;
 
        return perf_evsel__open_per_thread(evsel, evsel_list->threads,
                                           group, group_fd);


but that seems to do it - thanks!

Tested-by: Will Deacon <will.deacon@arm.com>

It would be nice to see this in -stable, but as it's not a regression I'll
leave it up to you.

Cheers,

Will