Date:	Sat, 20 Dec 2008 15:15:26 +0100
From:	"Frédéric Weisbecker" <fweisbec@...il.com>
To:	"Steven Rostedt" <rostedt@...dmis.org>
Cc:	"Pekka Paalanen" <pq@....fi>,
	"Pekka J Enberg" <penberg@...helsinki.fi>, mingo@...e.hu,
	linux-kernel@...r.kernel.org,
	"Markus Metzger" <markus.t.metzger@...il.com>
Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

2008/12/20 Steven Rostedt <rostedt@...dmis.org>:
> On Sat, 20 Dec 2008, Pekka Paalanen wrote:
>
>> Steven,
>>
>> I have some critique on where the tracing infrastructure has been going
>> to lately. Please, let me know if I am just out-of-date on the current
>> state or misunderstood something.
>
> I could always use a critique ;-)
>
>>
>> On Fri, 19 Dec 2008 12:20:18 -0500 (EST)
>> Steven Rostedt <rostedt@...dmis.org> wrote:
>>
>> >
>> > On Fri, 19 Dec 2008, Fr?d?ric Weisbecker wrote:
>> > >
>> > >
>> > > That's looks good.
>>
>> The mmiotrace part looks good, too.
>>
>> > > By the past, I also suggested Steven to automatically reset the traces
>> > > buffer each time a tracer is started, that
>> > > would factor out the code a bit more. I don't think one tracer would
>> > > avoid to reset the buffer once it is started, and
>> > > I don't think it is needed to reset twice on tracer switching: on stop
>> > > of the old tracer and on start on the new. Only
>> > > on start should be enough.
>> >
>> > I'm actually against the idea of reseting a trace everytime we enable it.
>> > That is:
>> >
>> > echo 1 > /debug/tracing/tracing_enabled
>> >
>> > This should not reset the tracer. I actually do tracing where I disable
>> > and enable it around areas I am interested in. I want all tracing, not
>> > just the last one.
>>
>> But doesn't this go against the fact, that you need to write 0 there to
>> be able to change the ring buffer size?
>>
>> I mean, is tracing_enabled a "pause button" as I recall you explaining
>> a long time ago, and again now, or "kill it all" as required for changing
>> the ring buffer size?
>
> It is more now a pause than kill it all. Although it never really did
> kill it all. Before the ring buffer, we needed to echo in 'none' to
> the current tracer before resizing. Now we can just get by with echoing 0
> to the tracing_enabled.
>
> I'm starting to like the idea that tracing_enabled is a lighter weight
> version of echoing the the tracer into the current_tracer file. Perhaps it
> should reset the buffer on a echo 1 > tracing_enabled. We now have a
> tracing_on that we can "pause" tracing with. The only thing that the
> tracing_on does is to stop writes to the ring buffer. It does not stop any
> of the infrastructure that does the tracing.
>
> Note, this is the main reason why you need to check for NULL on return of
> a ring_buffer_lock_reserve. That will return NULL when the tracing_on is 0.


I agree with the fact that tracing_enabled and tracing_on are both
confusing for a new user.
Perhaps it needs a little disambiguation.
With not rename tracing_on to ringbuffer_on ? That would give the idea
that tracing_enabled
acts on the tracing layer whereas ringbuffer_on is more a low level solution.

Or another solution if you consider to reset buffer when echo 0 >
tracing_enabled, so this
file can stay tracing_enabled and tracing_on could become tracing_pause.


>> Unless you have an answer to this, I'd like to suggest we resurrect the
>> "none" tracer. When "none" is the current tracer, there would be no
>> buffers allocated at all, and you could request a new buffer size.
>> "none" would be the default. Do you see any problems here?
>>
>> AFAIK the "nop" tracer will not do, because it still allows text
>> messages (markers) to be written, and hence the ring buffer must
>> exist. Or am I wrong?
>
> No, you are quite right. We could recreate the 'none' tracer again that
> has no buffer. At boot up it would be the default tracer, unless something
> else changes that.


If none tracer is recreated, I would suggest to rename nop tracer to
"print tracer" with
the single purpose of allowing to print the ftrace_printk entries....
Since there already have been
a patch to add an ftrace_printk tracer, that would make sense.

Again, that would be a future disambiguation between the nop and none
tracer inside available_tracer file.
--
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