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@xxxxxxx>

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