Re: [BUG(?)] perf_events: combining multiple tracepoint events into a group produces no counts on member events

[Frederic Weisbecker <fweisbec@gmail.com>]

On Wed, Dec 01, 2010 at 01:04:37PM +0100, Peter Zijlstra wrote:
> @@ -545,6 +546,11 @@ struct hw_perf_event {
>  			struct task_struct		*bp_target;
>  		};
>  #endif
> +		/*
> +		 * Same fudge as for breakpoints, trace-events needs
> +		 * it too,.. convert the bp crap over..
> +		 */
> +		struct task_struct *event_target;

Yeah, looks like we can merge the bp_target and event_target.


>  struct task_struct {
>  	volatile long state;	/* -1 unrunnable, 0 runnable, >0 stopped */
>  	void *stack;
> @@ -1452,6 +1458,9 @@ struct task_struct {
>  	struct perf_event_context *perf_event_ctxp[perf_nr_task_contexts];
>  	struct mutex perf_event_mutex;
>  	struct list_head perf_event_list;
> +#ifdef CONFIG_EVENT_TRACING
> +	struct perf_tp_idr *perf_tp_idr;

Why not attaching this to the ctx eventually? This makes one pointer less
in task_struct.

> @@ -370,6 +372,7 @@ list_del_event(struct perf_event *event,
>  	 */
>  	if (event->state > PERF_EVENT_STATE_OFF)
>  		event->state = PERF_EVENT_STATE_OFF;
> +	++ctx->generation;

What's the role of the ctx->generation? It seems to be incremented two times
but doesn't appear to have any purpose.


>  }
>  
>  static void perf_group_detach(struct perf_event *event)
> @@ -1228,6 +1231,12 @@ void perf_event_context_sched_out(struct
>  	if (!cpuctx->task_ctx)
>  		return;
>  
> +#if 0
> +	/*
> +	 * Need to sort out how to make task_struct::perf_tp_idr
> +	 * work with this fancy switching stuff.. tracepoints could be
> +	 * in multiple contexts due to the software event muck.
> +	 */

Not sure what's the issue here. Each ctx have the perf_tp_idr matching
active tracepoints, isn't it?

> +static struct perf_tp_idr *perf_event_idr(struct perf_event *event, bool create)
> +{
> +	struct perf_tp_idr *tp_idr;
> +	struct task_struct *task;
> +
> +	if (event->attach_state & PERF_ATTACH_TASK) {
> +		task = event->hw.event_target;
> +		tp_idr = task->perf_tp_idr;
> +		if (!tp_idr && create)

Is it possible that task->perf_tp_idr can eventually disappear
under us there? Like when an event is released from that task?

> +			tp_idr = perf_tp_init_task(event, task);
> +	} else
> +		tp_idr = &per_cpu(perf_tp_idr, event->cpu);
> +
> +	return tp_idr;
> +}