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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, Feb 06, 2009 at 01:10:15AM -0200, Arnaldo Carvalho de Melo wrote:
> 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


Ok, yes by making the flags per tracer, it will goes well.
Just one thing, the insertion of an event is sometimes a hot path
like with the functions tracer. And such facility adds some unused function calls
and branch checking.
But on such cases, we can use directly the ring buffer functions :-)

Frederic.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/