Message-ID: <20090203125712.GC17781@ghostprotocols.net>
Date:	Tue, 3 Feb 2009 10:57:12 -0200
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
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 1/3] trace: better manage the context info for
	events

Em Tue, Feb 03, 2009 at 10:09:03AM +0100, Frederic Weisbecker escreveu:
> On Mon, Feb 02, 2009 at 09:32:51PM -0500, Steven Rostedt wrote:
> > 
> > On Tue, 3 Feb 2009, Frederic Weisbecker wrote:
> > 
> > > On Mon, Feb 02, 2009 at 08:29:21PM -0200, Arnaldo Carvalho de Melo wrote:
> > > > From: Frederic Weisbecker <fweisbec@...il.com>
> > > > 
> > > > Impact: make trace_event more convenient for tracers
> > > > 
> > > > All tracers (for the moment) that use the struct trace_event want to
> > > > have the context info printed before their own output: the pid/cmdline,
> > > > cpu, and timestamp.
> > > > 
> > > > But some other tracers that want to implement their trace_event
> > > > callbacks will not necessary need these information or they may want to
> > > > format them as they want.
> > > > 
> > > > This patch adds a new default-enabled trace option:
> > > > TRACE_ITER_CONTEXT_INFO When disabled through:
> > > > 
> > > > echo nocontext-info > /debugfs/tracing/trace_options
> > > > 
> > > > The pid, cpu and timestamps headers will not be printed.
> > > > 
> > > > IE with the sched_switch tracer with context-info (default):
> > > > 
> > > >      bash-2935 [001] 100.356561: 2935:120:S ==> [001]  0:140:R <idle>
> > > >    <idle>-0    [000] 100.412804:    0:140:R   + [000] 11:115:S events/0
> > > >    <idle>-0    [000] 100.412816:    0:140:R ==> [000] 11:115:R events/0
> > > >  events/0-11   [000] 100.412829:   11:115:S ==> [000]  0:140:R <idle>
> > > > 
> > > > Without context-info:
> > > > 
> > > >  2935:120:S ==> [001]  0:140:R <idle>
> > > >     0:140:R   + [000] 11:115:S events/0
> > > >     0:140:R ==> [000] 11:115:R events/0
> > > >    11:115:S ==> [000]  0:140:R <idle>
> > > > 
> > > > A tracer can disable it at runtime by clearing the bit
> > > > TRACE_ITER_CONTEXT_INFO in trace_flags.
> > > > 
> > > > The print routines were renamed to trace_print_context and
> > > > trace_print_lat_context, so that they can be used by tracers if they
> > > > want to use them for one of the trace_event callbacks.
> > > 
> > > 
> > > Actually, I wonder if this is not breaking the sense of the TRACE_ITER_CONTEXT_INFO
> > > flag.
> > 
> > Yeah, I agree with Frederic here. Let the user decide this as a global 
> > flag for about of data to print. The tracer should just provide a callback 
> > incase the tracer has a different context format.
> > 
> > > 
> > > In the first patch I made about it, I thought this flag was to decide whether we want to
> > > print the context information in the standard way.
> > > 
> > > Then, Steven suggested to actually provide callbacks for the tracers which want
> > > to override the standard context information printer.
> > > The flag then got more logical: TRACE_ITER_CONTEXT_INFO was not only about deciding
> > > whether we want or not the standard context info, it tells if we want in a global way
> > > the context info to be printed, whatever how the tracer decides to print it.
> > > (This is the theory, but since we couldn't override the bin/raw/hex cases, the
> > > practical case didn't follow this idea).
> > > 
> > > If someone doesn't want to see these informations in the blk tracer, this flag will
> > > not help him. Worst, it will double print the context info if the user enables the flag.
> > > 
> > > Now that I see the practical case, I'm not sure the design of my patch was valuable.
> > > A tracer has to play with the flag if it wants to override the context info in
> > > the bin/raw/hex cases. And I don't think this is a good way to proceed.
> > > 
> > > In my opinion, the ITER_CONTEXT_INFO flag should mostly be set by the user.
> > 
> > I think it should _only_ be set by user.
> > 
> > > 
> > > And only one callback could be added to trace_event: context_info()
> > > Then, the tracer will manage itself the raw/hex/bin/normal cases inside
> > > this callback.
> > 
> > Yeah, this is a good idea. The callback can be passed an enum to what kind
> > of trace it is: TRACE_FMT_RAW TRACE_FMT_HEX ...
> > 
> > 
> > > 
> > > We can provide the default callbacks available for the tracers which want it and even
> > > one function which proceed all of them, depending on the flags.
> > > ftrace/preempt/sched.... tracers can register this function for their context_info callback
> > > and other tracers too if they want.
> > > 
> > > Or they can override it, and even pick the default callbacks for dedicated flags when they want.
> > > 
> > > Then, when the user wants the context info to be printed or not, he just have to
> > > set/clear the context-info flag manually.
> > > 
> > > A tracer can even decide to set/clear it by default, but for its real sense: print or not
> > > these context info.
> > > 
> > > What do you think?
> > 
> > I'm not sure a tracer should decide if it should have the format or not. 
> > In its context callback, it might decide there. But if the user does not 
> > want it, it should be off.
> 
> 
> Of course, I just thought that some tracers could choose if it's default enabled
> or disabled when they are selected.

As the 3rd patch does? :)
 
> > Unless you are saying have the tracer decide if it is enabled on or off 
> > when the trace is selected?
> 
> 
> Exactly, I meant that printing the context can be mostly relevant for several tracers
> (ftrace, sched) but can be encumbering for some other traces (mmiotrace).
> 
> So perhaps a tracer could touch this flag when it's selected
> and restore its value when unselected.

I just had to save the previous setting for context_info, in
trace->start(), but I took the lazy approach "at trace->start() time
TRACE_ITER_CONTEXT_INFO will always be turned on, let the tracer decide
if it should disable it"

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