Messages in this thread | | | Date | Fri, 6 Feb 2009 01:10:15 -0200 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH tip 2/2] tracing: Introduce trace_buffer_{lock_reserve,unlock_commit} |
| |
Em Fri, Feb 06, 2009 at 03:39:45AM +0100, Frederic Weisbecker escreveu: > On Thu, Feb 05, 2009 at 11:54:16PM -0200, Arnaldo Carvalho de Melo wrote: > > Em Thu, Feb 05, 2009 at 11:58:37PM +0100, Frederic Weisbecker escreveu: > > > > +void trace_buffer_unlock_commit(struct trace_array *tr, > > > > + struct ring_buffer_event *event, > > > > + unsigned long flags, int pc) > > > > +{ > > > > + ring_buffer_unlock_commit(tr->buffer, event); > > > > + > > > > + ftrace_trace_stack(tr, flags, 6, pc); > > > > + ftrace_trace_userstack(tr, flags, pc); > > > > + trace_wake_up(); > > > > +} > > > > > > > > > I have mitigate feelings about this part. The name of this function could > > > have some sense if _most_ of the tracers were using the stack traces. But that's > > > not the case. > > > > > > We have now this couple: > > > > > > _ trace_buffer_lock_reserve() -> handles the ring-buffer reservation, the context info, and the type > > > _ trace_buffer_unlock_commit() -> unlock, commit, wake and... stacktraces? > > > > > > In my opinion, the latter doesn't follow the logic meaning of the first. > > > And the result is a mixup of (trace_buffer | ring_buffer)(lock/unlock/reserve/commit). > > > > > > You are sometimes using trace_buffer_lock_reserve followed by ring_buffer_unlock_commit. > > > That looks a bit weird: we are using a high level function followed by its conclusion > > > on the form of the low lovel function. > > > > > > I think the primary role of this new couple should be to simplify the low level ring buffer > > > bits as it does. But the stack things should stay separated. > > > > Well, the whole reason for this cset was to provide a way to check for > > things like stacktrace while reducing the number of explicit calls the > > poor driver, oops, ftrace plugin writers had to keep in mind. > > > I agree, but that forces those who don't need stacktraces to use > a paired trace_buffer_lock_reserve() / ring_buffer_unlock_commit() > The poor newcomers will become dizzy with these different namespaces... > And it's like managing a file with fopen() and then close() ... :-) > > > > So it may well be the case for a better name, but frankly I think that > > this is something better left _hidden_, a magic that the plugin writers > > doesn't have to deal with. > > I agree with you, the stacktraces are used by several tracers, and then > it deserves some code factoring. > What I would suggest is to have two different trace_buffer_unlock_commit() > > Thinking about the name of these functions, since they are in a higher layer > than the ring buffer which performs some things with locking and buffers, we could > let this latter do his tricky low level work and simply offer some magic functions > with magic names: > > _ trace_reserve() > _ trace_commit() > _ trace_commit_stacktrace()
The point I was trying to make is that the magic is not just stacktraces, it may well be some other whizbangfoobar that I don't know right now.
So perhaps, we indeed need some per tracer flags where the driver writer can state which kind of magic it _doesn't_ want performed.
The default would be: magic is in the air... I.e. do whatever magic you may find interesting, as I can't foretell.
- Arnaldo
| |