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 07:52:06PM +0100, Peter Zijlstra wrote:
> On Wed, 2010-12-01 at 19:02 +0100, Frederic Weisbecker wrote:
> 
> > >  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.
> 
> What context? :-) There's now two context's (with the possibility of
> even more), which one will hold the tracepoint stuff?
> 
> Also, since we only need one such structure, adding it to the context
> doesn't make sense.

Oh you're right, I forgot the per pmu context thing :)


> 
> > > @@ -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.
> 
> You didn't look hard enough, its a sequence stamp on the context for
> inheritance, then later, when we want to compare inherited contexts we
> can simply compare generation numbers, if they're the same the contexts
> are the same.

Ah right.

> > >  }
> > >  
> > >  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?
> 
> No, there's only 1 idr per task. Having one per context means we have to
> iterate all contexts when a tracepoint triggers and it adds yet another
> pointer chase. It also means we have to manage more stuff when
> tracepoints change context etc..
> 
> But yes, it would make this part easier, I just don't like the added
> fast path overhead.

Ok.