lkml.org 
[lkml]   [2009]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2009-02-06 04:13    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site