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    [W:0.025 / U:30.500 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site