[PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Deng-Cheng Zhu]

Currently, when grouped events are created disabled and enable-on-exec, the
siblings won't be enabled on exec in fact. The problem looks like this:

======================================================================
-sh-4.0# perf stat -g -e r14,cycles,instructions,r12 find / >/dev/null
^Cfind: Interrupt

 Performance counter stats for 'find /':

          60684699 r14
     <not counted> cycles
     <not counted> instructions
     <not counted> r12

       4.291975113 seconds time elapsed
======================================================================

This patch fixes it.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: LKML <linux-kernel@vger.kernel.org>
---
 kernel/events/core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0e8457d..300bc66 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1651,6 +1651,8 @@ retry:
 	raw_spin_unlock_irq(&ctx->lock);
 }
 
+static int event_enable_on_exec(struct perf_event *event,
+				struct perf_event_context *ctx);
 /*
  * Put a event into inactive state and update time fields.
  * Enabling the leader of a group effectively enables all
@@ -1668,6 +1670,7 @@ static void __perf_event_mark_enabled(struct perf_event *event,
 	event->state = PERF_EVENT_STATE_INACTIVE;
 	event->tstamp_enabled = tstamp - event->total_time_enabled;
 	list_for_each_entry(sub, &event->sibling_list, group_entry) {
+		event_enable_on_exec(sub, sub->ctx);
 		if (sub->state >= PERF_EVENT_STATE_INACTIVE)
 			sub->tstamp_enabled = tstamp - sub->total_time_enabled;
 	}
-- 
1.7.1

Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Peter Zijlstra]

On Tue, 2011-11-22 at 11:30 +0800, Deng-Cheng Zhu wrote:
> Currently, when grouped events are created disabled and enable-on-exec, the
> siblings won't be enabled on exec in fact. The problem looks like this:

Arguably that's a daft thing to do, since if the leader is disabled the
group won't get scheduled anyway. But I guess we should at least try to
deal with it when people do do it.

Seems perf-stat is a bit daft this way.

> This patch fixes it.

I guess it does, but its not pretty, event_enable_on_exec() already
calls __perf_event_mark_enable(), now this recursion is limited because
siblings can't have a sibling list of their own, but still.

> +++ b/kernel/events/core.c
> @@ -1651,6 +1651,8 @@ retry:
>  	raw_spin_unlock_irq(&ctx->lock);
>  }
>  
> +static int event_enable_on_exec(struct perf_event *event,
> +				struct perf_event_context *ctx);
>  /*
>   * Put a event into inactive state and update time fields.
>   * Enabling the leader of a group effectively enables all
> @@ -1668,6 +1670,7 @@ static void __perf_event_mark_enabled(struct perf_event *event,
>  	event->state = PERF_EVENT_STATE_INACTIVE;
>  	event->tstamp_enabled = tstamp - event->total_time_enabled;
>  	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> +		event_enable_on_exec(sub, sub->ctx);
>  		if (sub->state >= PERF_EVENT_STATE_INACTIVE)
>  			sub->tstamp_enabled = tstamp - sub->total_time_enabled;
>  	}

The below is a somewhat larger patch that avoids the recursion (and does
a small cleanup by eradicating all those useless ctx arguments). Quick
testing seems to indicate it works, but please confirm.

---
Subject: perf: Fix enable_on_exec for sibling events
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue Nov 22 11:25:43 CET 2011

Deng-Cheng Zhu reported that sibling events that were created disabled
with enable_on_exec would never get enabled.

Reported-by: Deng-Cheng Zhu <dczhu@mips.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/events/core.c |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

Index: linux-2.6/kernel/events/core.c
===================================================================
--- linux-2.6.orig/kernel/events/core.c
+++ linux-2.6/kernel/events/core.c
@@ -1664,8 +1664,7 @@ perf_install_in_context(struct perf_even
  * Note: this works for group members as well as group leaders
  * since the non-leader members' sibling_lists will be empty.
  */
-static void __perf_event_mark_enabled(struct perf_event *event,
-					struct perf_event_context *ctx)
+static void __perf_event_mark_enabled(struct perf_event *event)
 {
 	struct perf_event *sub;
 	u64 tstamp = perf_event_time(event);
@@ -1703,7 +1702,7 @@ static int __perf_event_enable(void *inf
 	 */
 	perf_cgroup_set_timestamp(current, ctx);
 
-	__perf_event_mark_enabled(event, ctx);
+	__perf_event_mark_enabled(event);
 
 	if (!event_filter_match(event)) {
 		if (is_cgroup_event(event))
@@ -1784,7 +1783,7 @@ void perf_event_enable(struct perf_event
 
 retry:
 	if (!ctx->is_active) {
-		__perf_event_mark_enabled(event, ctx);
+		__perf_event_mark_enabled(event);
 		goto out;
 	}
 
@@ -2453,8 +2452,7 @@ void perf_event_task_tick(void)
 	}
 }
 
-static int event_enable_on_exec(struct perf_event *event,
-				struct perf_event_context *ctx)
+static int event_enable_on_exec(struct perf_event *event)
 {
 	if (!event->attr.enable_on_exec)
 		return 0;
@@ -2463,11 +2461,25 @@ static int event_enable_on_exec(struct p
 	if (event->state >= PERF_EVENT_STATE_INACTIVE)
 		return 0;
 
-	__perf_event_mark_enabled(event, ctx);
+	event->state = PERF_EVENT_STATE_INACTIVE;
 
 	return 1;
 }
 
+static int group_enable_on_exec(struct perf_event *event)
+{
+	struct perf_event *sub;
+	int ret = 0;
+
+	ret += event_enable_on_exec(event);
+	list_for_each_entry(sub, &event->sibling_list, group_entry)
+		ret += event_enable_on_exec(sub);
+
+	__perf_event_mark_enabled(event);
+
+	return ret;
+}
+
 /*
  * Enable all of a task's events that have been marked enable-on-exec.
  * This expects task == current.
@@ -2496,13 +2508,13 @@ static void perf_event_enable_on_exec(st
 	task_ctx_sched_out(ctx);
 
 	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
-		ret = event_enable_on_exec(event, ctx);
+		ret = group_enable_on_exec(event);
 		if (ret)
 			enabled = 1;
 	}
 
 	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
-		ret = event_enable_on_exec(event, ctx);
+		ret = group_enable_on_exec(event);
 		if (ret)
 			enabled = 1;
 	}

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Zhu, DengCheng]

> ________________________________________
> From: Peter Zijlstra [a.p.zijlstra@chello.nl]
> Sent: Tuesday, November 22, 2011 6:51 PM
> To: Zhu, DengCheng
> Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML
> Subject: Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec
>
> On Tue, 2011-11-22 at 11:30 +0800, Deng-Cheng Zhu wrote:
>> Currently, when grouped events are created disabled and enable-on-exec, the
>> siblings won't be enabled on exec in fact. The problem looks like this:
>
> Arguably that's a daft thing to do, since if the leader is disabled the
> group won't get scheduled anyway. But I guess we should at least try to
> deal with it when people do do it.

Well, by "grouped events" I mean "all of the grouped events", not only the
group leader. In fact the leader (and only the leader) will be enabled by
going through ctx->flexible_groups in perf_event_enable_on_exec().

>
> Seems perf-stat is a bit daft this way.
>
>> This patch fixes it.
>
> I guess it does, but its not pretty, event_enable_on_exec() already
> calls __perf_event_mark_enable(), now this recursion is limited because
> siblings can't have a sibling list of their own, but still.

I did it like this just by reading the code comment of
__perf_event_mark_enabled(): "Enabling the leader of a group effectively
enables all the group members that aren't explicitly disabled ... Note:
this works for group members as well as group leaders since the non-leader
members' sibling_lists will be empty."

So I suppose dealing with siblings' state in this traversal is the right
thing to do and introduces minimal code turmoil, although the latter is by
no means critical.

> The below is a somewhat larger patch that avoids the recursion (and does
> a small cleanup by eradicating all those useless ctx arguments). Quick
> testing seems to indicate it works, but please confirm.

I have no objection of deleting the redundant ctx arguments, but that's
another topic.


Deng-Cheng

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Peter Zijlstra]

On Tue, 2011-11-22 at 13:24 +0000, Zhu, DengCheng wrote:
> > ________________________________________
> > From: Peter Zijlstra [a.p.zijlstra@chello.nl]
> > Sent: Tuesday, November 22, 2011 6:51 PM
> > To: Zhu, DengCheng
> > Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML
> > Subject: Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec
> >
> > On Tue, 2011-11-22 at 11:30 +0800, Deng-Cheng Zhu wrote:
> >> Currently, when grouped events are created disabled and enable-on-exec, the
> >> siblings won't be enabled on exec in fact. The problem looks like this:
> >
> > Arguably that's a daft thing to do, since if the leader is disabled the
> > group won't get scheduled anyway. But I guess we should at least try to
> > deal with it when people do do it.
> 
> Well, by "grouped events" I mean "all of the grouped events", not only the
> group leader. In fact the leader (and only the leader) will be enabled by
> going through ctx->flexible_groups in perf_event_enable_on_exec().

Right, I understood that. What I said was daft was to tag the
non-leaders as enabled_on_exec,disabled. They wouldn't get scheduled
anyway for as long as the leader is off.

> > Seems perf-stat is a bit daft this way.
> >
> >> This patch fixes it.
> >
> > I guess it does, but its not pretty, event_enable_on_exec() already
> > calls __perf_event_mark_enable(), now this recursion is limited because
> > siblings can't have a sibling list of their own, but still.
> 
> I did it like this just by reading the code comment of
> __perf_event_mark_enabled(): "Enabling the leader of a group effectively
> enables all the group members that aren't explicitly disabled ... Note:
> this works for group members as well as group leaders since the non-leader
> members' sibling_lists will be empty."
> 
> So I suppose dealing with siblings' state in this traversal is the right
> thing to do and introduces minimal code turmoil, although the latter is by
> no means critical.

Yeah, I just don't really like the recursion thing... Also, there's more
ways to get to __perf_event_mark_enabled() and not all those want to
actually do enable_on_exec().

> > The below is a somewhat larger patch that avoids the recursion (and does
> > a small cleanup by eradicating all those useless ctx arguments). Quick
> > testing seems to indicate it works, but please confirm.
> 
> I have no objection of deleting the redundant ctx arguments, but that's
> another topic.

Yeah, I should probably split that into a separate patch.

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Zhu, DengCheng]

> ________________________________________
> From: Peter Zijlstra [a.p.zijlstra@chello.nl]
> Sent: Tuesday, November 22, 2011 9:45 PM
> To: Zhu, DengCheng
> Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML
> Subject: RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec
>
> On Tue, 2011-11-22 at 13:24 +0000, Zhu, DengCheng wrote:
>> > ________________________________________
>> > From: Peter Zijlstra [a.p.zijlstra@chello.nl]
>> > Sent: Tuesday, November 22, 2011 6:51 PM
>> > To: Zhu, DengCheng
>> > Cc: Barzilay, Eyal; Fortuna, Zenon; Paul Mackerras; Ingo Molnar; Arnaldo Carvalho de Melo; ralf@linux-mips.org; LKML
>> > Subject: Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec
>> >
>> > On Tue, 2011-11-22 at 11:30 +0800, Deng-Cheng Zhu wrote:
>> >> Currently, when grouped events are created disabled and enable-on-exec, the
>> >> siblings won't be enabled on exec in fact. The problem looks like this:
>> >
>> > Arguably that's a daft thing to do, since if the leader is disabled the
>> > group won't get scheduled anyway. But I guess we should at least try to
>> > deal with it when people do do it.
>>
>> Well, by "grouped events" I mean "all of the grouped events", not only the
>> group leader. In fact the leader (and only the leader) will be enabled by
>> going through ctx->flexible_groups in perf_event_enable_on_exec().
>
> Right, I understood that. What I said was daft was to tag the
> non-leaders as enabled_on_exec,disabled. They wouldn't get scheduled
> anyway for as long as the leader is off.
>
>> > Seems perf-stat is a bit daft this way.
>> >
>> >> This patch fixes it.
>> >
>> > I guess it does, but its not pretty, event_enable_on_exec() already
>> > calls __perf_event_mark_enable(), now this recursion is limited because
>> > siblings can't have a sibling list of their own, but still.
>>
>> I did it like this just by reading the code comment of
>> __perf_event_mark_enabled(): "Enabling the leader of a group effectively
>> enables all the group members that aren't explicitly disabled ... Note:
>> this works for group members as well as group leaders since the non-leader
>> members' sibling_lists will be empty."
>>
>> So I suppose dealing with siblings' state in this traversal is the right
>> thing to do and introduces minimal code turmoil, although the latter is by
>> no means critical.
>
> Yeah, I just don't really like the recursion thing... Also, there's more
> ways to get to __perf_event_mark_enabled() and not all those want to
> actually do enable_on_exec().

Yep, two other functions call it. And whether doing enable_on_exec() in
__perf_event_mark_enabled() depends on how we interpret the meaning of the
latter. And if we do enable_on_exec() in it, uninterested events will be filtered
out in enable_on_exec().

One thing in your patch is uncertain to me:

> @@ -2463,11 +2461,25 @@ static int event_enable_on_exec(struct p
>        if (event->state >= PERF_EVENT_STATE_INACTIVE)
>                return 0;
>
> -       __perf_event_mark_enabled(event, ctx);
> +       event->state = PERF_EVENT_STATE_INACTIVE;
>
>        return 1;
>  }

By simply setting the event state in here, we bypass time stamp stuff as a result.
This might lead to inaccuracies...


Deng-Cheng

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Peter Zijlstra]

On Tue, 2011-11-22 at 14:20 +0000, Zhu, DengCheng wrote:
> > @@ -2463,11 +2461,25 @@ static int event_enable_on_exec(struct p
> >        if (event->state >= PERF_EVENT_STATE_INACTIVE)
> >                return 0;
> >
> > -       __perf_event_mark_enabled(event, ctx);
> > +       event->state = PERF_EVENT_STATE_INACTIVE;
> >
> >        return 1;
> >  }
> 
> By simply setting the event state in here, we bypass time stamp stuff as a result.
> This might lead to inaccuracies... 

Ah, but it calls a __perf_event_mark_enabled() at the tail of
group_enable_on_exec() which should fix that up, right?

Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Deng-Cheng Zhu]

On 11/22/2011 10:23 PM, Peter Zijlstra wrote:
> On Tue, 2011-11-22 at 14:20 +0000, Zhu, DengCheng wrote:
>>> @@ -2463,11 +2461,25 @@ static int event_enable_on_exec(struct p
>>>         if (event->state>= PERF_EVENT_STATE_INACTIVE)
>>>                 return 0;
>>>
>>> -       __perf_event_mark_enabled(event, ctx);
>>> +       event->state = PERF_EVENT_STATE_INACTIVE;
>>>
>>>         return 1;
>>>   }
>>
>> By simply setting the event state in here, we bypass time stamp stuff as a result.
>> This might lead to inaccuracies...
>
> Ah, but it calls a __perf_event_mark_enabled() at the tail of
> group_enable_on_exec() which should fix that up, right?

Oh, yes. Aside from a slight piece that the __perf_event_mark_enabled()
in group_enable_on_exec() will reassign PERF_EVENT_STATE_INACTIVE to the
group leader, your patch looks good.

In fact, my another candidate for this patch is as follows (The
comment of perf_event_enable_on_exec() says "Enable all of a task's
events that have been marked enable-on-exec", so I think the added
traversal makes sense in here):

From: Deng-Cheng Zhu <dczhu@mips.com>
Date: Wed, 23 Nov 2011 11:31:18 +0800
Subject: [PATCH] perf: Enable enable-on-exec siblings

Currently, when grouped events are created disabled and enable-on-exec,
the siblings won't be enabled on exec in fact. The problem looks like:

======================================================================
-sh-4.0# perf stat -g -e r14,cycles,instructions,r12 find / >/dev/null
^Cfind: Interrupt

  Performance counter stats for 'find /':

           60684699 r14
      <not counted> cycles
      <not counted> instructions
      <not counted> r12

        4.291975113 seconds time elapsed
======================================================================

This patch fixes it.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
---
  kernel/events/core.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0e8457d..63527d0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2476,6 +2476,12 @@ static void perf_event_enable_on_exec(struct 
perf_event_context *ctx)
  	raw_spin_lock(&ctx->lock);
  	task_ctx_sched_out(ctx);

+	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
+		ret = event_enable_on_exec(event, ctx);
+		if (ret)
+			enabled = 1;
+	}
+
  	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
  		ret = event_enable_on_exec(event, ctx);
  		if (ret)

Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Peter Zijlstra]

On Wed, 2011-11-23 at 11:38 +0800, Deng-Cheng Zhu wrote:

> +	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
> +		ret = event_enable_on_exec(event, ctx);
> +		if (ret)
> +			enabled = 1;
> +	}
> +
>   	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
>   		ret = event_enable_on_exec(event, ctx);
>   		if (ret)

This isn't correct either, in this case you should then remove the other
two iterations of pinned/flexible group lists.

Also your use of list_for_each_entry_rcu() is incorrect, either you then
should also use rcu_read_lock(), or its not needed and not use the _rcu
list primitive at all.

Now since event_list is modified under ctx->lock, and we hold that lock
no fancy stuff is needed and we can do without.

---
Subject: perf: Fix enable_on_exec for sibling events
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue Nov 22 11:25:43 CET 2011

Deng-Cheng Zhu reported that sibling events that were created disabled
with enable_on_exec would never get enabled. Iterate all events instead
of the group lists.

Reported-by: Deng-Cheng Zhu <dczhu@mips.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/n/tip-299rxrpmmle8hp3spyxfo202@git.kernel.org
---
 kernel/events/core.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Index: linux-2.6/kernel/events/core.c
===================================================================
--- linux-2.6.orig/kernel/events/core.c
+++ linux-2.6/kernel/events/core.c
@@ -2494,13 +2494,7 @@ static void perf_event_enable_on_exec(st
 	raw_spin_lock(&ctx->lock);
 	task_ctx_sched_out(ctx);
 
-	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
-		ret = event_enable_on_exec(event, ctx);
-		if (ret)
-			enabled = 1;
-	}
-
-	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
+	list_for_each_entry(event, &ctx->event_list, group_entry) {
 		ret = event_enable_on_exec(event, ctx);
 		if (ret)
 			enabled = 1;

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Zhu, DengCheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3088 bytes --]



在 2011年11月23日星期三，Peter Zijlstra <a.p.zijlstra@chello.nl> 写道：
> On Wed, 2011-11-23 at 11:38 +0800, Deng-Cheng Zhu wrote:
>
>> +     list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
>> +             ret = event_enable_on_exec(event, ctx);
>> +             if (ret)
>> +                     enabled = 1;
>> +     }
>> +
>>       list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
>>               ret = event_enable_on_exec(event, ctx);
>>               if (ret)
>
> This isn't correct either, in this case you should then remove the other
> two iterations of pinned/flexible group lists.
>
> Also your use of list_for_each_entry_rcu() is incorrect, either you then
> should also use rcu_read_lock(), or its not needed and not use the _rcu
> list primitive at all.
>
> Now since event_list is modified under ctx->lock, and we hold that lock
> no fancy stuff is needed and we can do without.

Ah, yes you are right. Thanks. Your following patch looks good except
that event_entry should be used for ctx event_list, rather than group_entry.
Tomorrow I'll test it and get back to you.


Deng-Cheng

> ---
> Subject: perf: Fix enable_on_exec for sibling events
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Tue Nov 22 11:25:43 CET 2011
>
> Deng-Cheng Zhu reported that sibling events that were created disabled
> with enable_on_exec would never get enabled. Iterate all events instead
> of the group lists.
>
> Reported-by: Deng-Cheng Zhu <dczhu@mips.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Link: http://lkml.kernel.org/n/tip-299rxrpmmle8hp3spyxfo202@git.kernel.org
> ---
>  kernel/events/core.c |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> Index: linux-2.6/kernel/events/core.c
> ===================================================================
> --- linux-2.6.orig/kernel/events/core.c
> +++ linux-2.6/kernel/events/core.c
> @@ -2494,13 +2494,7 @@ static void perf_event_enable_on_exec(st
>        raw_spin_lock(&ctx->lock);
>        task_ctx_sched_out(ctx);
>
> -       list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
> -               ret = event_enable_on_exec(event, ctx);
> -               if (ret)
> -                       enabled = 1;
> -       }
> -
> -       list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
> +       list_for_each_entry(event, &ctx->event_list, group_entry) {
>                ret = event_enable_on_exec(event, ctx);
>                if (ret)
>                        enabled = 1;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Peter Zijlstra]

On Wed, 2011-11-23 at 12:40 +0000, Zhu, DengCheng wrote:
> 
> Ah, yes you are right. Thanks. Your following patch looks good except
> that event_entry should be used for ctx event_list, rather than
> group_entry. 

D'0h yes, I figure the next compile cycle would've caught that. Fixed it
though, thanks!

Re: [PATCH v2 5/5] perf: Enable applicable siblings when group leader is enable-on-exec

[Deng-Cheng Zhu]

On 11/23/2011 08:48 PM, Peter Zijlstra wrote:
> On Wed, 2011-11-23 at 12:40 +0000, Zhu, DengCheng wrote:
>>
>> Ah, yes you are right. Thanks. Your following patch looks good except
>> that event_entry should be used for ctx event_list, rather than
>> group_entry.
>
> D'0h yes, I figure the next compile cycle would've caught that. Fixed it
> though, thanks!

I tested it this morning. It passed my cases :) You may also add my:

Tested-by: Deng-Cheng Zhu <dczhu@mips.com>


Deng-Cheng

[tip:perf/core] perf: Fix enable_on_exec for sibling events

[tip-bot for Peter Zijlstra]

Commit-ID:  b79387ef185af2323594920923cecba5753c3817
Gitweb:     http://git.kernel.org/tip/b79387ef185af2323594920923cecba5753c3817
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Tue, 22 Nov 2011 11:25:43 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 6 Dec 2011 08:34:01 +0100

perf: Fix enable_on_exec for sibling events

Deng-Cheng Zhu reported that sibling events that were created disabled
with enable_on_exec would never get enabled. Iterate all events
instead of the group lists.

Reported-by: Deng-Cheng Zhu <dczhu@mips.com>
Tested-by: Deng-Cheng Zhu <dczhu@mips.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1322048382.14799.41.camel@twins
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/events/core.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index eeda540..3c1541d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2497,13 +2497,7 @@ static void perf_event_enable_on_exec(struct perf_event_context *ctx)
 	raw_spin_lock(&ctx->lock);
 	task_ctx_sched_out(ctx);
 
-	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
-		ret = event_enable_on_exec(event, ctx);
-		if (ret)
-			enabled = 1;
-	}
-
-	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
+	list_for_each_entry(event, &ctx->event_list, event_entry) {
 		ret = event_enable_on_exec(event, ctx);
 		if (ret)
 			enabled = 1;