Date:	Fri, 6 Feb 2009 03:39:45 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Cc:	Steven Rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...e.hu>,
	Jens Axboe <jens.axboe@...cle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH tip 2/2] tracing: Introduce
	trace_buffer_{lock_reserve,unlock_commit}

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()

Even if the stack traces layer can be somewhat hidden to the user, we can still
let him decide what he really wants but in an easier way.

Hm?

> 
> 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.


Right! And with our three above functions, the new magic way can
be completely performed without beeing harassed by some bits from the
...lucky way :-)


Frederic.

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

Powered by blists - more mailing lists