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

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

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.

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.

But... if they feel lucky and smart, they can just call
ring_buffer_unlock_commit(tr->buffer, event) and do any other things in
a open coded way, as was done in other cases where
trace_buffer_lock_reserve was paired with ring_buffer_unlock_commit.

- Arnaldo