Re: [PATCH 3/8] pstore: Add persistent function tracing

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 2012-07-17 at 13:01 -0700, Anton Vorontsov wrote:
> On Tue, Jul 17, 2012 at 03:38:18PM -0400, Steven Rostedt wrote:
> [...]
> > > +void notrace pstore_ftrace_call(unsigned long ip, unsigned long parent_ip)
> > 
> > BTW, you can make the entire file 'notrace' without adding annotations
> > by including in the Makefile:
> > 
> > CFLAGS_REMOVE_ftrace.o = -pg
> 
> Actually it was in the first version in the patch, but then I changed
> it 'notrace' for just this func. This is for the case if the file would
> contain some more code which we actually may trace. Doing things fine-
> grained seemed to be better than making the whole file as notrace. Plus
> it is one line less. :-)
> 
> But I have no preference, so I can change it.

I have no preference, it was just an FYI. For just a single function in
a file, it really makes no difference which you use. The Makefile trick
is better for needing to make sure an entire file is not traced.


> 
> > > +{
> > > +	struct pstore_ftrace_record rec = {};
> > > +
> > > +	if (unlikely(oops_in_progress))
> > > +		return;
> > > +
> > > +	rec.ip = ip;
> > > +	rec.parent_ip = parent_ip;
> > > +	pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
> > > +	psinfo->write_buf(PSTORE_TYPE_FTRACE, 0, NULL, 0, (void *)&rec,
> > > +			  sizeof(rec), psinfo);
> > > +}
> > 
> > BTW, can any of the called functions go into module code that can be
> > removed? If so, then this is not safe at all. Normal function tracing
> > can not be synced in a preemptible kernel.
> 
> Um. Yes, psinfo->write_buf() might be in the module. Nice catch.
> 
> > Also, I'm starting to wonder if this should be in its own utility
> > (separate debugfs?) than hooking directly into ftrace. Then you don't
> > need to modify ftrace at all and you can do the following:
> > 
> > static struct ftrace_ops trace_ops {
> > 	.func = pstore_ftrace_call;
> > };
> > 
> > then in your write to debugfs file:
> > 
> > 	register_ftrace_function(&trace_ops);
> > 
> > To turn off tracing:
> > 
> > 	unregister_ftrace_function(&trace_ops);
> > 
> > Note, it's safe to use if the trace_ops (or anything the callback calls)
> > is a module. That's because it detects the trace_ops is not kernel core
> > code and will place a wrapper around it that allows the function tracing
> > to by synced with module unload. You still need to unregister the
> > trace_ops before unloading the module, or you can have a crash that way.
> 
> Hehe. Like this? http://lkml.org/lkml/2012/5/26/80 :-D
> 
> So, do you want something like this, but combinded: we don't register
> another tracer, but register our own ftrace_ops? This sounds doable.

Yeah, the combined. That is, don't make a tracer out of it. Use another
mechanism to enable the tracing and not as a tracer
in /debug/tracing/available_tracers. We can probably set up events for
you too, but at a later time.

-- Steve