Re: [BUG] perf_event: when events are grouped, the time enabled / running values are incorrect

[Corey Ashford <cjashfor@linux.vnet.ibm.com>]

Thanks, Peter,

I had copied and pasted the patch from the web gossamer-threads lkml archive (I didn't have access to my email at the time), and had problems with the patch program complaining that the patch was in improper format.  I couldn't see anything wrong with what it was complaining about, so I assumed that it was something to do with the code it was actually patching.  Probably a bad assumption in retrospect.

I will try your patch for 2.6.33.3, and since I have access to my email again, there probably won't be any problems.   Stay tuned :)

- Corey

On 5/12/2010 10:50 AM, Peter Zijlstra wrote:
> On Wed, 2010-05-12 at 10:25 -0700, Corey Ashford wrote:
> 
>> Thanks for looking into this problem!  Unfortunately, I've been unable 
>> to test your patch.  It doesn't apply to 2.6.33.3 
> 
> Strange the code looks very similar, the below is a diff against a
> virgin v2.6.33.3.
> 
> ---
>  kernel/perf_event.c |   26 +++++++++++++-------------
>  1 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 32d0ae2..e8ae0c2 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -290,6 +290,18 @@ static void update_event_times(struct perf_event *event)
>  }
> 
>  /*
> + * Update total_time_enabled and total_time_running for all events in a group.
> + */
> +static void update_group_times(struct perf_event *leader)
> +{
> +	struct perf_event *event;
> +
> +	update_event_times(leader);
> +	list_for_each_entry(event, &leader->sibling_list, group_entry)
> +		update_event_times(event);
> +}
> +
> +/*
>   * Add a event from the lists for its context.
>   * Must be called with ctx->mutex and ctx->lock held.
>   */
> @@ -337,7 +349,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  	if (event->group_leader != event)
>  		event->group_leader->nr_siblings--;
> 
> -	update_event_times(event);
> +	update_group_times(event);
> 
>  	/*
>  	 * If event was in error state, then keep it
> @@ -508,18 +520,6 @@ retry:
>  }
> 
>  /*
> - * Update total_time_enabled and total_time_running for all events in a group.
> - */
> -static void update_group_times(struct perf_event *leader)
> -{
> -	struct perf_event *event;
> -
> -	update_event_times(leader);
> -	list_for_each_entry(event, &leader->sibling_list, group_entry)
> -		update_event_times(event);
> -}
> -
> -/*
>   * Cross CPU call to disable a performance event
>   */
>  static void __perf_event_disable(void *info)
> 
> 
> --
> 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/

-- 
Regards,

- Corey

Corey Ashford
Software Engineer
IBM Linux Technology Center, Linux Toolchain
Beaverton, OR 
503-578-3507 
cjashfor@us.ibm.com