Message-ID: <20100206122004.GF5062@nowhere>
Date:	Sat, 6 Feb 2010 13:20:07 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Paul Mackerras <paulus@...ba.org>,
	Hitoshi Mitake <mitake@....info.waseda.ac.jp>,
	Li Zefan <lizf@...fujitsu.com>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [PATCH 02/11] tracing: Introduce TRACE_EVENT_INJECT

On Fri, Feb 05, 2010 at 10:07:32AM -0500, Steven Rostedt wrote:
> How would it be dumping all?
> 
> You need to pick an event to register, not all events will do this.
> 
> How is this different than defining a TRACE_EVENT_INJECT()?  Now
> lock_class_init will always call this inject.
> 
> What I am proposing is to add the lock_class_init just as TRACE_EVENT()
> and then in a initcall, do:
> 
> register_event_command("lock_class_init", NULL, NULL, inject_me, 
> 			inject_me, data);
> 
> The above would have lock_class_init, when enabled or disabled call
> inject_me. Isn't that exactly what the TRACE_EVENT_INJECT() is doing?
> 
> I have no qualms about adding lock_class_init, I just don't think we
> need another TRACE_EVENT macro, that is very inflexible. I rather
> consolidate the macros in ftrace.h than be adding new ones.


This won't work for perf needs because:

1) we need this call to be asynchronous, ie: called whenever we
   want.

Basically, the injection of this event is required for other
lock events to be understandable. It is split in two parts:

* a normal trace event, synchronous, that has a normal trace_* thing
  call site. This tracks the new lock classes created.
* an asynchronous point that catch up with old events we need.

These two parts cover all lock init events we need.

But there is a reason I haven't made this injection callback something
we call once the event gets enabled (I had a first version using
TRACE_EVENT_CALLBACK, but it didn't work), because when perf registers
a trace event, it is not yet scheduled in, not at the same time but
a bit after (it's even more complicated from perf tools as we
create every events as disabled first and enable them later).

So if you call the injector right after registering a user for
the trace event, the injected events will be refused by perf,
as it is not yet set up in the context.

This is why perf needs to choose the time when this injector
is called.

2) we don't want to call that for every lock init events
   registered

We create one lock_init perf event per cpu, only one will need
to call this injector. We just need to catch up with old events
only once.


3) perf and ftrace need the same injector callback here.

4) but ftrace needs this callback to be called when the event gets
enabled (otherwise, other lock init events just don't make sense).

I agree with you that creating a new trace event macro is a bit
insane. ftrace.h is already a nightmare. I just thought that
having the injector set inside the same macro that the synchronous
event is defined helped to make it clear about its nature: that
it needs a secondary async catch up thing.

But a register_event_injector thing will work too, we probably
better want that, given the maintainance pain otherwise.

I really would like to make something useful for everyone, could
you tell me more about johill needs?

Thanks.

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

Powered by blists - more mailing lists