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

[Peter Zijlstra]

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.

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

> >  }
> >  
> >  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.

> > +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?

We hold a ref on the task on the create path, I'd have to actually think
about the destroy path, but I think there's a problem there.

I used to have a PERF_ATTACH_CONTEXT/GROUP test in there as well, dunno
why that didn't work out.

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


--
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/