Re: [PATCH tip 2/2] tracing: Introduce trace_buffer_{lock_reserve,unlock_commit}

[Arnaldo Carvalho de Melo <acme@redhat.com>]

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