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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 2 Dec 2010 18:56:53 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Stephane Eranian <eranian@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [BUG(?)] perf_events: combining multiple tracepoint events
 into a group produces no counts on member events

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/