Messages in this thread | | | Date | Thu, 28 Aug 2008 11:04:36 +0100 | From | "Frédéric Weisbecker" <> | Subject | Re: [Patch] Tracing/ftrace: Adds a marker to allow user comments |
| |
Hello Pekka,
2008/8/27 Pekka Paalanen <pq@iki.fi>: > IMHO we could just make ftrace_printk() work for all tracers. I doubt > there will be need to be able to record nil bytes. The idea of a marker > is to be a line of human readable text. > >> > +int __ftrace_printk(unsigned long ip, const char *fmt, ...) >> > +{ >> > + struct trace_array *tr = &global_trace; >> > + static DEFINE_SPINLOCK(trace_buf_lock); >> > + static char trace_buf[TRACE_BUF_SIZE]; >> > + struct trace_array_cpu *data; >> > + struct trace_entry tmp_ent; >> > + unsigned long flags; >> > + long disabled; >> > + va_list ap; >> > + int cpu, len = 0; >> > + >> > + if (likely(!ftrace_function_enabled)) >> > + return 0; > > And that is the kind of check that makes current ftrace_printk() unusable. > I think this restriction should be lifted and allow it to work on all > tracers. I would also like to be able to have e.g. mmiotrace_marker(), > which would have the same signature as ftrace_printk(), but is a no-op if > mmiotrace is not active. So, we could have an __ftrace_vprintk() to make > writing such wrappers easy. And the wrappers could be #ifdef'd away, if > the corresponding tracer is not built.
I like your idea of a ftrace_vprintk wrapper. But actually I'm questioning about the fact that the tracing api itself is not generic enough.
At this time, a tracer has to put a function inside the tracing api when it wants to add an entry into the list. See __trace_mmiotrace_rw() for example.
Just imagine if one wants to implement a tracer by using a simple module. The way seems to be simple at the beginning: you define your struct tracer, implement your functions and register your tracer. But the simple, single function that will add your entries has to be put in tracing api. It's quite impossible without changing the trace.c file and so impossible without recompiling your kernel.
And if I make my own tracer and want to implement my handler for the marker messages, I will have to put my foo_trace_printk in trace.c and add it on the wrapper.
IMHO the choice of a wrapper is good but it should dynamically dispatch the message on a handler proposed by the tracer. I think about a new function pointer which could be called "mark" on the struct tracer.
And (always IMHO) I think that functions like __trace_mmiotrace_rw() shouldn't be located on trace.c but implemented on the appropriate tracer (in this example: trace_mmiotrace.c). And for this purpose, functions like tracing_generic_entry_update() (or even more higher level function of entry adding) should be symbol's exported.
With such a new environment, building personal and quick tracers by using modules may be more appreciated if one has not to touch the tracing API or the whole running kernel.
> I recall somewhere mentioning, that one shouldn't leave ftrace_printk's > lying around in "final" code, so I'm not sure how people feel about this > wrapping idea, especially when e.g. mmiotrace is hoped to be built-in by > default. OTOH, I don't think in-tree drivers and stuff want to use them, > so they would be mostly offered for out-of-tree modules > (reverse-engineering) and developers (debugging). That makes me wonder > if they would be accepted (as exported symbols).
Yes I think such a function should really be exported. For example using mmiotrace on a module and beeing able to signal the fact that we are entering the "foo" function seems to me very useful. It's important to know where we are in all these IO for debugging....
>> > static int print_trace_line(struct trace_iterator *iter) >> > { >> > + if (iter->ent->type == TRACE_MARK) >> > + return print_mark(iter); >> > + > > Please, put this call after the following... > >> > if (iter->trace && iter->trace->print_line) >> > return iter->trace->print_line(iter); > > ..so that tracers may override it at will. Mmiotrace needs to replace > newlines and nil characters, before printing it in its own style, i.e. > prefixed with a tag and a timestamp, which are specified in the mmiotrace > log format.
Perhaps the idea of an appropriate "mark" handler for each tracer could solve this...
> I didn't try the patch yet, or read it too carefully, but I should. > Unfortunately I'm away the next weekend, so I hope to continue working > on it the weekend after that.
No problem.
Thanks!
| |