Messages in this thread | | | Date | Sat, 20 Dec 2008 15:15:26 +0100 | From | "Frédéric Weisbecker" <> | Subject | Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper) |
| |
2008/12/20 Steven Rostedt <rostedt@goodmis.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@goodmis.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.
| |